fix: prevent open redirect in OIDC callback flow#61
Conversation
Add isSafeRedirectPath() validation to ensure redirect URLs in the OIDC callback are relative paths only. This prevents attackers from crafting malicious callback URLs that redirect users to external phishing sites. Defense-in-depth: validation is applied both in parseOidcCallback() (lib/oidc.ts) and in the callback page component. Co-authored-by: overtrue <1472352+overtrue@users.noreply.github.com>
…checks
Address code review feedback:
- Improved backslash URL detection with /^[\\/]{2,}/ and explicit \\ check
- Added directory traversal prevention for /../ patterns
Co-authored-by: overtrue <1472352+overtrue@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Addresses an open-redirect risk in the OIDC callback flow by validating the redirect value extracted from the URL hash before client-side navigation.
Changes:
- Added
isSafeRedirectPath()helper to validate same-origin redirect targets and apply a fallback. - Sanitized the parsed
redirectvalue inparseOidcCallback(). - Re-validated the redirect path in the OIDC callback page before calling
router.replace().
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lib/routes.ts | Introduces isSafeRedirectPath() for redirect target validation. |
| lib/oidc.ts | Sanitizes redirect during OIDC hash parsing via isSafeRedirectPath(). |
| app/(auth)/auth/oidc-callback/page.tsx | Applies defense-in-depth validation before navigation in the callback page. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| redirectPath.current = credentials.redirect || "/browser" | ||
| redirectPath.current = isSafeRedirectPath(credentials.redirect, "/browser") |
There was a problem hiding this comment.
credentials.redirect has already been sanitized in parseOidcCallback() (and will be a non-empty string due to its fallback), so this second isSafeRedirectPath(..., "/browser") call will never hit the "/browser" fallback. Either remove the duplicate sanitization here, or make the two layers use the same fallback so the effective default isn’t surprising.
| /** | ||
| * Validate that a redirect path is safe (relative, no protocol, no external domain). | ||
| * Returns the path if safe, or the fallback otherwise. | ||
| */ |
There was a problem hiding this comment.
The JSDoc says the redirect path is validated as "relative", but the implementation requires it to start with "/" (i.e., an absolute-path reference). Consider updating the comment to match the actual constraint (same-origin path starting with "/", and explicitly disallowing "foo/bar").
| expiration: params.get("expiration") ?? "", | ||
| redirect: params.get("redirect") ?? "/", | ||
| redirect: isSafeRedirectPath(params.get("redirect") ?? "", "/"), | ||
| } |
There was a problem hiding this comment.
With params.get("redirect") ?? "" and isSafeRedirectPath treating empty strings as invalid, a callback containing redirect= (present but empty) will now produce "/" (fallback) rather than propagating an empty value to the caller. If the caller-level fallback (e.g. "/browser") is intended for empty redirects, consider aligning the behavior by either (a) sanitizing only at the call site with the desired fallback, or (b) using a fallback here that preserves the empty-string case (so the page-level fallback can still apply).
Pull Request
Description
The OIDC callback flow accepts a
redirectparameter from the URL hash fragment without validation. An attacker can craft a callback URL with#...&redirect=https://evil.comto redirect users to a phishing site after SSO login.Added
isSafeRedirectPath()to validate redirect targets, applied at two layers:lib/routes.ts— NewisSafeRedirectPath(path, fallback)utility that blocks protocol URLs, protocol-relative URLs (//), backslash variants (\\,\/),javascript:/data:URIs, non-relative paths, and directory traversal (/../)lib/oidc.ts— Sanitizesredirectat parse time inparseOidcCallback()app/(auth)/auth/oidc-callback/page.tsx— Defense-in-depth validation beforerouter.replace()Type of Change
Testing
Validated
isSafeRedirectPathagainst 25 test vectors covering safe paths, absolute URLs, protocol-relative URLs, backslash variants,javascript:/data:URIs, directory traversal, empty/null input, and whitespace edge cases. All pass.Checklist
Related Issues
GHSA-v9fg-3cr2-277j
Screenshots (if applicable)
N/A — no UI changes.
Additional Notes
CodeQL scan returned 0 alerts after changes.
Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
www.google.com/home/REDACTED/work/_temp/ghcca-node/node/bin/node /home/REDACTED/work/_temp/ghcca-node/node/bin/node --enable-source-maps /home/REDACTED/work/_temp/copilot-developer-action-main/dist/index.js(dns block)If you need me to access, download, or install something from one of these locations, you can either:
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.