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: disallow setSession loophole #536

Merged
merged 9 commits into from
Dec 1, 2022

Conversation

j4w8n
Copy link
Contributor

@j4w8n j4w8n commented Nov 13, 2022

What kind of change does this PR introduce?

I managed to jack up my fork, so have to resubmit this.

Adds checks for the passed-in access_token. Also refactors setSession(), to align the method with common behaviors of other methods.

What is the current behavior?

Some people, including myself, are passing in an empty string for the access token, to force a session refresh even if the session isn't expired. There is now a refreshSession() method to accomplish this, as of supabase-js version 2.0.4

What is the new behavior?

Checks if the access token is an empty string. Also adds a regex check to the decodeJWTPayload() helper, to verify the access token is in base64url format; otherwise we could still pass in something like hello.there.world and we'd get the same forced refresh behavior.

As for the refactor, I noticed that this method is directly calling _refreshAccessToken() then calling _saveSession(). However, _callRefreshToken() is used on other methods; and is a better fit, since it seems to handle multiple refresh calls. Plus it calls _saveSession() itself, which eliminates the need for that call in setSession().

Additional context

Closes #490

Copy link
Member

@kangmingtay kangmingtay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @j4w8n, thanks for making the time to fix this issue for us! just left a few comments but overall, it lgtm!

src/GoTrueClient.ts Outdated Show resolved Hide resolved
src/lib/helpers.ts Show resolved Hide resolved
@kangmingtay kangmingtay merged commit 21e496c into supabase:master Dec 1, 2022
@kangmingtay
Copy link
Member

thanks for making the changes! i'm marking this as a feat since it will be breaking for those who have been passing in an empty 'access token' as a hacky workaround

@github-actions
Copy link
Contributor

github-actions bot commented Dec 1, 2022

🎉 This PR is included in version 2.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@j4w8n j4w8n deleted the fix-set-session-loophole branch December 20, 2022 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify usage of setSession() in v2.0.0
2 participants