-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix(server): redact sensitive env vars and headers from connection logs #1296
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
983aac3
fix(server): redact sensitive env vars and headers from connection logs
SarthakB11 7ef9447
fix(server): redact all forwarded headers and non-flat env payloads
cliffhall f3e7feb
fix(server): redact by default instead of by secret-looking name
cliffhall dd20252
ci: run the server test suite
cliffhall File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<string, unknown>, | ||
| ): Record<string, string> => { | ||
| const out: Record<string, string> = {}; | ||
| 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<string, string>, | ||
| ): Record<string, string> => 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<string, unknown>); | ||
| }; | ||
|
|
||
| // 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<string, unknown> = { ...(q as Record<string, unknown>) }; | ||
| if (out.env !== undefined) { | ||
| out.env = redactEnvForLogging(out.env); | ||
| } | ||
| return out; | ||
| }; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<string, unknown>; | ||
| 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<string, unknown>).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<string, unknown>).env, | ||
| "***", | ||
| ); | ||
|
|
||
| const scalar = JSON.stringify(42); | ||
| assert.equal( | ||
| (redactQueryForLogging({ env: scalar }) as Record<string, unknown>).env, | ||
| "***", | ||
| ); | ||
|
|
||
| const nullEnv = JSON.stringify(null); | ||
| assert.equal( | ||
| (redactQueryForLogging({ env: nullEnv }) as Record<string, unknown>).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"); | ||
| }); |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.