Skip to content

Refuse the MCP server addresses this check already means to refuse - #206

Merged
davidmckayv merged 3 commits into
CopilotKit:mainfrom
beardthelion:fix/custom-mcp-url-floor
Aug 24, 2026
Merged

Refuse the MCP server addresses this check already means to refuse#206
davidmckayv merged 3 commits into
CopilotKit:mainfrom
beardthelion:fix/custom-mcp-url-floor

Conversation

@beardthelion

Copy link
Copy Markdown
Contributor

Closes #205.

What this changes

customUrlRefusal is the floor for a URL an administrator types into "add an MCP server", and it
compares 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 the
single-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 than
added 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.

.svc joins the suffix list. It is how a Kubernetes service is addressed from inside the
cluster, and 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.

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: 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 is
stored 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.

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 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

  • New state that outlives a request? None. customUrlRefusal is a pure function of the
    string it is handed, called before anything is written.
  • What happens on the second replica? Identical. No shared state, no ordering, no cache; the
    decision depends only on the argument.
  • Anything serialised? No. Nothing is written by this path; it only decides whether the
    existing write proceeds.
  • Anything fanned out to a browser? No. The refusal is the return value of the call the
    administrator's own request made.
  • New listener, port, or schedule? None.

Boundary and audit

  • Every acting call still goes through the gateway: resolve, decide, audit, then act. Untouched;
    this sits in front of a configuration write, not an acting call.
  • New refusals and new failures each write a row. The refusal throws CustomServerRefusedError
    before 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.changed row is written for a
    server 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.
  • Nothing new is trusted from the client that the server can resolve itself. This removes trust
    rather than adding it.

Changelog

  • A line in CHANGELOG.md under Unreleased. It names the credential case specifically, because
    the 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 addCustomServer and then reading the rows back with
SQL. Five cases, the last one a control so that "no row" means refused rather than a broken harness:

PASS  https://oauth:SECRET@mcp.example.com/mcp  -> refused (CustomServerRefusedError)   mcp_servers rows: 0
PASS  https://localhost./mcp                    -> refused, local to the deployment     mcp_servers rows: 0
PASS  https://vault.internal../mcp              -> refused, not reachable               mcp_servers rows: 0
PASS  https://kubernetes.default.svc/mcp        -> refused, not reachable               mcp_servers rows: 0
PASS  https://mcp.vendor.example/mcp            -> ACCEPTED                             mcp_servers rows: 1

audit_events rows containing the secret : 0
mcp_servers rows containing the secret  : 0

The same probe with this branch's catalogue.ts reverted to main, which is what makes the zeros
mean something:

audit_events rows containing the secret : 1
mcp_servers rows containing the secret  : 1

  configuration.changed  {"url":"https://oauth:SECRET@mcp.example.com/mcp","change":"mcp_server_added",...}
  mcp_servers            https://oauth:SECRET@mcp.example.com/mcp

Deleting that audit row is refused by prevent_audit_event_mutation(), which is the append-only
trigger 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 .svc reds the cluster case, dropping the userinfo
branch reds both credential cases. A 22-case matrix covers uppercase spellings, .svc with a
trailing 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, supervisor and shared. Typecheck clean on all four workspaces, lint and format
clean.

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.
@beardthelion
beardthelion force-pushed the fix/custom-mcp-url-floor branch from 175a573 to 7969499 Compare August 23, 2026 22:51

@davidmckayv davidmckayv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@davidmckayv
davidmckayv merged commit bc049c1 into CopilotKit:main Aug 24, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The custom MCP server URL check is bypassed by a trailing dot, by .svc, and by a credential in the address

2 participants