diff --git a/CHANGELOG.md b/CHANGELOG.md index cbee9e84..e28efca2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,6 +59,32 @@ this port has to reach it another way**, which is what publishing it on every in This does not reach back in time. A deployment that has been running with the two on one network should assume a Bot could have read or written the database, and look at the trail with that in mind. +### A credential in an MCP server address is refused in the query and the fragment too + +Refusing `https://user:token@vendor.example/mcp` closed the userinfo spelling of a credential in the +address and left the two obvious ones open. `?token=`, `?api_key=` and their neighbours were still +accepted, and the address is stored and named in the trail exactly as given: audit redaction keys on +the field name, `url` is not a sensitive one, so the secret was written to `mcp_servers` and to an +append-only audit row in clear text. That is the same disclosure the userinfo rule exists to prevent, +one character away. + +A parameter whose name reads as a credential is now refused, in the query string and in the fragment, +and the refusal points at the token field without repeating what was typed. The name is read rather +than matched against a list, so `?auth_token=`, `?x-api-key=` and `?X-Amz-Signature=` are refused +alongside `?token=`: a rule that only catches the spellings somebody thought of reads as a guard +while behaving like a gap. The test is on the parameter name rather than on the presence of a query, +because vendors route and version with parameters and a floor that refused every one of them would +be one an operator works around instead of with. `https://mcp.example.com/mcp?workspace=acme&version=2` +is unaffected, and so is an ordinary fragment. A credential written into the *path* is still +accepted: it is indistinguishable from a route, and at least one hosted provider addresses servers +that way. **A deployment where somebody has put a credential in an address should treat it as +disclosed and rotate it**, for the same reason as before: the audit row cannot be deleted. + +`metadata.goog` is refused too. It is Google's own short name for the metadata server, published +beside `metadata.google.internal`, and it carries a dot and none of the suffixes this check lists, so +it read as an ordinary vendor name. The long spelling was only ever refused incidentally, by the +`.internal` rule. Both are now named, so the address this check was written for is refused on purpose +rather than by luck. ### Name the private addresses an agent may live at diff --git a/server/src/computer/target.ts b/server/src/computer/target.ts index a0874141..0304b9a9 100644 --- a/server/src/computer/target.ts +++ b/server/src/computer/target.ts @@ -39,6 +39,20 @@ const NEVER_ALLOWED_HOSTNAMES = new Set([ "100.100.100.200", ]); +/** + * Is this the address of a cloud metadata service? + * + * Exported because the same question is asked outside browsing: an MCP server address an + * administrator types is refused on the same grounds, and the answer has to come from one list. + * Two copies drift, and the copy that misses an alias is the one that lets a credential endpoint + * through. + * + * Canonicalises first, so the trailing-dot and IPv6 spellings are seen through here as well. + */ +export function isNeverAllowedHostname(hostname: string): boolean { + return NEVER_ALLOWED_HOSTNAMES.has(canonicalHostname(hostname.toLowerCase())); +} + /** Hostnames inside the deployment. Reachable only when a deployment opts in. */ const INTERNAL_HOSTNAMES = new Set([ "localhost", @@ -204,9 +218,7 @@ export function checkComputerAddress(raw: string): TargetVerdict { // Canonicalised for the same reason navigation is: the address reaches a fetch either way, so the // spellings that gate has to see through are the spellings this one has to see through. - if ( - NEVER_ALLOWED_HOSTNAMES.has(canonicalHostname(url.hostname.toLowerCase())) - ) { + if (isNeverAllowedHostname(url.hostname)) { return { allowed: false, reason: @@ -244,7 +256,7 @@ export function checkNavigationTarget( const hostname = canonicalHostname(url.hostname.toLowerCase()); // Checked before the opt-in, so no configuration can reach it. - if (NEVER_ALLOWED_HOSTNAMES.has(hostname)) { + if (isNeverAllowedHostname(hostname)) { return { allowed: false, reason: diff --git a/server/src/plugins/catalogue.ts b/server/src/plugins/catalogue.ts index 3c81ef4d..1252642d 100644 --- a/server/src/plugins/catalogue.ts +++ b/server/src/plugins/catalogue.ts @@ -26,6 +26,9 @@ * These are where this deployment sends a person's authorization code and receives the refresh * token that stands in for their access, so they are a reviewed source contract too. */ +// The one place browsing and this check agree on: the addresses that hold the deployment's own +// cloud credentials. `target.ts` imports nothing itself, so asking it here adds no dependency. +import { isNeverAllowedHostname } from "../computer/target"; // Type-only, so naming the transport here creates no import cycle with the registry that resolves it. import type { TransportKind } from "./transport"; @@ -249,6 +252,61 @@ export function classifyTool( return entry.writeTools.includes(toolName) ? "write" : "read"; } +/** + * Words that make a parameter name a credential, wherever they appear in it. + * + * A containment test rather than a list of exact names, because the exact-name version of this rule + * refused `?token=` and accepted `?auth_token=`, `?api_token=`, `?session_token=` and every other + * spelling one word away. An operator has no way to know which of those the check happens to hold, + * so a rule that only refuses the names somebody thought of reads as a guard while behaving like a + * gap. + * + * Not shared with `sensitiveKeys` in `audit.ts`: that module reaches the database and this function + * deliberately imports nothing that does. The two also want different contents, since audit redacts + * `content`, `prompt` and `result`, which are payload field names and mean nothing here. + */ +const CREDENTIAL_WORDS = [ + "token", + "secret", + "password", + "passwd", + "credential", + "signature", + "bearer", +]; + +/** + * Names that are a credential on their own but are too short to contain safely. + * + * `sig` is the reason this list is separate from the one above: "design" contains it. These are + * compared whole, so an ordinary word carrying the same three letters is left alone. + */ +const CREDENTIAL_NAMES = new Set([ + "auth", + "authorization", + "pass", + "pwd", + "sig", +]); + +/** + * Does this parameter name say it holds a credential? + * + * Names are compared with their separators dropped, so `api_key`, `apiKey` and `x-api-key` are one + * question rather than three. A name ending in "key" is a credential and a name merely containing it + * is not, which is what keeps `keyword` and `monkey` apart; "author" is likewise not "auth". + * + * It over-refuses in one direction on purpose. A parameter this rule misreads costs an operator a + * rename, and one it misses is written to an append-only audit row that cannot be deleted. + */ +function readsAsCredential(name: string): boolean { + const normalized = name.replaceAll(/[^a-zA-Z0-9]/g, "").toLowerCase(); + if (CREDENTIAL_NAMES.has(normalized) || normalized.endsWith("key")) { + return true; + } + return CREDENTIAL_WORDS.some((word) => normalized.includes(word)); +} + /** * Is this a URL an administrator may point the deployment at? * @@ -285,6 +343,32 @@ export function customUrlRefusal(raw: string): string | null { return "Put the credential in the token field rather than in the address."; } + /* + * The query is the other half of the same hole, and the fragment is the half after that. + * + * No host rule below reads either one, and both are stored and audited with the rest of the + * string, so a token written here is as durable and as readable as one written into the userinfo. + * The fragment never reaches the server at all, 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 test is on the parameter name rather than on the presence of a query, because vendors + * legitimately route and version with parameters. A floor that refused every one of them would be + * one an operator works around rather than with, and an ordinary `#section` is left alone for the + * same reason. + */ + const hash = url.hash.replace(/^#/, ""); + const marker = hash.indexOf("?"); + const fragment = + marker === -1 ? [hash] : [hash.slice(0, marker), hash.slice(marker + 1)]; + const named = [ + ...url.searchParams.keys(), + ...fragment.flatMap((part) => [...new URLSearchParams(part).keys()]), + ]; + if (named.some(readsAsCredential)) { + return "Put the credential in the token field rather than in the address."; + } + // A trailing dot is the root-anchored spelling of the same name and resolves to the same place, so // they are stripped here rather than added to each comparison below. Without it "localhost." // misses the equality test, "vault.internal." misses the suffix tests, and "database." picks up @@ -296,6 +380,20 @@ export function customUrlRefusal(raw: string): string | null { if (host.includes(":") || /^[0-9.]+$/.test(host)) { return "Give a hostname rather than an IP address."; } + /* + * The cloud metadata endpoint, by name rather than by luck. + * + * `metadata.goog` is Google's own short alias for it, published beside `metadata.google.internal`, + * and it carries a dot and none of the suffixes below, so it read as an ordinary vendor name. The + * long spelling was refused only incidentally, by the `.internal` test. + * + * Asked of the list browsing already uses rather than a second copy here. That list holds the + * aliases somebody has already had to think about, including the ones Alibaba and ECS answer on, + * and a new alias added there should not have to be remembered here as well. + */ + if (isNeverAllowedHostname(host)) { + return "That address holds this deployment's own cloud credentials."; + } if (host === "localhost" || host.endsWith(".localhost")) { return "That address is local to the deployment."; } diff --git a/server/tests/plugin-catalogue.test.ts b/server/tests/plugin-catalogue.test.ts index a122a5a7..15cf1b9c 100644 --- a/server/tests/plugin-catalogue.test.ts +++ b/server/tests/plugin-catalogue.test.ts @@ -284,6 +284,114 @@ describe("a URL an administrator typed", () => { expect(refusal).not.toContain("oauth"); }); + test("a credential in the query string is refused", () => { + // The same harm as the userinfo case above, reached through the other part of the URL no host + // rule looks at. addCustomServer writes the string it was given into mcp_servers.url and into + // the configuration.changed audit payload, audit redaction keys on the field name, and "url" is + // not a sensitive name, so a token here sits in an append-only trail in clear text. + expect( + customUrlRefusal("https://mcp.example.com/mcp?token=sk-live-abcdef"), + ).not.toBeNull(); + expect( + customUrlRefusal("https://mcp.example.com/mcp?api_key=SECRET"), + ).not.toBeNull(); + expect( + customUrlRefusal("https://mcp.example.com/mcp?access_token=SECRET"), + ).not.toBeNull(); + expect( + customUrlRefusal("https://mcp.example.com/mcp?client_secret=SECRET"), + ).not.toBeNull(); + }); + + test("the names a credential is actually given are refused too", () => { + // The first version of this rule listed exact names, which is a corner of the class rather than + // the class: every one of these was accepted while `?token=` was refused, and an operator does + // not know which spelling the check happens to hold. The match reads the name for what it says. + for (const name of [ + "auth_token", + "api_token", + "apiToken", + "access_key", + "secret_key", + "private_key", + "session_token", + "x-api-key", + "subscription-key", + "X-Amz-Signature", + "bearer", + "pwd", + ]) { + expect( + customUrlRefusal(`https://mcp.example.com/mcp?${name}=s3cret`), + ).not.toBeNull(); + } + }); + + test("an ordinary query parameter is still accepted", () => { + // The rule reads the parameter name, not the presence of a query, because vendors route and + // version with parameters. Refusing every query string would make this floor an outage rather + // than a guard, and an operator who cannot add a working server will find a way around it. + expect( + customUrlRefusal("https://mcp.example.com/mcp?workspace=acme&version=2"), + ).toBeNull(); + // The near misses, which are what a rule that reads names rather than matching them exactly has + // to get right: "keyword" is not a key and "author" is not auth. + expect( + customUrlRefusal("https://mcp.example.com/mcp?keyword=x&author=jane"), + ).toBeNull(); + }); + + test("refusing a credential in the query does not repeat it", () => { + // Same property as the userinfo refusal: this string is rendered to an administrator and can + // reach a log, so it must not carry the secret it exists to reject. + const refusal = customUrlRefusal( + "https://mcp.example.com/mcp?token=s3cret", + ); + expect(refusal).not.toBeNull(); + expect(refusal).not.toContain("s3cret"); + expect(refusal).not.toContain("mcp.example.com"); + }); + + test("a credential in the fragment is refused too", () => { + // The fragment never leaves the browser, but that is not the harm here. addCustomServer stores + // and audits the whole string, so a secret written after the hash is as durable and as readable + // as one in the query. Refusing one and not the other would leave the same bypass a character + // away. + expect( + customUrlRefusal("https://mcp.example.com/mcp#token=s3cret"), + ).not.toBeNull(); + // The shapes a fragment is actually written in. A hash route or an OAuth-style callback puts a + // path before the question mark, and reading the whole fragment as one query string turns all + // of it into a single name that matches nothing. + expect( + customUrlRefusal("https://mcp.example.com/mcp#/callback?token=s3cret"), + ).not.toBeNull(); + expect( + customUrlRefusal("https://mcp.example.com/mcp#!/x?token=s3cret"), + ).not.toBeNull(); + expect( + customUrlRefusal("https://mcp.example.com/mcp#token%3Ds3cret"), + ).not.toBeNull(); + // An ordinary fragment is not a credential and is left alone. + expect(customUrlRefusal("https://mcp.example.com/mcp#section")).toBeNull(); + }); + + test("the short name for the cloud metadata endpoint is refused", () => { + // metadata.goog is Google's own alias for the metadata server, published beside + // metadata.google.internal and 169.254.169.254. It carries a dot and none of the suffixes + // above, so it read as an ordinary vendor name, while the long spelling was caught only + // incidentally by the .internal test. + expect( + customUrlRefusal("https://metadata.goog/computeMetadata/v1/"), + ).not.toBeNull(); + expect( + customUrlRefusal("https://metadata.goog./computeMetadata/v1/"), + ).not.toBeNull(); + expect( + customUrlRefusal("https://METADATA.GOOG/computeMetadata/v1/"), + ).not.toBeNull(); + }); + test("nonsense is refused rather than thrown", () => { expect(customUrlRefusal("not a url")).toBe("That is not a URL."); });