Set SameSite on session cookies

Strict means that if you click on a link on foo.org that goes to drupal.org, then you would *not* send the session cookie and you would not be logged in. So strict is IMHO never an option for the session cookie, it has to be at most Lax.

Even Lax has caused problems for us for example in combination with payment gateways that POST the user back to Drupal, then the session is missing, the user has no access to the order and payment fails.

Agreed, Lax should be the default. It actually is the default in some modern browsers if none is set.

Attached is a version of the patch with the attribute set to "Lax", as a slightly better default.

Is that really the default and not actually forcing that value?

I am also in then need for something like this configurable, as I need to set it to 'None' so that a third party iframe (rocketchat) I am including in my site is able to make an authenticated request to my site from the iframe. However, None only works if the cookie is secure, so I am doing:

    if ($request->isSecure()) {
      $options['cookie_samesite'] = 'None';
    }

on an overridden SessionConfigurator service.

Enhancing cookie policy settings is a request by every security firm that we meet on projects, so I am changing the priority of this task.

Furthermore, is there a valid reason for not setting this cookie attribute to the default secure option of "Strict"

There is, like it was mentioned earlier also. If you set Strict then one-time login links stop working from web based email clients hosted on different domain than the site. I had to fix this problem by changing Strict to Lax months ago. This is the comment that I left in my code, I hope it helps.

    // When a user opens a one-time login link Drupal does a redirection, it
    // redirects the user from the user.reset route (that contains a unique
    // hash and a timestamp) to the user.reset.form route. Before the
    // redirection the controller saves the hash and the timestamp to session,
    // this is how it passes the data to the user.reset.form which actually
    // using them. Using session as an intermediate storage causing this
    // access denied issue when "Strict" cookie policy because the email
    // client website is considered a "third party website" and the cookie
    // that would store the initial session id (created by user.reset) gets
    // blocked. (Unless the web based email client is running on a same parent
    // domain as the Drupal site (ex. http://mail.foo.bar and
    // http://site.foo.bar))
    // In practice it means:
    // * user.reset saves the data to a newly initiated session
    // * the cookie with the session id gets rejected
    // * user.reset.form tries to retrieve the data from the session but it
    // gets a new, empty session because the previous one (even if we are in the
    // same page request) cannot be retrieved because its id is missing from the
    // cookie storage.
    // @see \Drupal\user\Controller\UserController::resetPass()
    // @see \Drupal\user\Controller\UserController::getResetPassForm()
    // @see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/SameSite

Related to cookie policies, I also have an in-review patch for SimpleSAML PHP module because it also conflicted with the enhanced cookie policy settings.

Since my previous comment, I also found a core related bug which could also block resolving this issue, see #3187037: Lost Symfony features cased by missing parent calls in SessionManager

It would be nice to be able to change the setting with a service parameter.

Now getting a warning in Firefox:

Cookie “SESS3e09ff6d94c856f40d453871b91a99ad” will be soon rejected because it has the “SameSite” attribute set to “None” or an invalid value, without the “secure” attribute. To know more about the “SameSite“ attribute, read https://developer.mozilla.org/docs/Web/HTTP/Headers/Set-Cookie/SameSite

Bumping this to critical because of that.

Drupal 7 just shipped this 😖

This is #13 with the code style adjusted and spellcheck exceptions.

The Drupal 7 version makes this configurable in settings.php and documents it there.

@alexpott: Did you set the samesite option to 'None' in the container (as in services.yml) when you tested?

Document in default.services.yml. Technically, defaulting to 'Lax' is a BC-break. AFAIK the D7 patch defaults to 'None'.

@cilefen nope - wasn’t using the patch. My comment was about a warning you get with a Drupal site on Firefox without this patch and justifying why it should be critical.

Apologies: I misread that comment obviously. It makes more sense to me now.

Update the scaffold file.

Add test coverage to SessionConfiguration and use cookie_samesite throughout for consistency.

This currently looks a lot different from D7's implementation, mostly because D9 uses the Symfony session component.

1. The container config value of cookie_samesite will always override PHP's value. This has the interesting BC possibility if that a site has set the value to 'Strict' then Drupal knocks it down to 'Lax' (or worse, 'None', see below) unless the site owner configures settings.yml too. We should think about checking the INI value in SessionConfiguration to see if it the value is 'Strict' before setting the default of 'Lax'.
2. This expression may be less expensive in a different order: !$request->isSecure() && isset($options['cookie_samesite']) && $options['cookie_samesite'] == 'None'
3. Defaulting to 'Lax' is different than D7 (which goes with 'None'). 'Lax' is the right way to go in principle because most up-to-date browsers consider the value to be 'Lax' if no value is set. But there exists a possibility this would break something. No properly-designed web application should require 'None' in my opinion, but there it is.

This emulates mimics the Drupal 7 behavior, by letting the container value then an INI value take precedence, and falling back to to 'None' or NULL (no value).

@cilefen nice to see green test runs on this issue.

I think we need at least new sites to be built with lax as the default. Changing default.services.yml is fine because it only affects new installations. For existing installations they will need update their services.yml file and we need to add something to system_requirements() telling people to update it and that none - the current value is not recommended.

+++ b/core/lib/Drupal/Core/Session/SessionConfiguration.php
@@ -50,12 +50,36 @@ public function getOptions(Request $request) {
+    $override = $this->options['cookie_samesite'] ?? NULL;
+    if ($override !== NULL) {
+      return $override;
+    }
+    $ini_options = session_get_cookie_params();
+    if (!empty($ini_options['samesite'])) {
+      return $ini_options['samesite'];
+    }
+    return !$request->isSecure() ? NULL : 'None';

I'm not sure that this fallback is relevant for D8 / D9. If you've set samesite in php.ini we're going to ignore at the moment anyway. Feels like additional complexity for not much gain.

FWIW D8 / D9 already completely ignore php.ini session.cookie_* variables or we overwrite them as far as I can see. For example, we overwrite the lifetime in \Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage::regenerate() and httponly in \Drupal\Core\DrupalKernel::bootEnvironment()

@alexpott Thank you for the feedback. Just so I'm clear, are you advocating to remove the INI config fallback and !$request->isSecure() ? NULL : 'None'; or only the former (INI)?

Haven't read everything here just yet but quickly responding to #31 : do note that right now, D8/D9 are honoring the ini_set('session.cookie_samesite') and that's been the recommended workaround in a bunch of issues for example in the commerce ecosystem, see for example https://www.drupal.org/project/commerce/issues/3051241 . I'm pretty sure there's a bunch of sites out there in PHP 7.3 setting it that way.

@DeFr Noted. I'm leaning toward simply not modifying SessionConfiguration::getOptions which should keep the current behavior but taking the Drupal-configured value only if one exists.

@DeFr thanks for that - going to try to work out how that is working and document here.

@alexpott

It looks like NativeSessionHandlerNativeSessionStorage does an ini_set if it encounters an allowed session config in the config that is passed to it. So otherwise the system value prevails.

Make the implementation simple and direct.

Sketch out the system requirements.

Constrain the requirements phases. I am not sure if these are correct.

Update to also set the samesite attribute when destroying a session cookie. Browser fails to remove the cookie across CORS if this isn't set.

There are other cookie settings listed in NativeSessionStorage::setOptions we could simply pass through if we were to document them in default.settings.yml.

Thus, #3248319: Add cookie_path for session cookies.

Re: #29, I am working on a progressively decoupled site that also leverages OAuth and Lax is the "most" you can do if you need attributes in the session loaded when users are redirected back to the site, e.g. during OAuth2.

I agree also that this is a great chance to defer as much as we can to the Symfony session handler and that this might be more an inline documentation item than anything.

Rerolled the patch for the 9.3.x version.

EDIT: Fixed patch below. Removed changes to sites/default/default.services.yml because patch was not applying with it. I don't know how to make changes outside core folder in case of core patch?

Fixed CS errors.

I am getting a failure when patching via composer update so I manually grabbed it and patched by hand and got the following:

patching file core/assets/scaffold/files/default.services.yml
patching file core/lib/Drupal/Core/Session/SessionConfiguration.php
patching file core/lib/Drupal/Core/Session/SessionManager.php
patching file core/misc/cspell/dictionary.txt
Hunk #1 FAILED at 1183.
1 out of 1 hunk FAILED -- saving rejects to file core/misc/cspell/dictionary.txt.rej
patching file core/tests/Drupal/Tests/Core/Session/SessionConfigurationTest.php

-> more core/misc/cspell/dictionary.txt.rej
--- core/misc/cspell/dictionary.txt
+++ core/misc/cspell/dictionary.txt
@@ -1183,6 +1183,7 @@ safa
sameline
samename
sameorigin
+samesite
sata
savepoints
sayre

Suggestions?

Do a formal reroll, or, apply the patch onto a clean checkout of Drupal 9.4.x, manually update that dictionary file as appropriate and create a new patch.

+++ b/core/tests/Drupal/Tests/Core/Session/SessionConfigurationTest.php
@@ -116,6 +116,17 @@ public function testCookieSecure($uri, $expected_secure) {
+  /**
+  * Test that session.cookie_samesite is configured correctly.
+  */

This needs fixing to comply with coding standards i.e.

  /**
   * Test that session.cookie_samesite is configured correctly.
   */

Updated patch, added description

About #51:

+++ b/core/modules/system/system.install
@@ -314,6 +314,25 @@ function system_requirements($phase) {
+      'description' => t('This attribute should be explicitly set to Lax or Strict. See <a href="https://www.php.net/manual/en/session.configuration.php#ini.session.cookie-samesite" target="_blank">PHP documentation</a>'),

Just a nitpick here, could you create a placeholder for the URL? something like:
'description' => t('This attribute should be explicitly set to Lax or Strict. See <a href=":url" target="_blank">PHP documentation</a>', [':url' => 'https://www.php.net/manual/en/session.configuration.php#ini.session.cookie-samesite']),
I think it's better when we read the code.

+++ b/core/modules/system/system.install
@@ -314,6 +314,25 @@ function system_requirements($phase) {
+    else {

I think we could avoid this else statement here, maybe doing something like:

$requirements['php_session']['severity'] = REQUIREMENT_OK;
if (!in_array($cookie, ['Lax', 'Strict'])) {
  $cookie = t('invalid');
  $requirements['php_session']['severity'] = REQUIREMENT_WARNING;
}

Both comments are just suggestions, and I'm ok to move on without changing anything. I'll move this back to needs work to see what you think.

+++ b/core/modules/system/system.install
@@ -314,6 +314,25 @@ function system_requirements($phase) {
+    if (in_array($cookie, ['Lax', 'Strict'])) {

For good practice let's add the $strict argument to in_array(). We really should have a PHPCS rule about this.

+++ b/core/modules/system/system.install
@@ -314,6 +314,25 @@ function system_requirements($phase) {
+  if ($phase == 'runtime' && PHP_SAPI != 'cli') {
+    // Test the session configuration.
+    $cookie = ini_get('session.cookie_samesite');
+    $requirements['php_session'] = [
+      'title' => t('Session'),
+      'description' => t('This attribute should be explicitly set to Lax or Strict. See <a href="https://www.php.net/manual/en/session.configuration.php#ini.session.cookie-samesite" target="_blank">PHP documentation</a>'),
+    ];
+    if (in_array($cookie, ['Lax', 'Strict'])) {
+      $requirements['php_session']['severity'] = REQUIREMENT_OK;
+    }
+    else {
+      $cookie = t('invalid');
+      $requirements['php_session']['severity'] = REQUIREMENT_WARNING;
+    }
+    $requirements['php_session']['value'] = t('SameSite cookie attribute: %samesite', [
+      '%samesite' => $cookie,
+    ]);
+  }

I think this should be:

  if ($phase == 'runtime' && PHP_SAPI != 'cli') {
    // Test the session configuration.
    $cookie = ini_get('session.cookie_samesite') ?: t('Not set');
    $requirements['php_session'] = [
      'title' => t('SameSite cookie attribute'),
      'value' => $cookie,
      'severity' => in_array($cookie, ['Lax', 'Strict'], TRUE) ? REQUIREMENT_OK : REQUIREMENT_WARNING,
      'description' => t('This attribute should be explicitly set to Lax or Strict. See <a href="https://www.php.net/manual/en/session.configuration.php#ini.session.cookie-samesite" target="_blank">PHP documentation</a>'),
    ];
  }

I don't understand why we would replace the invalid value with the word invalid. That seems to be losing helpful information.

The reason is that default value for the most distros is empty string, which is useless to display

For good practice let's add the $strict argument to in_array(). We really should have a PHPCS rule about this.

That's a good idea, it would be nice to have an issue upstream to address this.

Just applying the suggestion here.

I've missed your comment @andypost, do you think Not set, is enough?

Forgot to change the title, here's a new patch. I'll move this to NR to see what you think.

Thank you, now only change record required to rtbc

Created the CR, moving back to needs review!

I think you can set the configuration value to 'None' also, and there are reasons for having to use None.

@cilefen, I think I've missed that part, the patch has some documentation mentioning "None", but reading #14, Firefox is aparently warning about "None", I think we need to change the documentation here right?

My bad here, actually it's fine to have "None" as cookie.samesite value, but we need to guarantee "Secure" attribute when the samesite is set to "None".

None gives a warning *if* the cookie is not marked as secure: "without the “secure” attribute". It's ok to set it to None and there are use case that break with Lax, typical examples include POST submissions from payment gateways back to your site. And Strict is in fact going to break _many_ use cases, including all offsite payment gateways that redirect back as well as any direct link to the site from elsewhere, like e-mails. Users will not be logged in on that first request.

IMHO, this should not add a requirement warning.

+++ b/core/assets/scaffold/files/default.services.yml
@@ -37,6 +37,12 @@ parameters:
+    # this value will override the server value. See
+    # https://www.php.net/manual/en/session.security.ini.php for more
+    # information.

This should use @see, https://www.drupal.org/docs/develop/standards/api-documentation-and-comm.... There is another comment in the patch that needs to be changed too.

@see should only be used on standalone docblock definitions, if it's parts of a sentence then "see" is correct, see also the example just below.

I agree with @Berdir None + Secure should not produce a warning either. I think given that browsers are defaulting to Lax for cookies that don't set SameSite setting our default to Lax makes sense. Unfortunately this will make the requirement logic more complex again.

Thank you for your explanation @berdir!

This patch just adds the "None" to avoid the warning, I thought about making a validation when the "none" is set and the "secure" attribute is not set, but AFAIK, drupal does not allow "session.cookie_secure" setting to be override. So I think the secure will always be set right?

@murilohp - it's not that simple - here's what Drupal does for the secure cookie setting...

    // If the site is accessed via SSL, ensure that the session cookie is
    // issued with the secure flag.
    $options['cookie_secure'] = $request->isSecure();

Therefore None will only work if you're forcing your site to use https. If you allow non-https traffic then you'll need to set SameSite to Lax. I think maybe something like:

  if ($phase == 'runtime' && PHP_SAPI != 'cli') {
    // Test the session configuration.
    $samesite = ini_get('session.cookie_samesite') ?: t('Not set');
    
    $requirements['php_session'] = [
      'title' => t('SameSite cookie attribute'),
      'value' => $samesite,
      'severity' => in_array($samesite, ['Lax', 'Strict'], TRUE) || ($samesite === 'None' && $request_object->isSecure()) ? REQUIREMENT_OK : REQUIREMENT_WARNING,
      'description' => t('This attribute should be explicitly set to Lax, Strict or None. If set to None then the request must be made via HTTPS. See <a href=":url" target="_blank">PHP documentation</a>', [
        ':url' => 'https://www.php.net/manual/en/session.configuration.php#ini.session.cookie-samesite',
      ]),
    ];
  }

I'm not sure about the

      'severity' => in_array($samesite, ['Lax', 'Strict'], TRUE) || ($samesite === 'None' && $request_object->isSecure()) ? REQUIREMENT_OK : REQUIREMENT_WARNING,

To me it feels just on the limits of okay for complexity / length.

Checking the request object in hook requirements is not going to work when these requirements are checked with drush/CLI. That will mess with https://www.drupal.org/project/monitoring for example.

I'd expect that in 2022, non-secure will mostly just be relevant for development and test sites, but maybe I'm wrong, not sure.

1. Applied the suggestion from #3150614-68: Set SameSite on session cookies to also report a warning if SameSite is None on a non-secure connection. I split the long expression over 2 lines to make it more readable.
2. #3150614-69: Set SameSite on session cookies makes sense, and the PHP_SAPI !== 'cli' check prevents this from throwing false positives on the command line. I added a short explanation so this is documented.

Here's a version of the patch from #71 compatible with Drupal 9.3.

The other cookie configuration values have defaults documented with @default. I guess we should do that here too? The default for SameSite is no value. How would we indicate that. Is it "null"? It can't be "None" because that is actually a specific SameSite option.

An example of the @default documentation:

    # Set the session ID string length. The length can be between 22 to 256. The
    # PHP recommended value is 48. See
    # https://www.php.net/manual/session.security.ini.php for more information.
    # This value should be kept in sync with
    # \Drupal\Core\Session\SessionConfiguration::__construct()
    # @default 48
    sid_length: 48

"Undefined" or "not set"?

"Undefined" in this context seems to imply something random could happen. I would prefer "not set" or "no value".

Fixed coding standards and remarks from comments #77-80 in the MR. Here is a backport of the latest iteration to D9.5.

Test out the patch from #81. it help out the SameSite cookie problem for me. +1 for RTBC

Last test failure was unrelated. Rebased on latest 10.1.x.

Unfortunately, #81 does not apply to Drupal 9.5.3.

Nitpick. This issue is for Drupal 10 which means we can use PHP 8.1 syntax here.

- $valid = in_array($samesite, ['Lax', 'Strict'], TRUE) || ($samesite === 'None' && $request_object->isSecure());
+ $valid = match ($samesite) {'Lax', 'Strict' => TRUE, 'None' =>  $request_object->isSecure(), default => FALSE};

The latter could be split into multiple lines.

• Addressed remark from #85, yay for match expressions!
• The requirement check was accidentally nested inside the PHP version check, fixed that.
• Updated documentation.
• Attaching new D9.5.x patch to address #84

Thanks!

OK the test coverage looks neat and the docs are fine. Looks like a lot of browsers, if not all, are defaulting to 'Lax' rather than none, so this shouldn't be disruptive, but better to get this into 10.1.x early.

I'm slightly weary about offering 'Strict' given Berdir's comments that it would break everything, but... maybe there's a use case and we probably want to allow configuration between 'Lax' and 'None' anyway.

I added a release note since this changes scaffold files.

The change record looks incorrect.

I made some edits to https://www.drupal.org/node/3275352. Have a look.

I think the change record was correct before. Commit a3bddae4 doesn't change the default - neither for existing nor for new sites. In order to change the default value for the session.cookie_samesite, it is necessary to set the container parameter in core/core.services.yml. The commit only touches sites/default/default.services.yml.

The following change is necessary in order to influence the default behavior (for new and existing sites):

diff --git a/core/core.services.yml b/core/core.services.yml
index 1c5d787c57..3beafcd932 100644
--- a/core/core.services.yml
+++ b/core/core.services.yml
@@ -9,6 +9,7 @@ parameters:
     gc_divisor: 100
     gc_maxlifetime: 200000
     cookie_lifetime: 2000000
+    cookie_samesite: Lax
     sid_length: 48
     sid_bits_per_character: 6
   twig.config:

I'll reverted the changes but it says things like "Drupal core will begin to warn if the session cookie SameSite is not set to Lax or Strict" which I do not think are true.

Drupal core will begin to warn if the session cookie SameSite is not set to Lax or Strict

That's in fact what happens. php core/scripts/drupal quick-start, then navigate to /admin/reports/status#warning, this is what I get:

I've updated the CR with the correct way to fix the warning.

Again, "Drupal core will begin to warn if the session cookie SameSite is not set to Lax or Strict" is untrue. I've looked at the commit. "None" is an allowed value under one condition.

@znerol - it's a bit trickier than that... completely new sites where sites/default/services.yml gets created from sites/default/default.services.yml will have it set to Lax and will not have a warning.

I do think we need a quick follow-up though. I think we should add cookie_samesite: Lax to core.services.yml in it's session.storage.options so that quickstart sites have the correct default.

@znerol - it's a bit trickier than that... completely new sites where sites/default/services.yml gets created from sites/default/default.services.yml will have it set to Lax and will not have a warning.

I tried the web installer as well as the quick-start script. Neither of them copies default.services.yml to services.yml. Both of them will copy default.settings.php to settings.php though. So the install behavior is different for services.yml and settings.php. I wonder whether we should fix that inconsistency.

So the install behavior is different for services.yml and settings.php.

It looks like this has a purpose, see #2547447: Stop creating services.yml by default.

Let's do #97 here, I was also under the mistaken impression that default.services.yml kicks in for newly installed sites without manual copying.

Reverted so all the changes can go back in togher.

I added the #97's suggestion, please review.

#2547447: Stop creating services.yml by default makes this really tricky. Because it means sites don't have a sites/default/services.yml - that means that the whole "existing sites can stay as they are and new sites get the new value" assumption won't work.

That said, I think the current approach is fine given https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/Sam... - it says

Cookies without SameSite default to SameSite=Lax

so defaulting to Lax makes sense as that is what most modern browsers are doing anyway.

I think the CR will need to document how to unset it in case someone is relying on the current behaviour and we need to call this out in the release notes.

Thanks @rpayanm, the SameSite attribute is now present on the session cookie for all sites. Going to revert the CR to @cilefen version and add instructions on how to modify the default.

Committed/pushed to 10.1.x (again), thanks!

Thank you, published CR

This is marked for change record updates and I think that happened because znerol restored the CR to a previous version. Removing tag.

Rerolled patch against 9.4

Rerolled patch against 9.4.10

Previous patch for 9.4 had patch for sites/default/default.services.yml which is scaffolded