Refuse a credential written into the rest of the address - #230
Open
beardthelion wants to merge 1 commit into
Open
Refuse a credential written into the rest of the address#230beardthelion wants to merge 1 commit into
beardthelion wants to merge 1 commit into
Conversation
beardthelion
requested review from
MikeRyanDev,
davidmckayv,
guidovizoso and
tylerslaton
as code owners
August 24, 2026 18:37
Refusing a credential in the userinfo closed one spelling and left the two beside it open. A token in the query string or the fragment was accepted, and addCustomServer writes the address it was given into mcp_servers.url and into the configuration.changed audit payload verbatim. Redaction keys on the field name and url is not a sensitive one, so the secret landed in an append-only row in clear text, which is the disclosure the userinfo rule exists to prevent. The name is read rather than matched against a list. An exact-name version of this rule refused token and accepted auth_token, api_token, x-api-key and X-Amz-Signature, and an operator has no way to know which spellings the check happens to hold. Reading the name over-refuses in one direction on purpose: a misread parameter costs a rename, a missed one cannot be deleted afterwards. The fragment is split at the first question mark first, because a hash route or an OAuth-style callback puts a path in front of the parameters and reading the whole fragment as one query string turns all of it into a single name that matches nothing. metadata.goog is refused too, by asking the list browsing already uses rather than keeping a second copy here. It is Google's own short alias for the metadata server and it carries a dot and none of the suffixes this check lists, so it read as an ordinary vendor name, while the long spelling was refused only incidentally by the .internal test.
beardthelion
force-pushed
the
fix/custom-mcp-url-secret-in-query
branch
from
August 24, 2026 18:38
96d0fc3 to
604e8d9
Compare
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
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.
Closes #228.
The userinfo refusal that landed in #206 closed one spelling of a credential in an MCP server address and left the two beside it open.
?token=,?api_key=and the fragment were all accepted, andaddCustomServerwrites the string it was given intomcp_servers.urland into theconfiguration.changedaudit payload verbatim. Redaction keys on the field name andurlis not a sensitive one, so the secret lands in an append-only row in clear text. The fragment never reaches the server, which is why it is not a request-forgery concern and is still a disclosure one: what this rule is about is where the string ends up, not where the request goes.The name is read rather than matched against a list. I wrote the exact-name version first and it refused
tokenwhile acceptingauth_token,api_token,session_token,access_key,secret_key,private_key,x-api-key,subscription-key,X-Amz-Signature,bearerandpwd, which is a corner of the class rather than the class, and an operator has no way to know which spellings a check happens to hold. It over-refuses in one direction deliberately: a misread parameter costs a rename, a missed one cannot be deleted afterwards.?workspace=acme&version=2and a plain#sectionare unaffected, and so are the near misses that make the rule worth testing, sincekeywordis not a key andauthoris not auth.The fragment is split at the first question mark before it is read, because a hash route or an OAuth-style callback puts a path in front of the parameters and reading the whole fragment as one query string turns all of it into a single name that matches nothing.
metadata.googis refused by askingisNeverAllowedHostname, which browsing already uses, rather than keeping a second list here. That list already held the alias along with the ECS and Alibaba addresses, and a new one added there should not have to be remembered here as well. Worth being accurate about the reach: Google documents the short alias over http and this check already requires https, so it closes the name rather than a live path today. The long spelling was only ever refused incidentally, by the.internalsuffix.Verification. Every case executed rather than reasoned. Each test group was observed failing before its implementation existed. 19 must-refuse and 6 must-accept URLs run directly against the function. Two mutations: neutering the metadata call fails that test, and making the shared predicate return false fails ten existing navigation and computer-address tests, which is what proves the two original call sites in
target.tsare still covered after being pointed at the new helper. Full suite 1427 pass, 0 fail against a real database; build, typecheck, lint and format clean.