From 983aac342d79fb563f1ec270c6ec7b3b8b7e76af Mon Sep 17 00:00:00 2001 From: SarthakB11 Date: Sat, 9 May 2026 18:53:18 +0000 Subject: [PATCH 1/4] fix(server): redact sensitive env vars and headers from connection logs Closes #847 --- server/package.json | 3 +- server/src/index.ts | 10 ++++-- server/src/redact.ts | 48 ++++++++++++++++++++++++++ server/test/redact.test.ts | 69 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 127 insertions(+), 3 deletions(-) create mode 100644 server/src/redact.ts create mode 100644 server/test/redact.test.ts diff --git a/server/package.json b/server/package.json index 5d348912a..3beb97ca7 100644 --- a/server/package.json +++ b/server/package.json @@ -24,7 +24,8 @@ "build": "tsc && shx cp -R static build", "start": "node build/index.js", "dev": "tsx watch --clear-screen=false src/index.ts", - "dev:windows": "tsx watch --clear-screen=false src/index.ts < NUL" + "dev:windows": "tsx watch --clear-screen=false src/index.ts < NUL", + "test": "node --import tsx --test test/*.test.ts" }, "devDependencies": { "@types/cors": "^2.8.19", diff --git a/server/src/index.ts b/server/src/index.ts index bdfe49019..db6c389b5 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -28,6 +28,7 @@ import express from "express"; import rateLimit from "express-rate-limit"; import { findActualExecutable } from "spawn-rx"; import mcpProxy, { type ProxyHeaderHolder } from "./mcpProxy.js"; +import { redactSensitiveEntries, redactQueryForLogging } from "./redact.js"; import { randomUUID, randomBytes, timingSafeEqual } from "node:crypto"; import { fileURLToPath } from "url"; import { dirname, join } from "path"; @@ -430,7 +431,10 @@ const createTransport = async ( headerHolder?: ProxyHeaderHolder; }> => { const query = req.query; - console.log("Query parameters:", JSON.stringify(query)); + console.log( + "Query parameters:", + JSON.stringify(redactQueryForLogging(query)), + ); const transportType = query.transportType as string; @@ -461,7 +465,9 @@ const createTransport = async ( const headerHolder: ProxyHeaderHolder = { headers }; console.log( - `SSE transport: url=${url}, headers=${JSON.stringify(headers)}`, + `SSE transport: url=${url}, headers=${JSON.stringify( + redactSensitiveEntries(headers), + )}`, ); const transport = new SSEClientTransport(new URL(url), { diff --git a/server/src/redact.ts b/server/src/redact.ts new file mode 100644 index 000000000..e482c47b1 --- /dev/null +++ b/server/src/redact.ts @@ -0,0 +1,48 @@ +// Patterns matching env-var/header keys whose values may contain secrets. +// When logging, we keep the key (so users can see what was passed) but +// replace the value with `***` so tokens don't end up in stdout/log files. +export const SENSITIVE_KEY_PATTERNS: RegExp[] = [ + /token/i, + /secret/i, + /password/i, + /passwd/i, + /credential/i, + /api[-_]?key/i, + /(^|_)key($|_)/i, + /auth/i, + /session/i, + /private/i, + /^aws_/i, +]; + +export const REDACTED = "***"; + +export const isSensitiveKey = (key: string): boolean => + SENSITIVE_KEY_PATTERNS.some((re) => re.test(key)); + +export const redactSensitiveEntries = ( + obj: Record | null | undefined, +): Record => { + if (!obj) return {}; + const out: Record = {}; + for (const [k, v] of Object.entries(obj)) { + out[k] = isSensitiveKey(k) ? REDACTED : v; + } + return out; +}; + +// Returns a copy of an Express query object with the `env` JSON value +// re-serialized with sensitive entries redacted, suitable for logging. +export const redactQueryForLogging = (q: unknown): unknown => { + if (!q || typeof q !== "object") return q; + const out: Record = { ...(q as Record) }; + if (typeof out.env === "string") { + try { + const parsed = JSON.parse(out.env); + out.env = redactSensitiveEntries(parsed); + } catch { + out.env = REDACTED; + } + } + return out; +}; diff --git a/server/test/redact.test.ts b/server/test/redact.test.ts new file mode 100644 index 000000000..035a990e9 --- /dev/null +++ b/server/test/redact.test.ts @@ -0,0 +1,69 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; + +import { + redactSensitiveEntries, + redactQueryForLogging, +} from "../src/redact.js"; + +test("redactSensitiveEntries: redacts common secret-bearing env vars and keeps benign ones", () => { + const input = { + GITHUB_TOKEN: "ghp_xxx", + PATH: "/usr/bin", + AWS_ACCESS_KEY_ID: "AKIA...", + }; + assert.deepEqual(redactSensitiveEntries(input), { + GITHUB_TOKEN: "***", + PATH: "/usr/bin", + AWS_ACCESS_KEY_ID: "***", + }); +}); + +test("redactSensitiveEntries: bare KEY and API_KEY are redacted", () => { + assert.deepEqual(redactSensitiveEntries({ KEY: "k" }), { KEY: "***" }); + assert.deepEqual(redactSensitiveEntries({ API_KEY: "k" }), { + API_KEY: "***", + }); + assert.deepEqual(redactSensitiveEntries({ "api-key": "k" }), { + "api-key": "***", + }); +}); + +test("redactSensitiveEntries: word containing 'key' is NOT redacted (boundary)", () => { + // The boundary in /(^|_)key($|_)/i prevents naive substring matches like + // MONKEY, KEYBOARD, etc. from being flagged as secrets. + assert.deepEqual(redactSensitiveEntries({ MONKEY: "m" }), { MONKEY: "m" }); + assert.deepEqual(redactSensitiveEntries({ KEYBOARD: "k" }), { + KEYBOARD: "k", + }); +}); + +test("redactSensitiveEntries: Authorization header is redacted", () => { + assert.deepEqual(redactSensitiveEntries({ Authorization: "Bearer x" }), { + Authorization: "***", + }); +}); + +test("redactQueryForLogging: env JSON is parsed and redacted entry-by-entry", () => { + const env = JSON.stringify({ PASSWORD: "p", PORT: "5432" }); + const out = redactQueryForLogging({ env, transport: "stdio" }) as Record< + string, + unknown + >; + assert.deepEqual(out.env, { PASSWORD: "***", PORT: "5432" }); + assert.equal(out.transport, "stdio"); +}); + +test("redactQueryForLogging: malformed env falls back to ***", () => { + const out = redactQueryForLogging({ env: "not-json" }) as Record< + string, + unknown + >; + assert.equal(out.env, "***"); +}); + +test("redactQueryForLogging: missing env passes through unchanged", () => { + assert.deepEqual(redactQueryForLogging({ transport: "sse" }), { + transport: "sse", + }); +}); From 7ef944710d7cf374177c7a989a1e4f0af25ee53a Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 19 Aug 2026 16:58:36 -0400 Subject: [PATCH 2/4] fix(server): redact all forwarded headers and non-flat env payloads Addresses two findings from Copilot review: - `x-custom-auth-header(s)` lets a caller nominate an arbitrarily-named header (`X-Foo`, `X-Access-Key`) as its credential carrier, so inferring sensitivity from the header name could not protect it. The SSE transport log now uses `redactHeadersForLogging`, which redacts every forwarded header value except a small known-non-credential allowlist (`Accept`, `Last-Event-ID`), keeping the names so the log still shows what is being forwarded. - `redactQueryForLogging` only handled a string `env` and assumed any successfully parsed value was a flat map. Express yields an array for `?env=a&env=b` and an object for `?env[x]=y`, and a nested payload such as `{"SAFE":{"PASSWORD":"p"}}` slipped past the shallow key-based redactor. `env` is now redacted wholesale unless it parses to a flat string-to-string map. This log runs before transport validation, so a malformed request could otherwise disclose its own payload. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H Signed-off-by: cliffhall --- server/src/index.ts | 4 +-- server/src/redact.ts | 62 +++++++++++++++++++++++++++++----- server/test/redact.test.ts | 69 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 124 insertions(+), 11 deletions(-) diff --git a/server/src/index.ts b/server/src/index.ts index db6c389b5..34f308e63 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -28,7 +28,7 @@ import express from "express"; import rateLimit from "express-rate-limit"; import { findActualExecutable } from "spawn-rx"; import mcpProxy, { type ProxyHeaderHolder } from "./mcpProxy.js"; -import { redactSensitiveEntries, redactQueryForLogging } from "./redact.js"; +import { redactHeadersForLogging, redactQueryForLogging } from "./redact.js"; import { randomUUID, randomBytes, timingSafeEqual } from "node:crypto"; import { fileURLToPath } from "url"; import { dirname, join } from "path"; @@ -466,7 +466,7 @@ const createTransport = async ( console.log( `SSE transport: url=${url}, headers=${JSON.stringify( - redactSensitiveEntries(headers), + redactHeadersForLogging(headers), )}`, ); diff --git a/server/src/redact.ts b/server/src/redact.ts index e482c47b1..e67b5fecd 100644 --- a/server/src/redact.ts +++ b/server/src/redact.ts @@ -15,6 +15,12 @@ export const SENSITIVE_KEY_PATTERNS: RegExp[] = [ /^aws_/i, ]; +// Header names that are never credentials: `Accept` is set by the proxy itself +// and `Last-Event-ID` is an SSE resumption cursor defined by the spec, not a +// secret. Every other forwarded header is redacted -- see +// redactHeadersForLogging for why sensitivity cannot be inferred by name there. +const NON_SENSITIVE_HEADERS = new Set(["accept", "last-event-id"]); + export const REDACTED = "***"; export const isSensitiveKey = (key: string): boolean => @@ -31,18 +37,56 @@ export const redactSensitiveEntries = ( return out; }; -// Returns a copy of an Express query object with the `env` JSON value -// re-serialized with sensitive entries redacted, suitable for logging. +// Redacts forwarded request headers for logging. Sensitivity cannot be inferred +// from a header's name here: `x-custom-auth-header(s)` lets a caller nominate an +// arbitrarily-named header (`X-Foo`, `X-Access-Key`) to carry its credential, so +// anything not on the known-safe list has its value replaced. Names are kept so +// the log still shows which headers are being forwarded. +export const redactHeadersForLogging = ( + headers: Record, +): Record => { + const out: Record = {}; + for (const [k, v] of Object.entries(headers)) { + out[k] = NON_SENSITIVE_HEADERS.has(k.toLowerCase()) ? v : REDACTED; + } + return out; +}; + +// A real `env` payload is a flat string-to-string map. Anything else -- an +// array, a nested object, non-string values -- cannot be redacted key-by-key by +// the shallow redactor, so it is replaced wholesale rather than logged verbatim. +const isFlatStringMap = (value: unknown): value is Record => + typeof value === "object" && + value !== null && + !Array.isArray(value) && + Object.values(value as Record).every( + (entry) => typeof entry === "string", + ); + +const redactEnvForLogging = (env: unknown): unknown => { + // Express query values are not necessarily strings: `?env=a&env=b` yields an + // array and `?env[x]=y` yields an object. Neither is a valid env payload, and + // this log runs before transport validation, so redact them entirely. + if (typeof env !== "string") return REDACTED; + + let parsed: unknown; + try { + parsed = JSON.parse(env); + } catch { + return REDACTED; + } + + if (!isFlatStringMap(parsed)) return REDACTED; + return redactSensitiveEntries(parsed); +}; + +// Returns a copy of an Express query object with the `env` value replaced by a +// redacted form, suitable for logging. export const redactQueryForLogging = (q: unknown): unknown => { if (!q || typeof q !== "object") return q; const out: Record = { ...(q as Record) }; - if (typeof out.env === "string") { - try { - const parsed = JSON.parse(out.env); - out.env = redactSensitiveEntries(parsed); - } catch { - out.env = REDACTED; - } + if (out.env !== undefined) { + out.env = redactEnvForLogging(out.env); } return out; }; diff --git a/server/test/redact.test.ts b/server/test/redact.test.ts index 035a990e9..41dbc7af6 100644 --- a/server/test/redact.test.ts +++ b/server/test/redact.test.ts @@ -3,6 +3,7 @@ import assert from "node:assert/strict"; import { redactSensitiveEntries, + redactHeadersForLogging, redactQueryForLogging, } from "../src/redact.js"; @@ -67,3 +68,71 @@ test("redactQueryForLogging: missing env passes through unchanged", () => { transport: "sse", }); }); + +test("redactQueryForLogging: non-string env is redacted wholesale", () => { + // Express produces an array for `?env=a&env=b` and an object for `?env[x]=y`. + // Neither is a valid env payload, so neither may be logged verbatim. + assert.equal( + (redactQueryForLogging({ env: ["a", "b"] }) as Record).env, + "***", + ); + assert.equal( + ( + redactQueryForLogging({ env: { PASSWORD: "p" } }) as Record< + string, + unknown + > + ).env, + "***", + ); +}); + +test("redactQueryForLogging: env parsing to a non-flat value is redacted wholesale", () => { + // A nested object would otherwise slip past the shallow key-based redactor. + const nested = JSON.stringify({ SAFE: { PASSWORD: "p" } }); + assert.equal( + (redactQueryForLogging({ env: nested }) as Record).env, + "***", + ); + + const array = JSON.stringify(["PASSWORD=p"]); + assert.equal( + (redactQueryForLogging({ env: array }) as Record).env, + "***", + ); + + const scalar = JSON.stringify(42); + assert.equal( + (redactQueryForLogging({ env: scalar }) as Record).env, + "***", + ); +}); + +test("redactHeadersForLogging: every forwarded header value is redacted by name-agnostic default", () => { + // `x-custom-auth-header(s)` lets a caller name any header as its credential + // carrier, so a name-pattern check would miss `X-Foo` entirely. + assert.deepEqual( + redactHeadersForLogging({ + Authorization: "Bearer secret", + "X-Foo": "secret", + "X-Access-Key": "secret", + "mcp-protocol-version": "2025-06-18", + }), + { + Authorization: "***", + "X-Foo": "***", + "X-Access-Key": "***", + "mcp-protocol-version": "***", + }, + ); +}); + +test("redactHeadersForLogging: known non-credential headers keep their value", () => { + assert.deepEqual( + redactHeadersForLogging({ + Accept: "text/event-stream", + "Last-Event-ID": "42", + }), + { Accept: "text/event-stream", "Last-Event-ID": "42" }, + ); +}); From f3e7febd57ebade0170438b0f99998c8acce2e51 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 19 Aug 2026 17:04:46 -0400 Subject: [PATCH 3/4] fix(server): redact by default instead of by secret-looking name Addresses two further Copilot findings, both of which come down to the same root cause: neither header names nor env-var names are a reliable signal of sensitivity, so any name-based rule leaks. - The env denylist preserved values whose *names* look innocuous but whose *values* are credentials -- `DATABASE_URL=postgres://user:password@host/db`, `AZURE_STORAGE_CONNECTION_STRING=...`. Env-var names are arbitrary, so the pattern list could never be complete. - The header allowlist (`accept`, `last-event-id`) was defeatable. `getHttpHeaders` forwards whatever name arrives in `x-custom-auth-header(s)`, so a caller can nominate `last-event-id` as its credential carrier, or lowercase `accept` -- which survives alongside the proxy's own `Accept` property, since object keys are case-sensitive. Either value passed the allowlist and was logged. Both surfaces now redact every value unconditionally and keep only the keys, so the log still shows which headers and which env vars are being passed through without asserting anything about a value. This removes `SENSITIVE_KEY_PATTERNS`, `isSensitiveKey`, and `redactSensitiveEntries` entirely rather than extending the heuristic. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H Signed-off-by: cliffhall --- server/src/redact.ts | 89 +++++++++--------------- server/test/redact.test.ts | 138 +++++++++++++++++-------------------- 2 files changed, 94 insertions(+), 133 deletions(-) diff --git a/server/src/redact.ts b/server/src/redact.ts index e67b5fecd..ec855774c 100644 --- a/server/src/redact.ts +++ b/server/src/redact.ts @@ -1,67 +1,37 @@ -// Patterns matching env-var/header keys whose values may contain secrets. -// When logging, we keep the key (so users can see what was passed) but -// replace the value with `***` so tokens don't end up in stdout/log files. -export const SENSITIVE_KEY_PATTERNS: RegExp[] = [ - /token/i, - /secret/i, - /password/i, - /passwd/i, - /credential/i, - /api[-_]?key/i, - /(^|_)key($|_)/i, - /auth/i, - /session/i, - /private/i, - /^aws_/i, -]; - -// Header names that are never credentials: `Accept` is set by the proxy itself -// and `Last-Event-ID` is an SSE resumption cursor defined by the spec, not a -// secret. Every other forwarded header is redacted -- see -// redactHeadersForLogging for why sensitivity cannot be inferred by name there. -const NON_SENSITIVE_HEADERS = new Set(["accept", "last-event-id"]); +// Helpers for logging connection parameters without disclosing credentials. +// +// Both the forwarded request headers and the caller-supplied `env` map are +// redacted *by default* rather than by matching secret-looking names. Name +// heuristics do not work for either surface: +// +// - Header names are caller-chosen. `getHttpHeaders` forwards whatever name +// arrives in `x-custom-auth-header(s)`, so the credential can land under +// `X-Foo`, or under a name that a heuristic would consider safe. +// - Env-var names are arbitrary, and plenty of ordinary ones carry secrets +// inside their value: `DATABASE_URL=postgres://user:password@host/db`, +// `AZURE_STORAGE_CONNECTION_STRING=...`. +// +// Keys are preserved, so the log still answers the question it exists to +// answer -- which headers and which env vars are being passed through -- while +// asserting nothing about any value. export const REDACTED = "***"; -export const isSensitiveKey = (key: string): boolean => - SENSITIVE_KEY_PATTERNS.some((re) => re.test(key)); - -export const redactSensitiveEntries = ( - obj: Record | null | undefined, -): Record => { - if (!obj) return {}; - const out: Record = {}; - for (const [k, v] of Object.entries(obj)) { - out[k] = isSensitiveKey(k) ? REDACTED : v; - } - return out; -}; - -// Redacts forwarded request headers for logging. Sensitivity cannot be inferred -// from a header's name here: `x-custom-auth-header(s)` lets a caller nominate an -// arbitrarily-named header (`X-Foo`, `X-Access-Key`) to carry its credential, so -// anything not on the known-safe list has its value replaced. Names are kept so -// the log still shows which headers are being forwarded. -export const redactHeadersForLogging = ( - headers: Record, +const redactAllValues = ( + obj: Record, ): Record => { const out: Record = {}; - for (const [k, v] of Object.entries(headers)) { - out[k] = NON_SENSITIVE_HEADERS.has(k.toLowerCase()) ? v : REDACTED; + for (const key of Object.keys(obj)) { + out[key] = REDACTED; } return out; }; -// A real `env` payload is a flat string-to-string map. Anything else -- an -// array, a nested object, non-string values -- cannot be redacted key-by-key by -// the shallow redactor, so it is replaced wholesale rather than logged verbatim. -const isFlatStringMap = (value: unknown): value is Record => - typeof value === "object" && - value !== null && - !Array.isArray(value) && - Object.values(value as Record).every( - (entry) => typeof entry === "string", - ); +// Redacts forwarded request headers for logging: every value is replaced, +// names are kept. +export const redactHeadersForLogging = ( + headers: Record, +): Record => redactAllValues(headers); const redactEnvForLogging = (env: unknown): unknown => { // Express query values are not necessarily strings: `?env=a&env=b` yields an @@ -76,8 +46,13 @@ const redactEnvForLogging = (env: unknown): unknown => { return REDACTED; } - if (!isFlatStringMap(parsed)) return REDACTED; - return redactSensitiveEntries(parsed); + // Only a plain object has env-var names worth showing. An array or a scalar + // has none, so there is nothing to preserve and it is redacted whole. + if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) { + return REDACTED; + } + + return redactAllValues(parsed as Record); }; // Returns a copy of an Express query object with the `env` value replaced by a diff --git a/server/test/redact.test.ts b/server/test/redact.test.ts index 41dbc7af6..0faab0ed8 100644 --- a/server/test/redact.test.ts +++ b/server/test/redact.test.ts @@ -2,59 +2,69 @@ import { test } from "node:test"; import assert from "node:assert/strict"; import { - redactSensitiveEntries, redactHeadersForLogging, redactQueryForLogging, } from "../src/redact.js"; -test("redactSensitiveEntries: redacts common secret-bearing env vars and keeps benign ones", () => { - const input = { - GITHUB_TOKEN: "ghp_xxx", - PATH: "/usr/bin", - AWS_ACCESS_KEY_ID: "AKIA...", - }; - assert.deepEqual(redactSensitiveEntries(input), { - GITHUB_TOKEN: "***", - PATH: "/usr/bin", - AWS_ACCESS_KEY_ID: "***", - }); -}); - -test("redactSensitiveEntries: bare KEY and API_KEY are redacted", () => { - assert.deepEqual(redactSensitiveEntries({ KEY: "k" }), { KEY: "***" }); - assert.deepEqual(redactSensitiveEntries({ API_KEY: "k" }), { - API_KEY: "***", - }); - assert.deepEqual(redactSensitiveEntries({ "api-key": "k" }), { - "api-key": "***", - }); +test("redactHeadersForLogging: every forwarded header value is redacted", () => { + // Sensitivity cannot be inferred from a header name: `getHttpHeaders` + // forwards whatever name arrives in `x-custom-auth-header(s)`, so the + // credential can land under any name at all -- including one an allowlist + // would consider safe. + assert.deepEqual( + redactHeadersForLogging({ + Authorization: "Bearer secret", + "X-Foo": "secret", + "X-Access-Key": "secret", + "mcp-protocol-version": "2025-06-18", + Accept: "text/event-stream", + accept: "secret", + "last-event-id": "secret", + }), + { + Authorization: "***", + "X-Foo": "***", + "X-Access-Key": "***", + "mcp-protocol-version": "***", + Accept: "***", + accept: "***", + "last-event-id": "***", + }, + ); }); -test("redactSensitiveEntries: word containing 'key' is NOT redacted (boundary)", () => { - // The boundary in /(^|_)key($|_)/i prevents naive substring matches like - // MONKEY, KEYBOARD, etc. from being flagged as secrets. - assert.deepEqual(redactSensitiveEntries({ MONKEY: "m" }), { MONKEY: "m" }); - assert.deepEqual(redactSensitiveEntries({ KEYBOARD: "k" }), { - KEYBOARD: "k", - }); +test("redactHeadersForLogging: an empty header set stays empty", () => { + assert.deepEqual(redactHeadersForLogging({}), {}); }); -test("redactSensitiveEntries: Authorization header is redacted", () => { - assert.deepEqual(redactSensitiveEntries({ Authorization: "Bearer x" }), { - Authorization: "***", +test("redactQueryForLogging: env var names are kept, every value is redacted", () => { + // A name-pattern denylist would have preserved DATABASE_URL and PATH; the + // former carries its credential inside the value. + const env = JSON.stringify({ + PASSWORD: "p", + PORT: "5432", + PATH: "/usr/bin", + DATABASE_URL: "postgres://user:password@host/db", }); -}); - -test("redactQueryForLogging: env JSON is parsed and redacted entry-by-entry", () => { - const env = JSON.stringify({ PASSWORD: "p", PORT: "5432" }); const out = redactQueryForLogging({ env, transport: "stdio" }) as Record< string, unknown >; - assert.deepEqual(out.env, { PASSWORD: "***", PORT: "5432" }); + assert.deepEqual(out.env, { + PASSWORD: "***", + PORT: "***", + PATH: "***", + DATABASE_URL: "***", + }); assert.equal(out.transport, "stdio"); }); +test("redactQueryForLogging: a nested env value is redacted, not descended into", () => { + const env = JSON.stringify({ SAFE: { PASSWORD: "p" } }); + const out = redactQueryForLogging({ env }) as Record; + assert.deepEqual(out.env, { SAFE: "***" }); +}); + test("redactQueryForLogging: malformed env falls back to ***", () => { const out = redactQueryForLogging({ env: "not-json" }) as Record< string, @@ -63,12 +73,6 @@ test("redactQueryForLogging: malformed env falls back to ***", () => { assert.equal(out.env, "***"); }); -test("redactQueryForLogging: missing env passes through unchanged", () => { - assert.deepEqual(redactQueryForLogging({ transport: "sse" }), { - transport: "sse", - }); -}); - test("redactQueryForLogging: non-string env is redacted wholesale", () => { // Express produces an array for `?env=a&env=b` and an object for `?env[x]=y`. // Neither is a valid env payload, so neither may be logged verbatim. @@ -87,14 +91,8 @@ test("redactQueryForLogging: non-string env is redacted wholesale", () => { ); }); -test("redactQueryForLogging: env parsing to a non-flat value is redacted wholesale", () => { - // A nested object would otherwise slip past the shallow key-based redactor. - const nested = JSON.stringify({ SAFE: { PASSWORD: "p" } }); - assert.equal( - (redactQueryForLogging({ env: nested }) as Record).env, - "***", - ); - +test("redactQueryForLogging: env parsing to a non-object is redacted wholesale", () => { + // An array or a scalar has no env-var names worth preserving. const array = JSON.stringify(["PASSWORD=p"]); assert.equal( (redactQueryForLogging({ env: array }) as Record).env, @@ -106,33 +104,21 @@ test("redactQueryForLogging: env parsing to a non-flat value is redacted wholesa (redactQueryForLogging({ env: scalar }) as Record).env, "***", ); -}); -test("redactHeadersForLogging: every forwarded header value is redacted by name-agnostic default", () => { - // `x-custom-auth-header(s)` lets a caller name any header as its credential - // carrier, so a name-pattern check would miss `X-Foo` entirely. - assert.deepEqual( - redactHeadersForLogging({ - Authorization: "Bearer secret", - "X-Foo": "secret", - "X-Access-Key": "secret", - "mcp-protocol-version": "2025-06-18", - }), - { - Authorization: "***", - "X-Foo": "***", - "X-Access-Key": "***", - "mcp-protocol-version": "***", - }, + const nullEnv = JSON.stringify(null); + assert.equal( + (redactQueryForLogging({ env: nullEnv }) as Record).env, + "***", ); }); -test("redactHeadersForLogging: known non-credential headers keep their value", () => { - assert.deepEqual( - redactHeadersForLogging({ - Accept: "text/event-stream", - "Last-Event-ID": "42", - }), - { Accept: "text/event-stream", "Last-Event-ID": "42" }, - ); +test("redactQueryForLogging: missing env passes through unchanged", () => { + assert.deepEqual(redactQueryForLogging({ transport: "sse" }), { + transport: "sse", + }); +}); + +test("redactQueryForLogging: a non-object query passes through unchanged", () => { + assert.equal(redactQueryForLogging(undefined), undefined); + assert.equal(redactQueryForLogging("nope"), "nope"); }); From dd202527ac7ea53678431609e1096ff05fcdd62b Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 19 Aug 2026 17:09:06 -0400 Subject: [PATCH 4/4] ci: run the server test suite The redaction tests were added under `server/test/` with a server-local `test` script, but nothing invoked it: the root `test` script runs only prettier plus the client suite, and the PR workflow has steps for the client tests and the build only. The new tests could therefore fail while CI stayed green -- which for a security fix means the redaction could silently regress. Adds a `test-server` script alongside the existing `test-cli`, folds it into the root `test`, and adds a matching workflow step after the client tests. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H Signed-off-by: cliffhall --- .github/workflows/main.yml | 4 ++++ package.json | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 6fd8a6dfb..7fa728da8 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -37,6 +37,10 @@ jobs: working-directory: ./client run: npm test + - name: Run server tests + working-directory: ./server + run: npm test + - run: npm run build publish: diff --git a/package.json b/package.json index 601ac3e2d..0f40b73dc 100644 --- a/package.json +++ b/package.json @@ -40,8 +40,9 @@ "start": "node client/bin/start.js", "start-server": "cd server && npm run start", "start-client": "cd client && npm run preview", - "test": "npm run prettier-check && cd client && npm test", + "test": "npm run prettier-check && npm run test-server && cd client && npm test", "test-cli": "cd cli && npm run test", + "test-server": "cd server && npm run test", "test:e2e": "MCP_AUTO_OPEN_ENABLED=false npm run test:e2e --workspace=client", "prettier-fix": "prettier --write .", "prettier-check": "prettier --check .",