fix: round four of the same leak, this time outside the dispatcher - #217
Merged
Conversation
Three entries were describing a state the code never reached: the security entry claimed the class closed after the first of three fixes, the real-process entry described the mock-detector branch in the present tense after it had been removed, and the concurrency entry did not mention that the README went on documenting the old behaviour for nine more commits. Every one of these is the same defect the release itself is about — prose asserting an outcome that was not checked.
A repo-wide sweep asked the question the previous three rounds did not: can a configured credential reach ANY user-visible surface, by any route? It found two more. usage(listModels:) reported a probe failure as a redacted URL followed by an UNSCRUBBED err.message. undici embeds the URL it was handed, so a base_url carrying userinfo or ?key= came back in full, sitting next to its own redaction — the exact shape scrubEndpointSecrets was written to prevent. The sink is worse than the dispatcher's: it is an MCP tool result, so it lands directly in an orchestrating agent's context, and callers are told to check usage before trying an unfamiliar route, which is precisely when a route is misconfigured. The route id is caller-supplied, so any agent talking to this server could aim it at any configured endpoint. The round-three enumeration that finally held is a comment in openai-compatible.ts and does not reach this module. Hence a named, shared safeEndpointText() here rather than a scrub call inlined per site. Second, lower: a YAML syntax error carries js-yaml's source snippet, so a parse failure near an api_key line put that key into the message — and the message becomes config.reloadError, then stateWarnings, which status, doctor, the HTTP status route and the harness-dispatch://status MCP resource all read. The value cannot be scrubbed by comparison there, because the file did not parse and there is no configured value to compare against; the key NAME is what is available, so credential-looking keys have their values blanked. js-yaml truncates snippet lines around 50 characters, so a long key was already arriving as a prefix — a short one arrived whole. Sabotage, each separately: neutering safeEndpointText fails exactly the listModels test; dropping redactSecretLines fails exactly the config test. Verified at the built artifact: both compiled call sites apply the scrub, and dist/status.js removes the userinfo password, the query key, the header key and the host from a real undici message. npm run check: 1228 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.
A repo-wide sweep asked the question the previous three rounds did not: can a configured credential reach any user-visible surface, by any route? It found two more.
Finding 1 — HIGH:
usage(listModels:)src/mcp/tools.tsreported a probe failure as a redacted URL followed by an unscrubbederr.message. undici embeds the URL it was handed, so abase_urlcarrying userinfo or?key=came back in full, sitting beside its own redaction — the exact shapescrubEndpointSecretsexists to prevent.The sink is worse than the dispatcher's. This is an MCP tool result, so it lands directly in an orchestrating agent's context, and callers are told to check
usagebefore trying an unfamiliar route — precisely when a route is misconfigured. The route id is caller-supplied, so any agent talking to the server can aim it at any configured endpoint.The round-three enumeration that finally held is a comment in
openai-compatible.tsand does not reach this module. Hence a named, sharedsafeEndpointText()here rather than a scrub call inlined per site.Finding 2 — LOW: YAML errors quote the
api_key:linejs-yaml embeds a source snippet, so a parse failure near a credential put it in the message — and that message becomes
reloadError→stateWarnings, read bystatus,doctor, the HTTP status route, and theharness-dispatch://statusMCP resource.The value can't be scrubbed by comparison there: the file didn't parse, so there is no configured value to compare against. The key name is what's available, so credential-looking keys have their values blanked. js-yaml truncates snippet lines near 50 chars, so a long key already arrived as a prefix — a short one arrived whole.
Sabotage
safeEndpointTextneuteredredactSecretLinesdroppedVerified at the artifact
Both compiled call sites in
dist/mcp/tools.jsapply the scrub, anddist/status.jsremoves the userinfo password, the query key, the header key and the host from a real undici message.npm run check: 1228 passed, 8 skipped, exit 0.Flagged, not fixed
The sweep noted a gap it could not reproduce:
generic-clihas no equivalent scrub, so a child CLI printing its own key to stderr would flow unscrubbed intoresult.errorand the job files. Not confirmed, and reproducing it needs a real harness — recorded here rather than fixed speculatively.