-
Notifications
You must be signed in to change notification settings - Fork 538
fix(realtime): preserve custom JWT tokens across channel resubscribe #1908
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
base: master
Are you sure you want to change the base?
fix(realtime): preserve custom JWT tokens across channel resubscribe #1908
Conversation
mandarini
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for this PR @reckziegelwilliam . May I request some changes?
- Please update the JSDoc for setAuth() to document the new behaviour, because it may end up being confusing to users.
* Sets the JWT access token used for channel subscription authorization and Realtime RLS.
*
* If param is null it will use the `accessToken` callback function or the token set on the client.
*
* On callback used, it will set the value of the token internal to the client.
*
* When a token is explicitly provided, it will be preserved across channel operations
* (including removeChannel and resubscribe). The `accessToken` callback will not be
* invoked until `setAuth()` is called without arguments.
*
* @param token A JWT string to override the token set on the client.
*
* @example
* // Use a manual token (preserved across resubscribes, ignores accessToken callback)
* client.realtime.setAuth('my-custom-jwt')
*
* // Switch back to using the accessToken callback
* client.realtime.setAuth()
*/
I also added a few comments around the code.
- Let's not mention the issue/bug directly, because this can end up being spammy (imagine adding "
fix for #1234" every time we fix a bug) - Let's not say like "Workaround" or "bug" or something, because a year from now we will not know what this workaround is for. If comments are to be added, they need to be simple and directly descriptive. But comments can also be removed.
- Let's not directly access that private member.
Overall, this is a great fix, and thank you very much! Once these changes are addressed, I'll give this another review!
| this.joinPush | ||
| .receive('ok', async ({ postgres_changes }: PostgresChangesFilters) => { | ||
| this.socket.setAuth() | ||
| // FIX for issue #1904: Don't overwrite manually-set tokens |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove "FIX for issue #1904:".
|
|
||
| if (token) { | ||
| tokenToSend = token | ||
| // FIX for issue #1904: Track if this is a manually-provided token |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove "FIX for issue #1904:".
| try { | ||
| tokenToSend = await this.accessToken() | ||
| } catch (e) { | ||
| this.log('error', 'error fetching access token from callback', e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have proper capitalization here.
| tokenToSend = this.accessTokenValue | ||
| } | ||
|
|
||
| // FIX for issue #1904: Set manual token flag even if token doesn't change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove "FIX for issue #1904:".
| this.setAuth().catch((e) => { | ||
| this.log('error', `error setting auth in ${context}`, e) | ||
| }) | ||
| // FIX for issue #1904: Don't overwrite manually-set tokens |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove "FIX for issue #1904:".
| // BUG: Second join should ALSO include access_token, but it might not! | ||
| const secondJoinCall = pushSpy.mock.calls.find((call) => call[0]?.event === 'phx_join') | ||
|
|
||
| expect(secondJoinCall).toBeDefined() | ||
|
|
||
| // THIS IS THE BUG: access_token is missing from second join |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's adjust this comments to either not exist, or be descriptive rather than referring to "bug", because it may seem arbitrary a year on from now.
| expect(secondJoinCall![0].payload).toHaveProperty('access_token', customToken) | ||
| }) | ||
|
|
||
| test('WORKAROUND: using accessToken callback works for resubscribe', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, here, "workaround" would refer to the "Specific" bug we're fixing, so I would rather not contain words that are describing a specific issue, but rather explain a behaviour. It will be easier for long term maintainability for new contributors to get context.
| }) | ||
|
|
||
| test('WORKAROUND: using accessToken callback works for resubscribe', async () => { | ||
| // This test shows that the workaround (using accessToken callback) works |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
|
|
||
| const secondJoin = pushSpy.mock.calls.find((call) => call[0]?.event === 'phx_join') | ||
|
|
||
| // With accessToken callback, this WORKS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe remove this comment?
| this.socket.setAuth() | ||
| // FIX for issue #1904: Don't overwrite manually-set tokens | ||
| // Only refresh if token wasn't manually set via setAuth(token) | ||
| if (!this.socket['_manuallySetToken']) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not really like accessing private members like that. Can we please add a getter? Or a helper method like isManualTokenSet(): boolean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mandarini yes
|
@mandarini ill proceed with your notes. Thank you for the review. |
be620bd to
6385a36
Compare
Fixes supabase#1904 When using setAuth(customToken) with private channels, custom JWTs are now preserved across removeChannel() and resubscribe operations. Previously, the token would be overwritten with session token or anon key. Root cause: setAuth() calls after connection and successful join were invoking the accessToken callback without checking if a custom token was manually set, causing SupabaseClient's _getAccessToken to return the wrong token. Solution: Track manually-set tokens with _manuallySetToken flag. Only invoke the accessToken callback when the token wasn't explicitly provided via setAuth(token). Changes: - Add _manuallySetToken flag to RealtimeClient - Update _performAuth() to track token source (manual vs callback) - Modify _setAuthSafely() to check flag before invoking callback - Update join callback in RealtimeChannel to check flag - Add error handling for accessToken callback failures - Add comprehensive regression tests (4 new tests) - Update existing tests for async subscribe Testing: - All 364 tests passing, zero regressions - Verified in React Native/Expo environment - Both setAuth(token) and accessToken callback patterns work - Workaround (accessToken callback) is now obsolete but remains supported Breaking changes: None
6385a36 to
a98640a
Compare
Fixes #1904
When using setAuth(customToken) with private channels, custom JWTs are now preserved across removeChannel() and resubscribe operations. Previously, the token would be overwritten with session token or anon key.
Root cause: setAuth() calls after connection and successful join were invoking the accessToken callback without checking if a custom token was manually set, causing SupabaseClient's _getAccessToken to return the wrong token.
Solution: Track manually-set tokens with _manuallySetToken flag. Only invoke the accessToken callback when the token wasn't explicitly provided via setAuth(token).
Changes:
Testing:
Breaking changes: None
🔍 Description
This PR fixes an issue where custom JWT tokens were being lost when resubscribing to private broadcast channels after calling
removeChannel().What changed?
Core Changes:
_manuallySetToken: booleanflag toRealtimeClientto track token source_performAuth()to set the flag when tokens are explicitly provided_setAuthSafely()and the channel join callback to check this flag before invoking theaccessTokencallbackaccessTokencallback invocationTest Changes:
RealtimeClient.auth.resubscribe.test.tswith 4 comprehensive testsWhy was this change needed?
User's Problem (from #1904):
Root Cause:
SupabaseClientalways setsaccessTokencallback to_getAccessToken()setAuth()was called without parameters_getAccessToken()which returned session token or anon keyImpact:
accessToken: async () => fetchToken())setAuth()vsaccessTokenshould be usedCloses #1904
📸 Screenshots/Examples
Before Fix (Broken):
After Fix (Working):
Verification Output:
🔄 Breaking changes
Note: The
subscribe()method signature changed from synchronous to async (returnsPromise<RealtimeChannel>), but this is backward compatible - existing code that doesn't await the promise continues to work.📋 Checklist
fix(realtime): preserve custom JWT tokens across channel resubscribenpx nx formatto ensure consistent code formatting📝 Additional notes
For Reviewers:
Why the workaround worked: The
accessTokencallback was invoked each time, providing fresh tokens, so it never relied on the cached value that was being overwritten.Why
setAuth()alone didn't work: Two places calledsetAuth()without params, triggeringSupabaseClient._getAccessToken()which returned the wrong token.Our solution preserves both patterns:
setAuth(customToken)- Now works reliably (was broken)accessToken: async () => token- Still works (for token rotation)Testing coverage:
Backward compatibility: