fix(sso): check providers array instead of provider for SSO auth#1833
Merged
Conversation
…dation app_metadata.provider (singular) is rewritten by Supabase Auth on every login based on the oldest/primary identity — after an account merge where the original user has an email identity, Supabase resets provider='email' even when the user logs in via SSO. providers[] (plural) is cumulative and retains sso:X once set. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughUpdated SSO detection and validation to read multiple providers from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
supabase/functions/_backend/private/sso/provision-user.ts (1)
127-127: Passing empty string togetTrustedSsoProvidersmay add''to trusted providers.When
userProviderisundefined, this passes''togetTrustedSsoProviders. Looking at that function (lines 45-61), the predicateisTrustedSsoProviderincludesprovider === userProvider, which would evaluate to'' === ''→true, causing an empty string to be added to the trusted providers set.While this won't cause runtime failures (the line 121 check would reject cases where both
userProvideranduserProvidersare empty/non-SSO), the resulting['', 'sso:...']array is semantically incorrect and could match unintended rows in database queries.♻️ Proposed fix: guard against empty userProvider
- const trustedSsoProviders = getTrustedSsoProviders(userProvider ?? '', userIdentities) + const trustedSsoProviders = getTrustedSsoProviders(userProvider || null, userIdentities)And update the function signature to handle
null:-function getTrustedSsoProviders(userProvider: string, userIdentities: any[]): string[] { +function getTrustedSsoProviders(userProvider: string | null, userIdentities: any[]): string[] { const trustedProviders = new Set<string>() - const isTrustedSsoProvider = (provider: string) => provider === 'sso' || provider.startsWith('sso:') || provider === userProvider + const isTrustedSsoProvider = (provider: string) => provider === 'sso' || provider.startsWith('sso:') || (userProvider && provider === userProvider) - if (isTrustedSsoProvider(userProvider)) { + if (userProvider && isTrustedSsoProvider(userProvider)) { trustedProviders.add(userProvider) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/private/sso/provision-user.ts` at line 127, The call to getTrustedSsoProviders currently passes userProvider ?? '' which allows an empty string to be treated as a trusted provider; change the caller to pass userProvider (or null/undefined) directly instead of coercing to '' and update getTrustedSsoProviders signature/logic so its predicate (isTrustedSsoProvider) explicitly guards for a non-empty userProvider (e.g., check provider === userProvider only when userProvider is truthy) to prevent '' being added to the trusted providers set; refer to the symbols userProvider, getTrustedSsoProviders, and isTrustedSsoProvider when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@supabase/functions/_backend/private/sso/provision-user.ts`:
- Around line 119-124: The enforcement check only inspects
claims?.app_metadata?.provider and treats 'email' as non-SSO; update the logic
in check-enforcement.ts to mirror provision-user.ts by checking both
app_metadata.provider and app_metadata.providers (array). Implement an
isSsoProvider helper (e.g., p => p === 'sso' || p.startsWith('sso:')) and set
isSsoAuth to true if either provider passes isSsoProvider or any entry in
providers[] passes isSsoProvider, then use that boolean for enforcement
decisions (replace the current provider/isSsoAuth logic).
---
Nitpick comments:
In `@supabase/functions/_backend/private/sso/provision-user.ts`:
- Line 127: The call to getTrustedSsoProviders currently passes userProvider ??
'' which allows an empty string to be treated as a trusted provider; change the
caller to pass userProvider (or null/undefined) directly instead of coercing to
'' and update getTrustedSsoProviders signature/logic so its predicate
(isTrustedSsoProvider) explicitly guards for a non-empty userProvider (e.g.,
check provider === userProvider only when userProvider is truthy) to prevent ''
being added to the trusted providers set; refer to the symbols userProvider,
getTrustedSsoProviders, and isTrustedSsoProvider when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b320606a-6f66-4748-9cfe-6634ef670545
📒 Files selected for processing (1)
supabase/functions/_backend/private/sso/provision-user.ts
…er.ts Same root cause: after account merge Supabase resets app_metadata.provider to 'email' while providers[] retains sso:X. Without this fix, SSO-merged users would be incorrectly blocked by the enforcement check on email/password login attempts, even though they authenticated via SSO. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
supabase/functions/_backend/private/sso/check-enforcement.ts (1)
33-39: Good fix for the SSO detection stability issue.The approach of checking both
provider(singular) andproviders[](array) correctly addresses the Supabase account-merge behavior described in the PR. The logic maintains backward compatibility by still checking the singular provider while adding the cumulative array check.One optional hardening: the
providersarray elements are assumed to be strings. If Supabase ever stores a non-string value,startsWith()would throw. Consider filtering for type safety:💡 Optional defensive typing
- const providers: string[] = Array.isArray(claims?.app_metadata?.providers) ? claims.app_metadata.providers as string[] : [] + const providers: string[] = Array.isArray(claims?.app_metadata?.providers) + ? (claims.app_metadata.providers as unknown[]).filter((p): p is string => typeof p === 'string') + : []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/private/sso/check-enforcement.ts` around lines 33 - 39, The current SSO detection uses providers array elements directly which can throw if a non-string appears; update the logic around provider, providers, isSsoProvider, and isSsoAuth to defensively coerce/filter providers to strings before testing—e.g., ensure providers is filtered/ mapped to string values (or skip non-strings) and then apply isSsoProvider and providers.some(isSsoProvider) so startsWith is only called on strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@supabase/functions/_backend/private/sso/check-enforcement.ts`:
- Around line 33-39: The current SSO detection uses providers array elements
directly which can throw if a non-string appears; update the logic around
provider, providers, isSsoProvider, and isSsoAuth to defensively coerce/filter
providers to strings before testing—e.g., ensure providers is filtered/ mapped
to string values (or skip non-strings) and then apply isSsoProvider and
providers.some(isSsoProvider) so startsWith is only called on strings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: de34d671-97df-464f-9923-60408ec80374
📒 Files selected for processing (1)
supabase/functions/_backend/private/sso/check-enforcement.ts
|
4 tasks
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
app_metadata.provider(singular) is rewritten by Supabase Auth on every login based on the oldest/primary identity. After an account merge where the original user has an email identity, Supabase resetsprovider='email'even when the user logs in via SSO — causingsso_auth_requiredon subsequent logins.app_metadata.providers[](array) instead. This is cumulative — Supabase addssso:Xonce and never removes it, so the check is stable regardless of which provider was used last.Test plan
sso_auth_required🤖 Generated with Claude Code
Summary by CodeRabbit