fix(nextjs)!: fail closed without a verifier secret, refuse a path that is not one - #110
fix(nextjs)!: fail closed without a verifier secret, refuse a path that is not one#110msalvatti wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the Next.js edge auth utilities to fail closed when a verifier secret is missing, and to ensure redirects emitted by the edge proxy never name an origin (mitigating Host-header influence), while adding a runtime-distinguishable signatureVerified signal for authorization decisions.
Changes:
- Make
verifyJwtTokenrefuse verification whensecretis missing/empty and addsignatureVerifiedto distinguish verified vs decode-only results. - Update
createAuthProxyto gate authentication onsignatureVerifiedand emit relativeLocationredirects via a newredirectToPathhelper. - Expand Vitest coverage around missing-secret behavior,
signatureVerified, and redirect-path validation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/rust-auth/tests/nextjs.test.ts | Adds regression tests for fail-closed verification, signatureVerified, and relative/validated redirect Locations. |
| packages/rust-auth/src/nextjs/proxy.ts | Switches auth gating to signatureVerified and introduces relative redirect construction + same-origin path reduction. |
| packages/rust-auth/src/nextjs/jwt.ts | Adds signatureVerified, removes verify decode-fallback, and documents/implements fail-closed secret handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Reject an unverified, expired, or non-access token. `signatureVerified` rather than | ||
| // `isValid`: the latter is also `true` on `verifyJwtToken`'s decode-only branch, so gating | ||
| // on it would admit a forged token the moment the secret went missing. The ternary above | ||
| // already refuses to call without a secret; this is the second lock on the same door, and | ||
| // it is the one that survives a refactor of the first. Admitting a non-access type — |
| } | ||
| return new NextResponse(null, { | ||
| status: 307, | ||
| headers: { location: `${url.pathname}${url.search}` }, |
| * Decode a token's header and payload WITHOUT verifying its signature. Never throws: a | ||
| * malformed token yields `{ isValid: false }`. The result is non-authoritative — it proves | ||
| * the token is well-formed, never that it is genuine — so it must not gate a decision. |
9ff4a7b to
77c0507
Compare
d6aacd4 to
c7cbed2
Compare
…t is not one The edge helper fell back to a decode when no secret was configured, and the two branches returned the same shape with the same success flag — so a caller reading it, the natural reading of a function named `verify`, admitted a token an attacker minted the moment the secret went missing. An unset environment variable was enough to arrange that. It now refuses, and the decode-only read stays available under its own correctly-named entry point. `redirectToPath` also states its same-origin invariant explicitly rather than relying on `URL` resolution to imply it. The open redirect was already closed here — resolving against our own origin and keeping only the path drops any authority the input carried — but it closed it by substituting a different destination for the one the caller asked for, and reported nothing. Of the shapes now refused, two were reachable before: a bare relative reference, which the browser resolves against the current directory, and a control character, which ends the header and lets what follows be read as another one. The `Location` itself stays absolute, which is a constraint and not a preference. Next hands the header this middleware sets straight to `new NextURL(location, ...)` with no base, so a relative `Location` throws `ERR_INVALID_URL` there and every unauthenticated request 500s. Naming the request's own origin is safe: Next compares the two hosts immediately afterwards and rewrites `Location` down to a path whenever they match, which here they always do. A redirect to a genuinely different origin is the case Next leaves alone — and the one the path validation above refuses.
77c0507 to
c40f6e9
Compare
c7cbed2 to
996e3ef
Compare
|
The Next does not send a middleware's response as written. Three lines after that parse, Next compares the redirect's host against the request's and, when they match, rewrites What Next does not normalise is a The same change has been applied to nest-auth, whose equivalent had already been merged — its unit tests drive a mocked |
|
Merged. This commit ( All review threads on this PR were answered and resolved before it landed. |
6 of 7 in the fifth audit round. Base:
audit5/05-rate-limit(PR 5).Paired with nest-auth's
audit5/05-nextjs-edge(PR #52, merged).What this closes — BREAKING
The edge helper fell back to a decode when no secret was configured, and the two branches
returned the same shape with the same success flag. A caller reading it — the natural reading
of a function named
verify— admitted a token an attacker minted the moment the secret wentmissing. An unset environment variable was enough to arrange that. It now refuses, and the
decode-only read stays available under its own correctly-named entry point.
redirectToPathstates its same-origin invariant where the header is written. The openredirect was already closed here — resolving against the placeholder base drops any authority —
but it was closed by silently substituting a different destination for the caller's, reporting
nothing. Of the shapes now refused, two were reachable: a bare relative reference, which the
browser resolves against the current directory, and a control character, which ends the header
and lets what follows be read as another one.
Verification
fmt,clippy -D warnings, the full--all-featuressuite, and thepackages/rust-authvitest suite pass at this commit in isolation.