"State doesn't match" race condition

[aaronadamsCA]

We started running into occasional Auth0 failures with "State doesn't match". After a day of banging my head against the wall, I think I have a pretty good idea of what's going on:

https://stackoverflow.com/questions/65493296/authorization-code-flow-concurrent-requests-from-multiple-tabs

If a user simultaneously loads multiple pages while unauthenticated, the result is a race condition:

1. Tab 1 updates state and redirects to OAuth
2. Tab 2 updates state and redirects to OAuth
   • Overwriting Tab 1 state
3. Tab 1 callback fails due to state mismatch
4. Tab 2 callback succeeds

This is pretty common when reopening a closed browser, for example.

[aaronadamsCA]

Some thoughts on solutions.

The Stack Overflow answers suggest a state store as a guard:

• Each time state is stored, it's pushed to an array.
• The callback compares state to the entire array.

This would appear to resolve the issue:

1. Tab 1 appends state to session, redirects to OAuth
2. Tab 2 appends state to session, redirects to OAuth
3. Tab 1 callback gets state from session, writes user to session
4. Tab 2 callback gets user from session

Due to race conditions with session storage itself, it's not a perfect solution:

1. Tab 1 gets session from session storage
2. Tab 2 gets session from session storage
3. Tab 1 commits session to session storage
4. Tab 2 commits session to session storage
   • Overwriting Tab 1 session

But the race condition window should be substantially narrowed from seconds to milliseconds.

It's also worth noting implementers are currently encouraged to store their own auth state (e.g. returnTo) separately, and this is subject to the same race condition (tab 1 returnTo is overwritten by tab 2 returnTo). If this library chooses to implement a state store, I think it would make a lot of sense to reconsider allowing custom state (or just returnTo) to be stored alongside the UUID for this reason.

[runjak]

I recognise it's a late response - but we solved this with the following pattern:

• We check for authentication on all loaders and actions in question, in a custom function we call isAuthenticated. It wraps our call to authenticator.isAuthenticated.
• Since multiple loaders could be asked for authentication at once and lead us to a race with authentication we instead redirect the client to some route like /auth/login, which performs the session handling. We set a returnTo value that allows us to bring them back to where they came from after login.
• /auth/login has only a single loader - executed for the route. This is where we execute authenticator.authenticate then - without any race as it's only called once.
• After complete auth we redirect users to their original route.

[aaronadamsCA]

That's a great idea, but I think it solves race conditions between routes whereas this issue is between tabs.

For now we're working around it with a retry inside our auth callback route:

try {
  return await authenticator.authenticate("auth0", request, {
    successRedirect: returnPath,
    throwOnError: true,
  });
} catch (exception) {
  if (exception instanceof AuthorizationError) {
    throw redirect(createAuthPath(returnPath));
  }

  throw exception;
}

But I still think the best way to solve this is a state store like this:

"oauth2:state": {
  "12345678": {},
  "90abcdef": {}
}

This would allow parallel authenticate() calls to complete successfully.

It would also allow #73 to be reopened so we could securely store our own values (like returnTo) inside those empty objects without any security impacts. I'm still pretty sure that issue was closed only because the ask was misunderstood.