Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"State doesn't match" race condition #67

Open
aaronadamsCA opened this issue Sep 27, 2023 · 2 comments
Open

"State doesn't match" race condition #67

aaronadamsCA opened this issue Sep 27, 2023 · 2 comments

Comments

@aaronadamsCA
Copy link

aaronadamsCA commented Sep 27, 2023

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
Copy link
Author

aaronadamsCA commented Sep 27, 2023

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
Copy link
Contributor

runjak commented Aug 26, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants