Defer unrecognized app-port credential rejection until route ownership is known - #2419
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements deferred credential rejection (#2418), allowing unrecognized credentials to pass through to downstream applications on unowned routes while ensuring Harper-owned routes still reject them once route ownership is established. The feedback highlights a critical issue where unrecognized authorization strategies (like Digest) bypass the deferral logic and default to anonymous access, potentially exposing public Harper-owned routes. Additionally, a potential runtime TypeError was identified in the success audit logging when newUser is null or undefined.
Dispatch durable state — QA PASS at
|
| Acceptance criterion | Verdict | Evidence |
|---|---|---|
| Valid Harper Basic/Bearer credentials still authenticate; protected Harper-owned routes retain existing authorization semantics | PASS | authCredentialDeferral.test.js, deferred-credential-rejection.test.ts — unchanged since 50fa681, still 0 failing at this head |
| Invalid Harper credentials to a protected route return generic unauthorized and never reach the catch-all | PASS | deferredAuthentication.test.js, deferred-credential-rejection.test.ts |
| Unrecognized WordPress/Woo Basic credential on an unowned route reaches the catch-all with Authorization byte-for-byte unchanged | PASS | deferred-credential-rejection.test.ts, appCatchAll.js fixture; integration suite 13/13 in CI |
| Bearer credentials owned by the downstream application receive equivalent treatment without breaking Harper refresh-token behavior | PASS | tokenRejectionClassification.test.js |
| Missing credentials and native public status/health routes behave normally | PASS | deferred-credential-rejection.test.ts controls; static.test.js |
| Internal authentication errors remain fail-closed, do not fall through as unknown credentials | PASS | tokenRejectionClassification.test.js, deferredAuthentication.test.js isCredentialRejection/isTokenRejection tagging tests |
| Middleware ordering tests cover authentication -> rest -> application catch-all; no path exemption/carrier header/rename/stripping shim | PASS | httpChainPortAll.test.js, deferred-credential-rejection.test.ts; diff inspection confirms no exemption list or header-rename shim introduced |
| A runnable regression test reproduces the 5.2.6 early-401 behavior on base and passes with the fix | PASS | deferred-credential-rejection.test.ts (fails-on-base per PR body evidence) |
| Config schema, security docs, API/runtime docs, and release notes explain deferred credential rejection and route-owner enforcement | PASS | HarperFast/documentation#663 (open, public contract documented in reference/http/overview.md, reference/security/overview.md, release-notes/v5-lincoln/5.3.md); server/DESIGN.md documents the internal contract including this head's Set-Cookie clarification; no configuration schema change needed since the behavior is automatic with no new setting (explicitly noted in #663's PR body) |
Commands:
~/.dispatch-dev-team/scripts/sprint.sh spec-path 2418~/.dispatch-dev-team/scripts/sprint.sh branch-of 2418gh pr list --head fix/issue-2418-defer-unrecognized-authorization --json numbergh pr view 2419 --json headRefOid,headRefName,baseRefName,title,body,url,state,mergeablegit fetch origin fix/issue-2418-defer-unrecognized-authorization && git diff --stat 50fa681 b1df138f5a4f7ccc3e59c39a6d1cdb03684a8f8enpx mocha unitTests/server/fallbackCacheFloor.test.js unitTests/server/serverHelpers/Headers.test.jsnpx prettier --check server/DESIGN.mdgh api graphql (reviewThreads isResolved query)gh pr checks 2419~/.dispatch-dev-team/scripts/qa-revert-verify.sh 2418 2419~/.dispatch-dev-team/scripts/sprint.sh test-candidates 2419npm run buildad-hoc script driving real mergeChainHeadersIntoFallback() with WordPress-shaped cookie scenario (not committed)temporary mutation of Headers.ts dedup key (exact-value -> name-prefix) + rerun focused suite + git checkout restorenpx mocha unitTests/server/*.jsnpx mocha unitTests/security/**/*.jsnpx mocha unitTests/server/fallbackCacheFloor.test.js unitTests/server/httpChainPortAll.test.js unitTests/server/mqtt.test.js unitTests/server/static.test.js unitTests/security/authCredentialDeferral.test.js unitTests/security/deferredAuthentication.test.js unitTests/security/tokenRejectionClassification.test.js unitTests/components/mcp/adapters/harperHttp.test.jsgh pr view 663 --repo HarperFast/documentation --json title,body,state,url,filesgh pr diff 663 --repo HarperFast/documentationcurl -sL PR diff-line anchors (200 OK)
Dismissed concerns:
- server/DESIGN.md and the PR body cite a specific WordPress-shaped/Max-Age=0 empirical scenario as the resolution to a review nit, but no committed test encodes that exact scenario. — Justification: pre-existing - The dedup-by-exact-value production behavior (server/serverHelpers/Headers.ts) is unchanged since the 50fa681 head, which already passed QA with recorded revert evidence for the full production surface. This head only adds prose documenting and justifying that pre-existing behavior. I independently verified the cited claim against the real function with an ad-hoc script and it holds true. The absence of a permanent regression test for this exact scenario predates this docs-only change and is not something this delta introduced or newly surfaced as a code risk.
- unitTests/security/**/*.js reports 2 failing tests. — Justification: pre-existing - Both are the jsLoader symlink test failures already documented in the 50fa681 QA pass as pre-existing macOS sandbox limitations unrelated to any file in this diff; identical failure count and test names at this head.
- qa-revert-verify.sh exited non-zero with tests-fail-before-revert. — Justification: pre-existing - The script's disposable worktree symlinks node_modules but never runs
npm run build, so dist is missing and mocha's typestrip loader fails before any revert happens; the 50fa681 QA pass hit and recorded this identical procedural gap. Recovered locally by building the primary checkout directly and confirming test outcomes there; this is an infra limitation of the harness script, not a code defect.
Provenance: stage qa-check, head b1df138f5a4f7ccc3e59c39a6d1cdb03684a8f8e, recorded 2026-09-03T21:17:06Z.
Prior heads:
50fa681— PASS, 0 blocking finding(s), 2026-09-03T18:29:03Z2d228f7— PASS, 0 blocking finding(s), 2026-09-03T16:51:43Zed68ac7— PASS, 0 blocking finding(s), 2026-08-31T23:30:45Zfed985f— PASS, 0 blocking finding(s), 2026-08-31T22:34:52Zd6fd366— FAIL, 1 blocking finding(s), 2026-08-31T21:47:21Zb40c194— PASS, 0 blocking finding(s), 2026-08-31T19:47:16Zd4cda3e— FAIL, 1 blocking finding(s), 2026-08-31T18:32:29Z
Dispatch status — head
|
| Signal | Source | State |
|---|---|---|
| CI | GitHub checks | passing |
| QA | Dispatch qa-check stage |
PASS (evidence) |
| Independent review | Dispatch reviewer stage — Gemini CLI (gemini-3.5-flash) |
RECOMMEND_MERGE — high risk, 6 lens(es) (review) |
| Other review | gemini-code-assist[bot] — automatic GitHub app | commented (open) |
| Unresolved blocking findings | QA + independent review | 0 |
Checked at this head
- QA exercised server/DESIGN.md's new paragraph documents that Set-Cookie is the deliberate exception to…, The RFC 6265 §5.3 justification (name+Domain+Path identity, so a same-name pair scoped to…, Mutating the dedup key from exact-value to name-prefix (simulating the declined nit)…, and 4 more; tried to break it with Diffed 50fa681..b1df138 directly, Tried to falsify the doc's empirical claim by driving the real…, Tried to find a committed automated test that pins the specific name-collision/Max-Age=0…, and 5 more.
- Independent review inspected async websocket authentication, auth credential deferral logic, deferred credential rejection, dynamic port chain rebuilding, and 6 more.
Recommended human action: Review and merge if you agree.
Basis: QA PASS at b1df138 (Dispatch qa-check stage); independent review RECOMMEND_MERGE at b1df138 (Dispatch reviewer stage, Gemini CLI gemini-3.5-flash); CI passing; gemini-code-assist[bot] commented.
Prior heads and machine state
50fa681— CI passing, QA PASS, review RECOMMEND_MERGE, 2026-09-03T18:36:59Z2d228f7— CI failing, QA PASS, review RECOMMEND_MERGE, 2026-09-03T17:00:44Zed68ac7— CI passing, QA PASS, review RECOMMEND_MERGE, 2026-09-02T17:48:53Zfed985f— CI passing, QA PASS, review CHANGES_NEEDED, 2026-08-31T22:49:38Zd6fd366— CI failing, QA FAIL, review CHANGES_NEEDED, 2026-08-31T21:47:45Zb40c194— CI failing, QA PASS, review CHANGES_NEEDED, 2026-08-31T20:13:44Zd4cda3e— CI failing, QA FAIL, review missing, 2026-08-31T18:32:44Z
hdbjeff
left a comment
There was a problem hiding this comment.
Independent review: 6 requested change(s) at b40c194.
Inspected Authentication middleware and deferred credential rejection flow, Authorization-header preservation and application catch-all handoff, Base-versus-head behavior and coverage in the added unit and integration tests, Basic and Bearer parsing, principal assignment, session and local-bypass ordering, authentication caching, and deferred-rejection classification, Built-in MCP application routing, missing-principal normalization, and anonymous session initialization, Comprehensive validation against acceptance criteria and architectural design, GraphQL querying handler HTTPError wrapping and credential rejection, GraphQL route ownership, deferred-rejection settlement, and HTTP error-envelope mapping, HTTP server middleware chain synchronization for the 'all' pseudo-port, HTTP, upgrade, and WebSocket chain rebuilding after late port-all registrations, Node, Bun, and uWS status-minus-one fallback behavior, legacy Fastify delegation, and response cache headers, Operation-token and refresh-token verification, JWT rejection mapping, user-cache loading, and internal-fault propagation, REST resource matching, OpenAPI ownership, route misses, WebSocket ownership, authorization handoff, and error serialization, REST router/handler and WebSocket connection deferred credential resolution, Token authentication error mapping and fault/rejection separation, deferred credential rejection and routing-aware authentication, http and upgrade middleware chain rebuilding on all ports.
Source: Dispatch reviewer stage — Gemini CLI (gemini-3.5-flash), not the automatic Gemini Code Assist app.
GitHub declined to record this as a formal REQUEST_CHANGES review (HTTP 422: the reviewing account also authored this pull request), so the review object is a COMMENT and GitHub's own review decision will not show changes requested. The verdict is still CHANGES_NEEDED and the finding(s) are blocking; verification reads the verdict from the Dispatch reviewer state rather than from the GitHub review event.
6 finding(s) are attached inline to the changed lines they apply to.
Findings without a diff anchor
security/auth.ts:248— The refresh-token probe discards every fault fromvalidateRefreshToken()and rethrows the earlier ordinary operation-token rejection, allowing refresh-validation storage or runtime faults to be deferred.- Trigger: A valid Harper refresh token targets an application-owned URL; operation-token validation rejects its refresh subject, then refresh-token validation encounters a user-store, password-validation, or runtime fault.
- Evidence: The nested
catchat lines 248-249 ignores its caught error and throws the outer 401invalid token. Line 280 classifies that outer error as deferable, so the application catch-all runs. Before this PR, the same masking still ended in authentication's immediate 401. - Required fix: Capture the refresh-validation error and propagate it when it is not an explicitly tagged credential rejection. Restore the original operation-token rejection only after an ordinary refresh-token rejection.
- Domain: auth
- Confidence: confirmed
- Found by: codex
- Location: no valid GitHub diff anchor at this path and line; reported here rather than dropped
server/http.ts:1015— Bun and uWS discard authentication's deferred-credential cache floor when forwarding an unhandled request to legacy Fastify, allowing a credential-dependent response to lose private scope and credential variance.- Trigger: On Bun or uWS, send downstream-owned credentials to a legacy Fastify route using application-owned authorization and let it return cacheable content without equivalent
Cache-Control: privateandVaryheaders. - Evidence: Authentication adds
Cache-Control: private, no-cacheandVary: Authorization/Cookieto the status-minus-one chain response.makeUwsHandler()creates fresh headers solely frominjectResult.headers; Bun similarly delegates without the chain response and rebuilds solely from Fastify. Node preserves the chain headers before emitting the fallback. The adapters already discarded headers at the base revision, but unknown credentials could not reach them until this PR introduced deferral. - Required fix: Carry chain headers through both fallback adapters and merge them with final Fastify headers, unioning
Varyand preserving the private cache floor unless final-response authentication policy explicitly permits shared caching. Add Bun and uWS fallback tests. - Domain: api
- Confidence: confirmed
- Found by: codex
- Location: no valid GitHub diff anchor at this path and line; reported here rather than dropped
- Trigger: On Bun or uWS, send downstream-owned credentials to a legacy Fastify route using application-owned authorization and let it return cacheable content without equivalent
Dispatch durable state — independent review at
|
hdbjeff
left a comment
There was a problem hiding this comment.
Independent review: 2 requested change(s) at fed985f.
Inspected Bun and uWS legacy Fastify fallback handling for Cache-Control, Vary, and no-fallback 404 responses, Deferred request-state property descriptors and object-spread behavior, Explicit credential-rejection provenance through Basic authentication, JWT verification, account-state checks, and unsupported authorization schemes, Explicit protection against storage/outage failures being misclassified as deferrable credential rejections, Focused unit and integration assertions added for credential classification, settlement, MCP handling, and fallback headers, Generic client serialization and server-side logging of internal authentication faults, Integration of deferred authentication checks across Harper Http/static files, GraphQL, WebSocket upgrade, MQTT, and MCP handlers, Preservation of exact authentication error envelopes (non-REST/non-GraphQL error bodies) on Harper-owned routes via direct settlement, Propagation of storage, key-material, password-verification, refresh-token, and runtime faults through authentication, REST and GraphQL authentication-error status, body envelope, content type, and owner-specific error mapping, REST, GraphQL, MCP, static, MQTT, and WebSocket route-ownership settlement paths, Regression comparisons against both b40c194 and 1e4c163, Retention of authentication's Cache-Control and Vary identity floors across legacy Fastify fallbacks inside Bun and uWS adapters, Strict separation of client-side credential rejections from internal faults via unique private Symbol tagging, all pseudo-port middleware chain synchronization, fallback cache floor preservation on legacy servers, integration testing of deferred credential rejections, mcp adapters credential rejection handling, security middleware credential deferral testing, token rejection classification and fault handling.
Source: Dispatch reviewer stage — Gemini CLI (gemini-3.5-flash), not the automatic Gemini Code Assist app.
GitHub declined to record this as a formal REQUEST_CHANGES review (HTTP 422: the reviewing account also authored this pull request), so the review object is a COMMENT and GitHub's own review decision will not show changes requested. The verdict is still CHANGES_NEEDED and the finding(s) are blocking; verification reads the verdict from the Dispatch reviewer state rather than from the GitHub review event.
3 finding(s) are attached inline to the changed lines they apply to.
Follow-up candidates (non-blocking)
- Run the focused Mocha and integration suites in a dependency-populated checkout; this read-only checkout has no node_modules or local Mocha binary.
hdbjeff
left a comment
There was a problem hiding this comment.
Independent review: 1 requested change(s) at ed68ac7.
Inspected Anonymous behavior when no deferred rejection exists, Authorization-header and deferred-state preservation on delegated requests, Current MCP and MQTT paths relevant to the prior findings, Deferred credential settlement before ordinary static-file responses, Inspected server/static.ts for ownership invariant resolution of deferred credential rejections across static files, redirects, and not-found fallbacks., Mounted-route prefix stripping and middleware ordering, Regression comparison against fed985f and 1e4c163, Static implementation and test-file syntax plus diff whitespace validation, Static mount-root and registered-directory redirect ownership and Location construction, deferred credential authentication middleware, fallback cache control headers and integration tests, fallthrough:true delegation versus fallthrough:false built-in and configured notFound responses, mcp http adapter authentication integration, static resource routing and redirection, token rejection classification and internal error handling.
Source: Dispatch reviewer stage — Gemini CLI (gemini-3.5-flash), not the automatic Gemini Code Assist app.
GitHub declined to record this as a formal REQUEST_CHANGES review (HTTP 422: the reviewing account also authored this pull request), so the review object is a COMMENT and GitHub's own review decision will not show changes requested. The verdict is still CHANGES_NEEDED and the finding(s) are blocking; verification reads the verdict from the Dispatch reviewer state rather than from the GitHub review event.
1 finding(s) are attached inline to the changed lines they apply to.
Follow-up candidates (non-blocking)
- Resolve the unchanged MQTT WebSocket authentication-completion race tracked by b96515b2ad2d91bd; it is non-blocking for this delta because it predates the pull request.
- Run the focused static unit suite in a dependency-populated checkout; this read-only checkout has no node_modules or Mocha binary.
…ership is known (#2418) Harper's app-port `authentication` middleware resolved `Authorization` before route matching, so a syntactically valid credential it did not recognize terminated the chain with a 401 and downstream route ownership never ran. An application could not both let Harper own its native routes and let its own routes receive their own credential scheme. A rejection is now recorded rather than answered. An unrecognized credential leaves `request.user` unset, leaves the inbound header byte-for-byte intact, and records request-local state behind a module-private Symbol. Any layer that establishes Harper owns the route settles that state first and renders the same generic 401 as before, so Harper-owned routes behave identically, protected or public. Only a URL no Harper route owns carries the original header on to an application catch-all. Internal authentication faults are never deferred: `isCredentialRejection` accepts only a 4xx-carrying error, and `validateToken` no longer masks a key-read or storage failure as `invalid token`. The operations API never defers, since it owns every route. No path exemption list, carrier header, credential rename, or pre-auth stripping shim. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ously (#2418) An `Authorization` header whose scheme Harper does not implement (`Digest`, an application's own, or a header with no scheme token at all) matched no case in the strategy switch and threw nothing, so it continued as an anonymous request. On an anonymously-readable Harper-owned route that hands the content over, which is the exact downgrade deferral exists to prevent — and it made the contract inconsistent, since an unrecognized Basic or Bearer credential was already rejected there. A `default` case now rejects it, routing it through the same audit, fail-closed, and deferral handling as any other unrecognized credential. The gate stays on a strictly undefined user so the legacy blank-Basic "no auth" form still continues anonymously. Also guards the success audit log against that legacy null user, which would have thrown a TypeError when logging.auditAuthEvents.logSuccessful is enabled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…2418) An entry registered on the 'all' pseudo-port is folded into each concrete port's chain when that chain is built, but nothing dispatches through chains.all itself. A registration arriving after the bound port's chain already existed therefore updated only chains.all and never reached a request. That is exactly the shape an application catch-all takes: `rest` registers first, so a handler ordered `after: 'rest'` is always late. buildChains() now rebuilds every already-built chain of that kind when the registration is on 'all', for http, upgrade, and websocket alike. Rebuilding is a pure function of the listener list and the port, so the extra passes can only reproduce a port's order or extend it. It also writes the get_status chain description in the same pass, which removes the #1573 caveat about a description outliving the chain it described. Also corrects the no-credentials case in the #2418 end-to-end test: the integration harness starts Harper with AUTHENTICATION_AUTHORIZELOCAL=true, so a loopback caller sending no Authorization header is still resolved to the local super user. That pre-existing path is untouched by credential deferral, and asserting it explicitly is what distinguishes it from the deferred-credential cases, which must attach no principal at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ery Harper owner (#2418) Deferred credential rejection classified provenance by status range and left several Harper-owned handlers unsettled. Both are addressed here. - Introduce security/credentialRejection.ts: a module-private symbol tag set only where authentication concludes a credential is unacceptable. isCredentialRejection() reads nothing else, so a default-status-400 ClientError raised while lazily loading the user cache is a fault that fails closed rather than a deferrable unknown credential. - validateToken() converts only tagged JWT syntax, signature, expiry, not-before, subject and credential-state rejections; verification key material is validated up front and key-material JsonWebTokenErrors stay faults. Untagged errors propagate unmasked. - The Bearer refresh-token probe propagates a refresh-validation fault and restores the operation-token rejection only after an ordinary tagged refresh rejection. A fail-closed response logs the original fault and returns the generic authentication failure. - Settlement goes through settleDeferredCredentialRejection(), which returns the authentication middleware's own descriptor (401, {error: message} in the negotiated content type) ahead of REST's Problem Details and GraphQL's {errors:[...]} mapping. The built-in MCP HTTP adapter, static file serving and the MQTT WebSocket handler now settle too; MCP previously served an unrecognized credential as anonymous. - Bun and uWS merge the middleware chain's headers into the legacy Fastify fallback response, unioning Vary and preserving the private cache floor unless the final response opts into shared caching. - Deferred state is installed with a non-enumerable descriptor so object spread and Reflect.ownKeys cannot carry it into an application. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tokenRejectionClassification.test.js resolved the JWT keys directory from env.getHdbBasePath() at module load and wrote the key files itself, then removed the whole shared keys directory in after(). Under full unit-suite ordering the base path in effect at module load is not the one getJWTRSAKeys() reads at test time, so every case in the suite failed with "no encryption keys" on all three Node unit jobs. Install the keys through testUtils.installTestJwtKeys(), which resolves the directory at call time and returns a cleanup scoped to the three files it wrote, and sign with the key material it installed. The internal-fault cases replace the installed public key in place and beforeEach() restores it, so no shared fixture outside those three files is created or deleted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nse (#2418) The static handler settled a deferred credential rejection only on the ordinary file serve. When it is ordered `after: 'rest'` it runs downstream of authentication, so its other responses — the mount-root redirect, the registered-directory trailing-slash redirect, and both `fallthrough: false` not-found forms — answered a rejected credential as if it were anonymous. Before deferral existed those responses were unreachable for a rejected credential, because authentication returned 401 ahead of them. Settlement now happens immediately before each response this handler originates, with no URL exempted. The two index redirects are merged into a single branch so they share one settlement point; they were already mutually exclusive, since a `null` index entry is never the mount-root serve. The not-found settlement is placed ahead of `notFound` option validation and file resolution, so a rejected credential cannot turn a missing file or bad config into a 500 on a request the pre-deferral revision answered with 401. The actual `next(req)` fallthrough still leaves the rejection deferred: there Harper declines the URL, so a downstream owner or an application catch-all applies its own scheme to the untouched Authorization header. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ed68ac7 to
a7fc136
Compare
kriszyp
left a comment
There was a problem hiding this comment.
Excellent PR Jeff, I like this, and this is good quality (and only 3 findings on a PR this size indicates you definitely had it well-reviewed). Leaving the codex comments here for you to consider addressing. The await in MQTT is complicated. I see your AI wrestled with it too. Generally it should be await'ed, but that can cause problems for the MQTT protocol, and not await'ing is probably just bug-preserving. I think you made the write call there. Probably one more round and this will be ready to merge, I'd guess.
🤖 Reviewed with Codex
Three defects review found in the deferred-credential-rejection change: - server/mqtt.ts read the deferred state synchronously while the HTTP chain was still pending, so the guard always saw undefined and an invalid Authorization header connected anonymously. It now settles on the same promise the session principal resolves from, with frame handlers still attached synchronously. - The Node fallback copied the chain's identity floor onto the ServerResponse and let a Fastify route replace Cache-Control and Vary outright, which can make a credential-dependent response shared-cacheable. All three bridges now reconcile through mergeChainHeadersIntoFallback. - security/auth.ts post-processed a settled or application-owned 401, overwriting WWW-Authenticate or rewriting it to a login redirect. That rewriting is now skipped when a rejection was deferred, so a settled rejection stays wire-identical to the in-line 401 it replaced and an application keeps its own challenge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review on HarperFast/harper#2419 found that authentication was re-decorating any 401 returned up the chain: overwriting WWW-Authenticate with Basic, or rewriting it to a login-page redirect for a browser. That silently replaced an application catch-all's own challenge, which is the case this feature exists to support. The fix scopes that rewriting away from deferred credentials, so the externally visible contract now includes response provenance. Also records the WebSocket/MQTT upgrade outcome for an unrecognized credential. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
hdbjeff
left a comment
There was a problem hiding this comment.
Dispatch reviewer stage — Gemini CLI (gemini-3.5-flash). No changes requested at 50fa681. 1 non-blocking note(s) inline. Verification state and evidence are in the Dispatch status comment.
Follow-up candidates (non-blocking)
- Refactor Set-Cookie merging in mergeChainHeadersIntoFallback to parse cookie names and ensure consistent owner precedence.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
kriszyp
left a comment
There was a problem hiding this comment.
Great job Jeff!
🤖 Reviewed with Codex
| // It settles on the same promise the session principal comes from, which onSocket awaits | ||
| // before it processes any packet — the handlers below still attach synchronously, so no | ||
| // frame that arrives in the meantime is dropped. | ||
| const authenticated = Promise.resolve(chainCompletion).then(() => { |
There was a problem hiding this comment.
The deferred-rejection race is fixed, but chainCompletion can also fulfill with an authentication failure. For example, malformed Basic base64 makes atob() throw; authentication treats that untagged error as an internal fault and returns a fulfilled 401 descriptor at security/auth.ts:272-281. This continuation ignores that descriptor, finds no deferred state, and resolves to an undefined user. With MQTT requireAuthentication: false, a queued CONNECT is then accepted anonymously. Please reject a fulfilled chain result that indicates authentication stopped the HTTP chain before returning request.user, and add coverage where chainCompletion resolves to a 401 without deferred state.
* Document route-owned authentication fallthrough * Separate route authentication security outcomes in docs * Document that Harper does not rewrite a deferred-credential 401 Review on HarperFast/harper#2419 found that authentication was re-decorating any 401 returned up the chain: overwriting WWW-Authenticate with Basic, or rewriting it to a login-page redirect for a browser. That silently replaced an application catch-all's own challenge, which is the case this feature exists to support. The fix scopes that rewriting away from deferred credentials, so the externally visible contract now includes response provenance. Also records the WebSocket/MQTT upgrade outcome for an unrecognized credential. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes #2418
Summary
Harper's app-port
authenticationresolvedAuthorizationbefore route matching, so a non-Harper credential returned 401 before Harper knew who owned the URL. An application could not have both Harper owning its native routes and its own routes receiving their own credential scheme untouched.Authentication still runs first, but a rejection it cannot yet decide is recorded rather than answered:
request.userstays unset, the inbound header stays byte-for-byte intact, and the state sits behind a module-private symbol. Whichever layer establishes that Harper owns the route settles it and returns the generic 401. Only an unowned URL carries the header onward; internal faults never defer.Framing verdict: chosen-approach-sound;
urlPathcannot enumerate an open-ended proxy remainder.Important changes
findAndValidateUser(),validateToken()'s JWT rejections, and the unimplemented-scheme branch.validateToken()checks the configured key is key material beforejwt.verify().findAndValidateUser()raises a status-400ClientErrorwhensystem.hdb_roleis unavailable, andjsonwebtokenreports an unusable public key exactly as a forged token.tokenRejectionClassification.test.jsdrives the real validators against the installed RSA keys.deferCredentialRejection()installs a frozen value under a non-enumerable, non-writable, non-configurable descriptor, and returns early when a rejection is already recorded.deferredAuthenticationtests that clear it by reflection and assert first-rejection preservation.settleDeferredCredentialRejection()returns the descriptorauth.tsused to return in line, and REST, GraphQL, static and MCP return it ahead of their own error mapping.auth.tsskips its own 401 post-processing once a rejection was deferred.WWW-Authenticateand rewrites a browser 401 to aresources.loginPathredirect, silently replacing an application catch-all's own challenge — the case this feature exists to support.fallthrough: falsenot-found forms. MCP settles before the body is read.mqtt.tssettles on the HTTP chain's completion promise.after: 'rest', so its redirects and not-found responses answered a rejected credential as anonymous. Andserver/http.tshands the WebSocket chain a still-pendinghttpChain[port](request), so a synchronous read always sawundefined.Varyis unioned, private scope is re-applied unless the response opts into shared caching, and a chainSet-Cookieis appended beside Fastify's, de-duplicated by exact value and never by cookie name.has(name)guard discarded every chain cookie once Fastify set one, and Node re-runs the merge atwriteHeadover headers already carrying it. Name is not cookie identity (RFC 6265 §5.3 keys on name +Domain+Path), so name-keying drops a/wp-admin-scoped twin or aMax-Age=0deletion.fallbackCacheFloor.test.jsdrives the real uWS and Bun adapters and a real Fastify 5 app over a realhttp.Server; reverting each cookie behaviour fails a distinct test.Where to focus
Set-Cookiesees Fastify's. Challenged on review and upheld;server/DESIGN.mdnow records why name-keying is wrong.Risks and boundaries
WWW-Authenticate: Basicand no longer redirects a browser to a login page — pre-deferral behavior exactly, but invalid and missing credentials now present differently.bridgeChainHeadersToNodeResponse()replaceswriteHeadon the declined request'sServerResponse; a fallback committing headers another way would bypass reconciliation.fastifyAuth.tsre-authenticates through its own Passport path, the intended 'a route Harper does not own applies its own scheme' behavior.Request.ts,middlewareChain.ts,operationsServer.ts,fastifyAuth.ts, two named unit tests): the state is request-local, ordering landed earlier, and that coverage lives inauthCredentialDeferral.test.jsandhttpChainPortAll.test.js.test:unit:main, theunitTests/server/**glob, and integration's sudo-provisioned loopback aliases — the first two fail identically on base.Validation
server/DESIGN.mdonly:fallbackCacheFloor.test.js+Headers.test.js54 passing / 0 failing,prettier --checkclean.npm run test:unit:security759 passing / 2 failing — two pre-existing macOSjsLoadersymlink failures, untouched by this branch.unitTests/server/*.js335/0, MCP components 499/0.npm run buildandnpm run lint:requiredclean at the prior head; CI there: 43 checks pass, 0 fail across Node 24, Windows, Bun and uWS, including all six integration shards.Dispatch evidence and stage history
Current head
b1df138· size classlarge(656 changed production lines) · visible body 990 words (guardrail 600-1000).Evidence:
Set-Cookiepreservation fails 1 test, its de-duplication 1, collapsing Bun's cookie array 1, restoring the mutable deferral descriptor 2.Set-Cookiede-duplication predicate was challenged on review as needing to key on cookie name. Declined on the merits and answered in-thread: driving the realmergeChainHeadersIntoFallbackwith a WordPress-shapedname-across-/-and-/wp-adminpair plus aMax-Age=0deletion, exact-value keeps all four cookies while name-keying drops the chain's/cookie and the deletion. Name-keying is also the pre-PRhas(name)guard narrowed to one name, which is the session-loss bug item 5 fixes.server/DESIGN.mdhad said 'Fastify wins every header it set' unqualified, which is what made line 196 read as a violation. This head narrows that to single-valued headers and records theSet-Cookieexception, its RFC 6265 rationale, and thewriteHeadidempotence constraint.mainmerged conflict-free (c1a78f8) plus guideline-retrofit commits.maintouched none of the files this PR changes.authentication -> rest -> application catch-allintegration suite passes 13/13 on Node and uWS in CI, with its regression assertions verified to fail on base behavior.server/DESIGN.mddocuments the tagging sites, the settlement obligation for any handler registered afterauthentication, the WebSocket settlement timing, the 401-provenance rule, and the three-adapter fallback merge.Object.getOwnPropertySymbols({...request}).catch2; echoed fault message 1; throw-based settlement 2 integration tests.Complexity:/Review-Coverage:/Human-Review-Need:footers are absent here; those are machine-derived by Harper'spr-body-update.mjsand are never hand-typed.Prior heads:
50fa681— large, 997 words, 5 important change(s), 2026-09-03T18:10:19Z2d228f7— large, 994 words, 4 important change(s), 2026-09-03T16:31:48Zed68ac7— large, 995 words, 5 important change(s), 2026-08-31T23:14:24Zfed985f— large, 966 words, 5 important change(s), 2026-08-31T22:16:48Zd6fd366— large, 995 words, 6 important change(s), 2026-08-31T21:26:55Zb40c194— medium, 600 words, 4 important change(s), 2026-08-31T19:35:45Zd4cda3e— medium, 595 words, 4 important change(s), 2026-08-31T17:58:40Zb1d0e9e— medium, 600 words, 4 important change(s), 2026-08-31T17:50:16ZReview-Coverage: authored=claude; ran=codex; declined=gemini,cursor-grok,cursor-composer,domain; rounds=4 @ b1df138
Human-Review-Need: 4 @ b1df138