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 .", 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..34f308e63 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 { redactHeadersForLogging, 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( + redactHeadersForLogging(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..ec855774c --- /dev/null +++ b/server/src/redact.ts @@ -0,0 +1,67 @@ +// 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 = "***"; + +const redactAllValues = ( + obj: Record, +): Record => { + const out: Record = {}; + for (const key of Object.keys(obj)) { + out[key] = REDACTED; + } + return out; +}; + +// 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 + // 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; + } + + // 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 +// 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 (out.env !== undefined) { + out.env = redactEnvForLogging(out.env); + } + return out; +}; diff --git a/server/test/redact.test.ts b/server/test/redact.test.ts new file mode 100644 index 000000000..0faab0ed8 --- /dev/null +++ b/server/test/redact.test.ts @@ -0,0 +1,124 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; + +import { + redactHeadersForLogging, + redactQueryForLogging, +} from "../src/redact.js"; + +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("redactHeadersForLogging: an empty header set stays empty", () => { + assert.deepEqual(redactHeadersForLogging({}), {}); +}); + +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", + }); + const out = redactQueryForLogging({ env, transport: "stdio" }) as Record< + string, + unknown + >; + 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, + unknown + >; + assert.equal(out.env, "***"); +}); + +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-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, + "***", + ); + + const scalar = JSON.stringify(42); + assert.equal( + (redactQueryForLogging({ env: scalar }) as Record).env, + "***", + ); + + const nullEnv = JSON.stringify(null); + assert.equal( + (redactQueryForLogging({ env: nullEnv }) as Record).env, + "***", + ); +}); + +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"); +});