FIX Don't set session value in endpoint accessed from mail link.

[GuySartorelli]

With the session cookie's "SameSite" value set to "Strict", we can't set session values when a request is the result of clicking a link from another website.
When a user clicks the reset password link from an email within a web client (e.g. Gmail), this results in not being able to reset their password.

This change sets the session value after the redirect, while avoiding putting the reset token into the browser history which could allow a bad actor to access it.

This change also allows that flow to work for silverstripe/mfa which needs to alter the second part of the flow, but detects that currently using the session value's presence or absence.

This PR is based off #11668 and #11676 though it does have some differences.

Issue

• Password reset links break when SameSite Strict is applied to sessions #11565

[GuySartorelli]

This condition wasn't part of the original PRs and handles an edge case where only one of ?t or ?m is included in the URL (won't happen normally but can happen if someone copies a URL incorrectly for example)

[GuySartorelli]

This returns a message that used to only be returned in one place - but is now used to give consistent message any time the token is found to be invalid or expired at any point during the process.

[GuySartorelli]

I've opted to keep the existing method name with the same default behaviour it has in CMS 5 so we don't have to deprecate it.

[GuySartorelli]

Unrelated refactoring - happens a few times in this PR.

[GuySartorelli]

Using Controller::join_links() here to catch the (unlikely) edge case where the link already has some query string. join_links() will merge the query strings together so we don't end up with some/path?var=1?th=123

[SimulatedPanda]

I approve this fix

[emteknetnz]

We should not include the 2nd commit, it's an enhancement basically unrelated to the original issue it doesn't belong in this bugfix PR. Also the default lifetime of the token is 2 days, since it's so long it's questionable if it's even worth making a friendly message at all.

Also you'll need to create a changelog PR telling people with custom authenicators to update the 'changepassword' method names

[GuySartorelli]

We should not include the 2nd commit, it's an enhancement basically unrelated to the original issue it doesn't belong in this bugfix PR.

Usually the distinction of enhancement vs bug fix is to determine whether a change should be included in a patch release or a minor release - which doesn't apply here. I'm not sure how that distinction is helpful in this context?

The second commit provides standardised messaging when a reset password token expires. Due to the way it changes the response return flow, I would hesitate to include this change in a minor release when we have the opportunity now to include it in a major release.

The second commit was part of the change I explained to Jenn and was approved, so from a "are we allowed to include it" perspective - yes we are.

Also the default lifetime of the token is 2 days, since it's so long it's questionable if it's even worth making a friendly message at all.

That value is configurable, and we both were surprised by how long it is by default. I wouldn't be surprised to find projects updating it to be only valid for a couple of hours at most. That's plenty of time to step away, have a meeting or go for lunch or any number of things, and then submit the form when you come back.

tl;dr I'm not sure what your reasoning is for not including the second commit. If there's a problem with it that I'm not seeing, I'll be happy to exclude it, but I need to know what that problem is first.

[emteknetnz]

I don't like the 2nd commit for several reasons:

• It's out of scope - the second commit is basically a random "scope creep" enhancement rather than a bugfix for the original issue, it should really be done on a separate card
• Incorrect process - The enhancement is adding API post beta, which we're supposed to avoid doing where possible. Correct process would be to target 6.1
• Makes peer review harder - The enhancement is on PR that affects a critical security flow, I don't want to have to think about it at the same time as a PR that already has a lot of nuance and risk

[GuySartorelli]

It's out of scope - the second commit is basically a random "scope creep" enhancement rather than a bugfix for the original issue, it should really be done on a separate card

I had considered doing it separately, but since I was hesitant to include it in a minor release and it is in-line with the other related changes to the standardisation of the message that I'm including in commit 1, it seemed like a good fit for this PR.

Incorrect process - The enhancement is adding API post beta, which we're supposed to avoid doing where possible. Correct process would be to target 6.1

It was already approved - and like I said, I'd be hesitant to do this in a minor release.

Makes peer review harder - The enhancement is on PR that affects a critical security flow, I don't want to have to think about it at the same time as a PR that already has a lot of nuance and risk

It's in a second commit so you can look at the first commit in isolation from the second commit.
The first commit is 90% your work anyway.

I recommend you look at the first commit, ensure you're happy with that, then look at the (very narrow scope very small) second commit after you're happy with the first one. I think you'll find the overhead that second commit adds is negligible.

It sounds like you don't actually have a problem with the code or the change itself, which should be the primary concern IMO.

[GuySartorelli]

If you still find it hard to review for some reason with two commits, let me know and I will split the second commit off as a separate PR in this issue with the understanding it will be reviewed and merged into 6.0 after this one

[emteknetnz]

Yup 2nd PR would be helpful thanks

Also remember to:

Also you'll need to create a changelog PR telling people with custom authenicators to update the 'changepassword' method names

[GuySartorelli]

Also you'll need to create a changelog PR telling people with custom authenicators to update the 'changepassword' method names

Done, please see link in issue

[GuySartorelli]

Second commit has been separated out into #11689

[emteknetnz]

The ?th hash for the redirect is non-predictable so gets updated multiple times if there are multiple authenticators. Presumably that processChangePasswordUrlVars() gets called multiple times.

Debugging on line 866 of Security::delegateToMultipleHandlers() - you can see there are 2x 302 responses with different th hashes.

I think what's written to the Member table isn't matching the redirect the browser makes. Means that that you end up with a The password reset link is invalid or expired. when you click the email link to reset password

Perhaps use an existing tempory hash on the member table if there is one rather than creating a new one?

# app/_config/myconfig.yml
SilverStripe\Core\Injector\Injector:
  SilverStripe\Security\Security:
    properties:
      Authenticators:
        hello: '%$HelloAuthenticator'

<?php
// app/src/HelloAuthenticator.php
use SilverStripe\Security\MemberAuthenticator\MemberAuthenticator;

class HelloAuthenticator extends MemberAuthenticator
{
    public function getChangePasswordHandler($link)
    {
        return HelloChangePasswordHandler::create($link, $this);
    }
}

<?php
// app/src/HelloChangePasswordHandler.php
use SilverStripe\Control\HTTPResponse;
use SilverStripe\Security\MemberAuthenticator\ChangePasswordHandler;

class HelloChangePasswordHandler extends ChangePasswordHandler
{
    private static $allowed_actions = [
        'changepassword',
    ];

    protected function createChangePasswordResponse(): array|HTTPResponse
    {
        return new HTTPResponse('Hello from HelloChangePasswordHandler');
    }
}

[GuySartorelli]

Can you please write some step-by-step instructions to get into that many-redirects state? I don't quite understand what you mean so being able to reproduce it myself will help a lot.

[emteknetnz]

1. Add the code sample above to a project and flush
2. Create a new user in the CMS [email protected] and give them to a user group
3. In a browser b, not logged in, click "I forgot password link" for [email protected]
4. In browser b, go to reset password link sent in email

[GuySartorelli]

Good catch! I've updated to reuse the existing temp hash if multiple authenticators try to set it in the same request.

[GuySartorelli]

Config class not used

[GuySartorelli]

CI failures are unrelated and will be fixed in silverstripe/.github#389

[emteknetnz]

Latest changes do not appear to work using the HelloAuthenticator code supplied, tried on a user with an MFA enabled and one without, same result, get redirected to the ?th url and I get a message that the token is invalid

I've debugged the reason to here ChangePasswordHandler::processChangePasswordUrlVars():

        $tempToken = $request->getVar('th');
        if ($tempToken !== null) {
            $member = Member::get()->find('AutoLoginTempHash', $tempToken);
            if (!$member) {
                return $this->getInvalidTokenResponse(); // <<<<<<< 2nd time
            }
            // Delete the temp token from the member so that it cannot be used again
            // This prevents the browser history from being used to access the change password form
            $member->AutoLoginTempHash = ''; // <<<<<<< 1st time
            $member->write();
            // ...

What's happening the ?th code blocks works correctly the first time and sets AutoLoginTempHash to blank string. However the 2nd time around (because of the 2nd authenticator) it hits this code again and it cannot find a corresponding member

[GuySartorelli]

Can you please show me a video (or in person) what that looks like from a user perspective? It worked for me locally but I may be doing something slightly different to what you're doing.

[emteknetnz]

I don't think we need a video - it's exactly same flow as the replication steps, I followed those steps and the invalid token method was triggered i.e. the user is not shown the new + confirm password fields

• If you try debugging the code block above when you have two authenticators does that code block get hit twice?
• On the second time through there in no Member with a AutoLoginTempHash?
• Are you using the exact same PHP classes foe the custom authenticator or you using something slightly different? (as some point I changed my own HelloChangePasswordHandler::createChangePasswordResponse() to return an empty array instead of HTTPResponse)

<?php

use SilverStripe\Control\HTTPResponse;
use SilverStripe\Security\MemberAuthenticator\ChangePasswordHandler;

class HelloChangePasswordHandler extends ChangePasswordHandler
{
    private static $allowed_actions = [
        'changepassword',
    ];

    protected function createChangePasswordResponse(): array|HTTPResponse
    {
        // return new HTTPResponse('Hello from HelloChangePasswordHandler');
        return [];
    }
}

[GuySartorelli]

That change to the HelloChangePasswordHandler is what was different between my test and yours.

What was happening before:

I had removed the implementation of HelloCahngePasswordHandler::createChangePasswordResponse(), so it was just using the implementation in ChangePasswordHandler::createChangePasswordResponse().

1. Security::delegateToMultipleHandlers() passed the request to both authenticators
2. HelloChangePasswordHandler::createChangePasswordResponse() returned an HTTPResponse after successfully setting the tokens correctly. The response included the form.
3. ChangePasswordHandler::createChangePasswordResponse() returned an array after being unsuccessful in setting the tokens.
4. Security::delegateToMultipleHandlers() passed those two returned values into Security::aggregateAuthenticatorResponses(), which returned the HTTPResponse`.

Now they're both arrays.
The first array doesn't contain a 'Content' or a 'Form' key, it Security assumes it failed to deal with the tokens (or rather that the tokens weren't relevant for it). It moves on to see if the next authenticator has anything for it. The next one has a 'Content' key with the failure message.

The problem is that the implementation in HelloChangePasswordHandler::createChangePasswordResponse() is invalid.

Potential solutions

1. No further changes. The way multiple authenticators are handled in series is weird, but not in scope for this issue. A valid implementation of createChangePasswordResponse() will return the form when $this->getRequest()->getSession()->get('AutoLoginHash') is a non-null valid hash.
2. Add another private static property to retain the temp hash and ID of the member for the duration of this session, so that even after AutoLoginTempHash has been cleared for the member, we still know it was a valid hash when the request began.

I lean towards 1, mostly because this happening is very edge-casey but partly because I think any valid implementation won't hit this problem.

That said, I don't feel at all strongly about it, so defer to you to decide which of these (or something else I didn't think of) to do.

[emteknetnz]

Add the following code to ChangePasswordHandler instead, have tested it works locally no matter what nonsense is returned from the custom password handler

    /**
     * Keep track of whether a temporary hash is already processed during this request cycle.
     * @internal
     */
    private static bool $tempHashAlreadyProcessed = false;
    
    // processChangePasswordUrlVars()
    // ...
        $tempToken = $request->getVar('th');
        if ($tempToken !== null && !ChangePasswordHandler::$tempHashAlreadyProcessed) { // <<<< update this
            // ...
            $this->setSessionToken($member, $encryptedToken, true);
            ChangePasswordHandler::$tempHashAlreadyProcessed = true; // <<<< add this
        }

[GuySartorelli]

Modified option 2 it is 👍

[GuySartorelli]

Rebased on 6.0 to clear out those unrelated CI failures

[GuySartorelli]

Added new behaviour to ignore invalid handler results if there are valid results from another handler, including test.

[GuySartorelli]

I would have checked for the presence of the form, except there isn't one due to silverstripe/startup-theme#17

Not sure how other tests in here that rely on the form are passing 😅

[GuySartorelli]

Updated PHPDocs in this class match return type hint for delegateToMultipleHandlers() which is the value all of these methods return.

[GuySartorelli]

This and delegateToMultipleHandlers() explicitly cannot return an array, because they transform arrays with a call to renderWrappedController()