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 (