Fix/link identity provider token#2069
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds handling for OAuth redirect URLs that include only provider_token and provider_refresh_token. On detecting such an implicit-grant callback, the client loads the existing session via _loadSession/_useSession, merges the provider tokens into that session object, clears the URL fragment, and returns the augmented session along with redirect metadata. Sequence Diagram(s)sequenceDiagram
participant Browser
participant GoTrueClient
participant SessionStore as _loadSession
Browser->>GoTrueClient: Redirect URL with provider_token & provider_refresh_token
GoTrueClient->>GoTrueClient: detect implicit grant via provider tokens
GoTrueClient->>SessionStore: _loadSession()
SessionStore-->>GoTrueClient: existing session (access_token present)
GoTrueClient->>GoTrueClient: merge provider_token(s) into session
GoTrueClient->>Browser: clear URL fragment
GoTrueClient-->>Browser: return augmented session + redirectType
Assessment against linked issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/core/auth-js/src/GoTrueClient.ts`:
- Around line 2048-2067: Replace the direct call to this.__loadSession() in
_getSessionFromURL with this._useSession() so the session load honors the
documented locking contract; call _useSession to obtain the same { data:
sessionData, error: sessionError } result (if _useSession uses a callback API,
invoke it to read session state inside the provided callback) and keep the
existing error handling, session existence check, creation of the Session object
(provider_token/provider_refresh_token), clearing window.location.hash, _debug
call, and the _returnResult({ data: { session, redirectType: params.type },
error: null }) return path.
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
| } = params | ||
|
|
||
| if (!access_token || !expires_in || !refresh_token || !token_type) { | ||
| if (provider_token || provider_refresh_token) { |
There was a problem hiding this comment.
🟠 Severity: HIGH
CSRF/Token Injection Vulnerability: This code allows arbitrary provider_token and provider_refresh_token values from URL fragments to be injected into existing sessions without validation. An attacker can craft a malicious URL and trick users into visiting it, causing attacker-controlled provider tokens to be merged into the victim's session. There's no CSRF protection (state parameter), origin validation, or server-side verification that these tokens are legitimate.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: This vulnerability requires server-side validation and CSRF protection. Implement the following multi-step fix: (1) Add a 'state' parameter to the OAuth flow that's generated server-side and validated on callback. Store the state parameter securely (e.g., in session storage with CSRF token) before initiating the OAuth flow. (2) Before merging provider_token and provider_refresh_token into the session, validate the state parameter from the URL matches the stored value. (3) Send the provider tokens to the auth server endpoint (e.g., POST to /token/verify) to verify they were issued for the current authenticated user before accepting them. (4) Only merge the tokens if both state validation and server-side token verification succeed. (5) Consider rejecting provider-token-only callbacks entirely if they're not expected in the linkIdentity flow, or require them to come with additional proof of authenticity from the auth server.
There was a problem hiding this comment.
Updated to use _useSession in _getSessionFromURL for provider‑token‑only callbacks to honor the documented locking contract.
We only merge provider tokens when a valid session already exists (user must be signed in), and the callback is handled by the Supabase auth redirect flow. Still, if maintainers want stricter validation or a type=link gate, I can add that in a follow‑up.
mandarini
force-pushed
the
fix/link-identity-provider-token
branch
from
cdb5220 to
c30e189
Compare
@supabase/auth-js
@supabase/functions-js
@supabase/postgrest-js
@supabase/realtime-js
@supabase/storage-js
@supabase/supabase-js
commit: |
|
Hi there @samarth212 , thanks for contributing to Supabase! :D I do have some changes to request on your PR!
// external.go ~L268
} else if token != nil {
q := url.Values{}
q.Set("provider_token", providerAccessToken)
if providerRefreshToken != "" {
q.Set("provider_refresh_token", providerRefreshToken)
}
rurl = token.AsRedirectURL(rurl, q) // always adds access_token, refresh_token, expires_in, token_type
}and // tokens/service.go ~L144
func (r *AccessTokenResponse) AsRedirectURL(...) string {
extraParams.Set("access_token", r.Token)
extraParams.Set("token_type", r.TokenType)
extraParams.Set("expires_in", strconv.Itoa(r.ExpiresIn))
extraParams.Set("refresh_token", r.RefreshToken)
...
}A redirect with provider_token but no access_token is not a scenario the current server produces for implicit flow. The new branch in _getSessionFromURL would never be reached under normal server operation and it can only be triggered by a manually crafted URL. Could you share a reproduction case or server version where this occurs? It's possible the issue originated from an older server behavior or a specific edge case worth documenting.
These parameter names are not Supabase-specific. More importantly, the server already stamps every implicit grant redirect with sb= specifically to help clients identify Supabase callbacks: // tokens/service.go L150
extraParams.Set("sb", "") // "Add Supabase Auth identifier to help clients distinguish Supabase Auth redirects"If detection needs broadening, params.sb !== undefined is a more reliable and intentional signal.
In summary: Before this can move forward, it would really help to understand the exact server version or configuration that produces a provider-token-only redirect. If that scenario is real, the fix should also consider using params.sb for safer detection, adding a getUser() refresh, and addressing the injection risk. If the root cause turns out to be a PKCE code-exchange issue (where provider tokens aren't forwarded to the client), the fix likely belongs in _exchangeCodeForSession instead. Once again, thank you very much for your contribution! |
mandarini
left a comment
mandarini
left a comment
There was a problem hiding this comment.
Please read my comment
2 participants
fix(auth): allow provider tokens on linkIdentity callbacks
🔍 Description
Fixes linkIdentity callbacks that only include provider tokens so apps can access
provider_token/provider_refresh_tokenafter linking.What changed?
provider_token/provider_refresh_tokenas valid implicit OAuth callbacks.Why was this change needed?
OAuth identity linking can return only provider tokens on redirect. The client previously ignored those callbacks, making
provider_tokeninaccessible and linkIdentity far less useful.Closes #1676
📸 Screenshots/Examples
N/A
🔄 Breaking changes
📋 Checklist
📝 Additional notes
Tests run:
npx nx test:auth auth-jsN/A (no docs changes required)