Conversation
When opening a deep link while unauthenticated, sign-in would redirect to /apis instead of the original destination. Now the original path is preserved as a `redirect` query parameter through the entire auth flow - middleware, OAuth, email verification, and org selection. - Middleware (proxy.ts) captures the current path in the redirect param - Server-side getAuth() sets a custom x-current-path header for fallback - SSO callback preserves redirect through org selection and email verify - Auto and manual org selection use window.location.href for clean state - Workspace slug is rewritten to match the selected org after sign-in - Open redirect prevention via allowlist validation on redirect paths so we can hijack into a new url or javacript.
perkinsjr
requested review from
Flo4604,
MichaelUnkey,
chronark,
imeyer,
mcstepp and
ogzhanolguncu
as code owners
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis pull request implements a redirect-back-to-original-link feature for the authentication flow. When users access a deep link while unauthenticated, their original destination is captured via query parameters, persisted through the sign-in process, and used to redirect them back after authentication and org selection complete. Changes
Sequence DiagramsequenceDiagram
actor User
participant Client as Client Browser
participant Middleware as Middleware<br/>(proxy.ts)
participant Layout as Layout/Guards<br/>(layout.tsx, auth.ts)
participant SignIn as Sign-in Page
participant Actions as Auth Actions
participant DB as Database
participant OAuth as OAuth Provider
User->>Client: Click deep link<br/>(/workspace/projects/proj_X)
Client->>Middleware: GET /workspace/projects/proj_X
alt Unauthenticated
Middleware->>Middleware: No session cookie
Middleware->>SignIn: Redirect to /auth/sign-in?<br/>redirect=%2Fworkspace%2Fprojects%2Fproj_X
SignIn->>SignIn: Extract & validate<br/>redirect param
SignIn->>Client: persist redirect to<br/>sessionStorage
Client->>User: Show sign-in form
User->>OAuth: Initiate OAuth flow
OAuth->>OAuth: Authenticate user
OAuth->>SignIn: Return to callback<br/>with state
SignIn->>SignIn: Extract redirectUrlComplete<br/>from state
Client->>Actions: completeOrgSelection(orgId)
Actions->>DB: Query workspace slug<br/>for selected org
DB-->>Actions: Return slug
Actions-->>Client: Return with workspaceSlug
Client->>Client: Retrieve redirect from<br/>sessionStorage
Client->>Client: resolveRedirectUrl(<br/>redirect, workspaceSlug)<br/>Rewrite first path<br/>segment if needed
Client->>Client: window.location.href =<br/>resolved URL
Client->>Middleware: GET /workspace/projects/proj_X<br/>(now authenticated)
Middleware->>Middleware: Session exists,<br/>set x-current-path header
Middleware->>Layout: Forward request
Layout->>Layout: Verify auth, render content
Layout-->>Client: Render page
Client->>User: Display deployment details
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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)
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: 3
🧹 Nitpick comments (1)
web/apps/dashboard/app/auth/sign-in/redirect-utils.ts (1)
19-28: Path traversal segments (..) pass validation and can defeat workspace slug rewriting.The regex allows
.characters, so..segments pass validation. This isn't an open redirect vulnerability (paths stay same-origin), but it can bypass the workspace slug rewriting logic:
- Attacker crafts:
/attacker-slug/../sensitive-pageresolveRedirectUrlrewrites to:/user-workspace/../sensitive-page- Browser normalizes to:
/sensitive-pageConsider explicitly rejecting
..and.segments:🛡️ Proposed fix to reject traversal segments
for (let i = 1; i < segments.length; i++) { // Allow empty segments only for trailing slash (last segment) if (segments[i] === "" && i === segments.length - 1) { continue; } + // Reject path traversal segments + if (segments[i] === "." || segments[i] === "..") { + return false; + } // Segments must be non-empty and contain only URL-safe path characters if (!segments[i] || !/^[a-zA-Z0-9\-._~!$&'()*+,;=@%]+$/.test(segments[i])) { return false; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/apps/dashboard/app/auth/sign-in/redirect-utils.ts` around lines 19 - 28, The validation loop over URL path segments (the for loop that checks segments[i] with the regex in redirect-utils.ts) currently allows "." and ".." because the regex includes dots; update the segment validation to explicitly reject traversal segments by returning false when a segment equals "." or ".." (in addition to the existing empty/regex checks) so resolveRedirectUrl cannot be bypassed by path traversal tokens; locate the loop that iterates over segments and add the explicit checks for "." and ".." before the regex test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/apps/dashboard/app/auth/sign-in/org-selector.tsx`:
- Around line 89-95: The redirect rewrite is currently applied to all safe paths
causing global routes like /settings or /apis to be overwritten; update the flow
around resolveRedirectUrl (called in org-selector.tsx with consumeRedirectUrl
and result.workspaceSlug) so it only rewrites the first path segment when the
deepLink is a workspace-scoped route (e.g., matches your workspace route pattern
or prefix used across the app), otherwise leave the original deepLink intact and
skip resolveRedirectUrl. In practice, add a guard that detects workspace-scoped
paths before calling resolveRedirectUrl (or make resolveRedirectUrl perform this
check internally) and fall back to result.redirectTo or the original deepLink
when the path is not workspace-scoped.
In `@web/apps/dashboard/app/auth/sso-callback/`[[...sso-callback]]/route.ts:
- Around line 19-29: The code double-decodes the state by calling
decodeURIComponent on the value from request.nextUrl.searchParams.get("state")
and uses unsafe casts to access redirectUrlComplete; remove the
decodeURIComponent call and parse the raw state with JSON.parse, then replace
the inline `as` assertions with a proper type guard (e.g., function
isRedirectState(x: unknown): x is { redirectUrlComplete: string }) or validate
with a schema (Zod) before reading redirectUrlComplete, and apply the same fix
in web/apps/dashboard/lib/auth/workos.ts.
In `@web/apps/dashboard/proxy.ts`:
- Around line 84-89: The middleware currently sets x-current-path on the
response via headers.set and calls NextResponse.next({ headers }), so downstream
server components can't read it; change the NextResponse.next call to forward
the headers as request headers by calling NextResponse.next({ request: { headers
} }) so that code reading request headers (e.g., web/apps/dashboard/lib/auth.ts
using headers() from 'next/headers') can access x-current-path; locate the
headers variable and the NextResponse.next invocation in proxy.ts and update the
options shape accordingly.
---
Nitpick comments:
In `@web/apps/dashboard/app/auth/sign-in/redirect-utils.ts`:
- Around line 19-28: The validation loop over URL path segments (the for loop
that checks segments[i] with the regex in redirect-utils.ts) currently allows
"." and ".." because the regex includes dots; update the segment validation to
explicitly reject traversal segments by returning false when a segment equals
"." or ".." (in addition to the existing empty/regex checks) so
resolveRedirectUrl cannot be bypassed by path traversal tokens; locate the loop
that iterates over segments and add the explicit checks for "." and ".." before
the regex test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 59fd12b7-1e14-490d-86c8-4cb6308f5131
📒 Files selected for processing (10)
web/apps/dashboard/app/(app)/layout.tsxweb/apps/dashboard/app/auth/actions.tsweb/apps/dashboard/app/auth/hooks/useSignIn.tsweb/apps/dashboard/app/auth/sign-in/[[...sign-in]]/page.tsxweb/apps/dashboard/app/auth/sign-in/oauth-signin.tsxweb/apps/dashboard/app/auth/sign-in/org-selector.tsxweb/apps/dashboard/app/auth/sign-in/redirect-utils.tsweb/apps/dashboard/app/auth/sso-callback/[[...sso-callback]]/route.tsweb/apps/dashboard/lib/auth.tsweb/apps/dashboard/proxy.ts
| const state = request.nextUrl.searchParams.get("state"); | ||
| if (state) { | ||
| try { | ||
| const parsed: unknown = JSON.parse(decodeURIComponent(state)); | ||
| const redirectUrlComplete = | ||
| typeof parsed === "object" && | ||
| parsed !== null && | ||
| "redirectUrlComplete" in parsed && | ||
| typeof (parsed as { redirectUrlComplete: unknown }).redirectUrlComplete === "string" | ||
| ? (parsed as { redirectUrlComplete: string }).redirectUrlComplete | ||
| : null; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does URLSearchParams.get()return percent-decoded values, and would callingdecodeURIComponent()on that result double-decode query parameter content such as%2For%25?
💡 Result:
Yes, URLSearchParams.get returns percent-decoded values. Calling decodeURIComponent on the result would double-decode query parameter content such as %2F (which decodes to /) or %25 (which decodes to %). When URLSearchParams parses a query string like ?key=%2F%25, it percent-decodes the value to "/%" during parsing. The get("key") method then returns this already-decoded string "/%". Applying decodeURIComponent to "/%" would interpret % as a percent-encoded sequence: - %2F in the original query becomes / (already decoded by URLSearchParams). - %25 becomes %. Further decoding "/%" treats the % (not followed by valid hex digits in this context, but if it were %25 originally leading to literal %25 decoded to %, then %2F%25 -> / % no—wait, precise: Example from MDN: new URLSearchParams("%24%25%26=%28%29%2B").get("$%&") returns "+" - %24%25%26 decodes to $%& (key) - %28%29%2B decodes to + (value) If value had %2F, it becomes /. Thus, the returned value has percent sequences already decoded to characters. decodeURIComponent on it would attempt to decode any remaining %XX in those characters, causing double-decoding for originally double-encoded content or errors/invalid decoding otherwise. Do not call decodeURIComponent on URLSearchParams.get results; they are ready-to-use decoded strings per the URL standard.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams
- 2: https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams.
- 3: https://url.spec.whatwg.org/
🏁 Script executed:
# Check if the file exists and read the relevant section
fd "route.ts" --path "*sso-callback*" | head -5Repository: unkeyed/unkey
Length of output: 272
🏁 Script executed:
# Once we locate the file, read lines 15-35 to see context
if [ -f "web/apps/dashboard/app/auth/sso-callback/\[\[...\]\]/route.ts" ]; then
sed -n '15,35p' "web/apps/dashboard/app/auth/sso-callback/[[...]]/route.ts" || true
fi
# Try a broader search
find . -type f -name "route.ts" -path "*sso-callback*" 2>/dev/null | head -3Repository: unkeyed/unkey
Length of output: 128
🏁 Script executed:
# Search for files with SSO callback routing
git ls-files | grep -i "sso-callback" | grep -i "route"Repository: unkeyed/unkey
Length of output: 126
🏁 Script executed:
cat -n "web/apps/dashboard/app/auth/sso-callback/[[...sso-callback]]/route.ts" | sed -n '15,35p'Repository: unkeyed/unkey
Length of output: 1095
🏁 Script executed:
# Check if there's a tsconfig and what TypeScript settings are used
find . -name "tsconfig.json" -o -name "tsconfig.*.json" | head -5Repository: unkeyed/unkey
Length of output: 238
🏁 Script executed:
# Look at the full context of the function to understand the parsing intent
cat -n "web/apps/dashboard/app/auth/sso-callback/[[...sso-callback]]/route.ts" | sed -n '1,50p'Repository: unkeyed/unkey
Length of output: 2496
🏁 Script executed:
# Check if the fault library is imported or used elsewhere
rg "fault" --type ts --type tsx | head -10Repository: unkeyed/unkey
Length of output: 84
🏁 Script executed:
# Check for type guard patterns or validation utilities in the auth directory
rg "type.*=.*is" web/apps/dashboard/app/auth --type ts --type tsx | head -10Repository: unkeyed/unkey
Length of output: 84
🏁 Script executed:
# Check if fault library is imported anywhere
rg "import.*fault" --type ts | head -10Repository: unkeyed/unkey
Length of output: 1714
🏁 Script executed:
# Look for type narrowing patterns used elsewhere in the codebase
rg "typeof.*===.*string" web/apps/dashboard/app/auth | head -10Repository: unkeyed/unkey
Length of output: 341
🏁 Script executed:
# Check tsconfig to understand TypeScript version and strict settings
cat "./web/apps/dashboard/tsconfig.json"Repository: unkeyed/unkey
Length of output: 773
🏁 Script executed:
# Check if there's a type guard helper or validation pattern used in auth module
rg "function.*is" web/apps/dashboard/lib/auth --type ts | head -10Repository: unkeyed/unkey
Length of output: 920
🏁 Script executed:
# Look for schema validation patterns in the codebase (zod, etc.)
rg "import.*z\.|import.*schema" web/apps/dashboard/app/auth --type ts | head -10Repository: unkeyed/unkey
Length of output: 39
🏁 Script executed:
# Test TypeScript type narrowing behavior with unknown and in operator
cat > /tmp/test-narrowing.ts << 'EOF'
// Test if direct property access works after typeof and in checks
const parsed: unknown = { redirectUrlComplete: "test" };
if (typeof parsed === "object" && parsed !== null && "redirectUrlComplete" in parsed) {
// Can we access parsed.redirectUrlComplete directly without as?
const url = parsed.redirectUrlComplete; // This line would error in strict TS
}
EOF
cat /tmp/test-narrowing.tsRepository: unkeyed/unkey
Length of output: 418
🏁 Script executed:
# Check TypeScript version in package.json
rg '"typescript"' --max-count 5 | head -5Repository: unkeyed/unkey
Length of output: 337
🏁 Script executed:
# Look for type guard patterns that might be better suited for this
rg "as const satisfies" web/apps/dashboard --type ts | head -5Repository: unkeyed/unkey
Length of output: 213
🏁 Script executed:
# Check if there's a better pattern for parsing JSON with type safety in the codebase
rg "JSON.parse" web/apps/dashboard/lib/auth --type ts -A 3 | head -20Repository: unkeyed/unkey
Length of output: 311
🏁 Script executed:
# Check the full context of workos.ts to see how it handles state parsing
cat -n "web/apps/dashboard/lib/auth/workos.ts" | sed -n '1,100p'Repository: unkeyed/unkey
Length of output: 3459
🏁 Script executed:
# Look for the complete parseState or state handling logic in workos.ts
rg "JSON.parse.*decodeURIComponent.*state" web/apps/dashboard/lib/auth -B 5 -A 5 | head -30Repository: unkeyed/unkey
Length of output: 773
🏁 Script executed:
# Let me check if there's a validation/parsing utility function in the codebase
rg "type.*guard|const.*is.*=.*\(|function.*is" web/apps/dashboard/lib/auth --type ts -A 2 | head -30Repository: unkeyed/unkey
Length of output: 1946
🏁 Script executed:
# Check if there's a schema validation approach (zod, etc.) used elsewhere for parsing
rg "z\." web/apps/dashboard/lib/auth --type ts | head -10Repository: unkeyed/unkey
Length of output: 39
🏁 Script executed:
# Verify if the exact suggested fix from review comment would actually compile
# by checking for similar patterns in codebase that safely narrow unknown
rg "typeof.*in.*typeof" web/apps/dashboard --type ts | head -10Repository: unkeyed/unkey
Length of output: 770
🏁 Script executed:
# Check if there are any type guard utilities in the codebase
fd "type.*guard|is.*type" web --type ts | head -10Repository: unkeyed/unkey
Length of output: 226
🏁 Script executed:
# Look for type predicate patterns or helper functions
rg "is.*\(.*\).*:" web/apps/dashboard --type ts | head -10Repository: unkeyed/unkey
Length of output: 1457
🏁 Script executed:
# Check if the suggested fix would actually work by looking for similar patterns
rg "in.*parsed" web/apps/dashboard --type ts -B 2 -A 2 | head -20Repository: unkeyed/unkey
Length of output: 1742
Remove double-decode and unsafe type assertions.
request.nextUrl.searchParams.get("state") is already percent-decoded by the URL standard. Calling decodeURIComponent(state) will double-decode, corrupting percent sequences like %2F (slash) or %25 (percent sign) in legitimate deep-link URLs and potentially throwing on invalid sequences, causing valid redirects to fail silently in the catch block.
Additionally, the as casts on lines 27–28 violate the "Never compromise type safety" guideline. While the type narrowing checks establish that parsed is an object with a redirectUrlComplete property, TypeScript's unknown type does not permit direct property access even after in checks. A proper type guard function or validation library (e.g., Zod) is required to narrow safely without type assertions.
Suggested approach
Remove the decodeURIComponent() call:
- const parsed: unknown = JSON.parse(decodeURIComponent(state));
+ const parsed: unknown = JSON.parse(state);For type-safe narrowing without as, consider a type predicate:
+ function isStateObject(obj: unknown): obj is { redirectUrlComplete: string } {
+ return (
+ typeof obj === "object" &&
+ obj !== null &&
+ "redirectUrlComplete" in obj &&
+ typeof (obj as any).redirectUrlComplete === "string"
+ );
+ }
const redirectUrlComplete =
- typeof parsed === "object" &&
- parsed !== null &&
- "redirectUrlComplete" in parsed &&
- typeof (parsed as { redirectUrlComplete: unknown }).redirectUrlComplete === "string"
- ? (parsed as { redirectUrlComplete: string }).redirectUrlComplete
+ isStateObject(parsed)
+ ? parsed.redirectUrlComplete
: null;Or use a schema validation library like Zod for JSON parsing at OAuth boundaries.
Note: web/apps/dashboard/lib/auth/workos.ts has the same double-decode pattern and should be fixed similarly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const state = request.nextUrl.searchParams.get("state"); | |
| if (state) { | |
| try { | |
| const parsed: unknown = JSON.parse(decodeURIComponent(state)); | |
| const redirectUrlComplete = | |
| typeof parsed === "object" && | |
| parsed !== null && | |
| "redirectUrlComplete" in parsed && | |
| typeof (parsed as { redirectUrlComplete: unknown }).redirectUrlComplete === "string" | |
| ? (parsed as { redirectUrlComplete: string }).redirectUrlComplete | |
| : null; | |
| const state = request.nextUrl.searchParams.get("state"); | |
| if (state) { | |
| try { | |
| const parsed: unknown = JSON.parse(state); | |
| function isStateObject(obj: unknown): obj is { redirectUrlComplete: string } { | |
| return ( | |
| typeof obj === "object" && | |
| obj !== null && | |
| "redirectUrlComplete" in obj && | |
| typeof (obj as any).redirectUrlComplete === "string" | |
| ); | |
| } | |
| const redirectUrlComplete = | |
| isStateObject(parsed) | |
| ? parsed.redirectUrlComplete | |
| : null; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/apps/dashboard/app/auth/sso-callback/`[[...sso-callback]]/route.ts around
lines 19 - 29, The code double-decodes the state by calling decodeURIComponent
on the value from request.nextUrl.searchParams.get("state") and uses unsafe
casts to access redirectUrlComplete; remove the decodeURIComponent call and
parse the raw state with JSON.parse, then replace the inline `as` assertions
with a proper type guard (e.g., function isRedirectState(x: unknown): x is {
redirectUrlComplete: string }) or validate with a schema (Zod) before reading
redirectUrlComplete, and apply the same fix in
web/apps/dashboard/lib/auth/workos.ts.
1 participant
What does this PR do?
When opening a deep link while unauthenticated, sign-in would redirect to /apis instead of the original destination. Now the original path is preserved as a
redirectquery parameter through the entire auth flow middleware, OAuth, email verification, and org selection.Fixes #5458
Type of change
How should this be tested?
Scenario 1:
Scenario 2:
Scenario 3:
Checklist
Required
pnpm buildpnpm fmtmake fmton/godirectoryconsole.logsgit pull origin mainAppreciated