Problem/Motivation

It looks like Symfony sets SameSite=Lax by default on cookies. For Drupal core, it's possible to set cookie_samesite: 'Strict' in your services.yml and session cookies are set with SameSite=Strict as expected. This module doesn't seem to set this value at all and so Persistent Login cookies get set with SameSite=Lax

More info on the attribute:

Steps to reproduce

Set cookie_samesite: 'Strict' in your services.yml; log in with the "Remember me" checkbox checked; look at cookies; session cookie should SameSite=Strict but Persistent Login cookie will be SameSite=Lax

Proposed resolution

Allow setting the SameSite attribute somehow, either directly in this module's configuration, or fetched from the container parameters if the value exists.

Remaining tasks

See above.

User interface changes

Either none or an additional field on the configuration form.

API changes

None?

Data model changes

Does additional config count?

Comments

Ambient.Impact created an issue. See original summary.

ambient.impact’s picture

Issue summary: View changes

Fixed typo.

gapple’s picture

PL uses config from core's session management as much as possible, so I think it should just use cookie_samesite. I don't expect there's a reason for a site to want different settings for session and PL cookies.

ambient.impact’s picture

That makes sense, yeah. In this case it's more that it doesn't seem to even be doing that, in that the cookie is set with Lax even though my container parameters are:

[
  "gc_probability" => 1
  "gc_divisor" => 100
  "gc_maxlifetime" => 200000
  "cookie_lifetime" => 0
  "cookie_domain" => ""
  "cookie_samesite" => "Strict"
  "sid_length" => 48
  "sid_bits_per_character" => 6
]

As far as I can tell, it looks like \Symfony\Component\HttpFoundation\Cookie::create() has the parameter $sameSite = self::SAMESITE_LAX, and you explicitly create an instance in \Drupal\persistent_login\EventSubscriber\TokenHandler::setTokenOnResponseEvent() like so:

      $response->headers->setCookie(
        Cookie::create(
          $this->cookieHelper->getCookieName($request),
          $this->token,
          $this->token->getExpiry(),
          '/', // @todo Path should probably match the base path.
          $sessionOptions['cookie_domain'],
          $sessionOptions['cookie_secure']
        )
      );

...which I'm assuming defaults to $sameSite = self::SAMESITE_LAX because you didn't provide the $sameSite parameter and it's using the default value hard coded into that Symfony class rather than automagically using the container parameters.

The quick fix would be to just get the cookie_samesite container parameter and pass it explicitly to \Symfony\Component\HttpFoundation\Cookie::create().

I could throw together a test to demonstrate this if you'd like. Either it turns out I'm doing something dumb (entirely possible), or this is a legit issue with this module that's flown under the radar because most people haven't tried to harden cookies with these attributes.

ambient.impact’s picture

Had this rattling around in my brain the other day and realized a better way to summarize what's probably going on here: the container session parameters very likely only apply to the cookie that identifies the session to Symfony/Drupal, not any other cookies; cookies that are set to expire with the session aren't the same thing, and Symfony doesn't know you want the same behaviour as the session identifier cookie because as far as it knows, you're setting a cookie that has little to do with the session itself.

  • gapple committed eefd74dd on 2.x
    Issue #3363233: Use all session.storage.options service parameters if...
gapple’s picture

Status: Active » Fixed
Related issues: +#3150614: Set SameSite on session cookies

This allowed me to figure out setting the cookie path consistently with the session cookie too 😀.
I don't expect anyone would want to change the httponly attribute, but that will now follow the service parameter as well if it's changed.

ambient.impact’s picture

Heck yeah. Glad to be of help!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.