Refuse a credential written into the rest of the address

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
 

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:

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.";
   }

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.");
   });