Repository navigation
Conversation
|
No actionable comments were generated in the recent review.
 |
| Objective | Addressed | Explanation |
|---|---|---|
Keep the local session and allow retry after an unconfirmed global logout ([#2779]) |
✅ |
Priority:
Change: Bug fix · Severity of issue fixed: Medium
Merge Risk: 
d63ca
-
Global sign-out now keeps the local session and returns the error when the server fails to log out, so callers can retry or choose local scope. No actionable merge-blocking risk remains in the reviewed changes.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
@supabase/auth-js
@supabase/functions-js
@supabase/postgrest-js
@supabase/realtime-js
@supabase/storage-js
@supabase/supabase-js
commit: |
…r failed to log out
359c13f to
6afe607
Compare
|
coverage: 82.311% (+0.05%) from 82.263% — fix/auth-global-signout-keep-session into master |
|
Looks good. One thing that might be worth a line in the release notes: since |
QuintinWillison
left a comment
QuintinWillison
left a comment
There was a problem hiding this comment.
I wanted to get you feedback nice and quickly on this, as well as validate that it aligns with work I'm in the middle of on the same capability for supabase-go... So Claude, in context of my current planning for supabase-go, plus inputs from our internal Slack discussion on this, analysed thus 🤖 ...
I have read #2786 at 6afe607 against the position the Slack thread reached: revoke first for global and others, tear down first for local, and treat a server answer as success by its error code rather than its HTTP status. The fix for global is right and worth merging. Two observations below, each with a concrete change, then a suggestion on where this should be settled.
What the PR gets right
- A failed
globalkeeps the session, fires noSIGNED_OUTand returns the error (GoTrueClient.ts#L4129-L4132). The retry test proves the refresh token is still in hand. keepLocalSessionOnErroris subsumed by the default rather than added as an option.scopeis on the wire for every call (GoTrueAdminApi.ts#L151), so the client never relies on the server's default.othersnever touches the local session, in all six tested outcomes.
1. The 401/403/404 filter still reports an unrevoked global as done, and the doc comment now makes that the contract
Observed. The filter at GoTrueClient.ts#L4122-L4124 is unchanged, and the new doc comment states it as behaviour: "A 401, 403, or 404 response counts as a successful sign-out" (#L4048).
Why it matters. On the current server the /logout handler itself answers only 204, 400 or 500 (logout.go#L39, #L67, #L70). Every 403 comes from requireAuthentication before the handler runs, and the status does not say which of two very different things happened:
| Error code | HTTP status | Meaning for a global sign-out |
|---|---|---|
session_not_found (auth.go#L155), user_not_found (#L139) |
403 | the goal state was already reached: success is right |
bad_jwt (#L112), user_banned (#L38) |
403 | the handler never ran and nothing was revoked |
For the second row the filter clears the session, fires SIGNED_OUT and returns error: null, which is the scenario of #2779 reached by a different door. 401 only ever means a missing bearer (#L77), which the SDK cannot produce, and 404 is not on this path at all. The inline comments the filter still carries ("ignore 404s since user might not exist anymore", "ignore 401s since an invalid or expired JWT should sign out the current session") describe an older server. To be fair about exposure: __loadSession refreshes a token inside the expiry margin before the request goes out (#L3066-L3081), so the expired-token route to bad_jwt is mostly closed. A rotated signing key, clock skew beyond the margin and a banned user remain.
What might change. AuthApiError already carries code (errors.ts#L64-L67), populated from the body (fetch.ts#L108-L112), so the filter can key on it:
const goalAlreadyReached =
isAuthSessionMissingError(error) ||
(isAuthApiError(error) &&
(error.code === 'session_not_found' || error.code === 'user_not_found'))Everything else returns the error, with the session kept for global and others. One refinement worth a line in the doc: under others, session_not_found describes the current session, not the others, so it is better surfaced than swallowed. Either way, the sentence at L4048 should not ship until this is settled, because once documented it is a contract rather than a fossil. The tests cannot catch any of this today: every mocked error body is { message: 'x' }, with no code.
2. local is documented as teardown-first but runs request-first
Observed. The doc comment reads "The local session is removed, and the client makes a best-effort request to revoke" (#L4040-L4041). The code awaits the request first (#L4116) and removes the session afterwards, on either outcome.
Why it matters. The outcome is the same, the timing is not. SIGNED_OUT fires only after the round trip, and a /logout that hangs rather than fails holds signOut({ scope: 'local' }) and the auth lock open for as long as the fetch takes, since no AbortSignal is passed and _request has no timeout of its own (fetch.ts#L56). An offline fetch rejects at once, so the tests pass. The thread's reason for teardown-first on local was exactly this: leaving this device must never depend on the network.
What might change. For local, capture the access token, call removeCurrentSession(), then send the request with the captured token and return its answer as a report. If request-first is the preferred shape for auth-js, the doc comment should say so instead, but then the local promise is only as good as the network.
Where this should be settled
We agreed in the thread that the spec in supabase/sdk is the venue. Today specs/auth/session/ holds get_claims.md, get_user.md and sign_out_reason.md and no sign_out.md, and auth.session.sign_out in capabilities/auth.yaml#L251-L254 is one sentence. This PR is the first SDK to move, which makes its doc comment the de facto spec, status filter included, for the Swift, Flutter, Kotlin and Python fixes to follow. The suggestion is to merge the global fix, since it is a real bug, and land a sign_out.md that fixes three things before the doc comment claims them: the scope is always sent, the ordering per scope and which server answers count as success. Then each SDK converges on the spec rather than on auth-js as it stands.
Small things
- "acknowleding" at
#L4128. - @shazilkhan's release-note point stands:
globalis the default, so a plainsignOut()while offline now keeps the user signed in and returns the error. That is the right behaviour, and a changelog line showing the fallback (global, then{ scope: 'local' }on failure) will save the next #2501.
While the status code handling is "wrong", it can only be changed for v3.
|
After our internal discussion, I updated the PR with:
|
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
| } | ||
| } | ||
| if (scope !== 'others') { | ||
| if (scope === 'global') { |
There was a problem hiding this comment.
🟡 Severity: MEDIUM
scope is optional in SignOut, so signOut({}) leaves it undefined; admin.signOut(..., undefined) applies its default global scope and can successfully revoke server sessions, but this strict check skips local cleanup. The stored access token remains usable until JWT expiry and no SIGNED_OUT event fires, leaving the user locally authenticated after signing out.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: Treat an undefined scope as 'global', which is what admin.signOut already does. The cleanest fix is to change the _signOut signature to { scope = 'global' }: SignOut = {}, so that signOut({}) also defaults to 'global'. That edit is about 48 lines above the flagged line, so the suggestion below changes only the check at the flagged line. Local cleanup then runs whenever the server applies the default global scope.
⚠️ Experimental Feature: This code suggestion is automatically generated. Please review carefully.
| if (scope === 'global') { | |
| if (scope === 'global' || scope === undefined) { |
🔍 Description
#2504 fixed it so
signOut(scope = local)always succeeds, regardless of network connectivity. Unfortunately, the fix also impactedsignOut(scope = global), so the local user would always be logged out regardless of whether the server logout was accepted. This could result in the client being logged out, no longer having the credentials to fix the situation with other sessions.This PR fixes this problem.
What changed?
With this change, a
globalsign out now (correctly) only succeeds and removes the local credentials after the server has confirmed the request. Any network failure will result in theglobalsign-out reporting the error. The user can then either choose to retry theglobaloperation or switch tolocal.The documentation has been modified to accurately describe these various edge cases.
The test suite has also been improved so it covers them.
Why was this change needed?
The previous behavior was an unintentional bug.
Closes #2779
🔄 Breaking changes
📋 Checklist
<type>(<scope>): <description>pnpm nx formatto ensure consistent code formatting