Block credential fields spelled with the x- header prefix - #450
Open
kevin9327 wants to merge 1 commit into
Open
Conversation
`inspectToolArguments` compares each argument name against a list of credential field names after lowercasing and folding `-`, `.` and whitespace to `_`. That made `api-key` a hit and `x-api-key` a miss, so the spelling that is more obviously a credential header was the one that left the deployment. Measured on `main`: `api-key` blocked; `x-api-key`, `X-API-Key`, `x-auth-token`, `auth_token`, `authToken`, `passwd`, `pwd`, `api_secret`, `session_token`, `bearer_token`, `ssh_key` and `signing_key` all passed. `x_` is the conventional prefix for a non-standard header and says nothing about the value, so it is dropped before the comparison. The list also gains the spellings of names it already carries -- `passwd`/`pwd` for `password`, `auth_token`/`bearer_token`/`session_token` for `token`, `api_secret`/`secret_key`/`signing_key` for `secret`, `ssh_key` for `private_key`. Neither change widens what counts as a credential: each new entry is another name for something already listed, and the prefix rule only strips a prefix that carries no meaning of its own. The new tests fail 13 of 13 against the current `main` and pass after. Ten names that only resemble credentials -- `query`, `url`, `path`, `token_count`, `max_tokens`, `tokenizer`, `x_axis`, `x_offset`, `xml`, `secretary` -- are pinned as still allowed, and pass both before and after.
kevin9327
requested review from
MikeRyanDev,
davidmckayv,
guidovizoso and
tylerslaton
as code owners
September 8, 2026 21:55
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This was referenced Sep 8, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
What is wrong
inspectToolArguments(added in #436, whose job is to keep credential material out of MCP toolarguments) normalises each argument name -- lowercase, then
-,.and whitespace folded to_--and compares the result against
sensitiveFieldNames.api-keynormalises exactly ontoapi_keyand is caught.
x-api-keynormalises ontox_api_key, which is not on the list, and goes out.The spelling that most obviously names a credential is the one that is not blocked.
Measured on
mainbefore the change, viainspectToolArguments({ [name]: "value" }):mainapi-keyx-api-keyX-API-Keyx-auth-tokenauth_token,authTokenbearer_token,session_tokenpasswd,pwdapi_secret,secret_key,signing_keyssh_keyThe change
Two parts, both narrow.
normalizedFieldNamedrops a leadingx_.x-is the conventional prefix for a non-standardheader; it says nothing about the value, so
x-api-keyis the same field asapi-key.sensitiveFieldNamesgains the spellings of names it already carries:passwdandpwdforpassword,auth_token/authtoken/bearer_token/bearertoken/session_token/sessiontokenfortoken,api_secret/apisecret/secret_key/secretkey/signing_key/signingkeyforsecret, andssh_key/sshkeyforprivate_key.Neither part widens what counts as a credential. Every added entry is another name for something
already on the list, and the prefix rule strips only a prefix that carries no meaning of its own.
Verification
bun test server/tests/content-governance.test.tsserver/src/plugins/content-governance.tsreverted tomainand only the tests applied:13 fail, 18 pass.
Ten names that merely resemble credential names are pinned as still allowed --
query,url,path,token_count,max_tokens,tokenizer,x_axis,x_offset,xml,secretary-- andpass both before and after, so the guard against over-blocking is a real one rather than a
restatement of the new behaviour.
bun run --filter server typecheckandbunx biome checkon both touched files are clean.