Refuse the MCP server addresses this check already means to refuse - #206
Conversation
24cba4f to
175a573
Compare
customUrlRefusal is the floor for a URL an administrator types, and it compares the literal hostname. A trailing dot is the root-anchored form of the same name and resolves to the same place, but it changes the string, so every rule in the function missed it: "localhost." is not "localhost", "vault.internal." does not end in ".internal", and "database." picks up the dot that the single-label test keys on. The fully qualified spelling of every address this refuses went through, cloud metadata included. Stripped where the host is read rather than added to each comparison, so a rule added later inherits it. All trailing dots, not the last one, since taking a single dot off "localhost.." leaves a string that still matches nothing. Also refuse .svc, which is how a Kubernetes service is addressed from inside the cluster. It carries dots and none of the other suffixes, so it read as an ordinary vendor name. .cluster.local was already caught by the .local rule. The test file already enumerated this input class and had the trailing-dot form of none of it.
Userinfo is not part of the host, so none of the host rules looked at it and https://oauth:secret@vendor.example/mcp was accepted. What is typed there does not stay in the form: addCustomServer writes the string it was given into mcp_servers.url and into the configuration.changed audit payload, and audit redaction keys on the field name rather than the value, so "url" is not sensitive and the secret sits in the trail in clear text. Refused rather than stripped. Stripping would quietly accept an address the administrator typed a credential into and leave them believing it was used; there is a token field for this, and the message points at it. The refusal does not echo the URL, since it is rendered to the administrator and can reach a log. There is a test for that, because it is the kind of thing a later edit undoes without noticing. addCustomServer is the only path that takes a URL from a caller; the catalogue path resolves its own and checks a per-instance host against an anchored pattern, so the guard is the whole surface.
The credential case changes what a deployment should do, not just how the form behaves: the trail is append-only, so a token already written there cannot be removed and has to be rotated. Said so in as many words.
175a573 to
7969499
Compare
davidmckayv
left a comment
There was a problem hiding this comment.
Drove every case against customUrlRefusal on main before reading the fix, and all six are accepted today:
ACCEPTED https://localhost./mcp
ACCEPTED https://database./mcp
ACCEPTED https://vault.internal./mcp
ACCEPTED https://metadata.google.internal./computeMetadata/v1/
ACCEPTED https://kubernetes.default.svc/mcp
ACCEPTED https://oauth:hunter2@mcp.example.com/mcp
With this, all six refuse and https://mcp.example.com/mcp still passes, so it is not over-refusing.
The trailing dot is the one I would not have thought of: a fully-qualified name with the root label spelled out resolves identically and compares unequal, so every host check in that function was one character from being bypassed — including the cloud metadata address, which the file goes out of its way to say nothing may reach.
The userinfo case is the sharpest, and for a reason beyond request forgery: addCustomServer writes input.url verbatim into mcp_servers.url and into the configuration.changed audit payload, and url is not in sensitiveKeys. So a credential in the address ends up in an append-only trail in clear text, where it cannot be removed by design. Telling somebody to put it in the token field is the right refusal.
Two things left for later, neither blocking:
- the same reasoning leaves a token in a query string (
?token=…) stored verbatim in both places; metadata.goog— Google's shorter metadata alias — is not on the never-allowed list.
Both are the same shape as what this closes and worth a follow-up.
Checks: typecheck, lint, format clean; catalogue suite 26 pass; CI green.
Closes #205.
What this changes
customUrlRefusalis the floor for a URL an administrator types into "add an MCP server", and itcompares the literal hostname. Three spellings of an address it means to refuse were getting past it.
A trailing dot is the root-anchored form of the same name and resolves to the same place, but it
changes the string, so every rule missed it:
"localhost."is not"localhost","vault.internal."does not end in".internal", and"database."picks up the dot that thesingle-label test keys on. The fully qualified spelling of every address this function refuses went
through,
metadata.google.internal.included. It is stripped where the host is read rather thanadded to each comparison, so a rule added later inherits it, and all trailing dots rather than the
last one, because taking a single dot off
"localhost.."leaves a string that still matches nothing..svcjoins the suffix list. It is how a Kubernetes service is addressed from inside thecluster, and it carries dots and none of the other suffixes, so it read as an ordinary vendor name.
.cluster.localwas already caught by the.localrule.A credential in the address is now refused. Userinfo is not part of the host, so no host rule
looked at it, and what is typed there does not stay in the form:
addCustomServerwrites the stringit was given into
mcp_servers.urland into theconfiguration.changedaudit payload, and auditredaction keys on the field name rather than the value, so
urlis not sensitive and the secret isstored verbatim. Refused rather than stripped, because stripping would quietly accept an address
somebody typed a credential into and leave them believing it was used; there is a token field for
this and the message points at it. The refusal does not echo the URL back, since it is rendered to
the administrator and can reach a log, and there is a test holding that.
Two commits, the host checks and the credential separately, because the second one is a different
question about a different part of the URL and has an audit consequence the first does not.
addCustomServeris the only path that takes a URL from a caller; the catalogue path resolves itsown and checks a per-instance host against an anchored pattern, so this function is the whole
surface. A DNS name whose A record points inside the network still passes, which the docblock scopes
out on purpose and which is a different change.
Where it runs
customUrlRefusalis a pure function of thestring it is handed, called before anything is written.
decision depends only on the argument.
existing write proceeds.
administrator's own request made.
Boundary and audit
this sits in front of a configuration write, not an acting call.
CustomServerRefusedErrorbefore the insert, the same shape the existing refusals in this function already take, so a
rejected address is reported to the caller and no
configuration.changedrow is written for aserver that was never added. That is the behaviour being restored: the row that used to be
written here is the one carrying the leaked credential.
rather than adding it.
Changelog
CHANGELOG.mdunderUnreleased. It names the credential case specifically, becausethe trail is append-only and a token already written there cannot be deleted, so a deployment
where somebody has done this has to rotate rather than clean up.
Proof
Against a migrated Postgres, driving the real
addCustomServerand then reading the rows back withSQL. Five cases, the last one a control so that "no row" means refused rather than a broken harness:
The same probe with this branch's
catalogue.tsreverted tomain, which is what makes the zerosmean something:
Deleting that audit row is refused by
prevent_audit_event_mutation(), which is the append-onlytrigger doing its job and is why the changelog says rotate.
Unit side: 5 new tests, each RED before its fix. Each of the three parts is separately load-bearing
by mutation, and each turns exactly its own test red and nothing else: reverting the multi-dot strip
to a single dot reds the FQDN case, dropping
.svcreds the cluster case, dropping the userinfobranch reds both credential cases. A 22-case matrix covers uppercase spellings,
.svcwith atrailing dot, password-only and percent-encoded userinfo, and the decimal, hex and octal IP forms,
alongside five ordinary vendor URLs with ports, paths, queries and subdomains that must still pass,
and they do.
Suite: 789 non-integration server tests and 185 integration tests, 0 failures, plus 199 across
agent-computer,supervisorandshared. Typecheck clean on all four workspaces, lint and formatclean.