audit+fix(security): the v1.7.3 surface Sessions 1-4 never read (V-01, V-02, F-01 residual) - #56
Merged
Merged
Conversation
…sion 5) Closes section 9.1 item 2. Passes 1, 3, 4 and 5 over c4f4149..445b3ed - the +859/-223 across 17 files that landed after Sessions 1-3 read the tree and that Session 4 opened only where an existing finding touched it. Two findings, both Gateway, both reproduced end to end. V-01 - the egress splice resolves duplicate JSON keys first-wins while JSON.parse resolves them last-wins. A message with two content keys is optimized as the second value and spliced over the first: the forwarded body lost DECOY entirely, kept both copies of the repeated block so the dedup achieved nothing, and carried a marker the provider cannot resolve. forwardableBody's never-grow guard does not catch it because the corruption makes the body shorter. Same divergence on Anthropic's system field. This is the M7/C4 class - bytes reaching a provider that are not what the caller sent. V-02 - Origin: null bypasses the browser-origin refusal. The guard reads originHeader !== 'null', so the one Origin value a browser sends when it is most sandboxed is the one exempted. Measured: a foreign origin is refused 403 with no session created; Origin: null returns 200 and creates one. V-02 also overturns a judgement made earlier in this document, recorded in 10.2 as a correction rather than a quiet amendment. Section 3.1 refused to hoist the Gateway credential check because hasAuthHeaders tests presence, so a local attacker passes it with "Bearer anything". That reasoning considered one attacker and there are two: a browser may not set authorization on a simple request at all, and asking for it forces a preflight this server refuses. So against the browser V-02 reaches, hoisting the check is a real control. The original paragraph stands as written - the measurement was honest, the scope of the conclusion was not. Seven clean results recorded, including two worth carrying: the hand-written JSON scanner is iterative rather than recursive, so nested attacker JSON cannot overflow the stack, and it is linear by measurement - 16x input for ~4x time, 1.86 MB in 22 ms. And drift-tracker's 332 changed lines touch no gate: shouldFallback, measurementGate and retentionGate appear 14 times in the file and zero changed lines contain any of them, thresholds identical in both trees. That negative was itself re-checked against the vacuous-grep failure mode section 8.3 records. Docs only. No source changed; the two findings are filed, not fixed. 905 tests pass, lint clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The three findings Session 5 filed against the v1.7.3 surface, each with the
test that would have caught it.
V-01 - findMemberValue returned the first match while JSON.parse resolves a
duplicate name to the last. The pipeline optimizes what the parser produced and
the span says where to write it back, so the two rules disagreeing meant the
splice overwrote the wrong value: given {"content":"A","content":"B"} the
engine reasoned about B and overwrote A. The forwarded body lost A, still
carried B unelided, and gained a marker the provider cannot resolve.
forwardableBody's never-grow guard misses it because the corruption makes the
body shorter. Now last-match-wins. Verified end to end: the shadowed value
survives and the real duplicate is elided, so the block appears once, not twice.
V-02 - the browser-origin guard read originHeader !== 'null', exempting the one
Origin value a browser sends when it is most sandboxed. Now refused. An absent
Origin still passes, because that is the non-browser client; the literal string
null is a browser declining to name itself.
F-01 residual - the credential check ran after the stages, so a 401 had already
created a session, and an empty session still occupies a slot under maxSessions.
Hoisted above getOrCreateSession. Measured: 20 unauthenticated requests leave
sessionCount at 0; a rejected method creates nothing either.
This fix was refused earlier in the review and the reversal is the point. It
was assessed vacuous because hasAuthHeaders tests presence, so a local process
passes it with "Bearer anything". That reasoning covered one attacker and there
are two: V-02 showed a browser reaches this handler, and a browser cannot set
authorization on a simple request at all - not CORS-safelisted, so asking for
it forces a preflight this server refuses. Against that caller the check is
real. The original assessment stands in section 3.1 as written; what was wrong
was the scope of its conclusion, not its measurement.
One regression introduced and caught by an existing test: hoisting the 401
removed, as a side effect, the branch where allowMissingUpstreamCredentials
answers locally instead of forwarding. gateway-response-headers.test.ts failed
on it immediately and it is restored explicitly in both routes - the M8 seam's
own regression test doing what it was written for.
ProxyRequestResult.session is optional as a consequence: a 401 or 405 has no
session to report, which is the point. No consumer reads it off a result.
914 tests pass, typecheck and lint clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Audits the v1.7.3 surface Sessions 1–4 never read, then fixes everything it found. Two commits: the audit, then the remediation.
Closes §9.1 items 2 and 4 of the security review.
The audit (Session 5)
Passes 1, 3, 4 and 5 over
c4f4149..445b3ed— the +859 / −223 across 17 files that landed after Sessions 1–3 read the tree and that Session 4 opened only where an existing finding touched it.Two findings, both Gateway, both reproduced end to end. Seven clean results.
V-01 — the egress splice overwrote the wrong value
findMemberValuereturned the first match;JSON.parseresolves a duplicate name to the last. The pipeline optimizes what the parser produced and the span says where to write it back, so the two rules disagreeing is not cosmetic. Given{"content":"A","content":"B"}the engine reasoned aboutBand the splice overwroteA:Adestroyed outrightBstill present, unelided — the dedup saved nothingforwardableBody's never-grow guard does not catch it, because the corruption makes the body shorter. Same divergence on Anthropic'ssystemfield.Fixed: last-match-wins. Verified end to end — the shadowed value survives and the real duplicate is elided, so the block appears once rather than twice. Both halves matter.
V-02 —
Origin: nullbypassed the browser-origin refusalThe guard read
originHeader !== 'null', so the oneOriginvalue a browser sends when it is most sandboxed — a sandboxed iframe, adata:URL, some redirect chains — was the one value exempted. Measured: a foreign origin → 403, no session;Origin: null→ 200, session created.Fixed:
nullis a foreign origin. An absentOriginstill passes, because that is the non-browser client; the literal stringnullis a browser declining to name itself.F-01's residual — and a reversal of my own earlier decision
The credential check ran after the stages, so a request ending in 401 had already created a session, stored its blocks and advanced the turn counter — and an empty session still occupies a slot under
maxSessions, which is the whole eviction primitive.This fix was refused earlier in the review, and the reversal is the substantive part. §3.1 assessed it vacuous because
hasAuthHeaderstests presence rather than validity: a local process passes withBearer anything. That reasoning covered one attacker and there are two. V-02 showed a browser reaches this handler, and a browser cannot setauthorizationon a simple request at all — the header is not CORS-safelisted, so asking for it forces a preflight this server answers405with noAccess-Control-*. Against that caller the check is real.§3.1's paragraph stands unedited in the report. The measurement was sound; the scope of the conclusion drawn from it was not, and §10.2 records that as a correction rather than an amendment.
Fixed: hoisted above
getOrCreateSession. Measured — 20 unauthenticated requests leavesessionCountat 0; a rejected method creates nothing either.A regression I introduced, caught by an existing test
Hoisting the 401 removed, as a side effect, the branch where
allowMissingUpstreamCredentialsanswers locally instead of forwarding.gateway-response-headers.test.tsfailed on it immediately — the M8 seam's own regression test doing exactly what it was written for. Restored explicitly in both routes.ProxyRequestResult.sessionis optional as a consequence: a 401 or 405 now has no session to report, which is the point. No consumer reads that field off a result.Clean results worth carrying
JSON.parseruns first and returns 400.drift-tracker.ts's 332 changed lines touch no gate.shouldFallback,measurementGateandretentionGateappear 14 times in the file and zero changed lines contain any of them; thresholds identical in both trees. That negative was itself re-checked against the vacuous-grep failure mode §8.3 records.Checks
98 test files / 914 tests pass, 2 skipped (POSIX-only file modes). Typecheck and lint clean. No corpus run: all three findings are Gateway-only, and the Gateway is off the corpus route entirely.
🤖 Generated with Claude Code