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

Concurrent token refreshes can lead to unintentional signing out of users when multiple tabs are open #213

Closed
bnjmnt4n opened this issue Jan 20, 2022 · 13 comments
Labels
bug Something isn't working p3 Priority 3

Comments

@bnjmnt4n
Copy link

Bug report

Describe the bug

gotrue-js uses localStorage to store the refresh token. When the user has multiple tabs open, multiple instances of the GoTrueClient will be instantiated, with each reading from localStorage the same identical access and refresh tokens. There can be cases where both tabs will attempt to refresh using the same token at the same time, which leads to the first request succeeding, but the second request failing due to token reuse detection (supabase/auth#226).

To Reproduce

  1. Reduce the JWT expiry from the default 1 hour to 1 minute or lesser.
  2. Login into an app using Supabase in a single tab.
  3. Open the app in a second tab.
  4. Open the network inspector and observe refresh token requests. Eventually both tabs will have failing requests due to reuse of the same refresh token.

Expected behavior

There is some kind of locking mechanism which avoids concurrent refreshes using the same refresh token. #203 attempts to do this, but it doesn't account for the multi-tab use case.

System information

This is likely reproducible on other browsers, but here's the one I tested with:

  • OS: Windows
  • Browser (if applies): Firefox
  • Version of supabase-js: 1.28.6

Additional context

I will be testing out some changes to gotrue-js and will send a PR if it successfully fixes this issue.

@bnjmnt4n bnjmnt4n added the bug Something isn't working label Jan 20, 2022
@TheOnlyBeardedBeast
Copy link

TheOnlyBeardedBeast commented Feb 1, 2022

@bnjmnt4n @inian I think this issue is not frontend related issue, I went deep into the gotrue server implementation, everytime you request a new refresh token, gotrue revokes the old one. As you are in different tabs with shared refresh tokens, the tab which does the refresh first revokes the refresh token for the other tabs, so the other tabs cant get a new access token. Overcoming this issue means that only one tab can make the refresh and it needs to share the access and refresh tokens with the others. Maybe the broadcast channell api or storage change event to share data between tabs? The second option is to not revoke the refresh token, but that would blow the security.

From the GoTrue server source code, clearly visible that it revokes the old token.

// GrantRefreshTokenSwap swaps a refresh token for a new one, revoking the provided token.
func GrantRefreshTokenSwap(tx *storage.Connection, user *User, token *RefreshToken) (*RefreshToken, error) {
	var newToken *RefreshToken
	err := tx.Transaction(func(rtx *storage.Connection) error {
		var terr error
		if terr = NewAuditLogEntry(tx, user.InstanceID, user, TokenRevokedAction, nil); terr != nil {
			return errors.Wrap(terr, "error creating audit log entry")
		}

		token.Revoked = true
		if terr = tx.UpdateOnly(token, "revoked"); terr != nil {
			return terr
		}
		newToken, terr = createRefreshToken(rtx, user)
		return terr
	})
	return newToken, err
}

But if I missread somehow the go source code, please let me know.

Edit: I checked also the gotrue-js implementation, they already listening for storage changes, when multitab support is enabled

@bnjmnt4n
Copy link
Author

bnjmnt4n commented Feb 2, 2022

@TheOnlyBeardedBeast The gotrue behaviour is correct; this is more of an issue with the gotrue-js implementation.

As you are in different tabs with shared refresh tokens, the tab which does the refresh first revokes the refresh token for the other tabs, so the other tabs cant get a new access token. Overcoming this issue means that only one tab can make the refresh and it needs to share the access and refresh tokens with the others.

This is exactly the gist of the problem. Both tabs do not notify each other that they are attempting a refresh, so it is possible for 2 parallel refresh attempts to occur at nearly the same time. One of them will likely succeed first, returning a new refresh token. However, the other refresh attempt will cause the new refresh token to be revoked due to the use of the same original refresh token.

I think there are possibly 2 paths to take:

  1. If the same refresh token is used multiple times within a given period, do not revoke any descendant refresh tokens. This solution is brittle since network latency differs across users and regions, and potentially replay attacks could be used to generate/obtain new refresh tokens.
  2. Implement a locking solution on the client-side to ensure that refreshes across multiple tabs will never use the same refresh token. I have some naive code which attempts to do locking via localStorage, but it is a bit of a mess, and I'll try to clean it up before potentially making a PR. (I need to double-check the integration with supabase-js as well.) BroadcastChannel or the web locking API isn't supported in Safari, so I didn't look into that. Perhaps the Web locking APIs could be used in the future as well.

@TheOnlyBeardedBeast
Copy link

TheOnlyBeardedBeast commented Feb 2, 2022

@bnjmnt4n thanks for the reply, I am working on a client myself handling the same issue (not gotrue-js, but the api was based on gotrue and behaves the same), currently what I am doing is listening to storage events and updating the clients refresh token memory representation in different tabs. It works well but I think in the future I can encounter similar issues as you when a new tab is opened on the edge of the refresh interval of the current tab so a refresh starts while a different refresh is in progress. Perhaps a simple onetime retry after the first failed refresh happens could also help, that failed refresh would mean that a new refresh token should already exist in the storage. But your idea with a locking mechanism sounds good. You could wrap a storage change event into a promise, checking for a specific key, before the refresh request you can await that given promise which resolves when the given key does not exist in the storage, or its value means that a refresh is not pending in a different tab.

I will definitely try something out, thanks for the brainstorming.

Edit: One more possible solution is to disable refresh for hidden tabs, and refresh them when they become activated.

@liaujianjie
Copy link

IMO this race condition can be resolved with leader election. That is, we select a single tab, using a leader election algorithm, to perform the token refresh when it is time to perform a refresh. There's a tab-election package that we can take reference from.

Alternatively, a temporary cheap solution would be to randomize the time at which the token refreshes. Though an incomplete solution, this would significantly reduce the likelihood of a clash.

@thorwebdev
Copy link
Contributor

This is now being worked on server-side in gotrue: supabase/auth#466

@chipilov
Copy link

@thorwebdev I see that even though supabase/auth#466 has been merged and deployed to my hosted project (I checked that my project is using v2.6.30 while the fix is in 2.6.28), the issue still reproduces. That is, if I have multiple tabs open, one of the refresh token calls fails and signs out the user from both tabs.

Is there anything I need to do to enable the fix?

@thorwebdev
Copy link
Contributor

@chipilov I'm not able to reproduce this anymore. Can you provide some screenshots of your network tabs? For me it is now correctly working and replaying the request (see screenshots). Do note that the default reuse interval is 10s. So if the requests are further apart than that the tokens will be revoked.

image

image

@chipilov
Copy link

Hi @thorwebdev,

I am attaching a screenshot of the issue:

token_requests

Some additional info:

  • supabase-js is at 1.22.15;
  • GoTrue server for the project as reported by the /health API is at v2.6.30
  • I am using a pretty frequent refresh interval (30seconds) just to make it easier to repro
  • you can probably see from the timestamps of the requests but they are almost simultaneous so they definitely fall within the 10s interval

Finally, I am not sure what you mean by "replaying the requests". My impression was that the fix was entirely on the server side so that if 2 token refresh requests for the same token arrive at the server within 10s from each other both of them will receive 200 responses with the new token. Is that not the case (i.e. is there an additional retry handling in the client)?

Let me know if you need more info.

@kangmingtay
Copy link
Member

Hey @chipilov,

I am not sure what you mean by "replaying the requests"
Yes, if the reuse interval is set to 10s, then the client has a leeway time of 10 seconds to make the refresh using the same token. Gotrue will simply return the latest valid child token instead of generated a new one.

For example:

token | parent 
foo       -
bar       foo
  1. First request to refresh using token foo returns token bar. Reuse interval starts here.
  2. As long as foo is still within the reuse interval, you can make as many requests to refresh as you like and gotrue will always return bar.
  3. A token can be reused as long as it satisfies both of these conditions:

a. Token is used within reuse interval
b. Token must be the last revoked token

For example:

token | parent | revoked
foo       -        t
bar       foo      t
baz       bar      f 

If a refresh request is made using bar to get baz, then trying to use foo to refresh again will trigger the automatic reuse detection and invalidate all it's descending tokens. This is because foo is now the second-last revoked token.

@chipilov
Copy link

Just to close the loop on my particular issue: @kangmingtay identified the problem (a newly introduced env var was not loaded in my project for some reason) and fixed it - thanks!

So the server-side fix seems to work as long as the new env vars are properly loaded into a project.

@egor-romanov egor-romanov added the p3 Priority 3 label Jun 16, 2022
@cinan
Copy link

cinan commented Nov 4, 2022

I can still reproduce this bug with local supabase instance. Supabase-cli: 1.11.4, supabase-js: 2.0.4. It looks like gotrue server (2.15.3) wasn't configured with GOTRUE_SECURITY_REFRESH_TOKEN_REUSE_INTERVAL variable. If I call two simultaneous refresh tokens requests, one of them always fails due to invalid refresh token.

@kangmingtay
Copy link
Member

@cinan how are you running the local supabase instance? is it with the docker-compose found here? if you are, then yeah we didn't add the necessary env vars to allow for that yet

@cinan
Copy link

cinan commented Nov 4, 2022

I run supabase startwhich probably uses the docker-compose file you referenced. In docker inspect I can see there's no GOTRUE_SECURITY_REFRESH_TOKEN_REUSE_INTERVAL variable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working p3 Priority 3
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants