fix(#464): session-expiry redirect + fix critical publicPaths auth-bypass - #531
Open
Vyacheslav-Tomashevskiy wants to merge 1 commit into
Conversation
…th-bypass in publicPaths - middleware.ts: redirect to /login?expired=1 (instead of a bare /login) when a protected page has no session cookie or the token fails verification, so the login page can show a friendly explanation instead of a silent, unexplained login form. - login/page.tsx: read ?expired=1 and show "Your session expired. Please log in again." then strip the query param; wrapped in Suspense per the existing useSearchParams() pattern used by review/activity pages (required for static export, since useSearchParams needs a Suspense boundary). While writing tests for the above, found that the pre-existing publicPaths allow-list was broken far beyond this feature: the "/" entry, matched via pathname.startsWith(p), is a prefix of every possible path, so publicPaths.some(...) returned true for literally any route (/dashboard, /api/transactions, everything) and the entire auth gate was a silent no-op - no cookie or token check ever ran, for any protected page or API route. Confirmed live with npm run dev: before this fix, GET /dashboard with no cookie returned 200 instead of redirecting; after the fix it correctly 307s to /login?expired=1, and GET /api/transactions with no cookie correctly 401s. Fixed by exact-matching "/" while keeping prefix matching for the genuine directory-style entries (/legal/, /api/public/, /_next/, etc). New test file src/__tests__/middleware.test.ts (none existed before) covers both: the new ?expired=1 redirect behavior and, implicitly, that protected routes actually gate now (3 of 5 tests would fail again if the publicPaths regression came back). Full suite: 66/66 relevant tests pass (npx vitest run). The other 4 suites reported as failing on a clean origin/main checkout fail at collection time due to a separate, already-reported pre-existing bug (missing isValidStellarPublicKey import in validations.ts, unrelated to this change) - fix already pending in open PR Northgate-Systems#529, left untouched here to avoid a third PR stepping on the same lines. Verified npm run build compiles clean once that one import line is present locally. eslint and tsc clean on all files touched by this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Someone is attempting to deploy a commit to the codex723's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
Author
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #464.
What
middleware.tsnow redirects to/login?expired=1(instead of a bare/login) when a protected page is hit with a missing session cookie or a token that fails verification. The login page reads that flag, shows "Your session expired. Please log in again.", and strips the query param so a refresh doesn't keep re-showing it (sameuseSearchParams()+Suspensepattern already used onactivity/review).🔴 Also fixes a critical, currently-live auth-bypass bug found while writing tests for the above
publicPathsincluded"/", checked viapublicPaths.some((p) => pathname.startsWith(p)). Since every pathname starts with"/", that allow-list matched literally every route -/dashboard,/api/transactions, all of it - so the auth gate was a silent no-op. No cookie/token check ever ran for any page or API route.Verified live with
npm run devon the unpatched code:After the fix:
Fix: exact-match
"/"(pathname === "/"), keep prefix-matching for the genuine directory-style entries (/legal/,/api/public/,/_next/, etc).Tests
New
src/__tests__/middleware.test.ts(none existed before) - 5 tests covering the?expired=1redirect on missing/invalid token, valid-session passthrough, plain 401 (no redirect) for API routes, and that/loginitself stays public without touchingverifyToken. 3 of the 5 would fail again if thepublicPathsregression came back, so this also guards the auth-bypass fix, not just the new feature.npx vitest run: 66/66 relevant tests pass. The 4 suites that fail on a cleanorigin/maincheckout fail at collection time due to a separate, already-reported bug (missingisValidStellarPublicKeyimport invalidations.ts- fix pending in #529, left untouched here to avoid a third PR touching the same lines).npm run buildverified to compile clean once that one import is present locally.eslint/tsc --noEmitclean on every file this PR touches.