fix(server): redact sensitive env vars and headers from connection logs

[SarthakB11]

NOTE

• Identified as useful in closing advisories in go-live 7/9: triage and bulk-close the v1 PR backlog #1819
• Closes Hide potentially sensitive env variables from being logged #847.

Summary

The proxy logs the full incoming connection request to stdout in two places in server/src/index.ts:

• console.log("Query parameters:", JSON.stringify(query)). query.env is a JSON-encoded map of process env vars supplied by the user, frequently containing *_TOKEN, *_KEY, *_SECRET, AWS_*, password, database URLs, and connection strings.
• SSE transport: url=..., headers=.... headers regularly carries Authorization, and getHttpHeaders also forwards any header the caller nominates via x-custom-auth-header(s) — so the credential can arrive under an arbitrary name.

Those values end up in the proxy's stdout and from there into terminals, shell history, log files, and screen recordings (the original scenario from #375). This PR sanitizes both log lines.

Change

Helpers live in a new server/src/redact.ts, imported by server/src/index.ts at both log sites.

Both surfaces redact by default rather than by matching secret-looking names, and keep only the keys. A name is not a reliable signal of sensitivity on either one:

• Header names are caller-chosen. getHttpHeaders forwards whatever name arrives in x-custom-auth-header / x-custom-auth-headers, so a credential can land under X-Foo, X-Access-Key, or a name that an allowlist would treat as safe (last-event-id; or lowercase accept, which survives alongside the proxy's own case-sensitive Accept key).
• Env-var names are arbitrary, and ordinary-looking ones carry secrets in their value — DATABASE_URL=postgres://user:password@host/db, AZURE_STORAGE_CONNECTION_STRING=.... No pattern list can be complete.

The API is two functions plus the "***" sentinel:

• redactHeadersForLogging(headers) — replaces every header value with "***", preserving names.
• redactQueryForLogging(query) — clones the query and replaces the embedded env with a redacted view: every value becomes "***" and only the env-var names are preserved. A non-string env (Express yields an array for ?env=a&env=b and an object for ?env[x]=y), an env that is not valid JSON, and one that parses to an array or a scalar are each redacted wholesale to "***" rather than logged raw — this log runs before transport validation, so a malformed request must not be able to disclose its own payload. A nested value is redacted at the top level, not descended into.

Both call sites log the redacted view only; the live env and headers objects are still passed unchanged to the spawned transport / SSEClientTransport. Keys are kept so the log still answers the question it exists to answer — which env vars and which headers are being forwarded — while asserting nothing about any value.

Scope notes

• Redaction is unconditional, so the log no longer shows the value of a benign var such as PATH. That is the deliberate trade: the previous name-based heuristic was demonstrably bypassable in both directions, and this only affects the log line, never what the spawned process receives.
• The command / args log a few lines below, and the url= in the SSE log, are left alone. Both can carry secrets in principle and are the same class of issue, but they are out of scope for this PR and better tracked separately.
• The streamable-HTTP path doesn't log headers today, so no change there. redactHeadersForLogging is available if that ever changes.

Test plan

server/ previously had no test runner. This PR adds a minimal node:test setup using tsx (already in server/devDependencies); zero new dependencies. New script: server/package.json "test": "node --import tsx --test test/*.test.ts".

It is wired into CI: a test-server script at the root (alongside the existing test-cli), folded into the root test, plus a Run server tests step in .github/workflows/main.yml after the client tests — so a redaction regression fails the build rather than passing unnoticed.

server/test/redact.test.ts covers:

• every forwarded header value redacted — Authorization, an arbitrary X-Foo, X-Access-Key, mcp-protocol-version, and the two former allowlist bypasses (Accept / lowercase accept, last-event-id)
• an empty header set stays empty
• env-var names kept, every value redacted — including PATH and DATABASE_URL, which a name-based denylist would have preserved
• a nested env value is redacted, not descended into
• malformed (non-JSON) env falls back to "***" instead of leaking the raw payload
• non-string env (array, object) redacted wholesale
• env parsing to a non-object (array, scalar, null) redacted wholesale
• queries without env pass through unchanged
• a non-object query passes through unchanged

Run with cd server && npm test (9 passed, 0 failed), or npm run test-server from the root. tsc --noEmit and prettier --check are clean.

[Copilot]

Pull request overview

Adds log sanitization for sensitive environment variables and SSE headers.

Changes:

• Introduces reusable redaction helpers.
• Applies redaction to connection logs.
• Adds focused Node test coverage.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
server/src/redact.ts	Implements key-based redaction.
server/src/index.ts	Sanitizes query and SSE header logs.
server/test/redact.test.ts	Tests redaction behavior.
server/package.json	Adds the test command.

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[cliffhall]

Agreed and fixed in 7ef9447.

redactQueryForLogging now routes env through a dedicated redactEnvForLogging:

• A non-string env (Express yields an array for ?env=a&env=b and an object for ?env[x]=y) is replaced with *** rather than passed through.
• A parsed value is only preserved when it is a flat string-to-string map (isFlatStringMap: object, non-null, not an array, all values strings). An array, a scalar, or a nested payload such as {"SAFE":{"PASSWORD":"p"}} is redacted wholesale, since the shallow key-based redactor cannot reach inside it.
• Malformed JSON still falls back to ***, as before.

Two new tests cover it: non-string env is redacted wholesale and env parsing to a non-flat value is redacted wholesale.

[cliffhall]

Declining this one -- it is a process judgement rather than a code finding, and the premise is not quite right for this repo.

v1/main is the deprecated maintenance line and it does still accept security fixes, published straight from that branch to the v1-latest npm dist-tag. Redacting secrets out of proxy connection logs is exactly that class of change, so v1/main is the correct base; retargeting it to v2 would leave every v1 user on v1-latest with the leak.

Whether to land this PR (versus close it and have a maintainer reimplement from the issue) is a maintainer decision, not one to take from a review bot -- and a maintainer is driving this review loop deliberately. No code change.

[cliffhall]

Agreed and fixed in 7ef9447.

You are right that a name-pattern check cannot cover this: getHttpHeaders honors x-custom-auth-header / x-custom-auth-headers, so the credential can arrive under any name the caller picks, and X-Foo matches none of the patterns.

The SSE log now uses a new redactHeadersForLogging, which is safe-by-default: every forwarded header value is replaced with *** unless its lowercased name is on a small known-non-credential allowlist (accept, which the proxy sets itself, and last-event-id, an SSE resumption cursor). Header names are kept, so the log still answers the question it was there to answer -- which headers are being forwarded -- without asserting anything about a value.

Covered by two new tests: one asserting Authorization, X-Foo, X-Access-Key, and mcp-protocol-version are all redacted, and one asserting the allowlisted pair is preserved.

[cliffhall]

Copilot review — round 1 response

Pushed 7ef94471. Inline replies go outdated once a fix lands, so mirroring here.

1. redact.ts — env only handled as a string, and any parsed value assumed to be a flat map. [FIXED]
Real leak. redactQueryForLogging now routes env through redactEnvForLogging:

• non-string env (Express yields an array for ?env=a&env=b, an object for ?env[x]=y) → ***;
• parsed value preserved only when it is a flat string→string map (isFlatStringMap);
• an array, a scalar, or a nested payload like {"SAFE":{"PASSWORD":"p"}} → *** wholesale, since the shallow key-based redactor cannot reach inside it;
• malformed JSON still → ***.

2. index.ts — key-pattern redaction does not protect custom auth headers. [FIXED]
Correct, and the strongest of the three. getHttpHeaders honors x-custom-auth-header(s), so the credential can arrive under any caller-chosen name (X-Foo, X-Access-Key) that matches no pattern. The SSE log now uses a new redactHeadersForLogging, safe-by-default: every forwarded header value is *** unless the lowercased name is on a small known-non-credential allowlist (accept, set by the proxy itself; last-event-id, an SSE resumption cursor). Header names are kept, so the log still shows which headers are forwarded.

3. redact.ts:1 — "v1 does not accept PRs; close this and move to v2." [DECLINED]
A process judgement, not a code finding, and the premise is off: v1/main is the deprecated maintenance line but it does take security fixes, published straight from that branch to the v1-latest dist-tag. Redacting secrets out of connection logs is that class of change, so v1/main is the right base — retargeting to v2 would leave v1-latest users with the leak. Whether to land this PR versus reimplement from the issue is a maintainer call.

Tests: 7 → 11, all passing (cd server && npm test). New coverage for non-string env, non-flat parsed env, name-agnostic header redaction, and the allowlist. tsc --noEmit clean; prettier --check clean.

Not changed (no finding raised, keeping the diff tight): STDIO transport: command=..., args=... still logs argv verbatim, and the SSE url= still logs any credential embedded in the URL. Both are the same class as this fix but were not flagged; happy to extend the PR if a maintainer wants them in scope rather than a follow-up.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

[cliffhall]

Agreed and fixed in f3e7feb. This is the right call, and it generalizes: DATABASE_URL and AZURE_STORAGE_CONNECTION_STRING are exactly the shape a name-based denylist cannot catch, because the credential is inside the value while the name looks like configuration. Env-var names are arbitrary, so the pattern list could never be complete.

Rather than extend the heuristic, the denylist is gone: SENSITIVE_KEY_PATTERNS, isSensitiveKey, and redactSensitiveEntries are all removed. env now has every value replaced with *** and only its keys preserved, so the log still shows which env vars are being passed to the spawned process without asserting anything about any of them. A nested value is redacted at the top level rather than descended into, and a non-string / unparseable / non-object env is still redacted wholesale.

[cliffhall]

Agreed and fixed in f3e7feb — you are right, and I should have caught that the allowlist was reachable by the very mechanism the previous round was about.

Confirmed both bypasses against getHttpHeaders:

• x-custom-auth-header: last-event-id plus last-event-id: <secret> writes headers["last-event-id"] = <secret>, which the allowlist then passed through verbatim.
• x-custom-auth-header: accept writes headers["accept"] = <secret>; the proxy separately sets headers["Accept"] = "text/event-stream", and since object keys are case-sensitive both survive — so the lowercase one was logged.

The allowlist is removed entirely. redactHeadersForLogging now replaces every value unconditionally and keeps only the names, which is the diagnostic the log was actually for. A test asserts that Accept, lowercase accept, and last-event-id are all redacted alongside Authorization and an arbitrary X-Foo.

[cliffhall]

Copilot review — round 2 response

Pushed f3e7febd. Both findings were valid, and both had the same root cause: a name is not a reliable signal of sensitivity on either surface. So rather than patch the two holes, this round deletes the name-based logic altogether.

4. redact.ts:35 — env denylist still logs credentials whose names do not look secret. [FIXED]
Correct, and not fixable by adding patterns. DATABASE_URL=postgres://user:password@host/db and AZURE_STORAGE_CONNECTION_STRING=... carry the secret inside the value while the name reads as ordinary configuration; env-var names are arbitrary, so the list could never be complete. SENSITIVE_KEY_PATTERNS, isSensitiveKey, and redactSensitiveEntries are removed. env now has every value replaced and only its keys preserved.

5. redact.ts:50 — the header allowlist reintroduced the arbitrary-custom-auth-header leak. [FIXED]
Also correct, and my mistake — the allowlist was reachable by the exact mechanism round 1 was about. Verified both bypasses against getHttpHeaders:

• x-custom-auth-header: last-event-id + last-event-id: <secret> → headers["last-event-id"] = <secret>, passed the allowlist.
• x-custom-auth-header: accept → headers["accept"] = <secret>; the proxy sets headers["Accept"] separately and object keys are case-sensitive, so both survived and the lowercase one was logged.

The allowlist is gone. redactHeadersForLogging replaces every value unconditionally, keeping only names.

Net effect. redact.ts is now ~30 lines of unconditional, safe-by-default redaction with no heuristic to bypass: both headers and env keep their keys and lose every value; a non-string, unparseable, or non-object env is redacted wholesale. Diff shrank by 133 lines against round 1.

Verified: 9 tests passing (cd server && npm test), including a test that Accept, lowercase accept, and last-event-id are all redacted, and one that DATABASE_URL/PATH no longer survive. tsc --noEmit clean, prettier --check clean.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (2)

server/package.json:28

• This adds a server-local test command, but neither the root test script (package.json:43) nor the PR workflow (.github/workflows/main.yml:29-40) invokes it. As a result, all of the new redaction tests can fail while CI remains green. Wire npm test --workspace=server (or the equivalent working-directory step) into the repository test/CI path.

    "test": "node --import tsx --test test/*.test.ts"

server/src/redact.ts:25

• The implementation now redacts every env value and exposes the parsed key map, but the PR description still specifies SENSITIVE_KEY_PATTERNS, selective value redaction (including preserving PATH), re-serialization of env, and seven passing tests. Those are materially different logging semantics from this code and the nine tests in the diff; update the PR description and test plan to document the final behavior.

  obj: Record<string, unknown>,
): Record<string, string> => {
  const out: Record<string, string> = {};
  for (const key of Object.keys(obj)) {
    out[key] = REDACTED;

[cliffhall]

Copilot review — round 3 response

No new inline comments this round. The review body carried two suppressed comments, both valid and both now addressed.

6. server/package.json:28 — the new test script is invoked by nothing, so the redaction tests can fail while CI stays green. [FIXED — dd202527]
Confirmed: the root test script ran only prettier-check plus the client suite, and .github/workflows/main.yml had steps for the client tests and the build only. For a security fix that is the worst version of the problem — the redaction could silently regress with a green check. Added a test-server script alongside the existing test-cli, folded it into the root test, and added a Run server tests step to the workflow after the client tests.

7. server/src/redact.ts:25 — the PR description no longer matches the code. [FIXED]
Also correct, and worth catching: after rounds 1–2 the description still specified SENSITIVE_KEY_PATTERNS, selective redaction preserving PATH, env re-serialization, and a 7-test plan — none of which survive. The description has been rewritten to document the final behavior: two functions (redactHeadersForLogging, redactQueryForLogging), unconditional redact-by-default on both surfaces with keys preserved, the wholesale-redaction rules for a non-string / unparseable / non-object env, the CI wiring, and the actual 9-test plan. The scope notes now state plainly that PATH is no longer shown and why, and that command/args and the SSE url= remain out of scope.

Verified: npm run test-server from the root — 9 passed, 0 failed. tsc --noEmit clean, prettier --check clean on all changed files.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

[cliffhall]

Thanks for this, @SarthakB11, and for the patience while it sat — this one had real work in it and deserves a real explanation. We're closing it.

The finding is legitimate

The connection logs did print forwarded environment values and header values verbatim, and an MCP server config routinely carries API keys and tokens in exactly those places. The review round on this PR sharpened it considerably: it started as a denylist of secret-looking key names and ended as unconditional redaction of every env value and every forwarded header value, which is the correct shape — a name-pattern heuristic silently misses DATABASE_URL=postgres://user:password@host/db, and an allowlist of "safe" headers turned out to be reachable by the very mechanism the PR was about, since a caller can nominate any header name through x-custom-auth-header.

Why we're closing it anyway

The exposure here is a local one. These values are written to the console of the process the user started themselves, on their own machine, and are visible to whoever can already read that terminal or its captured output. That's a real hygiene problem — it matters for screen shares, pasted bug reports, and CI logs — but it is not a vulnerability that lets a remote party obtain anything they couldn't already.

v1/main is deprecated and is kept alive for a single purpose: shipping fixes for high-severity vulnerabilities to users still on the v1-latest dist-tag. It isn't taking general security hardening or hygiene improvements, however well built. That calculus is sharpened by the state of the branch: fork PRs here don't run workflows at all, so nothing on this line is verified by the repo's own CI, and this PR necessarily touched more than the leak — it added a test-server script and a workflow step, because the server suite it relies on wasn't being run by anything. Sound changes, but they're branch maintenance on a branch we've stopped maintaining.

Worth carrying forward

Two things surfaced here that are worth keeping, and neither is v1-specific:

• The same class of leak still exists in the STDIO and SSE paths — command=/args= is logged verbatim, and the SSE log prints url=, which can carry embedded credentials. Copilot never raised either.
• The "redact by default, never by secret-looking name" conclusion is the durable lesson, and it applies to v2's logging too.

If you'd like to take either to v2, please open an issue describing it and a maintainer will pick it up from there. Three commits were pushed to this branch during review (7ef94471, f3e7febd, dd202527) — yours to keep or discard, nothing merges from here. Genuinely good work on this one.