Autologout cookie is not secure

Uploading a patch for use until this is merged and released

I'll review it.

The changes are working as intended but it also broke a lot of tests. Mostly related to the missing schema for cookie_lifetime. I'll see if i can fix them.

Done.

i will review it.

The changes are working great, I tested it in my local environment following the step by step and everything went as expected, so I'm moving to RTBC.

I think this patch needed httpOnly parameter as well.

So I updated patch at #4, to include httpOnly option.

Patch #11 works fine, but REQUEST_TIME is deprecated, so I added a small fix for that.

I added the post update hook for the new settings, so existing users have values set and they only need to export the configuration (to have it set "default"). And also applied the changes from patch in comment #12
The testing is now done via GitLab CI and test are passing, so setting the status to needs review and "hidding" the patch files, as the issue fork exists and all changes should be done there.

When the user logs in the cookie correctly sets values of the cookie. The problem comes when JS sets a new cookie cookies.set("Drupal.visitor.autologout_login", Math.round((new Date()).getTime() / 1000));this resets all values to None.

Looking into fixing it.

Now the cookie setting in JS also adds the configurations.

Good catch for the JS configuration. Just removed the casting from int as it's not necessary and updated post update hook to use correct variable for lifetime. The code works, it sets the cookie base on the settings defined in the configuration, so marking this as RTBC.

This is working really, except that the settings form always reverts back to the default value for cookie lifetime. To replicate, change the value to `0` and save. The form will show `31536000`.

There appears to be something wrong with the logic. After the initial timeout, the user is presented with a dialog on every page request. I have it set to "Logout user regardless of activity".

Updated code for issue to set cookie lifetime to 0

I figured out why the timer never gets reset after the initial one times out when the module is configured to "Logout user regardless of activity". The cookie with HttpOnly configured to true cannot be accessed by js, so var login_time = cookies.get("Drupal.visitor.autologout_login"); is undefined.

So the HttpOnly setting is currently incompatible with the "Logout user regardless of activity" setting.

Hello @jonraedeke,
I can have a look into js undefined error.

I think we can remove the HttpOnly from this patch and keep the rest.
Do let us know your opinion @jonraedeke

Recently, I have updated to the latest [1.5] release of this module and I have also faced the same issue of cookies being undefined error.
However, if you go to the autologout.libraries.yml file, you can see it is dependent on the libraries of [https://www.drupal.org/project/js_cookie] module.
You can install and enable the module and the cookie_undefined error will be resolved.

Hello, I've re-rolled the patch based on the latest merge request #23
It was falling due to this line on the autologout.settings.yml file

So, what's left :

• Force HttpOnly to false and remove it from the admin form
• Update the MR with the minor fixes from 3308456-30.patch #30
• Change the MR target branch to 2.x (or create a new one if this fix should still apply to 1.x)

I have:

1. Updated the fork with the latest changes.
2. Created a new branch from the new target 2.x branch
3. Update the MR with the fixes from 3308456-30.patch #30

So, what's left :

• Force HttpOnly to false and remove it from the admin form
• Update the MR with the minor fixes from 3308456-30.patch #30
• Change the MR target branch to 2.x (or create a new one if this fix should still apply to 1.x)

Drupal Core added the cookie_samesite to session.storage.options service parameter. This should be used over an override by autologout. Same with the cookie_lifetime value.

Marking NW per comment #38. Because of the new service parameter, this module should just those values. Note, that option was added in Drupal 10.1.

It'd be preferable to not have additional config. To that end, samesite and lifetime settings do not need to be set. For Drupal 10+, use core's settings. For Drupal 10.0 and lower, just set a default 'Lax' and cookie lifetime to 1 month? 1 year seems long. This maintains compatibility with older versions of Drupal that likely wouldn't need anything but the defaults anyway.

Also, shouldn't the cookie secure setting default to TRUE? Sites should be on https and only need this override in cases like https->http load balancer setups. If you're running on http and forget to set it to FALSE, not quite sure what will happen there, but not sure we should care...

Using the new service parameter also change the patch to make the module only available to Drupal 10.1 and above.
We can backport a different solution for earlier versions if required.
For the backport I would think we should copy the current core setting which is 200000 and set 'Lax' by default.

I have removed the additional config for samesite and lifetime settings, but grabbed those values from core and added them to the JS settings.

I left the cookie secure setting default to FALSE just in case sites don't have an SSL cert. I am also not sure what will happen if that is on without one, so thought it best to leave it as False as turning it on is easier than potentially debugging an error if it is on and causes problems.

If anyone needs a patch this can be used: https://git.drupalcode.org/project/autologout/-/merge_requests/71.diff

Aware about patch link on #41 however if MR is updated the patch is updates as well

So uploading the same patch in a file to avoid being updated. So it can be used on composer if needed

Marking this as RTBC as the checkbox is there and the settings are getting set. One small observation, if user is already logged in when the change in settings happens, the users needs to log out and log in again for changes to take.

I tried to rebase this one using the latest 2.x from the upstream repository, but I think a wire is getting crossed somewhere :/ I can't rebase the branch from GitLab UI since the merge conflict in `autologout.js` needs to be resolved manually.

I think this particular forked repos `2.x` branch isn't hooked up to the 2.x from the main autologout repo?

Would a maintainer mind merging in the updated project:2.x into the 2.x for this repo? Happy to resolve the merge conflict after.

The removal of jscookie was needed to allow the composer test to pass, which was blocking the merge of the #3339695: Use Drupal.dialog call instead of jQuery dialog.

As both the js cookie and this are both dealing with the setting of cookies, there are merge conflicts that will need to be addressed.

I'll bring the 2.x branch in asap to keep momentum at least.

There are probably enough changes to warrant another review.

Tested on my local with a Drupal 11.2.8 site and I'm experiencing a couple of issues:

1) I get an error on the site that says "Undefined array key "cookie_samesite" in /var/www/html/web/modules/custom/autologout-3308456/autologout.module". I was able to fix this by updating the operator from "?:" to "??" so that it didn't try to access the undefined array key and would fall back on "Lax" in those cases.

2) It seems like the toggle isn't working? The cookie is spawning regardless of whether I have the option in the config form toggled or not, even when the cookie shows as not being secure and the samesite value is "Lax". I tried logging out and logging back in per Comment #43, but am still seeing the cookie spawning.

I'm stepping away for the moment, but will keep troubleshooting this tonight.

I'll admit that working with cookies is newer territory for me, so if someone more experienced wants to verify this behavior to make sure this isn't a PEBCAK issue on my part, that'd be great. Otherwise, I'll keep working at it.

1) is getting the options from the core setup:

// Get the Drupal cookie lifetime and samesite from Core.
  $options = \Drupal::getContainer()->getParameter('session.storage.options');

so the array key needs to exists. I'm testing with D 11.3.1 and do not have that issue.
2) Why would the cookie not get generated or spawned? It still does, just has the Secure option set to false instead of true, and vice versa when checkbox is checked.

I would say this is still working as it was before, so it can be RTBC, but leaving status as is, if somebody else also wants to have a go at testing.

Gotcha. I may have been misinterpreting the intended behavior in that case.

When I read the help text for the configuration option ("Specifies whether or not the cookie should only be transmitted over a secure HTTPS connection. The cookie will only be set if a secure connection exists."), I was interpreting that as "'Set' = 'Generated'. Therefore, if this is on and the connection is not secure, there should be no cookie."

I definitely yield to more knowledgeable folks in this case for the rubber stamp of approval.

Just to be clear I re-ran my own tests:

ddev remove
ddev restart
ddev drush si -y
ddev drush en autologout -y
ddev drush cset autologout.settings timeout 60 -y
ddev drush cset autologout.settings padding 20 -y
ddev drush cset autologout.settings cookie_secure 1 -y
ddev drush uli

I saw this in the console

Trying to think of a scenario where we may get:
"Undefined array key "cookie_samesite"

Do you think you need to run the update hook to ensure the settings include the new configuration?

I think I found my problem with the "Undefined array key "cookie_samesite".

When I got that error, the way that I was testing the issue was by creating patches from the forked issue and applying them to the stable version of the module on the existing site I'm working on. I'd noticed that some of the commit patches weren't applying, but tried trucking on, but was getting the errors I mentioned.

Given the weirdness and that fact others were getting different results, I tried making a fresh sandbox site this AM and cloning the forked repo into my "modules/custom" directory.

I ran "drush en -y autologout" and got an error that I was missing the "js_cookie" module dependency. I added that and the module enabled fine and, with no additional code changes, I'm no longer getting the "Undefined array key "cookie_samesite".

So I think the weirdness I was getting was the fact I didn't have the js_cookie module dependency. Because my first method of patching was for an already installed and enabled version of autologout, I didn't see the missing dependency issue, just the downstream error message that was ultimately due to the missing dependency.

I looked back through this issue and saw Comment #29 actually had a similar issue. I wonder if we should update the composer.json for this module so that it automatically pulls in the js_cookie module on install?

js_cookie dependency was removed in #3557620: Remove JS Cookie dependency. Did not follow why, just that it was. If the module is cloned with it 2.x branch and then switched to the MR, there should be no issues with the js_cookie dependency. So I'm thinking this is just a user error maybe, probably?

I am 100% willing to chalk it up to user error. I know this is the last big issue on the new release roadmap, so I'm personally comfortable calling it a PEBCAK issue on my part so that we can get it merged in and cut the new release.

If the issue persists after updating the new release, I can circle back up with a separate issue ticket.

We can then set it to RTBC as @the_g_bomb also confirmed it working for him.

Merged, thanks all

Hi team,

Please don't use "Implements hook_post_update_NAME()." as a doc block for an update hook - it really needs to be a description to the user about what the update hook is doing.

Currently it displays like this when running updates:

drush updatedb           
 ------------ ----------- ------------- ------------------------------------- 
  Module       Update ID   Type          Description                          
 ------------ ----------- ------------- ------------------------------------- 
  autologout   10101       post-update   Implements hook_post_update_NAME().  
 ------------ ----------- ------------- ------------------------------------- 

#62 makes sense

This description is useful for people when reviewing the database updates

The original issues is already fixed, but to not create more issues, created a new branch, where the only change is the comment mentioned in #62.

Reviewed and tested

+1 approval on MR 96