fix(cross-domain): follow redirect on linkSocial callbacks (no newSession)#389
fix(cross-domain): follow redirect on linkSocial callbacks (no newSession)#389mishafyi wants to merge 1 commit into
Conversation
…sion) When an already-authenticated user links an additional social account, the OAuth callback runs through the cross-domain after-hook but ctx.context.newSession is undefined (the user already has a session, so no new one is created). The hook logs "No session found" and returns, swallowing the provider redirect — the browser dead-ends on the callback URL instead of returning to the app. Follow the redirect from the response headers in that case; the one-time-token flow is only needed when a session was just created.
|
@mishafyi is attempting to deploy a commit to the Convex Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe cross-domain plugin's OAuth/magic-link callback handler now preserves provider redirect behavior for callbacks without a new session. When Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/plugins/cross-domain/index.ts (1)
169-169: 💤 Low valueConsider extracting redirect location retrieval to reduce duplication.
The redirect location is fetched from
ctx.context.responseHeaders?.get("location")at two points: line 169 (no-session branch) and line 183 (session branch). While both branches need the value independently, extracting it once near the top of the handler would establish a single source of truth.♻️ Optional refactor to reduce duplication
handler: createAuthMiddleware(async (ctx) => { // Mostly copied from the one-time-token plugin + const redirectTo = ctx.context.responseHeaders?.get("location"); const session = ctx.context.newSession; if (!session) { // linkSocial callbacks have no newSession — the user is already // signed in. Follow the provider redirect without the // one-time-token flow instead of dead-ending with an error. - const redirectTo = ctx.context.responseHeaders?.get("location"); if (redirectTo) { throw ctx.redirect(redirectTo); } ctx.context.logger.error("No session found"); return; } const token = generateRandomString(32); const expiresAt = new Date(Date.now() + 3 * 60 * 1000); await ctx.context.internalAdapter.createVerificationValue({ value: session.session.token, identifier: `one-time-token:${token}`, expiresAt, }); - const redirectTo = ctx.context.responseHeaders?.get("location"); if (!redirectTo) { ctx.context.logger.error("No redirect to found"); return; } const url = new URL(redirectTo); url.searchParams.set("ott", token); throw ctx.redirect(url.toString()); }),Also applies to: 183-183
🤖 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 `@src/plugins/cross-domain/index.ts` at line 169, Extract the redirect location retrieval into a single variable at the start of the request handler so both branches use the same source of truth: add a local const (e.g., redirectTo) assigned from ctx.context.responseHeaders?.get("location") near the top of the handler in src/plugins/cross-domain/index.ts and replace the duplicate occurrences inside the no-session and session branches (the spots currently reading ctx.context.responseHeaders?.get("location")). This keeps logic centralized and avoids duplicated calls.
🤖 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.
Nitpick comments:
In `@src/plugins/cross-domain/index.ts`:
- Line 169: Extract the redirect location retrieval into a single variable at
the start of the request handler so both branches use the same source of truth:
add a local const (e.g., redirectTo) assigned from
ctx.context.responseHeaders?.get("location") near the top of the handler in
src/plugins/cross-domain/index.ts and replace the duplicate occurrences inside
the no-session and session branches (the spots currently reading
ctx.context.responseHeaders?.get("location")). This keeps logic centralized and
avoids duplicated calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: get-convex/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 85664097-326d-4c91-91c7-6b0fe0cc7d79
📒 Files selected for processing (1)
src/plugins/cross-domain/index.ts
|
@mishafyi thanks for this! Have you tried running without your patch in newer versions of better auth? I'm seeing the bare return still allows the redirect to be followed (although an error is logged, we should at least change that). |
Problem
When an already-authenticated user calls
linkSocialand completes the provider OAuth flow, the callback passes through the cross-domain plugin's after-hook. Because the user already has a session,ctx.context.newSessionisundefined— the hook logsNo session foundand returns, swallowing the provider's redirect. The browser dead-ends on the callback URL instead of returning to the app, so account linking appears broken whenever the cross-domain plugin is active.Fix
When there is no new session but the response carries a
locationheader, follow it. The one-time-token flow is only meaningful when a session was just created; for linkSocial the existing session is already valid on the client.We've been running this as a patch in production (Convex self-hosted + GitHub account linking) since the 0.10.x line — happy to adjust the approach if you'd prefer the check somewhere else (e.g. gating the matcher on
linkSocialcallbacks instead).