feat(core): redact sensitive fields in fetch-log bodies + URL query strings

[cliffhall]

Closes #1593

Follow-up to the header-redaction slice (#1585). core/mcp/fetchTracking.ts records url, headers, requestBody, and responseBody per request; before this change only headers was redacted, so the secrets that ride in OAuth token exchanges still leaked verbatim through the request/response body and the URL query string. This extends redaction to cover both, alongside the existing header logic (which is untouched).

What is redacted

Known-sensitive field / param names (case-insensitive), reusing the [REDACTED] sentinel style:
client_secret, code, refresh_token, access_token, id_token, code_verifier, client_assertion, assertion, password, token.

How

• redactBody(body, contentType) — masks values in application/x-www-form-urlencoded bodies (re-serialized via URLSearchParams) and JSON bodies (recursive walk over nested objects/arrays, values-only, structure preserved). JSON is also sniffed when the content-type is missing. Best-effort: empty, non-string, scalar-JSON, and unparseable bodies are returned unchanged — it never throws.
• redactUrlQuery(url) — masks sensitive query params while keeping the path, non-sensitive params, and any trailing #fragment readable; repeated sensitive params collapse to one redacted value. Returns the URL unchanged when there is nothing to redact.
• Applied to the request success entry, the request error-path entry, and the async response-body callback, all before the entry reaches any sink (in-memory log, pino, session storage).
• Live outbound requests are byte-identical — only the logged copy is redacted (verified by asserting the outbound init.body is unchanged).

Tests

Added coverage for form + JSON body redaction (request and async response), URL-query redaction on both success and error paths, non-sensitive passthrough, malformed-body no-throw, nested-object/array recursion, fragment/case-insensitive/repeated-param handling, and live-object untouched. Unit tests for redactBody / redactUrlQuery directly.

clients/web per-file coverage for fetchTracking.ts: 98.27% stmts / 98.55% branch / 100% funcs / 98.18% lines (gate is 90 on all four). Web validate, full test:coverage gate, and test:integration all pass.

🤖 Generated with Claude Code

https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 49s —— View job

---

Review: body + URL-query redaction

Read the implementation (core/mcp/fetchTracking.ts), the new tests, and traced how the tracker is wired in (inspectorClient.ts + node/transport.ts). Overall this is a clean, well-tested extension of the header-redaction slice — the changed/best-effort design is careful, the "live request is byte-identical" guarantee is real and directly tested, and coverage is thorough. No blocking bugs. A few things worth discussing before merge.

---

Discussion — the tracker wraps all MCP traffic, not just OAuth

createFetchTracker wraps the transport fetch, so redactBody runs over every MCP JSON-RPC request/response body, not only token-endpoint calls. Combined with the generic field names in SENSITIVE_BODY_FIELDS — code, token, password, assertion — this can redact legitimate payload data the user opened the inspector to see:

• a tool result whose JSON contains a code field (status/error code, a code-search hit, a snippet);
• a tools/call where the tested tool legitimately takes a password or token argument;
• any MCP body with a top-level or nested key literally named token.

Mitigating factor: matching is exact key match (SENSITIVE_BODY_FIELDS.has(name.toLowerCase())), not substring — so grant_type=authorization_code and a field named authorization_code are untouched. That's a good call and meaningfully narrows the blast radius. But keys named exactly code/token/password still occur in normal MCP payloads.

Worth confirming this is the intended trade-off (#1593). Options if over-redaction becomes a problem: scope body redaction to OAuth/token endpoints (by URL/host), or drop the most generic names (code, token) from the body list while keeping them for the header/URL paths. Not a blocker — flagging so it's a conscious decision.

Minor notes (non-blocking)

1. redactBody JSON branch runs on every non-form body regardless of content-type — any text/plain/untyped body that happens to parse as a JSON object gets a JSON.parse + redactJsonValue + JSON.stringify round-trip. For large tool-result bodies this is extra CPU (on top of the .text() read) and re-serialization drops the original formatting/whitespace in the logged copy. Fine for correctness; just noting the cost on the hot path.

2. Form re-serialization re-encodes non-sensitive fields. When a sensitive field is present, params.toString() rebuilds the whole query, so e.g. a +/%20/%2B in a non-sensitive value may render differently in the logged copy. Cosmetic (logged copy only), but the recorded body is no longer byte-identical to what was sent for the non-sensitive parts.

3. multipart/form-data bodies are not redacted — only x-www-form-urlencoded and JSON. OAuth doesn't use multipart so risk is low, but a secret in a multipart body would pass through verbatim. Probably fine to leave; worth a one-line comment noting the deliberate scope.

Nits

• REDACTED_HEADER_VALUE and REDACTED_VALUE are now two exported constants with the identical "[REDACTED]" value. Intentional (independent evolution), but a reader may wonder why both exist — a one-line comment on REDACTED_VALUE cross-referencing the header constant would help.

Tests

Coverage is strong: form + JSON (request and async response), URL redaction on success and error paths, case-insensitivity, fragment preservation, repeated-param collapse, nested object/array recursion, JSON-scalar and malformed no-throw passthrough, and the live-body-untouched assertion. Nothing obvious missing. One small addition worth considering: a case asserting a nested sensitive key in a request body is redacted (the existing nested test exercises redactBody directly, not the request path through the tracker) — low priority given the direct unit test.

---

Nice work — the security intent is met and the best-effort/no-throw discipline is exactly right for a logging path. My only substantive ask is confirming the broad-redaction trade-off in note above is intended.

[cliffhall]

Thanks for the thorough review. Responses to each point (pushed in 775b98a):

Broad-redaction trade-off (substantive ask) — intended, confirming. Wrapping all MCP traffic is deliberate for #1593: the tracker is the single choke point every body flows through before it reaches any sink (in-memory log, pino, persisted session storage), so redacting there gives one uniform guarantee rather than URL/host-sniffing per request. The exact-key-match design (no substring) is exactly what keeps the blast radius acceptable — authorization_code, a code-search result field, etc. are untouched; only a key named literally code/token/password is masked. We accept that a tool legitimately named-arg password/token shows [REDACTED] in the log as the safe default; if over-redaction is reported in practice we'll revisit (scope-by-endpoint or drop the most generic names), but not preemptively.

Minor 1 (JSON round-trip cost on every non-form body) — acknowledged, no change. Correct that a parse+re-stringify runs on any body that parses as a JSON object, and that whitespace in the logged copy isn't preserved. This is on the fire-and-forget async read path (never blocks the transport), and correctness/security win over preserving log-copy formatting. Noting, not changing.

Minor 2 (form re-serialization re-encodes non-sensitive fields) — acknowledged, no change. Only fires when a sensitive field is present, and only affects the logged copy (the live body is provably byte-identical — directly tested). Cosmetic; leaving as-is.

Minor 3 (multipart not redacted) — done. Added a JSDoc note on redactBody documenting that form-encoded + JSON is the deliberate scope and multipart/binary passes through (OAuth never uses multipart; revisit if a multipart-secret path appears).

Nit (two identical [REDACTED] constants) — done. Added a cross-reference comment on REDACTED_VALUE explaining it's kept separate from REDACTED_HEADER_VALUE so the header vs body/URL sentinels can evolve independently.

Tests (nested sensitive key through the request path) — done. Added a test that sends a JSON request body with a nested access_token through the tracker and asserts the recorded copy is masked (siblings kept) while the live outbound body stays byte-identical.

fetchTracking.ts coverage remains ≥90 on all four dims (98.27 / 98.55 / 100 / 98.18). validate + coverage + integration all green.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 0s —— View job

---

Re-review: body + URL-query redaction (after 775b98a0)

Re-read the implementation (core/mcp/fetchTracking.ts) and the full test file against the follow-up commit. All six points from the prior review are resolved, and nothing new is blocking. Good to merge.

Follow-ups from the last review — verified

Item	Status
Broad-redaction trade-off	✅ Confirmed intended; exact-key-match keeps blast radius acceptable
JSON round-trip cost on non-form bodies	✅ Acknowledged (async read path, correctness > log-copy formatting)
Form re-serialization cosmetic re-encode	✅ Acknowledged (logged copy only; live body proven byte-identical)
multipart/form-data scope	✅ JSDoc note added (fetchTracking.ts:133-136)
Two identical [REDACTED] constants	✅ Cross-reference comment added (fetchTracking.ts:44-50)
Nested sensitive key through the request path	✅ Test added (fetchTracking.test.ts:480-507) — asserts nested access_token masked, siblings kept, live body byte-identical

The redactBody JSDoc is now precise about scope, and the new test closes the one coverage gap I flagged (nested redaction through the tracker, not just the direct unit call).

One new minor nit (non-blocking)

redactUrlQuery splits on the first ? (fetchTracking.ts:83). If a URL carries a ? inside its fragment with no real query — e.g. https://x/p#a?b=c — the ? in the fragment is treated as the query start and b would be considered a param. Real OAuth URLs always put the query before the fragment, so this can't occur in the targeted flows; flagging only for completeness. The #-after-? case (the normal ordering) is handled correctly (fetchTracking.ts:88-91).

Also note the same cosmetic re-encode you acknowledged for form bodies applies to redactUrlQuery when it hits params.toString() (non-sensitive params in the logged URL may re-encode) — logged copy only, live input untouched. Purely cosmetic; no action needed.

Tests

Comprehensive and correct: form + JSON (request and async response), URL redaction on success and error paths, case-insensitivity, fragment preservation, repeated-param collapse, nested object/array recursion, JSON-scalar and malformed no-throw passthrough, non-sensitive passthrough, and the live-body-byte-identical assertions on both the form and nested-JSON paths. No gaps I can see.

---

The security intent is met, the best-effort/no-throw discipline is right for a logging path, and the "live request is byte-identical, only the logged copy is redacted" guarantee is directly tested on every redaction path. LGTM. ✅
• 1593-redact-bodies

[cliffhall]

📋 Smoke test & AGENTS.md audit for this PR: #1593 (comment) (posted on the linked issue #1593).