fix(security): close V-03, found by falsifying V-01's own fix - #57
Merged
Conversation
An adversarial pass over the ten fixes merged in PRs #53, #55 and #56. It is not independent - the agent that wrote the fixes wrote the pass - and section 11 says so before anything else. Section 9.1 item 3 stays open. It found one real defect, which is the argument for running the independent version. V-03 - V-01 made findMemberValue last-match-wins, fixing literal duplicate keys. It still compared the undecoded source slice against 'content', but "content" is content to JSON.parse. So the parser resolved the duplicate to the escaped key's value while the scanner matched only the literal one, and V-01's exact corruption came back around V-01's fix: DECOY destroyed, both copies of the block surviving, dedup saving nothing. Reproduced end to end after the fix was in. The key is decoded with JSON.parse now. How it survived matters more than what it was. V-01's fix passed four unit tests and an end-to-end reproduction, and every one of them shared the fix's own assumption about what a key is - so agreement between check and thing checked proved nothing about the world. That is this review's recurring failure mode at one more remove, and the author of a fix writing its tests is how it keeps happening. Five other bypass hypotheses failed and are recorded as verified negatives, the most useful being the SSRF guard against alternate IP notation: decimal 2852039166, hex 0xA9FEA9FE, octal 0251.0376.0251.0376 and trailing-dot forms are all refused, because WHATWG URL normalises them before the guard sees them. That is the commonest way such an allowlist is defeated and it was worth confirming rather than assuming. One hardening deliberately abandoned: U+0085, U+2028 and U+2029 survive the label escaping. The demonstrated guarantee is intact - header count by \n is unchanged with all three present - and whether a consumer renders them as breaks is section 9.1 item 6's question. Three attempts to escape them inserted the literal characters into the regex, which TypeScript cannot hold, and the change hardens against a speculative renderer rather than a demonstrated exploit. Recorded as a residual. 915 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.
An adversarial pass over the ten fixes merged in #53, #55 and #56. It found one real defect, and fixes it.
Read this first: the pass is not independent
The protocol assigns falsification to a fresh agent precisely because an author cannot check their own reasoning — and the agent that wrote every fix examined here wrote this pass too. §11 of the report says so before any result, and §9.1 item 3 stays open. A confirmation here is worth materially less than one in §8. The single finding below is evidence the exercise was worth doing, not evidence that it was sufficient.
V-03 — V-01's fix was incomplete
V-01 made
findMemberValuelast-match-wins, which fixed literal duplicate keys. It still compared the undecoded source slice against'content'— but"content"iscontenttoJSON.parse.So given
{"content":"AAAA","content":"BBBB"}the parser resolves toBBBBwhile the scanner matched only the literal key and returnedAAAA's span: V-01's exact corruption, reached around V-01's fix. Reproduced end to end after the fix was in —DECOY survived? false, block occurrences2. One value destroyed, and the dedup saved nothing.Fixed by decoding the key with
JSON.parse, which is the only comparison that cannot disagree with the parser downstream.Why this matters more than its severity
V-01's fix was verified end to end and by four unit tests, and it was still wrong — because the fix and its tests shared an assumption about what a "key" is. Agreement between the check and the thing checked proved nothing about the world.
That is this review's recurring failure mode at one more remove, and the author of a fix writing its tests is the mechanism by which it keeps happening. It is the concrete argument for §9.1 item 3.
Five bypass hypotheses that failed
Each a specific attack, not a re-read:
https://2852039166, hexhttps://0xA9FEA9FE, octalhttps://0251.0376.0251.0376, trailing-dothttps://169.254.169.254.— all four refused. WHATWGURLcanonicalises every one to169.254.169.254before the guard sees it, so the check inherits the parser's normalisation instead of reimplementing it. This is the single most common way such an allowlist is defeated; worth verifying rather than assuming.storeContentis only ever called with a 64-character digest, and a library caller who stores a short key already holds its content. Not an oracle.NULL,nulland every other spelling reachnew URL(), throw, and are refused.x-api-keyis no more CORS-safelisted thanauthorization.One hardening deliberately abandoned
U+0085 NEL, U+2028 LINE SEPARATOR and U+2029 PARAGRAPH SEPARATOR survive the
\r/\nescaping. The demonstrated guarantee is intact — header count by\nis unchanged with all three present, so no forged header appears — and whether a consumer renders them as breaks is §9.1 item 6's question, not this code's.Three attempts to escape them anyway inserted the literal characters into the regex, which TypeScript cannot hold. The change hardens against a speculative renderer rather than a demonstrated exploit, so it is reverted and recorded as a residual instead of pursued further.
Checks
98 test files / 915 tests pass, 2 skipped (POSIX-only file modes). Typecheck and lint clean. Gateway-only change; no corpus run applies.
🤖 Generated with Claude Code