From 85e4a299905438b6c855c9fca02b9c839ceced97 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 14:03:40 -0400 Subject: [PATCH] fix: redact secrets nested inside metadata in `servers/show` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The redaction checked only top-level keys. That was sufficient while values were strings — a secret could not hide beneath one — but #1910 lets a value be an object, so `{ trace: { accessToken: "…" } }` printed in full: `trace` is not sensitive, so the whole subtree went out with it. `servers/show` output is meant to be safe to paste into an issue, so this is a regression this PR introduced rather than a pre-existing gap. `redactMetadataValue` walks the payload: a key matching `isSensitiveHeader` replaces its entire value (so a sensitive key holding an object cannot leak through its members either), anything else is descended into, and arrays are mapped elementwise since objects inside them can carry sensitive keys. Recursion is bounded — the payload came from `JSON.parse`, so it is a finite tree with no cycles. Tests cover a secret two levels under a non-sensitive key and one inside an array element. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall --- clients/cli/__tests__/servers-list.test.ts | 18 ++++++++- clients/cli/src/handlers/servers-list.ts | 46 +++++++++++++++++----- 2 files changed, 54 insertions(+), 10 deletions(-) diff --git a/clients/cli/__tests__/servers-list.test.ts b/clients/cli/__tests__/servers-list.test.ts index 5772ab3e..daa1c444 100644 --- a/clients/cli/__tests__/servers-list.test.ts +++ b/clients/cli/__tests__/servers-list.test.ts @@ -226,6 +226,15 @@ describe("showServerEntry / servers/show", () => { // the whole value, not walk into it (#1910). "X-Api-Key": { primary: "sk-live-1", fallback: "sk-live-2" }, nested: { keep: true }, + // A secret buried under a non-sensitive key. A top-level-only check + // would print `accessToken` in full. + trace: { + id: "t-1", + accessToken: "sk-live-3", + deeper: { refresh_token: "sk-live-4", ok: 1 }, + }, + // Objects inside an array are reached too. + attempts: [{ password: "hunter2" }, { attempt: 2 }], }, env: [ { key: "TOKEN", value: "secret" }, @@ -250,8 +259,15 @@ describe("showServerEntry / servers/show", () => { Authorization: "[redacted]", "X-Custom": "ok", "X-Api-Key": "[redacted]", - // A structured value under a non-sensitive key survives intact. + // A structured value under a non-sensitive key survives intact... nested: { keep: true }, + // ...but a sensitive key nested inside one does not. + trace: { + id: "t-1", + accessToken: "[redacted]", + deeper: { refresh_token: "[redacted]", ok: 1 }, + }, + attempts: [{ password: "[redacted]" }, { attempt: 2 }], }); expect(sanitized.env).toEqual([ { key: "TOKEN", value: "[redacted]" }, diff --git a/clients/cli/src/handlers/servers-list.ts b/clients/cli/src/handlers/servers-list.ts index d0247851..d580f47c 100644 --- a/clients/cli/src/handlers/servers-list.ts +++ b/clients/cli/src/handlers/servers-list.ts @@ -157,15 +157,10 @@ export function sanitizeServerSettings( key: h.key, value: isSensitiveHeader(h.key) ? REDACTED : h.value, })), - // Metadata is a JSON object whose values may be any JSON (#1910), so the - // redaction replaces the whole value rather than a string — a sensitive - // key holding an object must not leak through its members. - metadata: Object.fromEntries( - Object.entries(settings.metadata ?? {}).map(([key, value]) => [ - key, - isSensitiveHeader(key) ? REDACTED : value, - ]), - ), + // Metadata values may be any JSON (#1910), so the redaction has to descend: + // a top-level key that is not itself sensitive can hold a nested one that + // is. See `redactMetadataValue`. + metadata: redactMetadataValue(settings.metadata ?? {}), env: (settings.env ?? []).map((e) => ({ key: e.key, value: REDACTED, @@ -217,6 +212,39 @@ function sanitizeInitRecord( return out; } +/** + * Redact secret-bearing entries anywhere inside a `_meta` payload. + * + * The pre-#1910 version only had to check the top level, because a value was + * always a string — a secret could not hide below one. Now that a value may be + * an object or an array, `{ trace: { accessToken: "…" } }` would sail through a + * top-level-only check: `trace` is not sensitive, so the whole subtree, + * `accessToken` included, would print. `servers/show` output is meant to be + * safe to paste into an issue, so the walk has to reach every key. + * + * A key matching {@link isSensitiveHeader} replaces its **entire** value rather + * than being descended into, so a sensitive key holding an object cannot leak + * through its members either. Arrays are mapped elementwise: their indices are + * not names anyone can mark sensitive, but objects inside them can be. + * + * Recursion is bounded by the payload, which came from `JSON.parse` and so is a + * finite tree with no cycles. + */ +function redactMetadataValue(value: unknown): unknown { + if (Array.isArray(value)) { + return value.map(redactMetadataValue); + } + if (value !== null && typeof value === "object") { + return Object.fromEntries( + Object.entries(value).map(([key, member]) => [ + key, + isSensitiveHeader(key) ? REDACTED : redactMetadataValue(member), + ]), + ); + } + return value; +} + function isSensitiveHeader(key: string): boolean { const k = key.toLowerCase(); return (