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

fix: refreshing tokens at the wrong time #55

Conversation

MilesV64
Copy link
Contributor

@MilesV64 MilesV64 commented May 2, 2023

StoredSession.isValid was being calculated incorrectly, because by comparing to "1 minute ago", it means an access token which expires "30 seconds ago" is still valid for 30 seconds. Changing to plus 60 seconds eliminates this problem, and bakes in plenty of time for latency issues.

In a future evolution I believe this value should be configurable.

What kind of change does this PR introduce?

This fixes is a critical bug in the token refresh logic, by ensuring refresh tokens are refreshed before they become expired.

What is the current behavior?

The logic to calculate if a stored access token is valid is flawed and results in consistently getting expired access tokens from await auth.session if it's within 60 seconds of expiring.

The current logic is:
isValid = expirationDate > Date().addingTimeInterval(-60)
In other words, a token is valid if its expiration date is later than a minute ago.

So let's say you have an access token which expires at 11:59:50.
And let's say you call await auth.session at 12:00:00. (hh:mm:ss)
isValid is then calculated as 11:59:50 > (12:00:00 - 60), or 11:59:50 > 11:59:00.
So, isValid is true even though actually the token expired 10 seconds ago.

What is the new behavior?

The new behavior changes the isValid calculation from subtracting 60 seconds from "now" to adding 60 seconds. This means the access token returned from auth.session will be guaranteed to be valid for at least 60 seconds. In a future evolution this could be a user-configurable value, but it's important it's greater than 0 or you will consistently get expired access tokens if you ask for one in <60s from when it expires. Being greater than 0 (rather than just 0) has the benefit of being proactive about the token, meaning it handles any server latency etc that could arise as you attempt to use the token.

Additional context

For context, my app which uses Supabase would 401 requests seemingly randomly. Then I realized it was happening every hour, which I realized is what I set my auth token expiry time to. This change fixes it!

Fixes #53

StoredSession.isValid was being calculated incorrectly, because by comparing to "1 minute ago", it means an access token which expires "30 seconds ago" is still valid for 30 seconds. Changing to plus 60 seconds eliminates this problem, and bakes in plenty of time for latency issues.

In a future evolution I believe this value should be configurable.
@MilesV64
Copy link
Contributor Author

MilesV64 commented May 2, 2023

@devi-prsd I noticed you recently committed, do you mind taking a look at this?

@MilesV64
Copy link
Contributor Author

MilesV64 commented May 2, 2023

@GRSouza mentioning you too if you could take a look!

@kevlust
Copy link

kevlust commented May 2, 2023

@MilesV64 thanks for this!

@devi-prsd
Copy link
Contributor

@MilesV64 I don't have permissions to approve it, but looks good to me!

@grdsdev
Copy link
Contributor

grdsdev commented May 3, 2023

Hey @MilesV64 thanks for fixing this.

@grdsdev grdsdev merged commit a972c99 into supabase-community:main May 3, 2023
@grdsdev
Copy link
Contributor

grdsdev commented May 3, 2023

Released on 1.1.1

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.

Auth session refreshing doesn't always happen at the right time
4 participants