Conversation
Co-authored-by: Evan Taylor <evan-taylor@users.noreply.github.com>
|
Cursor Agent can help with this pull request. Just |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| const getSafeRedirectPath = (path: string | null) => { | ||
| if (!path?.startsWith("/")) { | ||
| return HOME_PATH; | ||
| } | ||
|
|
||
| if (path.startsWith("//")) { | ||
| return HOME_PATH; | ||
| } | ||
|
|
||
| if (path === SIGN_IN_PATH || path.startsWith(`${SIGN_IN_PATH}?`)) { | ||
| return HOME_PATH; | ||
| } | ||
|
|
||
| return path; | ||
| }; |
There was a problem hiding this comment.
Duplicated getSafeRedirectPath utility
getSafeRedirectPath is defined identically in both proxy.ts and app/signin/page.tsx. If the validation logic ever needs to change (e.g., to patch a security bypass), it must be updated in both places, which is easy to miss. Consider extracting it to a shared utility such as lib/redirect.ts and importing it from both call sites.
Prompt To Fix With AI
This is a comment left during a code review.
Path: proxy.ts
Line: 10-24
Comment:
**Duplicated `getSafeRedirectPath` utility**
`getSafeRedirectPath` is defined identically in both `proxy.ts` and `app/signin/page.tsx`. If the validation logic ever needs to change (e.g., to patch a security bypass), it must be updated in both places, which is easy to miss. Consider extracting it to a shared utility such as `lib/redirect.ts` and importing it from both call sites.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| const getSafeRedirectPath = (path: string | null) => { | ||
| if (!path?.startsWith("/")) { | ||
| return HOME_PATH; | ||
| } | ||
|
|
||
| if (path.startsWith("//")) { | ||
| return HOME_PATH; | ||
| } | ||
|
|
||
| if (path === SIGN_IN_PATH || path.startsWith(`${SIGN_IN_PATH}?`)) { | ||
| return HOME_PATH; | ||
| } | ||
|
|
||
| return path; |
There was a problem hiding this comment.
Potential open redirect via /\ bypass
The guard blocks // (protocol-relative URLs) but does not catch /\ followed by a domain. Some browsers and HTTP clients normalize a backslash to a forward slash, so a crafted query param like ?redirect=%2F%5Cevil.com could decode to /\evil.com, pass all three checks, and potentially redirect to an external host depending on the runtime's URL normalization.
Adding an explicit check for path.startsWith("/\\") (or, more robustly, parsing the path with new URL(path, window.location.origin) and verifying the origin matches) would close this gap:
if (path.startsWith("/\\")) {
return HOME_PATH;
}
The same applies to the identical function in proxy.ts.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/signin/page.tsx
Line: 14-27
Comment:
**Potential open redirect via `/\` bypass**
The guard blocks `//` (protocol-relative URLs) but does not catch `/\` followed by a domain. Some browsers and HTTP clients normalize a backslash to a forward slash, so a crafted query param like `?redirect=%2F%5Cevil.com` could decode to `/\evil.com`, pass all three checks, and potentially redirect to an external host depending on the runtime's URL normalization.
Adding an explicit check for `path.startsWith("/\\")` (or, more robustly, parsing the path with `new URL(path, window.location.origin)` and verifying the `origin` matches) would close this gap:
```
if (path.startsWith("/\\")) {
return HOME_PATH;
}
```
The same applies to the identical function in `proxy.ts`.
How can I resolve this? If you propose a fix, please make it concise.
Fix post-login redirect to return to the originally requested page instead of always going to home.
Previously, after successful authentication, users were always redirected to the home page (
/) even if they had attempted to access a protected route like/admin. This PR ensures that after signing in, users are returned to the page they initially intended to visit.Slack Thread
Greptile Summary
This PR fixes the post-login redirect so users are returned to their originally requested page rather than always landing on
/. Theapp/admin/page.tsxclient-side guard andapp/signin/page.tsxsign-in handler are updated correctly, butproxy.tshas an incomplete implementation that leaves a gap for middleware-protected routes.app/admin/page.tsx: Adds anauthLoadingguard to prevent premature redirects, builds the?redirect=query param fromwindow.location, and usesrouter.replace(correct choice to avoid polluting browser history).app/signin/page.tsx: Reads and validates theredirectsearch param viagetSafeRedirectPathbefore callingrouter.pushafter both sign-in and sign-up — correctly restores the intended destination.proxy.ts/signinthe middleware correctly honors theredirectparam. However, when the middleware itself redirects unauthenticated users away from routes like/boardor/dashboard, the original path is discarded — those users still land on/after signing in, so the feature is effectively broken for all middleware-enforced routes.getSafeRedirectPathis defined identically in bothproxy.tsandapp/signin/page.tsx; a shared utility would reduce maintenance risk.getSafeRedirectPathblock//-prefixed paths but do not guard against/\-prefixed paths, which some runtimes normalize to//and could enable an open-redirect bypass.Confidence Score: 3/5
/board,/dashboard,/server), meaning the PR's stated goal is only partially achieved. The duplicated validation function and the minor/\open-redirect gap add maintenance and security considerations that should be addressed before a full rollout.proxy.ts— the unauthenticated redirect to/signinat line 41 needs the?redirect=param added to complete the feature for middleware-protected routes.Important Files Changed
getSafeRedirectPathhelper and honours theredirectquery param when an authenticated user re-visits/signin, but omits theredirectparam when the middleware itself redirects unauthenticated users away from protected routes — making the feature incomplete for/board,/dashboard, and/server.authLoadingguard before the unauthenticated redirect and constructs the?redirect=query param correctly usingwindow.location.pathname+window.location.search; also switches torouter.replaceto avoid polluting browser history.redirectsearch param and validates it throughgetSafeRedirectPathbefore using it inrouter.pushafter both sign-in and sign-up flows; function is duplicated fromproxy.tsand has a minor/\bypass gap.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[User navigates to protected page] --> B{Middleware-protected route?} B -- Yes, e.g. /board --> C{Authenticated?} C -- No --> D[Redirect to /signin\nno redirect param added] D --> E[User signs in] E --> F[router.push to HOME\noriginal destination lost] C -- Yes --> G[Allow access] B -- No, e.g. /admin --> H[Page renders client-side] H --> I{authLoading done?} I -- No --> J[Show loader] I -- Yes --> K{currentUser null?} K -- Yes --> L[router.replace to /signin\nwith redirect param] L --> M[User signs in] M --> N[router.push to original page\ncorrect destination restored] K -- No --> O[Show page content]Comments Outside Diff (1)
proxy.ts, line 40-42 (link)Missing
redirectparam for middleware-protected routesThe middleware correctly reads and forwards the
redirectquery param when an already-authenticated user visits/signin(lines 34–38). However, when it redirects unauthenticated users away from protected routes (/server,/board(.*),/dashboard(.*)), the destination path is not preserved. Users hitting those routes will still land on/after sign-in, making the feature incomplete for middleware-enforced pages.Prompt To Fix With AI
Last reviewed commit: b5d9cd3