Uh oh!
There was an error while loading. Please reload this page.
fix(web): reject OAuth account-linking without a signed-in session - #1221
Merged
Conversation
If a user clicks "Connect Bitbucket" and their session-token cookie is missing or expired by the time the BB redirect arrives at our callback, @auth/core silently falls through to createUser and mints a new orphan User row from the OAuth profile. The orphan has no email, no UserToOrg, and the user's session cookie gets rebound to it, leaving them on a "request access" page. Add a signIn callback that calls auth() and refuses the request when the provider's purpose is account_linking and no session is present. SSO providers and credentials login are unaffected. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Caution Review failedPull request was closed or merged during review Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a NextAuth ChangesOAuth Account-Linking Session Validation
Sequence DiagramsequenceDiagram
participant Client
participant NextAuth
participant ProviderRegistry
participant AuthResolver
Client->>NextAuth: OAuth/OIDC provider callback (account)
NextAuth->>ProviderRegistry: getProviders() -> find provider for account.provider
NextAuth->>AuthResolver: auth() to resolve current session (if provider.purpose == "account_linking")
AuthResolver-->>NextAuth: session or null
NextAuth-->>Client: allow or deny sign-in (deny if account_linking && session == null)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
The fix is gated behind the sso entitlement (no OAuth identity providers are loaded in OSS deployments), so the [EE] prefix is appropriate per CLAUDE.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/web/src/auth.ts (1)
283-290: ⚡ Quick winOnly resolve the session for account-linking flows.
auth()runs on every sign-in attempt, but only theaccount_linkingbranch uses its result. Moving the lookup inside that branch keeps credentials/email/SSO logins off the extra session decode/DB path.♻️ Proposed change
- const isAccountLinkingAttempt = matchingProvider?.purpose === 'account_linking';- const session = await auth();-- if (isAccountLinkingAttempt && session === null) {- return false;- }-- return true;+ if (matchingProvider?.purpose !== 'account_linking') {+ return true;+ }++ const session = await auth();+ return session !== null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/auth.ts` around lines 283 - 290, The code currently calls auth() unconditionally; change it so we only call auth() when matchingProvider?.purpose === 'account_linking' by moving the session lookup inside that branch: compute isAccountLinkingAttempt from matchingProvider, and if true then await auth() and return false if the session is null, otherwise proceed to return true; remove the unconditional auth() call so non-account_linking sign-ins avoid the extra session decode/DB path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/web/src/auth.ts`:
- Around line 243-260: Extract a shared helper (e.g.,
getEffectiveProviderId(provider)) that returns config.options?.id ?? config.id
and replace the inline resolver in callbacks.signIn() (where matchingProvider is
computed) and in getIssuerUrlForAccount() so both paths use the same effective
provider id logic; additionally, defer calling auth() to fetch the session by
moving "const session = await auth()" behind a guard so it only runs when
matchingProvider?.purpose === 'account_linking' (instead of running
unconditionally during every sign-in).
---
Nitpick comments:
In `@packages/web/src/auth.ts`:
- Around line 283-290: The code currently calls auth() unconditionally; change
it so we only call auth() when matchingProvider?.purpose === 'account_linking'
by moving the session lookup inside that branch: compute isAccountLinkingAttempt
from matchingProvider, and if true then await auth() and return false if the
session is null, otherwise proceed to return true; remove the unconditional
auth() call so non-account_linking sign-ins avoid the extra session decode/DB
path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Uh oh!
There was an error while loading. Please reload this page.
Shared between signIn callback and getIssuerUrlForAccount. The latter previously read only the top-level provider id, which silently returned undefined for factory-based providers with an `id:` override (Bitbucket Cloud, GitHub, GitLab, Google, Okta, Keycloak, Entra, Authentik), meaning the stored issuerUrl was never resolved correctly for those accounts.
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 freeto 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
OAuth providers configured with `purpose: "account_linking"` should only ever attach an identity to an existing signed-in user, never mint a new Sourcebot user. `@auth/core` does not know about this distinction and falls back to `createUser` when its session lookup returns null, which can happen when the user's session cookie expires while they are on the upstream consent screen. The result is a silent orphan `User` row and a confused user.
Add a `signIn` callback that refuses the request in that case.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation