Backport login-CSRF fix (GHSA-xf67-jxfx-jf88) to 1.x (Harper 4) - #215
Conversation
F1 (login-CSRF, the primary vuln): add stable per-browser secret cookie binding to the human login flow, adapted for 1.x (no MCP machinery). Helpers land in a new self-contained src/lib/browserBinding.ts module. handleLogin mints or reuses one __Host-oauth_browser cookie per browser, stores hash(secret) as browserNonceHash in the CSRF state. handleCallback verifies the hash constant-time before any upstream code exchange or session write; tokens without the hash (pre-upgrade in-flight) pass through (in-flight tolerance). The existing #185 session↔state binding is preserved unchanged. F2 (CRLF log injection, CWE-117, low severity): JSON.stringify the error and error_description callback params before logging them. Also tighten engines.harperdb from >=4.6.0 to >=4.6.0 <5.0.0 so 1.x is not installed against Harper 5, where the session model differs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Reviewed; no blockers found. Suggestions (non-blocking)
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a stable per-browser secret cookie (__Host-oauth_browser) to bind the human login flow to the initiating browser, mitigating login-CSRF attacks for logged-out requests. It adds helpers for generating, hashing, reading, and matching the secret, integrates them into the login and callback handlers, and includes comprehensive unit tests. The review feedback suggests validating the format and length of the read cookie value to prevent CPU exhaustion attacks from hashing excessively long inputs, and defensively verifying that both arguments in browserSecretMatches are strings to avoid runtime TypeErrors.
Fix 1 — readCookieHeader array join (browserBinding.ts)
Harper 4 Headers (extends Map; append with commaDelimited) can store a
repeated Cookie header as string[]. readCookieHeader returned
headers.get('cookie') verbatim, so readBrowserSecret rejected the
non-string and the binding cookie was never found, locking out browsers
whose HTTP/2 connection crumbled the header. Now joins crumbs with '; '
when the value is an array; plain-object test-double path receives the
same treatment. Matches 2.x consentBinding.ts readCookieHeader.
Fix 2 — state token consumed on error callback (handlers.ts)
handleCallback returned on the error= branch before calling
verifyCSRFToken(state), leaving the single-use state replayable. The
fix separates the no-state early-return (no token to consume) from the
with-state path: verifyCSRFToken is now called first regardless of
whether the IdP returned an error, then the error redirect uses
tokenData.originalUrl — matching 2.x ordering and the comment "consuming
state on error is intentional."
Fix 3 — CRLF-safe username log (handlers.ts)
Two log lines interpolated user.username raw, unlike the error-path
logging which already used JSON.stringify (CWE-117). Wrapped both
occurrences identically. Raw interpolation also exists in 2.x (lines
406 and 501 of origin/main:src/lib/handlers.ts) — filed for follow-up.
Tests
Added six new assertions covering the three fixes: array-crumbs via
.get() and via plain object (Fix 1); error= with-state consumes token
and error= without-state skips verification (Fix 2); CRLF-in-username
on both log paths (Fix 3). All 394 tests pass.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Review complete (cross-model + Harper-domain + live Harper-4 smoke). Landed: multi-crumb Cookie join, single-use state consumed on the error path, and cookie-value validation / typeof guards (04c4705). All threads resolved. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Reviewed; no blockers found. |
…ing compare inputs
readBrowserSecret now rejects cookie values that don't match
/^[A-Za-z0-9_-]{1,64}$/, preventing hashing of attacker-supplied
arbitrary-length strings.
browserSecretMatches adds explicit typeof checks so a non-string
stored value (e.g. from a corrupt or unexpected source) cannot reach
hashBrowserSecret/Buffer.from and throw.
Tests added for: valid 43-char base64url accepted, malformed value
rejected, over-length (65+ chars) rejected, and non-string arguments
(number, object, null) returning false without throwing.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
heskew
left a comment
heskew
left a comment
There was a problem hiding this comment.
Review at 04c4705: no blockers and no new non-blocking defects found.
The 1.x backport preserves the existing provider and session bindings, adds a 256-bit stable __Host- browser secret, stores only its SHA-256 hash in single-use server-side state, and checks the browser binding before code exchange, hooks, or session mutation. Cookie attributes satisfy the host-prefix rules. Harper 4 header-wrapper and repeated-cookie shapes fail closed correctly, malformed values are bounded, and pre-upgrade state remains compatible only for the existing short state lifetime.
The provider-error path now consumes and validates state before redirecting, and browser-controlled callback values plus mapped usernames are CRLF-safe in logs.
Validation: targeted browser-binding and handler tests 78/78 passed; explicit TypeScript and lint passed; GitHub Node 22/24 and Bun checks are green. #214 is independent and can merge before or after this PR; both should land before the separate 1.x release/version/changelog PR.
Reduces the verbose JSDoc and inline comments added by the GHSA-xf67 fix down to terse Harper style: one line per invariant, no history or narration. No logic changed; every diff line is a comment or blank. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 participant
What this ports
This is the 1.x / Harper 4 backport of the login-CSRF fix already released on the 2.x line in v2.5.0 under GHSA-xf67-jxfx-jf88.
Two security items and one low-severity hardening fix are included:
Browser-secret binding (
__Host-oauth_browsercookie) —handleLoginmints or reuses a stable per-browser secret, storeshash(secret)asbrowserNonceHashin the CSRF state, andhandleCallbackverifies the hash constant-time before any code exchange or session write. Tokens minted before the upgrade (in-flight at deploy time) pass through without the check. Implemented in the new self-containedsrc/lib/browserBinding.tsmodule.State↔session binding — the existing session-id binding introduced in #185 is preserved unchanged (
tokenData.sessionId && tokenData.sessionId !== request.session?.id).CRLF log injection fix (CWE-117) — browser-controlled
erroranderror_descriptioncallback parameters are now passed throughJSON.stringifybefore logging.Scope
This branch contains only the security backport. The
peerDependencies.harperdbengine-pin tightening (>=4.6.0 <5.0.0) is tracked separately in #214 and is not included here; thepackage.jsonin this branch is identical to 1.6.1's.Version bump and release steps are handled separately — this PR targets
v1.xas a pre-release checkpoint.Test results
All 388 non-skipped tests pass. The 2 skipped tests are pre-existing environment-dependent skips (Harper table availability in unit context).
Verification
Build:
npm run build— clean (tsc emits no errors).Tests:
npm teston the worktree — 388/388 passing.Complexity: Low — isolated module addition + targeted handler changes; no new external dependencies.