fix(security): close S-03 and S-04, the last two Session 7 findings - #62
Merged
Merged
Conversation
With the previous commit this closes every finding in
docs/security-review-2026-08-30.md: F-01..F-08, V-01..V-03, S-01..S-04.
Unlike S-01 and S-02, neither of these is a fix that failed to do what it
claimed — each is an incomplete guarantee its author had no particular reason
to anticipate.
S-03 — a JSON syntax error no longer quotes the payload back. JsonValidator
forwarded V8's JSON.parse message verbatim, and one of its forms embeds the
input: `Unexpected token 'q', ..."Ab9x","r":qq}" is not valid JSON` — about
fifteen characters either side of the error, and for a document shorter than
that window, all of it. The offending character in the quotes is itself a
payload byte. That string reaches ValidationIssue.message, is joined into
`reason`, and lands in trace.fallbackReason, which the CLI writes to stderr on
every run. It is the defect F-05 fixed for constraint directives, on the same
field, six lines away, through a sibling message nobody had looked at.
The message is now built from a fixed vocabulary — a prefix match against a
seventeen-entry table returning this file's own constants, never a slice of
V8's string — plus the validator's own line and column. An unrecognised form
degrades to `invalid JSON`, so a future Node makes the message vaguer rather
than leakier; regex-stripping the quoted clause would have failed open on the
same day. Message shapes were enumerated against Node 22 and 26, which agree:
only the `Unexpected token` family ever carried input. The sibling validators
were checked rather than assumed — they interpolate positions and structural
characters only.
S-04 — the gateway does not follow an upstream redirect. §6.3's guard runs
once, at start(), on the configured base URL; forwardUpstreamRequest set no
`redirect` option, so fetch defaulted to `follow` and a 302 is not that
string. Demonstrated end to end: a stub provider redirecting to a stand-in
metadata listener delivered x-api-key to it, and the gateway relayed that
listener's body to the caller as a 200. authorization was stripped, but by
undici implementing the Fetch spec's cross-origin rule rather than by anything
here — and x-api-key is a vendor header on no such list, so the OpenAI-shaped
credential was safe by inheritance and the Anthropic-shaped one was not.
`redirect: 'manual'` now, and a 3xx (or the spec's opaque-redirect status 0)
becomes a 502 naming the status; the body is cancelled so a redirect-only
upstream cannot hold connections open. Location is upstream-controlled text
and is deliberately not echoed. This is a behavioural change: an endpoint that
answers a redirect now fails instead of being followed. Neither
api.openai.com nor api.anthropic.com redirects an API POST, and an endpoint
that does needs its destination configured as the upstream URL, which leaves
the guard covering it. No new flag: following safely means re-validating
Location, deciding what to strip and bounding the hops, for a case no provider
needs.
Written test-first. S-03 failed as `not to contain 'Ab9x'` and as a
505-document property violation; S-04 failed as `expected { host:
'127.0.0.1:51845', ... } to be undefined` — the metadata listener's own
headers, arriving. Nine cases added, including a control that a
non-redirecting upstream still receives the key and still returns 200.
Verified end to end against the rebuilt artifact: fallbackReason now reads
`JSON Syntax Error: unexpected token at line 2, column 1` with no payload
anywhere in the trace, and the redirect harness gets 502 with nothing reaching
the listener at all. 99 files / 936 passed, 2 skipped. Typecheck, lint, build
clean. No corpus run: S-04 is Gateway-only and S-03 moves no optimized byte,
because no stage reads a validation message.
Also corrects two stale claims now that the pass exists: the report's status
box said the fixes had been falsified by nobody, and CLAUDE.md listed the same
thing as open.
DECISIONS §74; report §12.9.
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.
With #61 this closes every finding in
docs/security-review-2026-08-30.md— F-01…F-08, V-01…V-03, S-01…S-04.Unlike S-01 and S-02, neither of these is a fix that failed to do what it claimed. Each is an incomplete guarantee its author had no particular reason to anticipate.
S-03 — a JSON syntax error no longer quotes the payload back
JsonValidatorforwarded V8'sJSON.parsemessage verbatim, and one of its forms embeds the input:About fifteen characters either side of the error — and for a document shorter than that window, all of it (
JSON.parse('ZZSECRETZZ')→Unexpected token 'Z', "ZZSECRETZZ" is not valid JSON). The offending character in the quotes is itself a payload byte. That string reachesValidationIssue.message, is joined intoreason, and lands intrace.fallbackReason, which the CLI writes to stderr on every run.It is the defect F-05 fixed for constraint directives, on the same field, six lines away, through a sibling message nobody had looked at.
The message is now built from a fixed vocabulary — a prefix match against a seventeen-entry table returning this file's own constants, never a slice of V8's string — plus the validator's own line and column:
An unrecognised form degrades to
invalid JSON, so a future Node makes the message vaguer rather than leakier; regex-stripping the quoted clause would have failed open on the same day. Message shapes were enumerated against Node 22 and 26, which agree — only theUnexpected tokenfamily ever carried input. Sibling validators were checked rather than assumed: they interpolate positions and structural characters only.S-04 — the gateway does not follow an upstream redirect
§6.3's guard runs once, at
start(), on the configured base URL.forwardUpstreamRequestset noredirectoption, sofetchdefaulted tofollow— and a302is not that string. Demonstrated end to end: a stub provider redirecting to a stand-in metadata listener deliveredx-api-keyto it, and the gateway relayed that listener's body to the caller as a 200.authorizationwas stripped — but by undici implementing the Fetch spec's cross-origin rule, not by anything here.x-api-keyis a vendor header on no such list, so the OpenAI-shaped credential was safe by inheritance and the Anthropic-shaped one was not.redirect: 'manual'now; a 3xx (or the spec's opaque-redirectstatus === 0) becomes a 502 naming the status, and the body is cancelled so a redirect-only upstream cannot hold connections open.Locationis upstream-controlled text and is deliberately not echoed.This is a behavioural change. An endpoint that answers a redirect now fails instead of being followed. Neither
api.openai.comnorapi.anthropic.comredirects an API POST, so nothing anyone does today changes; an endpoint that does needs its destination configured as the upstream URL, which leaves the guard covering it. No new flag — following safely means re-validatingLocation, deciding what to strip and bounding the hops, for a case no provider needs.Written test-first
The last one is the metadata listener's own request headers, arriving. Nine cases added, including two controls: a 500-document property test that no marker ever survives into a message, and a non-redirecting upstream that must still receive the key and still return 200.
Verified
fallbackReasonreadsJSON Syntax Error: unexpected token at line 2, column 1, no payload anywhere in the trace.Also
Two stale claims corrected now that the pass exists: the report's status box said the fixes had been falsified by nobody, and
CLAUDE.mdlisted the same thing as open. Both now point at §12 and DECISIONS §73–§74.Docs: DECISIONS §74, CHANGELOG
[Unreleased], report §12.9.🤖 Generated with Claude Code