-
Notifications
You must be signed in to change notification settings - Fork 893
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
Firebase Auth: the pendingRedirect sessionStorage key signInWithRedirect creates can be cleared in race condition #6827
Comments
Hi @WestonThayer, thanks for bringing this to our attention. I was able to replicate the behavior. Let me check this with our Auth team or bring someone here that can provide more context about it. I’ll update this thread if I have any information to share. |
Thanks, I am seeing a lot of page refreshes happen as well. I think its very closely related to this issue. |
Thanks for filing this @WestonThayer .. I am curious why the auth initialize is also done as part of a button click? If auth initialize happens at the time of loading and signInWithRedirect is called in an event handler (like button click), that will reduce the chance of this happening. Having said that, I think we can
cc @sam-gc |
Thanks for the investigation all.
Moving The issue is the To expand on how we hit this in a real world example, we have a Next.js app, and for the most part each route is statically rendered to HTML, then hydrated & enriched on the client. For most routes, we set up an That choice was rooted in the dilemma of deciding what
We went with the latter approach, which I can now understand might be at odds with Firebase Auth's design — especially since native apps don't really have the notion of supporting a In any case, because |
Thanks for the additional context.
This is the pattern I was more familiar with and also what we do in the demo app - firebase-js-sdk/packages/auth/demo/src/index.js Line 1654 in 7e237cd
I opened an internal bug to investigate the race condition issue - b/261039380 |
I think I am able to repro the issue with the following changes to the demo app - https://github.com/firebase/firebase-js-sdk/compare/redirect-race2 If I await on the initialization promise in signInWithRedirect, the login upon demo app init is successful. Without that, it fails. I will do a closer review of the code + add unit tests and send a PR. |
Work around rn with the signInWithPopup |
Any update on this issue? |
Not sure if this will help investigate the issue, but I see this error before Google redirects back to the app: |
[REQUIRED] Describe your environment
[REQUIRED] Describe the problem
Steps to reproduce:
I've set up a test Firebase instance and a codesandbox for easy reproduction. All I did to the Firebase instance was to create a web app, initialize auth, and add codesandbox as a trusted domain.
Actual: the console log will contain
window.sessionStorage\n[]
.Expected: the console log contains
window.sessionStorage\n['firebase:pendingRedirect:AIzaSyA8msq271ik2ryYJw99pZreiAWmErgBqww:[DEFAULT]']
.Notes
When the button is clicked, that's the first time
.getAuth()
is called. getAuth() apparently has an internal async task queue. One of the async tasks added to that queue is initializeCurrentUser, which tries to process a pending redirect.But before that async task can complete, our code calls
signInWithRedirect()
, which ends up creatingwindow.sessionStorage["firebase:pendingRedirect:AIzaSyA8msq271ik2ryYJw99pZreiAWmErgBqww:[DEFAULT]"] = "true"
beforeinitializeCurrentUser
runs.When
initializeCurrentUser
does run a few seconds later, it manages to delete the sessionStorage key before the browser redirects to the next page.The impact of this is pretty much Firebase Auth w/Provider: Calling getRedirectResult() always returns NULL user #3115, when the user does finally get redirected back to your app, they aren't signed in because the sessionStorage key doesn't exist.
The Firebase SDK appears to assume that
getAuth()
will never be called for the first time in close proximity tosignInWithRedirect
. I worry some other bugs might be lurking as a result of that assumption, but this is the issue facing our app.The text was updated successfully, but these errors were encountered: