fix(security): close all seven security-review findings - #53
Merged
Conversation
Closes the gap section 8.5 recorded: Session 3 landed while Session 4 was in
flight, so R-12 and clean-list entries 29-36 had never been attacked. All eight
examined; all eight hold. None retracted, none downgraded.
The value is not the verdicts. Four of the eight rested on a sample of one or
on a narrow pattern, and each now rests on the full population:
- R-12 admitted it did no entropy scoring, so a high-entropy secret in an
unrecognised format would be missed, and invited disagreement. Added it:
25,512 candidate tokens over 198 commits / 1,050 blobs, 217 high-entropy
mixed-case, of which 214 are npm lockfile integrity hashes and 3 are benign
identifiers. Zero credentials. The admitted limit costs nothing here.
- Entry 30 checked one .js.map for sourcesContent; checked all 70, plus the
absence of any non-.d.ts TypeScript in the tarball.
- Entry 34 rested on grep "fetch(" - the same narrow shape that already failed
for clean-list item 22. Redone as an exhaustive builtin-import census: src/
imports child_process, crypto, events, fs, http, path, stream and nothing
else. No net, tls, dgram, https or dns anywhere.
- Entry 31 re-measured at v1.7.3: zero issuer-pattern matches in the tarball
(Session 3's sk-tolerance is a substring of --risk-tolerance and does not
survive a length-bounded pattern), zero high-entropy tokens from 876.
Also recorded: every credential pattern in the whole of history resolves to one
path, this report, introduced by three commits; a size-filter coverage
hypothesis of my own, tested and disproven (0 blobs >= 2MB); citation drift in
entry 35 (server.ts:71 -> 134), the same v1.6.0-coordinates problem section 8.4
records for Sessions 1-2; and a developer path in tools/corpus-harness that is
committed but not shipped.
Nothing under src/ was modified.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… mode
Two findings from the 2026-08-30 security review, both with the test that
would have caught them (oxaudit-split.md section 9). Neither changes optimized
output; no corpus run applies.
F-02 - GatewaySessionStore.getContent resolved an arbitrarily short prefix.
The scan returned stored plaintext whenever exactly one hash started with the
ref, with no minimum length, so ref=a recovered a block in ~16 guesses per hex
digit. Session 4b found it is worse than reported: ref= (empty) recovers it in
zero guesses when the session holds one block, because startsWith('') matches
every hash. Both are reachable from a string, not just an API call, because
normalizeHashOrRef extracts ref= out of a marker-shaped argument.
Refused below ELISION_HASH_PREFIX_LENGTH now, mirroring TokenHasher.resolve,
which has always had the guard. No shipping caller narrows: session-dedup
emits contentHash.slice(0, 12). Full digests still resolve at any length
because the exact-match branch runs first.
F-04 - the --diff-html report embeds every item's full before and after
content and was written with no mode, landing at 0666 & ~umask. Measured on
ext4: 644. Now 0600.
Both a write mode and a following chmodSync, and the second is not redundant:
writeFileSync's mode applies only when the file is created, so overwriting an
existing report kept its older wider mode. Verified on ext4 under WSL2 both
ways - fresh file 600, pre-existing 644 report narrowed to 600. The two POSIX
assertions skip on Windows, where ACLs and not mode bits are the operative
control; README now states both the 0600 guarantee and that the report is a
full plaintext copy.
98 files / 876 tests pass, typecheck and lint clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two more findings from the 2026-08-30 security review, each with the test that would have caught it. F-05 - CONSTRAINT_DIRECTIVE_LOST quoted the directive verbatim, and that message becomes trace.fallbackReason: stderr on every CLI run, and the full trace returned to an MCP client by get_optimization_trace. The report's own reproduction shows a secret riding along inside the quoted clause. Now reported as length, offset and a 12-char digest. Truncation was the other candidate and loses on measurement: the secret in the repro starts at character 24 of a 53-character line, so any cap wide enough to stay readable still emits it, and any cap tight enough to suppress it is no longer diagnostic. Verified against both populations end to end. The markdown case matters: it reaches this with no elision and no language support at all, through whole-item hashing, so the report's narrowing to "three languages inside an elided region" was wrong in the direction of under-reporting. validation-integration.test.ts asserted the message contained the directive text. That assertion is now inverted, with the reason at the site - it was pinning the behaviour F-05 removes. I initially claimed no test depended on that message, from a grep for the message prefix rather than the directive text; the wider grep is what found it. F-06 - the multi-item envelope prints ==> item.path <== unescaped, and a POSIX filename may contain newlines, so a crafted name planted a second well-formed header naming a file that does not exist. Demonstrated on ext4: three real files, four headers, and the attacker's own file appearing only as the malformed remainder. Line breaks in the label are escaped now: three files, three headers. Label vector only. A delimiter-shaped line in file *content* still passes through, because escaping content would corrupt the bytes this tool exists to deliver intact; documented instead, and mitigated by shape, since every genuine label on a shipping CLI route is an absolute path. No corpus row moves - no corpus file has a newline in its name - which is a real fix the instrument cannot see rather than an inert one. 98 files / 885 tests pass, typecheck and lint clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…g, F-07 docs The last three findings from the 2026-08-30 security review. All four remaining design calls were the user's, taken before any code was written. F-01 - getSessionIdFromHeaders fell back to the literal 'default-session', so every client setting no session header shared one session object. That is the common case: the tools exec exists to wrap generally set none. Two unrelated local processes wrote into each other's dedup state, and a third could flush it with a single request carrying 130 blocks. The fallback is now per connection, keyed on the socket - the only thing here a caller cannot choose, since every peer is 127.0.0.1 and the address discriminates nothing. Measured: two connections give two sessions where they gave one, and keep-alive keeps a client's second turn in its own, so cross-turn dedup is unaffected. Explicit x-session-id is unchanged and still shared, which is the documented exec trust boundary. Scope stated rather than overclaimed: this narrows a default, it does not authenticate. Session state is also still mutated before any credential check, so a request ending in 401 has already written to the store. Recorded, not fixed. F-03 - ingestion never consults .gitignore, so optimize <dir> read secrets.yaml, serviceAccount.json and config/credentials.yaml and wrote them to stdout. It now names them on stderr. It reports rather than filters: skipping would change which bytes the pipeline sees, and .gitignore covers plenty a caller may legitimately mean to optimize. Two things the implementation had to get right, both found by testing rather than by reading. git check-ignore answers according to the repository it runs in, so running it in the process's own cwd reported nothing at all for optimize /other/repo - silently, which is the failure shape this warning exists to prevent; it now runs in the tree being read. And -z is load-bearing: without it core.quotePath C-quotes every Windows path, printing "C:\Users\..." rather than something the caller can act on. Paths go over stdin with execFile and an argument array, so no filename reaches a command line. F-07 - documented rather than fixed. A per-run nonce in markers would change emitted bytes every run and make output non-deterministic across runs, which collides with invariant 1. The README note records the practical discriminator instead: every genuine header on every CLI route carries an absolute path, so a bare or relative one did not come from the ingester. Optimized stdout is unchanged by all three. F-03 writes to stderr only (verified: 0 occurrences of the warning on stdout, 4 headers unchanged), F-01 is off the optimize route entirely, F-07 is documentation. The one prior change that can touch stdout, F-06's label escaping, alters only paths containing CR or LF: 0 of 70 corpus files and 0 of 4 fixture files qualify. 98 files / 887 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
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.
Closes all seven findings from the 2026-08-30 security review, and records Session 4b.
The review ran four sessions, the last an independent falsification pass that reproduced every finding against a build, downgraded one, and retracted one of the report's own narrowings.
docs/security-review-2026-08-30.mdcarries the full record; this PR is the remediation.What changed
'default-session'literalELISION_HASH_PREFIX_LENGTH, mirroringTokenHasher.resolve--diff-htmlreport written0600, then narrowed again600, pre-existing644report narrowed to600Each fix lands with the test that would have caught it, per
oxaudit-split.md§9 —test/unit/security-review-findings.test.ts, 22 cases.Design calls
Four were the user's, taken before any code was written, because they are product decisions rather than defects with an obvious fix:
x-session-idcan still bind to any id including another client's — unchanged, and the documentedexectrust boundary. Session state is also still mutated before any credential check, so a request ending in 401 has already written to the store. Recorded in the CHANGELOG, not fixed.Output impact
Optimized stdout is unchanged. F-03 writes to stderr only (verified: zero occurrences of the warning on stdout, header count unchanged), F-01 is off the optimize route entirely, F-05 changes the trace on stderr, and F-07 is documentation. The one change that can touch stdout — F-06's label escaping — alters only paths containing CR or LF: 0 of 70 corpus files. That is a real fix the instrument cannot see rather than an inert one, in the shape of OX-L7.
No corpus run is included for that reason; the affected populations are named above instead.
Two implementation notes worth reading
Both of these were wrong on first write and only testing caught them:
git check-ignoreran in the process's own cwd, sooptimize /other/reporeported nothing at all — silently, which is precisely the failure shape the warning exists to prevent. It now runs in the tree being read.-zis load-bearing. Without it,core.quotePathC-quotes every Windows path, so the warning printed"C:\\Users\\..."rather than something a caller can act on. Paths go over stdin withexecFileand an argument array, so no filename ever reaches a command line and a path beginning with-cannot become a flag.Also in this branch
90721bbadds §8.6 to the review — Session 4b, which falsified Session 3's own work (R-12 and clean-list entries 29–36). All eight hold; four now rest on the full population rather than a sample of one. Notably it adds the Shannon-entropy sweep R-12 said it lacked: 25,512 candidate tokens over 198 commits and 1,050 blobs, 217 high-entropy mixed-case, of which 214 are npm lockfile integrity hashes and 3 are benign identifiers. Zero credentials.Checks
98 test files / 887 tests pass, 2 skipped (POSIX-only file-mode assertions, verified separately under WSL2).
typecheck,lintandbuildall clean.🤖 Generated with Claude Code