Setting of state_handler in Auth0Service causes "Invalid state" error

[iAmRoland]

Description

Upgraded from 5.2.0 to 5.3.0 and ran into this issue.

When I attempt to login I get a "Invalid state" error. Went through multiple threads and couple issues here and on other repos without success.

After some time of debugging I found the following line to be a issue:

laravel-auth0/src/Auth0/Login/Auth0Service.php

Line 55 in a008725

$auth0Config['state_handler'] = $sessionStateHandler;

Commenting it out made the login work again, but editing vendor files is no fix.

Attempted to find out why. Went into the SDK and dumped out the state, the store does not seem to contain anything. The state variable does never seem to get set. So the validate method returns false all the time. Maybe i'm incorrectly understanding how this should work.

Also is it supposed to set the state handler even if I have state_handler set to false in my config?
Or is that config meant only for the SDK?

Reproduction

This might be specific to something in my project, a bit unsure still.

I'm using the database connection in Auth0, logging in with username and password.

My setup looks pretty much like this guide, with custom user handling:
https://auth0.com/docs/quickstart/webapp/laravel#integrate-auth0-in-your-application

Only differences are the login and logout methods.
On login i'm simply checking if user is logged in and then returning a login view if they're not. On that view I have Lock.js setup and configured.

Maybe a relevant section from that configuration:

auth: {
    redirectUrl: '{{ $auth0Config["redirect_uri"] }}',
    responseType: 'code',
    params: {
        scope: 'openid profile name email'
    }
}

Environment

• Version of this library used: 5.3.0
• Version of the platform or framework used, if applicable: Laravel 5.8 and PHP 7.2
• Other modules/plugins/libraries that might be involved: Using the latest SDK

[joshcanhelp]

@iAmRoland - Apologies for the delay in getting back to you. We have a few reports of state handling issues in this library, possibly because of the previous release, but I have not been able to reproduce anything so far. We're adjusting how state is handled and stored in the SDK in an upcoming major release but happy to do a patch release if we figure out that the source of this issue is indeed in the SDK or this package.

I would say that Lock.js will complicate things and, in general, we recommend using Universal Login (hosted page) rather than the embedded one.

So, the 5.3.0 was meant to address and fix issues with state handling, mainly due to misuse of the Laravel session handing. Commenting the line out you mentioned will default the SDK to use "regular" session handling for state, which will work for many cases but not all.

In short, state is:

1. Generated when the login link is generated
2. Stored in a session store (by default)
3. Sent to Auth0 when logging in
4. Returned unchanged to your application's callback URL in a query param
5. Checked against the stored value
6. Deleted after the check, valid or not

... so if you're checking what's getting sent and what's stored, you'll want to:

1. Check both the auth URL and the session store here to make sure both have state values that match
2. Check both the URL parameters and the session store here to makes sure they are the same

Auth0 Application and Connection settings on the dashboard won't have any effect on state checking, it's all handled in the application code itself.

[mraypold]

I'm encountering similar issues when state_handler is set to false upon upgrading from 5.1 to 5.3

I've dug through some code and believe its related to this pull request. #135

If I manually set the constructor in Auth0Service to use the DummyStateHandler instead of the SessionStateHandler then everything works as expected. I believe this is how the library used to handle the scenario when state_handler was false.

I've been unsuccessful in using the Laravel IoC to construct Auth0Service with a DummyStateHandler. I believe this is because the constructor is expecting the SessionStateHandler class instead of the StateHandler interface.

The root of the issue appears to be here.

    /**
     * Auth0Service constructor.
     *
     * @param array $auth0Config
     * @param StoreInterface $sessionStorage
     *
     * @throws \Auth0\SDK\Exception\CoreException
     */
    public function __construct(
        array $auth0Config = null,
        StoreInterface $sessionStorage = null,
        SessionStateHandler $sessionStateHandler = null
    )
    {
        // Backwards compatible fallbacks
        if (!$auth0Config instanceof Repository && !is_array($auth0Config)) {
            $auth0Config = config('laravel-auth0');
        }
        if (!$sessionStorage instanceof StoreInterface) {
            $sessionStorage = new LaravelSessionStore();
        }
        if (!$sessionStateHandler instanceof SessionStateHandler) {
            $sessionStateHandler = new DummyStateHandler($sessionStorage);
        }

        $auth0Config['store'] = $sessionStorage;
        $auth0Config['state_handler'] = $sessionStateHandler;
        $this->auth0 = new Auth0($auth0Config);
    }

Apologies if this isn't clear. I'll try and clarify further if you have questions.

[joshcanhelp]

@mraypold - Appreciate the detailed report here. You're correct about the instance checking here, that should be StateHandler, not SessionStateHandler. Apologies for the miss there.

But, more critically, we went from accepting a store and state_handler from the configuration to always passing one in LoginServiceProvider->register(). Regardless of the instance check, those will override anything that is in config. Apologies for the even bigger miss there, it didn't occur to me that anyone would be passing along the store and handler that way (though perfectly acceptable).

This definitely needs a patch release. Can you take a look at the diff below and see if this addresses your situation, both the config file and the IoC? My local tests are working.

https://github.com/auth0/laravel-auth0/compare/fix-store-and-state-handing

1. Fixes the type hint and instance check in the Service and ServiceProvider
2. Overrides the injected ones with the config values, if present
3. More clear local var names

If the logic makes sense, I'll get tests in there and submit a PR.

@nstapelbroek - Would you take a look here and make sure this still solves your use case as well?

[mraypold]

@joshcanhelp I've taken a look at the branch and tested it against our local environments. It appears to have solved the issues we are encountering.

We're currently running a temporary fork with very similar code changes, but your solution looks a bit cleaner.

My only comment would be that I think the composer.json states the library requires PHP 5.5 or higher and the null coalescing used in the new constructor code is a PHP 7 feature.

[joshcanhelp]

@mraypold - Thanks for the quick look here. You're correct about the null coalescing ... I didn't run any PHP compatibility scans yet.

[iAmRoland]

Tested the changes as well, seems to fix the issue on my end 👍

[nstapelbroek]

Hey everyone,

Thanks for notifying me. I completely missed this issue in the last couple of days.
I agree with the conversation above: the bug seemed to be introduced in the last PR that did some overhaul on the state and sessionHandler construction logic. The root cause seems simple enough: I did not realise you could set the state_handler or storage to false in the config 😅. My apologies.

The suggested fix in #156 seems good and works on my local setup. 👍

edit: Typos 🤓

[joshcanhelp]

@nstapelbroek - Appreciate the testing and response!

@iAmRoland @mraypold - One thing that would be helpful to know is why you are skipping state checking in your application. I still plan on releasing this patch but disabling state may not be possible in an upcoming major.

[mraypold]

@joshcanhelp One of our clients requires IdP initiated SSO which required the state_handler to be disabled for this one workflow. In all other cases it was possible to use state handling as recomended by Auth0.

[joshcanhelp]

#156 is ready for final review. Our team will look at it this morning and get a release out today 👍 Thanks for the assistance here everyone

[iAmRoland]

Awesome!

I disabled state handling when I implemented Lock.js, this was about a year or two ago though. Have not had the time to go through and possibly revamp it, only upgrades and patches mostly.