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

Getting 400 on /auth/v1/token?grant_type=refresh_token #454

Closed
miguelespinoza opened this issue Sep 13, 2022 · 17 comments
Closed

Getting 400 on /auth/v1/token?grant_type=refresh_token #454

miguelespinoza opened this issue Sep 13, 2022 · 17 comments
Labels
auth bug Something isn't working released on @rc

Comments

@miguelespinoza
Copy link

miguelespinoza commented Sep 13, 2022

Bug report

Describe the bug

I'm having an issue where /auth/v1/token?grant_type=refresh_token suddenly returns 400. It's unclear what could cause this issue. When the token is approaching expiration supabase-js fires correctly, but it gets to a point where the 400 request occurs.

This originated after finding out that the supabase token disappears from localStorage on my extension.
The video walks through the situation in more detail: https://share.cleanshot.com/mc6hDzOEoD7lzwk4ZSrk

I've read that supabase v2 fixes "getting logged out” issues, but I'm not so sure this happens from the javascript side. To be precise, I'm initializing the client without the multiTab option, so it's not the race condition mentioned throughout the discussions/issues on Github.
Also, the Network logs, are isolated to one process. Meaning only one supabase client. Inside this process, each request uses the previous request's refresh token from the response. Being that it's isolated to only one process, I'm not sure how this could be a library issue. I could be wrong though

This is how I'm creating the supabase client.

const browserStorageInterface: SupportedStorage = {
  async getItem(key: string): Promise<string | null> {
    const storage = await browser.storage.local.get(key);
    return storage?.[key];
  },
  setItem(key: string, value: string) {
    browser.storage.local.set({
      [key]: value,
    });
  },
  removeItem(key: string) {
    browser.storage.local.remove(key);
  },
};

const supabase = createClient(supabaseUrl, supabaseKey, {
  autoRefreshToken: true,
  persistSession: true,
  localStorage: browserStorageInterface,
});

To Reproduce

Steps to reproduce the behavior, please provide code snippets or a repository:

  1. Setup supabase client
  2. Authenticate using Google oAuth
  3. Set JWT expiry to 20 seconds (to reproduce quickly)
  4. supabase refreshes token several times
  5. Eventually errors with status code 400.
  6. Token is removed from localStorage

Expected behavior

At no point should refreshing the token return status code 400.

Screenshots

https://share.cleanshot.com/mc6hDzOEoD7lzwk4ZSrk

System information

  • OS: macOS
  • Browser: chrome - browser extension
  • Version of supabase-js: v1.35.3
  • Version of Node.js: v12.17.0

Additional context

Help post on Discord: https://discord.com/channels/839993398554656828/1019328795129421944/1019328795129421944

@miguelespinoza miguelespinoza added the bug Something isn't working label Sep 13, 2022
@miguelespinoza
Copy link
Author

update: I'm going to give supabase-js@v2 a try. I'll report soon

@miguelespinoza
Copy link
Author

miguelespinoza commented Sep 14, 2022

Well, that didn't last long. Here's the 400 error using supabase v2.
I upgraded, only running auth endpoints, not using supabase db request and 💥
Here's how I'm initializing the auth token:

  1. redirect to site.com
  2. site.com sends access_token and refresh_token to extension using chrome.runtime.sendMessage
  3. extension uses supabase.auth.setSession(refreshToken). Unfortunately setAuth is deprecated and global: { headers: {} does not work for me.
  4. Wait a while, open a few extension instances in different tabs and eventually get a 400 error. According to the release notes, multiTab was not an issue anymore, but I didn't do anything out of the ordinary.

The verdict, still seeing auth issues with supabase v2
edit: shortly after this occurs, token is removed from localStorage, because of "Invalid Refresh Token"

cleanshot_09_14_at_09_48@2x

@miguelespinoza
Copy link
Author

I'm starting to think that the gotrue server can not handle a certain number of tabs.
I'm not sure if the team has tested a limit of tabs that the supabase-js client and server can handle.

At this point, I'm thinking the only solution is to isolate the supabase instance to only one process.
Avoiding replay attacks.

cleanshot_09_14_at_10_09@2x

@miguelespinoza
Copy link
Author

@kangmingtay any thoughts on this? I'm noticing 3 tabs is the minimum needed to trigger this 400 response code.

I have a few logs that could give you a few pointers on what could be causing this issue:

Token refresh logs

t1, t2, t3 indicates tab 1, 2, 3

11:26:6:793 -t1- auth.event:  TOKEN_REFRESHED 1663169166.793
11:26:6:793 -t1- new.token - expires in:  1663169187 2w3To_30sYO4jz3hsEe1qg
11:26:7:961 -t2- auth.event:  TOKEN_REFRESHED 1663169167.961
11:26:7:961 -t2- new.token - expires in:  1663169188 2w3To_30sYO4jz3hsEe1qg
11:26:15:263 -t3- auth.event:  TOKEN_REFRESHED 1663169175.263
11:26:15:263 -t3- new.token - expires in:  1663169195 2w3To_30sYO4jz3hsEe1qg
11:26:17:43 -t1- auth.event:  TOKEN_REFRESHED 1663169177.043
11:26:17:43 -t1- new.token - expires in:  1663169197 Gwz-MP20glWi7phSADOCWQ
11:26:18:228 -t2- auth.event:  TOKEN_REFRESHED 1663169178.228
11:26:18:228 -t2- new.token - expires in:  1663169198 Gwz-MP20glWi7phSADOCWQ
11:26:26:279 -t3- auth.event:  TOKEN_REFRESHED 1663169186.279
11:26:26:279 -t3- new.token - expires in:  1663169206 Gwz-MP20glWi7phSADOCWQ
POST https://kepynfotwvuetexxphcf.supabase.co/auth/v1/token?grant_type=refresh_token 400
11:26:27:318 -t1- auth.event:  TOKEN_REFRESHED 1663169187.318
11:26:27:318 -t1- new.token - expires in:  1663169207 cFCa5xrDMx2aj0XZf074Wg
POST https://kepynfotwvuetexxphcf.supabase.co/auth/v1/token?grant_type=refresh_token 400
11:26:28:413 -t2- auth.event:  TOKEN_REFRESHED 1663169188.413
11:26:28:413 -t2- new.token - expires in:  1663169208 cFCa5xrDMx2aj0XZf074Wg
POST https://kepynfotwvuetexxphcf.supabase.co/auth/v1/token?grant_type=refresh_token 400

should the auth token expires_at property be changing?

cleanshot_09_14_at_11_38@2x

Explanation of the logs

cleanshot_09_14_at_11_47@2x

@miguelespinoza
Copy link
Author

What are some optimal minimum configurations for JWT expiry limit and Reuse Interval to test this?
I'm using these and still getting a 400

cleanshot_09_14_at_12_53@2x

@GaryAustin1
Copy link

GaryAustin1 commented Sep 15, 2022

Edit: I'm not sure this is an edge case at all in my test below. It appears if you start up two tabs that share a refresh token and do setSession() in a tab anytime before the reuse window of the other tab refreshing, it fails when the other tab trys to timer refresh. Makes sense as the setSession burned the token and in v2 the other tab does not know. So the question is can a similar thing happen just based on when you start a tab, or move between tabs? I've added an issue in gotrue-js for the setSession case. Can't use that to simulate the delay like I wanted...

I was able to somewhat force an error with two tabs at 30 second/10 second reuse. Same thing occurs with 60/10.
Basically I put a timeout in at 20 seconds (30-10expiremargin-10reusemax) less than the initial expire time from getSession at start of tab.
Let 1 tab start running for a few cycles then start the next tab. When it runs about 10 seconds out of phase with the first tab on refresh cycles it hits the 400 fairly quickly. The problem is I have to try a couple of times to get the timers right around 10 seconds. Need to figure out a better way to set the delay of one tab from the other by about 10 seconds to show the error more easily.

  1. tab 1 refresh and gets token1
  2. tab 2 starts up and gets session from localStorage with token1
  3. tab 2 is forced to getSession at 10 seconds before expire of tab 1 to simulate starting up right around 10 seconds before expire. It burns token 1 and gets token 2.
  4. tab 1 tries to refresh the original token 1 and can not as reuse window is up.

The question is the a real world simulation of tabs out of sync?
image

@hf
Copy link
Contributor

hf commented Sep 18, 2022

Thank you for the detailed issue submission! We'll definitely take a look at what's going on. If you're having production issues on Supabase, you can get priority access if you open a ticket on the dashboard or write to support@supabase.com.

@miguelespinoza
Copy link
Author

Thanks, @hf. I submitted a ticket on the dashboard.

Also, I've been working on a candidate solution for this issue I've encountered.
More details can be found in this PR - #444

@andyjakubowski
Copy link

Hey @miguelespinoza — thanks for doing so much work on this issue. So helpful!

I noticed that when Supabase clients are initialized and they set the timeout to refresh the token, they use the value of the refresh token at initialization time. I wonder if this could be causing some of this trouble?

Imagine this scenario:

  • Let’s say our token lasts 60 minutes.
  • We open a tab at 2 PM and get a new access and refresh token.
  • Then we open another 2 tabs at 2:01 PM. They are now all scheduled to fire a POST request at /token at 2:59:50 (because of the 10-second expiry margin).
  • Now, we call setSession 3 times in one of the tabs. This tab has now updated the access and refresh tokens three times, and the previous refresh tokens have been revoked.
  • Those other 2 tabs are still scheduled to fire with the initial refresh_token, though! And when they do, they get a 400 response, which also revokes the entire refresh token family. This effectively logs out all of the tabs.

What if the scheduled refresh requests accessed whatever is the current value of the refresh token from storage? Instead of using the closed over value from the _startAutoRefreshToken call?

@miguelespinoza
Copy link
Author

Hey @andyjakubowski, happy this has helped you. I'm hoping we can come to a solution very soon. Although if you look at the PR I submitted, there's been a few updates. I encourage you to check it out -> #444

You're on the right track with checking for a new token in storage before making a request, which is what the PR handles.

But I'm curious about this statement:

Now, we call setSession 3 times in one of the tabs. This tab has now updated the access and refresh tokens three times, and the previous refresh tokens have been revoked.

I'm not sure if you mean you're calling setSession 3 times in one tab and outside the library. I would recommend for the library to handle refreshing the token. It's true, it'll invalidate the next tabs, which will cause the session to be removed and log out the user. So you'll want to make sure you're following the approach intended by the library.

If you leave the library to make a request. The backend has logic for reusing tokens within an interval. This is defaulted to 10 seconds. So technically the "stale" refresh token for tab 2 and 3 should reconcile with the latest refresh token

Also, there's an effort in supabase v2 to consolidate auth logic. This addresses your first point about:

when Supabase clients are initialized and they set the timeout to refresh the token, they use the value of the refresh token at initialization time

This is not a problem in v2.

@J0 J0 transferred this issue from supabase/supabase Sep 26, 2022
@andyjakubowski
Copy link

andyjakubowski commented Sep 26, 2022

Thanks for elaborating @miguelespinoza. I’m looking at the gotrue-js codebase in the rc branch. I presume this is the v2 you are referring to?

The scenario I described makes any and all calls through the Supabase client instance. In my actual code I use the autoRefreshToken and let the client instance handle refreshing the access token. I’m merely pointing out that the code which schedules tokens to be refreshed uses refresh_token values from the time of scheduling. So if in the meantime anything, for example another tab, actively calls setSession for whatever reason, then that scheduled refresh code will be using now-revoked refresh tokens.

I don’t know if this is the cause of the issue you’re experiencing, but I wanted to point out this fact in case it’s relevant.

@miguelespinoza
Copy link
Author

Yup, we're both on the same page.

That's something that should be accounted for by the library. To pick up the latest refresh_token, rather than the value from the time of schedule. FWIW, this is partly working. When making a supabase request in v2, fetch verifies if it's using an up to date accessToken. Check out fetchWithAuth. This just has to also apply to the auth logic we're discussing

And yup, you're on the right branch, rc.

@gprieto
Copy link

gprieto commented Sep 26, 2022

Hey @miguelespinoza , I have encountered a related issue while trying to set up auth in a browser extension and keeping a session in the popup and service-worker. Would you mind having a look? Maybe you can shed some light...

#456

@andyjakubowski
Copy link

When making a supabase request in v2, fetch verifies if it's using an up to date accessToken. Check out fetchWithAuth. This just has to also apply to the auth logic we're discussing

Thanks for pointing this out; I didn’t realize supabase-js ran any custom options.global.fetch through fetchWithAuth first!

This just has to also apply to the auth logic we're discussing

Yep! 👌

@kangmingtay
Copy link
Member

Hey everyone, i think i've figured out the issue, seems like the session passed into the _startAutoRefreshToken method can possibly become stale and fall outside the reuse interval. I've made a PR. Would be awesome if you can try it out and see if it fixes the problem for you!

Tested with the following scenarios:

  • JWT_EXPIRY: 30s, REUSE_INTERVAL: 10s, 5 tabs opened (firefox)
  • JWT_EXPIRY: 20s, REUSE_INTERVAL: 5s, 5 tabs opened (firefox)

@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2022

🎉 This issue has been resolved in version 2.0.0-rc.7 🎉

The release is available on:

Your semantic-release bot 📦🚀

@kangmingtay
Copy link
Member

just merged #461 in, will bump the supabase-js branch to include this fix next - shoutout to @miguelespinoza, thanks for pointing this out and writing up such a detailed issue!

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

No branches or pull requests

7 participants