fix(auth): preserve intended URL through login redirects - #1650
fix(auth): preserve intended URL through login redirects#1650brianphillips wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (17)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. WalkthroughAuthentication redirects now preserve validated paths and query strings across login, signup, verification, OAuth, magic-link, credential, chat, and request-header flows. ChangesAuthentication callback URL preservation
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to Authentication redirects preserve safe original destinations across the updated login flows, with no unresolved material risk identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 16 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/web/src/lib/authRedirect.ts">
<violation number="1" location="packages/web/src/lib/authRedirect.ts:29">
P0: normalizeCallbackUrl returns a protocol-relative `//evil.com` for crafted inputs like `/..//evil.com` or `/%2e%2e//evil.com`, because the URL parser removes the dot-segment and normalizes the following `//` into the pathname. The raw-input `startsWith('//')` guard (lines 17-18) and the origin check (line 25) both pass, so this value flows into `redirect(callbackUrl)` in login/page.tsx and verify/page.tsx, producing an open redirect to an external host. Check the parsed `url.pathname` for a leading `//` after the origin check and reject it.</violation>
</file>
<file name="packages/web/src/proxy.ts">
<violation number="1" location="packages/web/src/proxy.ts:41">
P3: The middleware sets x-sourcebot-request-path on every request, but only the (app) layout and authenticatedPage HOC read it. API routes, /login, /signup, /oauth, and static assets get an unused header plus a request rewrite. Restrict it to the (app) segment, or only compute the header on the routes that consume it.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| return `${url.pathname}${url.search}`; | ||
| } catch { | ||
| return '/'; | ||
| } | ||
| } |
There was a problem hiding this comment.
P0: normalizeCallbackUrl returns a protocol-relative //evil.com for crafted inputs like /..//evil.com or /%2e%2e//evil.com, because the URL parser removes the dot-segment and normalizes the following // into the pathname. The raw-input startsWith('//') guard (lines 17-18) and the origin check (line 25) both pass, so this value flows into redirect(callbackUrl) in login/page.tsx and verify/page.tsx, producing an open redirect to an external host. Check the parsed url.pathname for a leading // after the origin check and reject it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/web/src/lib/authRedirect.ts, line 29:
<comment>normalizeCallbackUrl returns a protocol-relative `//evil.com` for crafted inputs like `/..//evil.com` or `/%2e%2e//evil.com`, because the URL parser removes the dot-segment and normalizes the following `//` into the pathname. The raw-input `startsWith('//')` guard (lines 17-18) and the origin check (line 25) both pass, so this value flows into `redirect(callbackUrl)` in login/page.tsx and verify/page.tsx, producing an open redirect to an external host. Check the parsed `url.pathname` for a leading `//` after the origin check and reject it.</comment>
<file context>
@@ -0,0 +1,37 @@
+ return '/';
+ }
+
+ return `${url.pathname}${url.search}`;
+ } catch {
+ return '/';
</file context>
| return `${url.pathname}${url.search}`; | |
| } catch { | |
| return '/'; | |
| } | |
| } | |
| if (url.origin !== CALLBACK_URL_ORIGIN || url.pathname.startsWith('//')) { | |
| return '/'; | |
| } | |
| return `${url.pathname}${url.search}`; |
|
|
||
| return NextResponse.next(); | ||
| const requestHeaders = new Headers(request.headers); | ||
| requestHeaders.set(REQUEST_PATH_HEADER, `${request.nextUrl.pathname}${request.nextUrl.search}`); |
There was a problem hiding this comment.
P3: The middleware sets x-sourcebot-request-path on every request, but only the (app) layout and authenticatedPage HOC read it. API routes, /login, /signup, /oauth, and static assets get an unused header plus a request rewrite. Restrict it to the (app) segment, or only compute the header on the routes that consume it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/web/src/proxy.ts, line 41:
<comment>The middleware sets x-sourcebot-request-path on every request, but only the (app) layout and authenticatedPage HOC read it. API routes, /login, /signup, /oauth, and static assets get an unused header plus a request rewrite. Restrict it to the (app) segment, or only compute the header on the routes that consume it.</comment>
<file context>
@@ -36,5 +37,12 @@ export async function proxy(request: NextRequest) {
- return NextResponse.next();
+ const requestHeaders = new Headers(request.headers);
+ requestHeaders.set(REQUEST_PATH_HEADER, `${request.nextUrl.pathname}${request.nextUrl.search}`);
+
+ return NextResponse.next({
</file context>
1 participant
Fixes #1649
Summary
Verification
yarn workspace @sourcebot/web test src/lib/authRedirect.test.ts --runyarn workspace @sourcebot/web exec tsc --noEmityarn workspace @sourcebot/web lintNo visual changes.
Note
Medium Risk
Touches authentication redirect boundaries across login guards and callback URL handling; validation reduces open-redirect risk but mis-handled callbacks could still send users to the wrong page after sign-in.
Overview
Fixes login redirects dropping query strings so users return to the same search or Ask/chat URL after signing in.
Introduces
normalizeCallbackUrlandcreateLoginUrlinauthRedirect.ts(with unit tests) to keep only safe same-origin relative paths and reject open-redirect targets. The edgeproxynow stampsx-sourcebot-request-pathwith the current path and search; the app layout, auth middleware, GCP IAP, and account-linking flows use that value when sending users to login or post-auth callbacks.Login, signup, magic-link verification, OAuth/credentials flows, and in-app sign-in links (chat share, banners, MCP connectors, login dialog) now pass the full
pathname?querythroughcallbackUrlinstead of path-only or/.Reviewed by Cursor Bugbot for commit b6b0b93. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes #1649. Auth guards now preserve the original path and query string when redirecting to login, so users land back on the page they intended after authenticating instead of the home page. Callback URLs are now validated as same-origin relative paths before use to prevent open redirects.
Bug Fixes
normalizeCallbackUrlandcreateLoginUrlhelpers with unit tests inpackages/web/src/lib/authRedirect.ts.x-sourcebot-request-pathheader set inproxy.tsand read by the app layout and authenticated page middleware./loginor/signupare now redirected to the requested URL instead of/.Written for commit b6b0b93. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests
Documentation