Repository navigation
Conversation
Groundwork for validating what this server advertises to MCP clients.
Checking the whole tool catalog from a test means the spawned server has
to register exactly the tools it would in production, and today every
test that starts one re-implements the spawn and spreads the whole of
process.env into the child. A stray TOOLS or GROUPS in a developer's
shell therefore silently shrinks the catalog under the test.
test/helpers/spawn_server.js owns that contract instead:
- an explicit inherit list. The MCP SDK already supplies HOME, PATH,
SHELL, TERM and USER, so this carries the network plumbing a
developer behind a proxy depends on, and nothing else.
- network: 'none', which points the child at a refused proxy for tests
that must never reach the real API.
- captured, drained stderr, so a test can assert on what the server
reported. Draining is not optional: a piped stderr nobody reads
blocks the child once the pipe buffer fills, and this server logs
every tool call.
- a deployed-server variant that sends the token in an Authorization
header rather than in a URL, and scrubs the exact value from
anything it throws, so a transport error cannot put a credential in
a terminal or a CI log.
The npm script now names test/*.test.js. Bare `node --test` treats every
.js file under test/ as a test, which would run this helper as an empty
passing test. The pattern stays single-level deliberately: under sh
without globstar, test/**/*.test.js matches only files below test/ and
would silently drop the entire suite.
… a failed batch entry
Builds on 2.11.2, which stopped scrape_batch from serialising a raw axios
rejection -- the shape that carried the API token into a tool result -- and
routed every tool error through one sanitizer. Two things that change left
open, both found while auditing what the tools return for a plugin review in
which a credential in a response is a rejection criterion.
The browser tools carried the zone password. The CDP endpoint embeds it
(wss://brd-customer-...-zone-...:PASSWORD@...), Playwright puts the endpoint
it could not reach into its error text, and fourteen catch sites
interpolated that text straight into the message the model sees. None of
those sites pass through tool_fn, so the 2.11.2 sanitizer never saw them.
Every browser catch now goes through one builder that redacts credentials
in ws:// and wss:// URLs and bounds the length, and the same redaction is
applied where the session logs the endpoint.
A rejected scrape_batch entry names its url. With several urls in flight, a
reason without the url it belongs to is not something a model can act on.
The fulfilled shape is untouched; a rejected entry is {status, url, reason},
the shape search_engine_batch has always used, and the 2.11.2 test now
asserts the url is there.
Tests: a unit test for the browser error builder, and an end-to-end check
that a real failing call returns no bearer credential, no authorization
header and no credentialed endpoint, with a positive control proving the
detector fires on the exact shape that leaked.
…dacted
2.11.2 made every tool error safe by handing the model the HTTP client's
generic message -- "Request failed with status code 400" -- whenever the
response body was JSON, while still forwarding a text body verbatim up to
4096 characters. That treats the same sentence two ways depending on its
Content-Type: 'zone "x" not found' reaches the model when the API sends it
as text/html and is dropped when it arrives as {"error": ...}. The dropped
case is the one that matters. A validation error naming the field and the
allowed values is the only thing a model has to correct itself with, and
without it the likely next move is a blind retry, which for the collection
tools starts another billed job.
One rule now applies to every body, about its content rather than its type:
a short sentence is kept; a JSON envelope is unwrapped to the sentence
inside it (validation_errors first, then error_description, message, error,
detail, reason); an HTML page or an oversized blob is replaced by the status
line. The result is bounded to 300 characters, and the token redaction from
2.11.2 stays as the backstop on every path. base_request attaches the
summary where it sees the response; tool_fn fills it in for the nine
request sites that bypass base_request; safe_error prefers it when present,
so scrape_batch's rejected entries carry it too. The free-tier usage-limit
instructions are untouched: that branch still reads the x-brd-err-code
header and throws its own message.
Verified against the live API on the shapes it actually returns: text/html
sentences, JSON validation_errors arrays, JSON error/error_description
envelopes, and JSON delivered as a text string by the tools that request
responseType: 'text'.
Two assertions in the 2.11.2 tests encoded the drop-the-body behaviour and
are updated to assert the invariant that matters instead: the reason is
surfaced, bounded, and free of the token, headers, config and raw objects.
Collaborator
|
Closing as superseded. The browser credential redaction landed in #188/#189, and the remaining error paths were redone on current main in bdee517 (branch |
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.
Why
The plugin directory review rejects a server whose tool results contain secrets or debugging output. 2.11.2 (#183) closed the worst case, a rejected
scrape_batchentry that serialised a raw axios error and so carried theAuthorizationheader and the API token into the tool result, and routed every tool error through a sanitizer. Two things it left open, both found while auditing what all 74 tools return, and one thing it does that makes errors less useful than a model needs.What changes
Three commits. The first two are independent of the third.
Shared test helper. Identical to the first commit of fix: complete and correct every tool's MCP annotations for plugin review #184; drops on rebase once that lands.
Zone credentials out of browser errors, and the url in a failed batch entry. The browser tools carry the zone password: the CDP endpoint embeds it (
wss://brd-customer-…-zone-…:PASSWORD@…), Playwright puts the endpoint it could not reach into its error text, and fourteen catch sites interpolated that text straight into the message the model sees. None of those sites pass throughtool_fn, so the 2.11.2 sanitizer never sees them. Every browser catch now goes through one builder that redacts credentials inws://andwss://URLs and bounds the length. This extends fix: redact the zone password from browser connection errors #170, which fixed one layer, and supersedes it. Separately, a rejectedscrape_batchentry now names its url,{status, url, reason}, the shapesearch_engine_batchhas always used, because with several urls in flight a reason without its url is not actionable.Surface the upstream reason, bounded and redacted. 2.11.2 hands the model the HTTP client's generic
Request failed with status code 400whenever the response body is JSON, while still forwarding a text body verbatim up to 4096 characters. That treats the same sentence two ways by Content-Type, and drops the case that matters: a validation error naming the field and the allowed values is the only thing a model has to correct itself with, and without it the likely next move is a blind retry, which for the collection tools starts another billed job.upstream_error.jsapplies one rule to every body, about its content rather than its type: a short sentence is kept; a JSON envelope is unwrapped to the sentence inside it (validation_errorsfirst, thenerror_description,message,error,detail,reason); an HTML page or an oversized blob is replaced by the status line. Everything is bounded to 300 characters and the 2.11.2 token redaction stays as the backstop on every path. Against the live API,search_datasetwith a bad operator now returnsHTTP 400: "filter.operator" must be one of [=, !=, <, …]instead of the status code alone.This commit changes two assertions in the 2.11.2 tests (
tool-error-boundaryandscrape-batch-offline), which encoded "the raw response body must never reach the tool output". They now assert the invariant that matters: the reason is surfaced, bounded, and free of the token, headers, config and raw objects. If the 2.11.2 policy is preferred, the first two commits stand alone; the branch passes its suite at that point (30 tests) as well as at the head (43).Verification
npm testat head: 43 pass, including the 2.11.2 offline tests. The end-to-end hygiene test fires a real failing call and asserts no bearer credential, no authorization header and no credentialed endpoint in the result, with a positive control proving the detector matches the exact shape that leaked. The free-tier usage-limit instructions are untouched; that branch readsx-brd-err-codeand throws its own message.Not verified
The hosted server is a separate build. On a probe with a failing batch call it did not leak the token, so the batch fix in 2.11.2 and here concerns the self-hosted path. The browser-side leak has not been probed on the hosted server.