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

If multiple login flows are initiated from multiple tabs, state is invalid and login fails #1356

Open
krzempekk opened this issue Apr 25, 2024 · 6 comments

Comments

@krzempekk
Copy link
Contributor

Issue and Steps to Reproduce

oidc-client saves state for login requests in sessionStorage under oidc.state.${configurationName} key. This causes issues in following scenario:

  1. Tab A with app is opened and login is initiated. State "AAA" is saved to session storage. Browser is redirected to IDP login page.
  2. Tab B with the same app is opened and login is initiated. State "BBB" is saved to session storage, overwriting state "AAA". Browser is redirected to IDP login page.
  3. On Tab A, user puts username and password and tries to log in. Browser is redirected back to app. State "BBB" is retrieved from session storage and compared with state from response. Login fails because state "BBB" was in session storage, but state "AAA" was received from response.
  4. If user tries to continue login on Tab B, it works, because state "BBB" is both in session storage and in request.

This can be reproduced on oidc-client demo app (https://black-rock-0dc6b0d03.1.azurestaticapps.net/) using above steps.

Versions

7.22.4

Expected

If login is initiated from multiple tabs, completing it on each tab should succeed.

Actual

If login is initiated from multiple tabs, completing it is succeeding only on last tab that initiated login flow.

Additional Details

Possible solution might be reusing exiting state from session storage. Or creating unique state session storage entry for each initiated login flow.

@guillaume-chervet
Copy link
Contributor

Thank you @krzempekk that a good issue !

To be able to reproduce it is the most difficult part thank you :)

I'am in Holiday (3 weeks) I do not know when I will be able to fixed it!

@krzempekk
Copy link
Contributor Author

Thanks @guillaume-chervet for the response!

No worries, enjoy your holidays :D. It's not a blocker for us, just wanted to report this.

@meesvandongen
Copy link
Contributor

I have seen similar behavior, but I don't think I can reproduce it outside of a service worker context; in principle each tab should have its own session storage. There is some behavior to copy session storage when cloning a tab in the browser; but that would not explain this issue I think (since the value from the original tab should only be editable from the same tab). @krzempekk are you able to reproduce this without a service worker?

@meesvandongen
Copy link
Contributor

meesvandongen commented May 29, 2024

@guillaume-chervet What are you thinking in terms of a solution? Here are my thoughts:

If this is indeed only related to the service worker, what could be done is generate a tabId which is stored in session storage. Then, when calling setState, or setNonce, (and getState, getNonce), we also send the tabId from the storage to the service worker, which will in turn set e.g. currentDatabase.state[tabId] = state.

The service worker only checks the nonce; the state is checked in the oidc client.

The nonce check in the service worker is done during a fetch, so I am not confident we can get a tabId of the tab that initiated the fetch. It seems like the only solution is to check all the available nonces for one that matches.

Edit: although if I look at the interface of the service worker api, it seems there are things like client id for the fetch and client for the messages. If those are able to identify tabs consistently, potentially there would be no impact on the oidc-client at all, only on the service-worker

Edit2: It does appear that the source attribute is not usable, since it is specific to a page rather than a session.

@meesvandongen
Copy link
Contributor

@guillaume-chervet I have made some progress on this issue (I think), as well as made some changes in the examples/react-oidc-demo package to make sure the existing tests are passing. I have not created any new tests to cover the functionality. I am going on holiday though, so this will be somewhat of a commit-and-run.

If you were going to look into this issue, it could be a starting point.

https://github.com/meesvandongen/oidc-client/tree/tab-specific-state-nonce

@guillaume-chervet
Copy link
Contributor

Thank you very much @meesvandongen iI am very busy these days. May you run a draft pullrequest to be able to see the change easily? Event if it is not finished yet.

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

3 participants