Repository navigation
feat(fastify): per-request returnTo on /auth/login (1.3.0) - #11
Merged
Merged
Conversation
The Fastify plugin computed its post-login redirect once at registration from `postLoginPath`, and GET /auth/login read only `prompt` from the query, so any `returnTo` a consumer passed was silently discarded and every callback landed on the same page. Gateway's /resume links depend on landing the user back on the step they came from; in production they all dropped the user on the dashboard instead. What changed: - core: optional `returnTo` on `OIDCState` (the per-attempt object stored in the signed `oidc_state` cookie; the existing validator only requires `state` + `codeVerifier`, so the extra field round-trips untouched), and a new pure `sanitizeReturnTo(raw, maxLength = 512)` exported from the package root. - fastify /auth/login: `?returnTo=` is validated and, if accepted, stored on THIS attempt's pending state before it is appended to the cookie. Rejected values are dropped (debug log with the value's type and length, never its content) and the login proceeds exactly as today. - fastify /auth/callback: redirects to `frontendUrl + returnTo` when the selected pending state carries one, else to the registration-time `postLoginRedirect` as before. Only the value in the signed cookie is trusted; nothing is re-read from the callback query. Error-path redirects are untouched. - README subsection + CHANGELOG entries (1.3.0, and the 1.2.0 release that was never recorded). core 1.2.0 -> 1.3.0, fastify 1.2.0 -> 1.3.0. express/admin untouched. Behaviour without `returnTo` is byte-identical to 1.2.0: same cookie shape, same redirect targets, same error paths. Validation (`sanitizeReturnTo`) accepts a value only if ALL hold, else the caller falls back to `postLoginPath`: - a non-empty string of at most 512 characters - exactly one leading `/` (`//host` and `/\host` are protocol-relative) - printable ASCII only (0x20-0x7E): no CR/LF/NUL, and nothing Node would refuse in a Location header; non-ASCII must be percent-encoded - no `:` before the first `?` or `#` (rules out `/javascript:` and `://`) - `decodeURIComponent` succeeds, and the decoded form also has exactly one leading `/` (blocks `%2F%2F` and `%5C` smuggling) Because each attempt keeps its own `returnTo`, overlapping logins (double-click, second tab) each land on their own path. Closes #10 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DDuBqJUDnKuKkjVSycgyAG
tests/pending-states.test.ts — `sanitizeReturnTo`: accepts `/nda`, `/candidates/123/step?x=1#frag`, `/`, a `:` inside the query, and percent-encoded non-ASCII; rejects non-strings, `''`, `nda`, `//evil.com`, `/\evil.com`, `https://evil.com/x`, `/javascript:alert(1)`, CR/LF/NUL, raw non-ASCII, `/%2F%2Fevil.com`, `/%5Cevil.com`, malformed escapes, and a 513-char path (512 passes; `maxLength` is overridable). Round-trip: an OIDCState with `returnTo` survives serialize -> parse intact and is selectable by state. tests/pending-states-plugin.test.ts — drives the real Fastify plugin through /auth/login and /auth/callback with `exchangeCode` stubbed on the IdentityClient prototype so the callback reaches its final redirect: (a) `?returnTo=/nda` lands on https://app.test/nda and never reaches the Identity authorize URL; (b) no `returnTo` lands on the registration-time default (`/dashboard`, or an explicit `postLoginPath: '/home'`), asserting the exact URL; (c) `//evil.com`, `https://evil.com/x`, `/%2F%2Fevil.com`, `/\evil.com` are dropped (never written to the cookie) and fall back to `/dashboard`; (d) two overlapping logins with `/a` and `/b`, callbacks in reverse order, each land on their own path. 14 suites, 187 tests pass (176 before). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DDuBqJUDnKuKkjVSycgyAG
|
Warning Review limit reachedNext included review available in 31 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 68 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Essentials Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (5)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Essentials Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (10)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. 📝 WalkthroughWalkthroughFastify login routes now accept validated per-request ChangesPer-request login destinations
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change adds validated per-request login destinations while preserving the existing default redirect behavior, with build, test, and type-check validation reported as passing. No actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Client
participant FastifyLogin
participant PendingOIDCState
participant FastifyCallback
Client->>FastifyLogin: Request login with returnTo
FastifyLogin->>PendingOIDCState: Store validated returnTo by state
Client->>FastifyCallback: Submit callback with state
FastifyCallback->>PendingOIDCState: Match pending state
PendingOIDCState-->>FastifyCallback: Return stored destination
FastifyCallback-->>Client: Redirect to returnTo or postLoginRedirect
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue Full details: Docstring CoverageExplanation Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 6 files. (4 skipped: 4 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
@coderabbitai review |
|
…rm; declare core ^1.3.0
sanitizeReturnTo scanned only the raw string for a ':' in the path
segment, so a percent-encoded scheme ('/javascript%3Aalert(1)', or a
fully-encoded 'javascript') passed the guard its own contract promised.
The decoded form now gets the same check as the raw one, mirroring the
leading-slash re-check that was already there. Bounded at the plugin's
call site (the raw, still-encoded value is appended to a trusted
frontendUrl), but the function is exported as a general validator and
the contract has to hold. Tests added for both encodings, plus a
positive case proving a ':' inside the query string is still accepted.
The fastify package imported core 1.3.0 exports while declaring ^1.1.0;
range now ^1.3.0.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DDuBqJUDnKuKkjVSycgyAG
Contributor
Author
|
@coderabbitai review |
|
…pts fit in a 4 KB cookie security-review measured five pending states each carrying a 512-char returnTo at ~4338 bytes URL-encoded in the oidc_state cookie, over the 4096-byte browser limit; the cookie is then dropped silently and every callback fails state_missing. 256 keeps the worst case near 3.1 KB. A post-login path has no legitimate need to be longer. Boundary pinned by tests at 256 accept / 257 reject. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DDuBqJUDnKuKkjVSycgyAG
Contributor
Author
|
@coderabbitai review |
|
…racter count The 256-character cap only bounded the cookie for alphanumeric paths: '?', '=', '&' triple after percent-encoding and '"' or '\' sextuple after JSON-escape-then-encode, so five valid 256-char values measured 5.6–9.5 KB in the oidc_state cookie — over the 4 KB limit the previous commit claimed to stay under. sanitizeReturnTo now also rejects any value whose JSON-escaped, percent-encoded form exceeds 384 bytes. The character cap stays as the readable bound. 384 rather than 448: measured through the real plugin (five logins, signature and cookie name included), each stored returnTo also costs 20 bytes of JSON key/separator overhead on top of its encoded length. At 448 the Set-Cookie name=value is 4032 bytes with gateway's 49-char callback URL, 4070 with a 54-char URL plus a cookie prefix, and 4337 with a 95-char URL, which the browser drops. At 384 the last two are 3758 and 4015. Pinned by a pure test that serializes five worst-case-accepted states through the same encoder and asserts the total stays under the limit, and by a plugin test that performs five real logins with a worst-case returnTo and asserts the Set-Cookie name=value stays under 4096 bytes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DDuBqJUDnKuKkjVSycgyAG
Contributor
Author
|
@coderabbitai review |
|
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
1 participant
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.
What this does
GET /auth/login?returnTo=/some/pathnow lands the user on that path after the OIDC callback, instead of always on the registration-timepostLoginPath. Overlapping logins each carry their own destination, riding on the per-attempt pending-state array from #8.Why: the fastify plugin computed
postLoginRedirectonce at registration and the callback redirected there unconditionally;returnToon/auth/loginwas silently discarded. Gateway's/resumelinks have been broken in production by this (api/src/routes/resume.ts:89-91passesreturnUrlbelieving the plugin honours it), and the invited-candidate flow (talent#372) needs to land on the NDA. Closes #10.Behaviour with no
returnTois unchanged — pinned by a test that asserts the exact registration-time URL.Validation (
sanitizeReturnTo, core)Rejected values are dropped silently and the login proceeds as today. Accepted only if: string, non-empty, ≤256 chars and ≤384 bytes once JSON-escaped and cookie-encoded — measured through the real plugin so five pending attempts plus a long callback URL and cookie prefix stay under the browser's 4 KB cookie limit; pinned by an end-to-end test that runs five logins and asserts the
Set-Cookielength, starts with exactly one/(not//, not/\), printable ASCII only (Node throws on non-ASCII in aLocationheader — percent-encode instead), no:in the path segment of either the raw or the percent-decoded form (blocks/javascript:,://, and/javascript%3A), and the decoded form also passes the leading-slash rules (blocks%2F%2F). Only the value stored in the signed state cookie is trusted at callback; nothing is re-read from the query.Changes
packages/core:OIDCState.returnTo?on the type;sanitizeReturnToexported; 1.2.0 → 1.3.0packages/fastify:/auth/loginreads and validatesreturnTo, stores it on the attempt; callback redirects to${frontendUrl}${returnTo}when present; 1.2.0 → 1.3.0; README section; CHANGELOG (also backfills the unrecorded 1.2.0 entry)dist/for core and fastify rebuilt and committed, as this repo ships prebuilt for tarball installsTest plan
npm run build— all four workspaces, exit 0npm test— 14 suites, 187 tests (176 before, 11 new), exit 0. New coverage:sanitizeReturnToaccept/reject matrix incl.//evil.com,/\evil.com,https://…, CRLF injection,%2F%2F, 513-char, non-ASCII raw vs percent-encoded; state round-trip through serialize/parse; plugin end-to-end: honoured path, absent → unchanged default, rejected → default, two overlapping logins each land on their own pathnpx tsc -p tsconfig.test.json --noEmit— exit 0 (no root typecheck script exists)Post-merge: consumers install by tarball SHA, so nothing changes for anyone until they bump. Gateway bumps to the new main SHA and passes
returnTofrom/resumeand the invite landing page; that first successful deep-link is the live verification.Only runtime signal: Jest's
ExperimentalWarning: VM Modules is an experimental feature— produced by the repo's own--experimental-vm-modulestest flag, expected.🤖 Generated with Claude Code
https://claude.ai/code/session_01DDuBqJUDnKuKkjVSycgyAG