diff --git a/CHANGELOG.md b/CHANGELOG.md index dba57a687..8911e11e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,18 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### An MCP call carrying `x-api-key` is stopped the same as one carrying `api-key` + +The check that keeps credentials out of MCP tool arguments compared each argument name against a +list, and `api-key` was on it while `x-api-key` was not -- so the spelling that is more obviously a +credential header was the one that went out. `x-` is the conventional prefix for a non-standard +header and says nothing about the value, so it is now dropped before the comparison. The same pass +adds the spellings of names already on the list that were missing from it: `passwd` and `pwd` for +`password`, `auth_token` and `bearer_token` and `session_token` for `token`, `api_secret` and +`secret_key` and `signing_key` for `secret`, and `ssh_key` for `private_key`. Nothing new counts as +a credential: an argument named `x_axis`, `token_count`, `max_tokens` or `secretary` is passed as +before. + ### One command to stop what `start.sh` started Stopping the local stack meant four commands read off the end of a successful start, and the one diff --git a/server/src/plugins/content-governance.ts b/server/src/plugins/content-governance.ts index 7b8e6381e..b22cae938 100644 --- a/server/src/plugins/content-governance.ts +++ b/server/src/plugins/content-governance.ts @@ -26,24 +26,43 @@ export type ToolArgumentInspection = findings: SensitiveArgumentFinding[]; }; +// Each entry is a spelling of a field already named here, not a widening of what counts as a +// credential: `passwd` is `password`, `api_secret` is `secret`, `ssh_key` is `private_key`. A tool +// argument carrying one of these carries the same thing under a different name. const sensitiveFieldNames = new Set([ "access_token", "accesstoken", "api_key", + "api_secret", "apikey", + "apisecret", + "auth_token", "authorization", + "authtoken", + "bearer_token", + "bearertoken", "client_secret", "clientsecret", "credential", "credentials", "id_token", "idtoken", + "passwd", "password", "private_key", "privatekey", + "pwd", "refresh_token", "refreshtoken", "secret", + "secret_key", + "secretkey", + "session_token", + "sessiontoken", + "signing_key", + "signingkey", + "ssh_key", + "sshkey", "token", ]); @@ -61,7 +80,12 @@ const MAX_FINDINGS = 20; const MAX_STRING_LENGTH = 64 * 1024; function normalizedFieldName(value: string): string { - return value.toLowerCase().replace(/[-.\s]/g, "_"); + const normalized = value.toLowerCase().replace(/[-.\s]/g, "_"); + // `x_` is the conventional prefix for a non-standard header and says nothing about the value, so + // `x-api-key` is the same field as `api-key`. Without this the list caught `api-key` -- which + // normalises exactly onto `api_key` -- and let through the spelling that is more obviously a + // credential, not less. + return normalized.startsWith("x_") ? normalized.slice(2) : normalized; } function categoryForValue(value: string): SensitiveArgumentCategory | null { diff --git a/server/tests/content-governance.test.ts b/server/tests/content-governance.test.ts index 9138f954a..771283071 100644 --- a/server/tests/content-governance.test.ts +++ b/server/tests/content-governance.test.ts @@ -110,4 +110,57 @@ describe("MCP tool argument content governance", () => { findings: [], }); }); + + test("blocks a credential field carrying the conventional non-standard header prefix", () => { + const secret = "do-not-copy-this-value"; + const result = inspectToolArguments({ + headers: { "x-api-key": secret }, + }); + + expect(result).toEqual({ + safe: false, + reason: "sensitive_content", + findings: [{ category: "credential_field", path: "$.headers.x-api-key" }], + }); + expect(JSON.stringify(result)).not.toContain(secret); + }); + + test.each([ + "X-API-Key", + "x-auth-token", + "authToken", + "auth_token", + "api_secret", + "bearer_token", + "passwd", + "pwd", + "secret_key", + "session_token", + "signing_key", + "ssh_key", + ])("blocks %s as a spelling of a name already listed", (field) => { + const result = inspectToolArguments({ [field]: "do-not-copy-this-value" }); + + expect(result).toMatchObject({ safe: false, reason: "sensitive_content" }); + expect(result).toMatchObject({ + findings: [{ category: "credential_field" }], + }); + }); + + test.each([ + "query", + "url", + "path", + "token_count", + "max_tokens", + "tokenizer", + "x_axis", + "x_offset", + "xml", + "secretary", + ])("allows %s, which only resembles a credential name", (field) => { + expect(inspectToolArguments({ [field]: "ordinary value" })).toEqual({ + safe: true, + }); + }); });