fix(security): validate the upstream base URL, and integrate the review into the docs - #55
Merged
Merged
Conversation
The report described three trees at once and said so nowhere. Sessions 1-3 audited c4f4149 (v1.6.0); Sessions 4 and 4b falsified against 445b3ed (v1.7.3); the fixes landed on top of v1.7.3. Findings F-01..F-07 read as open while F-08 carried a FIXED marker, and five statements elsewhere had become false. A report is the gate deciding what a later session may file, so a stale row does damage in both directions - it makes fixed behaviour read as known-broken, and it makes a session skip verifying something. Nothing that constitutes evidence was edited. No finding text, no reproduction, no pass record. The seven findings rows keep their bodies verbatim and gain only the FIXED marker in the ID cell, matching how F-08 was already marked. Corrections are strikethroughs with the replacement beside them, which is this repository's existing convention and the reason its own retracted claims are still legible. Added: - A status box in the header. Says which tree each section describes, that all eight findings are fixed, that the coordinates in sections 3-5 are v1.6.0, and what is still open. - Section 3.1, a remediation table naming what each fix deliberately does NOT cover - F-01's mutate-before-credential-check, F-06's content vector, F-07's unauthenticated markers. - A staleness box on section 1. Four exclusion-list rows are false at v1.7.3 (OX-M8, M9, M15, H2 all shipped), plus section 6 item 35's line drift. - Section 9, a remediation record with the six open items. Corrected: section 6.1's SSRF and OX-M8 entries, section 6.2's F-04 POSIX caveat (closed by measurement in section 8), section 6.3's OX-M8 bullet and its "these three compose" conclusion (two of the three are now closed), and section 7.2's target list, which read as open work after all four were taken. One thing deliberately not decided. The unvalidated upstream base URL sits in 6.3 as a library hazard while F-02 was filed as a Medium finding and fixed, on identical reachability terms - and the URL case is the more severe, delivering a live provider credential to a host the embedder names. Re-verified live at proxy.ts:250-253. That asymmetry is now stated plainly in three places rather than resolved, because which way it goes is a product decision. Docs only; 887 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ew into the docs Closes the last substantive security item, and puts the review where later sessions will actually find it. ## SSRF (security review 6.3) buildUpstreamUrl trimmed a trailing slash and concatenated, with no validation of any kind, while buildForwardHeaders delivers the caller's Authorization and x-api-key to whatever came out. R-06 demonstrated a one-line config change putting a live-looking bearer token on an arbitrary listener; 169.254.169.254 is what makes that worth doing. start() now refuses a base URL that is not https: or that names a literal private, loopback or link-local address. allowInsecureUpstream is the named opt-in, the same shape as OX-M8's allowUnauthenticatedNonLoopback, and its honest use is a local test stub - this repo's own gateway tests are exactly that, which is why the escape hatch exists rather than the rule being weakened to let them pass. Placement at start() follows M8: a misconfiguration that would leak a credential is named once where it can still be fixed, not surfaced as an opaque 502 per call. Two limits stated rather than glossed. Literal addresses only - a hostname resolving into a private range is not caught, since this reads the configured string and not the address the socket connects to, and resolving at startup would still leave a TOCTOU window. And the guard is on GatewayServer.start(), so a direct handleProxyRequest caller bypasses it. One bypass in the first draft was caught by its own test, not by reading: URL normalises [::ffff:169.254.169.254] to [::ffff:a9fe:a9fe], so a check understanding only dotted IPv4-mapped notation passes the metadata service itself. Both spellings handled. This was never filed in section 3 - it sat in 6.3 as a library hazard because no CLI surface reaches it. That reasoning was inconsistent: F-02 was library-only on identical terms, filed as a Medium, and fixed. This closes the asymmetry from the other side. ## F-01's residual: a fix investigated and refused Section 9.1 listed "session state mutates before any credential check" as fixable. Measured before writing it, hoisting the check stops nothing: hasAuthHeaders tests presence, not validity, so a request with no header and a request carrying "Bearer literally-anything" both create the session. The gate would read as a credential check in the source and the changelog while being a formality an attacker steps around by typing a word. Recorded in section 3.1 with the measurement. A control that only stops an attacker who declines to bypass it is the vacuity the protocol exists to prevent, and shipping one is worse than the honest gap. ## Documentation integration CLAUDE.md had zero mentions of the security review - the file every session reads first had no record that four sessions ran, found eight findings and fixed seven. It now carries the parts that change how the rest of that file is read, including that driftScore 0 / measured true / fallbackUsed false is not a semantic-safety attestation. docs/audit-remediation-status.md, which CLAUDE.md designates as the current index, also had zero mentions, and its own heading said oxaudit.md "is open" while line 7 said section 70 closes it in full. Heading corrected, both later audits added with their remaining items named. 905 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.
Closes the last substantive item from the security review, and puts the review where later sessions will actually find it.
Stacked on #54 (
docs/security-review-reconciliation), so it includes that commit. Merge #54 first, or merge this one and #54 becomes empty.SSRF — §6.3's unvalidated upstream base URL
buildUpstreamUrltrimmed a trailing slash and concatenated, with no validation of any kind, whilebuildForwardHeadersdelivers the caller'sAuthorization/x-api-keyto whatever came out. R-06 demonstrated a one-line config change putting a live-looking bearer token on an arbitrary listener;169.254.169.254is what makes that worth doing.start()now refuses a base URL that is nothttps:or that names a literal private, loopback or link-local address.allowInsecureUpstreamis the named opt-in — the same shape as OX-M8'sallowUnauthenticatedNonLoopback— and its honest use is a local test stub. This repo's own gateway tests are exactly that case, which is why the escape hatch exists rather than the rule being quietly weakened to let them pass.Placement at
start()follows M8's reasoning: a misconfiguration that would leak a credential is named once, where it can still be fixed, rather than surfacing as an opaque 502 on every call.Two limits, stated rather than glossed
GatewayServer.start(), so a library caller invokinghandleProxyRequestdirectly bypasses it.One bypass caught by its own test
URLnormalises[::ffff:169.254.169.254]to[::ffff:a9fe:a9fe]. A validator understanding only dotted IPv4-mapped notation passes the metadata service itself. Both spellings are handled now — found by the test, not by reading.Why this was never in §3
It sat in §6.3 as a "library hazard, no shipping caller". That reasoning was inconsistent: F-02 was library-only on identical terms, was filed as a Medium, and was fixed in #53. Fixing this closes the asymmetry from the other side, which is one of the two dispositions §8.2 asked for.
F-01's residual — a fix investigated and refused
§9.1 listed "session state mutates before any credential check" as fixable. Measured before writing it, hoisting the check stops nothing:
hasAuthHeaderstests presence, not validity, so an attacker passes it withBearer literally-anything. The gate would read as a credential check in the source and in the changelog while being a formality an adversary steps around by typing a word.Recorded in §3.1 with the measurement instead. A control that only stops an attacker who declines to bypass it is the vacuity the protocol exists to prevent, and shipping one is worse than the honest gap, because the next reader stops looking.
Documentation integration
Two gaps found by sweeping all four audit documents:
CLAUDE.mdhad zero mentions of the security review. The file every session reads first had no record that four sessions ran, found eight findings and fixed seven. It now carries the parts that change how the rest of that file is read — including thatdriftScore: 0, measured: true, fallbackUsed: falseis not a semantic-safety attestation.docs/audit-remediation-status.mdalso had zero mentions, despite CLAUDE.md designating it the current index — and its own heading saidoxaudit.md"is open" while line 7 said §70 closes it in full. Heading corrected; both later audits added with their remaining items named.Checks
905 tests pass (2 skipped, POSIX-only), typecheck and lint clean. The new guard has 16 cases including every private range, both IPv4-mapped spellings, and negative cases (
https://api.openai.comand public8.8.8.8must still be allowed — a rule rejecting every literal IP would be easy to write and wrong).🤖 Generated with Claude Code