Password reset links break when SameSite Strict is applied to sessions

[Cheddam]

Module version(s) affected

4.12 and later

Description

In Silverstripe CMS 4.12, support for the SameSite attribute on cookies was introduced. This defaults to Lax, but the related documentation suggests setting this to Strict on both standard and session cookies, which in principal is a sensible recommendation.

However, this mode has caveats. In particular, opening a URL from an external site that immediately performs an internal redirect results in that subsequent request also being treated as external, preventing cookies from being transmitted. E.g. when clicking a link on external.website that goes to my.website/redirect, which returns a 302 and redirects to my.website/goal, both of the my.website requests will be treated as external and not send cookies, so any cookies sent in the my.website/redirect request are dropped.

This has caused issues with multiple SSO modules:

• Signing into RealMe loses session data in SilverStripe silverstripe-realme#165
• Session-cookie with samesite = strict breaks login-procedure silverstripe-saml#45

It turns out that password reset URLs also break in this mode, as the password reset route shifts the token into the session and performs a redirect to itself sans-token, expecting to access it via the session in the next request. Users can work around this by copying the link and pasting it into their browser instead of clicking it, but this is obviously not ideal.

How to reproduce

Reproduction steps

Enable Strict mode on session cookies via config:

SilverStripe\Control\Session:
  cookie_samesite: 'Strict'

Generate a password reset email, and attempt to click the link from a browser-based email client. After resolving redirects, the browser will arrive at the login form instead of the password reset form.

(NOTE: If the mail client is on the same domain as the application, this won't trigger the behaviour. Unlikely, but some dev environments may be structured this way.)

Possible Solution

There are a few different paths we could take. I've listed my suggestions in order of personal preference:

1. Stop using session storage for the token

Storing the token in the session isn't the only option for keeping track of it. Laravel, for example, pushes the token into a hidden field in the form so that it can be referenced during submission. This would avoid any visible change in behaviour for users, and I've raised a draft PR that takes this approach.

2. Perform redirect client-side

Rather than performing a 302 immediately, if we instead return a 200 with a document.location call, the browser should treat the redirect as internal and transmit the session cookie. The only downside here is that the redirect will be slightly slower.

3. Enforce Lax mode on password reset route

Since we know this only impacts a specific route, we could downgrade the configured mode for this specific response. This would be transparent to end-users, but could be confusing for developers, and reduces the security of the application - so it's probably not the best way forward.

Additional Context

Relevant logic lives in ChangePasswordHandler::changepassword().

Validations

• Check that there isn't already an issue that reports the same bug
• Double check that your reproduction steps work in a fresh installation of silverstripe/installer (with any code examples you've provided)

[GuySartorelli]

All three of the suggested solutions solve the narrow-scope problem of the password reset link - but they don't resolve this problem for any other redirects.

Is this a known issue with redirects like this in general? Is there a header we can add to the redirect response which would allow this to work as expected in the general case?

[Cheddam]

You're right - after further testing, I found that any externally-hosted link to a protected route can trigger a logout (since Silverstripe tries to authorise the request, fails because the session cookie isn't present, and triggers a 302 to the Login form, invoking the same issue.)

To the best of my understanding, this isn't solvable with headers - instead, we'd need to introduce a global mechanism along the lines of Option 2, where we resolve the request as a 200 and redirect via JS. The related MDN article touches on this too.

The reset password mechanism is most in need of adjustment, but honestly a simpler 'solve' might just be adjusting the documentation to acknowledge the problem; For the sites I'm supporting, we've opted to abandon usage of Strict entirely.

[GuySartorelli]

To the best of my understanding, this isn't solvable with headers - instead, we'd need to introduce a global mechanism along the lines of Option 2, where we resolve the request as a 200 and redirect via JS.

We could maybe do this in a few select places that are already controlled primarily by core/supported code (e.g. login forms or the CMS) - but we should avoid injecting JS into the main project front-end.

The reset password mechanism is most in need of adjustment, but honestly a simpler 'solve' might just be adjusting the documentation to acknowledge the problem; For the sites I'm supporting, we've opted to abandon usage of Strict entirely.

It might be worth doing both - if option 1 is sufficiently secure and robust (and the fact that a widely used framework uses this approach suggests it probably is) then we should probably implement that.

At the same time it would be good to update the docs to point out that this problem may exist in other scenarios when strict is used.