Close the two residual MJ-001 reflection paths in the hosted doctor - #4884
Conversation
…r attempt MJ-001 left two hosted doctor fields still reflecting a socket outcome. `probe.oauth.discoveryError` was not in the redaction's field list. It reports the RFC 9728 metadata fetch, whose host comes from the `resource_metadata` parameter of the target's own `WWW-Authenticate` challenge — a second origin, chosen separately from the server URL — so the DNS, socket and TLS text for that host reached the caller verbatim. `bench-probe-child` copies the same string onto a user-visible check detail. The gate was also whole-envelope: one attempt with a response returned early and rewrote nothing, so a target whose first transport answered and whose second was refused at the socket leaked the second attempt's message. Attempts are now judged one at a time on their own `response`. The envelope-level summaries name no attempt, so they pass through only when every recorded attempt received a response, and a run with no attempts at all is redacted.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. WalkthroughHosted-mode doctor redaction now covers Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to Hosted doctor diagnostics retain answered-attempt details while redacting failed transport and discovery paths in hosted mode. No current merge-blocking risk was identified. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Internal previewPreview URL: https://mcp-inspector-pr-4884.up.railway.app |
The redaction rewrote `error.message` and left `error.code` beside it. `normalizeServerDoctorError` derives that code from the raw message by substring, so a refused connect stayed `SERVER_UNREACHABLE`, an open cleartext port's TLS record error stayed `INTERNAL_ERROR` and a filtered port stayed `TIMEOUT` — the same three-way split the message had just stopped making. The code was not even on the redactor's envelope type. It is now, and it collapses whenever the message it was derived from is replaced. The early exit read only the probe's attempt array. The doctor's connect leg runs after the probe, records no attempt, and writes its raw transport error onto `connection.detail`, `checks.connection.detail` and `error`, so a target that answered the probe cleanly and then redirected the connect elsewhere had that socket outcome reflected verbatim. The exit now also requires that the connect leg did not fail. The docblock claimed an all-answered run was "the one state in which no socket text existed to be summarised"; it was not, and the test that pinned the gap as intended behaviour is replaced by one that pins the connect leg's two outcomes as indistinguishable. `attempts[].durationMs` was the same oracle with a stopwatch — about a millisecond for a refused port, the whole timeout for a filtered one — and shipped unredacted. A redacted attempt now reports 0, the value the probe already writes for an attempt it never dialled; an attempt that received a response keeps its real latency, which is a diagnostic rather than a measurement of somebody's firewall. The fixtures were the reason `JSON.stringify(closed) === JSON.stringify(open)` passed: they hand-wrote three keys per attempt and a hardcoded `error.code`, so every field that still carried the differential was absent from the test. Both are rebuilt on a real `ProbeHttpAttempt` with the error derived through `normalizeServerDoctorError`, and the two envelopes now differ in duration before redaction.
`buildInitializeRequest` puts the stored access token on the attempt's `request.headers`, and that same object is what the caller receives inside `transport.attempts[]`. The MCPJam#4884 redactor rewrites `error.message`, `connection.detail`, `checks[].detail`, `error.code` and `durationMs`; it never touched `request.headers`. So a token that never had to leave the server arrived in a JSON response body, and from there in browser memory, HAR exports and any support bundle someone pastes it into. The attempt is both the request to send and the record of it. `performRequest` now splits the two at the one function that dials: the real headers become a local, and what stays on the attempt has its credential VALUES replaced. The names stay — "this attempt carried an Authorization header" is the whole diagnostic value of the record, and the bearer string beside it carries none. `probeMcpServer` sweeps once more at its single exit, for attempts recorded before their request is made. The predicate is `isSecretKeyName`, exported from `conformance-redaction` rather than written again here, for the reason that module already gives for having one list: a second list is a second thing to forget. So `x-api-key` and a vendor-prefixed `x-vendor-access-token` are covered too, not just `Authorization`. Fixes MCPJam#5001
This branch was successfully deployed
2 participants
#4783 pinned the hosted transport so a private target cannot be reached, and added
redactHostedDoctorTransportDetailso the failed attempt cannot describe what it found. Several paths through that redaction still carried the open-versus-closed differential the finding calls Scenario B. This closes them. It does not close MJ-001: that needs a deployed environment and a pentester retest.What leaked
probe.oauth.discoveryError. The field was not in the redaction's list, so in hosted mode it reached the caller verbatim. It reports the RFC 9728 protected-resource-metadata fetch, and the host that fetch dials comes from theresource_metadataparameter of the target's ownWWW-Authenticatechallenge — a second origin, chosen separately from the server URL. Soconnect ECONNREFUSED 10.0.0.5:6379against a closed port and a TLS record error against an open one were still distinguishable, for an address the attacker selects on the second hop.server/services/bench-probe-child.ts:215copies the same string onto a user-visible check detail, so it surfaces in the checklist as well as the raw envelope.Sibling attempts on a mixed run. The gate was
attempts.some((attempt) => attempt?.response !== undefined)followed by an early return. One attempt with a response meant nothing in the envelope was rewritten, including attempts that never got past the socket. A target whose first transport answers and whose second is refused against a different port or host leaked the second attempt's message.error.code, beside the message that was already being rewritten.normalizeServerDoctorErrorderives the code from the raw message by substring: a refused connect matcheseconnand becomesSERVER_UNREACHABLE, an open cleartext port's TLS record error matches nothing and becomesINTERNAL_ERROR, a filtered port times out and becomesTIMEOUT. Rewriting onlyerror.messageleft the same three-way split one key over — and the code was not even modelled on the redactor's envelope type.The connect leg, exempted by an attempts-only test. The early exit read the probe's attempt array. The doctor's connect leg (
sdk/src/http-server-doctor.ts:165) runs after the probe, records no attempt, and writes its raw transport error ontoconnection.detail,checks.connection.detailanderror. So a target that answers the probe cleanly and then redirects the connect elsewhere had that socket outcome reflected verbatim. The docblock called an all-answered run "the one state in which no socket text existed to be summarised", which was false, and the test athosted-doctor-redaction.test.ts:140pinned the gap as intended behaviour.attempts[].durationMs. The same oracle with a stopwatch.sdk/src/server-probe.ts:430and:448record it, it ships intransport.attempts[], and it was never redacted: a refused port returns in about a millisecond, a filtered one burns the whole timeout.Why each survived #4783
The redaction was written against the four fields
normalizeServerDoctorErrorpopulates plusattempts[].error. OAuth discovery writes its failure somewhere else, and the existing regression test asserted only thatoauth.discoveryErrorwas defined after a refused private-address metadata fetch, never what it contained — so the field was exercised and still unexamined.The docstring's reasoning was already per attempt ("if no probe attempt received a response, nothing HTTP-level happened"); the implementation applied it once to the whole envelope.
error.codeanddurationMssurvived because the fixtures hid them.hosted-doctor-redaction.test.tshand-wrote three keys per attempt and a hardcodederror.code, and omitteddurationMs,request.method,request.headers,response.statusTextand the attemptnamethat a realProbeHttpAttemptcarries. That absence, not the redaction, is what madeexpect(JSON.stringify(closed)).toBe(JSON.stringify(open))pass.The fix
Attempts are judged one at a time on their own
response. An attempt that received one reached a host answering as a public server and keeps its diagnostic; an attempt that did not is replaced with the uniform message.The envelope-level fields —
probe.error,connection.detail,checks[].detail,error, andoauth.discoveryError— are summaries that name no attempt, so they cannot be resolved per attempt on a mixed run and may be quoting the refused hop. They pass through only when every recorded attempt received a response and the connect leg did not fail, and a run that recorded no attempt at all is redacted too. That is strictly more redaction than before, never less.error.codecollapses to a single value whenever the message it was derived from is replaced, so the three outcomes are one outcome after redaction.durationMs: collapsed, not bucketed. A redacted attempt reports0— the valueserver-probe.tsalready writes for an attempt it never dialled — and an attempt that received a response keeps its real latency untouched. Quantising or bucketing was the wrong trade here: any bucket wide enough to hide a 1 ms refusal from a 10 s timeout has thrown the number away anyway, and a coarse bucket still separates the two outcomes at the boundary that matters. Collapsing costs nothing real, because the timing of an attempt that never got past the socket is not a diagnostic about the user's server — it is a stopwatch on whoever's firewall was in the way. The answered case, which is the only one where latency tells the user something, is unchanged.isEgressRefusalDetailis unchanged and still applies to the new fields, so the guard's own verdict keeps its wording — it names the hostname the target chose and never the address it resolved to. TheHOSTED_MODEshort-circuit is unchanged: locally the socket error is the answer.What the tests prove
Both fixtures in
hosted-doctor-redaction.test.tsare rebuilt on a realProbeHttpAttempt, with the error derived throughnormalizeServerDoctorError(new Error(socketError))rather than hand-written, and with the caller supplying the attempt duration. The closed-port and open-port envelopes now genuinely differ before redaction — in the message, the derived code, and the duration — so the indistinguishability assertion is a test of the redaction instead of a test of the fixture. Each of the three source fixes was reverted individually and the suite watched go red, then restored.ECONNREFUSED,tls_get_more_records,127.0.0.1or6379. Same for the OAuth-discovery pair.SERVER_UNREACHABLE,INTERNAL_ERRORandTIMEOUTbefore redaction and a single code after it.durationMsthrough.In
hosted-doctor-probe-egress.test.ts, the two hosted refusal cases assert whatdiscoveryErroractually says — the guard's verdict, with no resolved address in it — instead of only that it exists. Both drive the real probe through the real guard.What this does not cover
Named so the next reader does not mistake the scope. None of these is addressed here:
attempts[].request.headers(sdk/src/server-probe.ts:501).Authorizationis stripped before dialing target-named metadata hosts, so a storedX-Api-Keyegresses (sdk/src/server-probe.ts:563).bench-probe-childpath copies redacted-envelope-adjacent strings onto check details on its own route; it is not run through this redactor.isEgressRefusalDetailis still a prose allowlist — a reworded refusal degrades to the uniform message rather than leaking, but the coupling to two message spellings remains.And, again: this PR does not close MJ-001. Closure needs the finding ID, owner, SLA mapping, the fix deployed to a named environment at a named version on a named date, an approver, and a pentester retest. None of that is claimed here.
Verification
Run from the repo root on the merge with
main(git merge origin/mainreported "Already up to date"), with a realrgon PATH so the! rgguards are not vacuous:npx tsc --noEmit -p mcpjam-inspector/server/tsconfig.json— 294 errors, all pre-existing; the same command on this branch with the diff reverted gives the identical 294, and none of them are in the two files touched here. (Nothing in CI typechecksserver/, and the count depends on whether the generated bundles underserver/services/**have been built: without them it is 301, seven unresolved-module errors higher, on both trees.)npm run docs:check-tokens— exit 0npm run typecheck— exit 0npm run typecheck:client -w @mcpjam/inspector— exit 0npm run test:checks— exit 0mcpjam-inspector/server/utils/__tests__/hosted-doctor-redaction.test.ts—Test Files 1 passed,Tests 11 passed. With fix 1 (error.code) reverted: 4 failed. With fix 2 (connect leg) reverted: 1 failed. With fix 3 (durationMs) reverted: 3 failed.hosted-doctor-probe-egress.test.ts—3 passed;servers-doctor-egress.test.tsandhosted-mcp-base-fetch.test.ts—14 passed.npm run test -w @mcpjam/inspector—Test Files 26 failed | 2077 passed | 11 skipped (2114). Every failing file is the Windows-local set (utils/harness/local/**,services/browserd/**,services/plugins/**, playwright integration spikes); none of them imports the redactor, whose only other consumer isserver/routes/web/servers.ts. CI is green on this branch (all sixInspector Tests N/6shards) and onmain.npm run test:ci:rest— the parallel run kills Node on this machine (Assertion failed: ncrypto::CSPRNG(nullptr, 0)during process init, plusERR_IPC_CHANNEL_CLOSEDin the vitest workers), so it was not usable.@mcpjam/sdkrun alone:Test Files 1 failed | 338 passed (339), the one failure anEBUSY: resource busy or locked, rmdiron a Windows temp directory. This diff touches no workspace outside the inspector.npm run build:inspector— exit 0Summary by cubic
Closes the residual MJ-001 reflection paths in the hosted doctor:
oauth.discoveryErrorwas never redacted, and the envelope gate let refused attempts' socket text ride out on a sibling attempt's response.oauth.discoveryErrornow goes through the same redaction; it reports the RFC 9728 metadata fetch, whose host comes from the target's ownWWW-Authenticatechallenge, so it was a second open-versus-closed oracle.response; envelope-level fields pass only when every recorded attempt answered and the connect leg did not fail.error.codeandattempts[].durationMscarried the same differential and are redacted with the message they belong to.Written for commit f7324b4. Summary will update on new commits.