fix: the API key leak was still live one branch over - #215
Merged
Conversation
An independent acceptance pass on the 0.10.0 candidate returned BLOCK. #210 scrubbed the two `HTTP <status>: <message>` paths in the endpoint dispatcher and its changelog entry declared the class closed. The two `describeUnusableBody` paths one branch over — a 200 carrying a body this dispatcher cannot read, which is how a gateway's HTML error page arrives — went on quoting up to 300 raw characters of that body straight back to the caller and into logs/dispatches.jsonl. The pass reproduced it against the built artifact on both the buffered and streaming paths. Separately, scrubEndpointSecrets only ever removed credential-bearing parts of the base URL. The configured api key is sent as an Authorization or x-api-key header by every endpoint in this project's own config and is not in the URL at all, so an endpoint echoing the request header leaked it on every path, including the two #210 did fix. - Every branch that returns endpoint-authored text now goes through one `#safe()` method, rather than a scrub call remembered per site — per-site is how this survived the first fix. - `scrubEndpointSecrets` takes the api key and removes it by value. - Four tests, one per leaking path. Sabotage: reverting the unusable-body scrubbing fails exactly its two; reverting the api-key line fails exactly the other two. Also from the same pass: - README and the supervisor docblock still described `max_concurrent_runs: 0` as bypassing the pool and giving every job a ~76 MB runner process. #206 removed that; a user was being steered away from the setting for a reason that no longer existed. - `spawnDetachedRunner` lost its last caller in #206 and was dead. The unused-symbol gate added in #194 is noUnusedLocals, which does not see unused exports. - doctor's routes line had no sentence separator before the installed-but-unconfigured note. npm run check: 1223 passed, 8 skipped, exit 0. Verified at the built artifact: doctor's line reads correctly and dist/status.js scrubs both credentials.
fstubner
added a commit
that referenced
this pull request
Sep 3, 2026
…#216) A verification pass on #215 returned NOT CONFIRMED. That commit asserted "every branch that returns endpoint-authored text now goes through one #safe()". A fifth branch did not: error: streamError ?? this.#safe(describeUnusableBody(rawSeen)) The scrub sat on the FALLBACK. `streamError` holds the endpoint's own message, parsed straight out of an SSE `error` event by both wire-protocol parsers, and it reached DispatchResult.error — and logs/dispatches.jsonl — verbatim. It is also the likeliest shape of all of them: a 200 with text/event-stream carrying an error object is how OpenAI-compatible gateways and Anthropic both report an auth failure once the stream is open, and jobs only ever stream. Reproduced against the built artifact with both a URL-embedded key and one echoed from the Authorization header. That is the second time a commit message about this file claimed a wider fix than the code delivered, so the #safe docblock no longer asserts completeness — it enumerates the branches, and the enumeration is checked. Also: describeFetchFailure has always scrubbed the base URL but was never given the api key, leaving the last unscrubbed-for-key site in the file. Sabotage, each separately: moving #safe back onto the fallback fails exactly the two mid-stream tests; dropping the key argument fails exactly the fetch-failure test. Verified at the built artifact, not the diff — all seven error branches driven with a stubbed fetch, no leak on any. npm run check: 1226 passed, 8 skipped, exit 0.
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.
An independent acceptance pass on the 0.10.0 candidate returned BLOCK. This clears all five of its findings.
F1 — HIGH / security: the leak #210 declared closed
#210 scrubbed the two
HTTP <status>: <message>paths and its changelog entry stated the class as fixed. The twodescribeUnusableBodypaths one branch over — a 200 carrying a body the dispatcher cannot read, which is how a gateway's HTML error page arrives — went on quoting up to 300 raw characters of that body back to the caller and intologs/dispatches.jsonl. Reproduced by the pass against the built artifact, buffered and streaming.F2 — MEDIUM: the configured key was never scrubbed at all
scrubEndpointSecretsremoved credential-bearing parts of the base URL. Every endpoint in this project's own config authenticates with anAuthorization: Bearerorx-api-keyheader, so the key was not in the URL and survived on every path — including the two #210 fixed.The shape of the fix
Every branch returning endpoint-authored text now goes through one
#safe()method. Scrubbing remembered per call site is precisely how this survived its first fix; there is now one place to get it right and adding a branch that skips it is the thing the method exists to make obvious.Sabotage
describeUnusableBodyscrubsscrubEndpointSecretsAlso from the pass
max_concurrent_runs: 0as bypassing the pool for a ~76 MB-per-job runner. fix: max_concurrent_runs: 0 lifts the cap without removing the pool #206 removed that, so users were being steered off the setting for a reason that no longer existed.spawnDetachedRunnerlost its last caller in fix: max_concurrent_runs: 0 lifts the cap without removing the pool #206. The unused-symbol gate from chore: remove 38 unused symbols and gate against more #194 isnoUnusedLocals, which does not see unused exports.Verification
npm run check: 1223 passed, 8 skipped, exit 0. Checked at the built artifact rather than the diff:node dist/bin.js doctorrenders the line correctly, anddist/status.jsscrubs both credentials.