Skip to content

fix: resolve race conditions when refreshing access token #203

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

Closed

Conversation

wattroll
Copy link

What kind of change does this PR introduce?

This fixes a race condition that arises when there are overlapping calls to GoTrueApi.refreshAccessToken. It does so by keeping reference to the result of this._callRefreshToken until it is settled and returning it to the concurrent calls.

What is the current behavior?

Concurrent token refreshes inevitably result in unrecoverable failure:

  • the earliest call will succeed, revoke current session and receive a new one;
  • the later calls will fail due to using revoked session AND revoke the new session due to token reuse detection.
  • client will be left with a valid access_token and revoked refresh_token
  • client might not notice the problem until the next time when they try to refresh the session

Currently library code doesn't trigger such conditions unless tokens are short lived and network latency is high. User code will trigger it if it calls refreshSession() naively (e.g. when handling a 401 "token expired" error from several parallel requests).

Currently to avoid this problem user code must implement similar kind of batching that I did here. But even then there is no clean way to avoid racing against the auto refresh timer apart from disabling it (which also disables the token recovery from local storage).

What is the new behavior?

If this._callRefreshToken is invoked while an earlier request is in-flight they will be settled with the same promise. Once the promise is settled the reference to it is deleted, so that future calls will initiate fresh requests.

This fix is intended to be fully compatible with existing code (including potential access to private properties) and allows user code to handle expired tokens more reliably.

@dthyresson
Copy link
Contributor

Hi @wattroll and thanks for this PR.

I recently opened #201 which described a few scenarios where you could get yourself into a situation where the the token would no longer refresh.

And I think we are onto the same thing here:

Concurrent token refreshes inevitably result in unrecoverable failure:

* the earliest call will succeed, revoke current session and receive a new one;
* the later calls will fail due to using revoked session AND revoke the new session due to token reuse detection.
* client will be left with a valid access_token and revoked refresh_token
* client might not notice the problem until the next time when they try to refresh the session

As that's what I say too -- but I was curious if you had found a way to get your client into that state without using a second browser or logging out as I had.

Also, I see that you are now returning a 401 error status vs simply an exception -- which I like since as you noted, it eats that error and doesn't inform the client such that tokens will continue to fail "silently".

I was wonder if you thought it would be helpful to bubble up a "TOKEN_REVOKED" event that the client could subscribe to -- just like you can subscribe to a TOKEN_REFRESHED or SIGNED_OUT event?

@wattroll
Copy link
Author

This has been resolved by #285 and #339.

@wattroll wattroll closed this Aug 12, 2022
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

Successfully merging this pull request may close these issues.

2 participants