diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d824c03..cc0614f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -120,6 +120,28 @@ without a word, because the input path looks for a viewer before it looks for an A close now stops casting only when the socket closing is the one that was casting. +### An MCP server address that points inside the deployment is refused in three more spellings + +Adding an MCP server by URL is checked before the address is stored, because that form is otherwise a +way to point the deployment at its own network. The check compared the literal hostname, and three +spellings of an address it means to refuse were getting through. + +A trailing dot is the root-anchored form of the same name and reaches the same place, but it changed +the string enough that every rule missed it, so `https://localhost./`, `https://printer.local./` and +`https://metadata.google.internal./` were all accepted. `kubernetes.default.svc`, which is how a +service is addressed from inside a cluster, carries dots and none of the listed suffixes, so it read +as an ordinary vendor name. + +The third is worth acting on rather than just noting. A credential typed into the address itself, +`https://user:token@vendor.example/mcp`, was accepted, and the address is stored and named in the +trail as given. Audit redaction works on field names and `url` is not one of the sensitive ones, so +the token was written to `mcp_servers` and to an audit row in clear text. The trail is append-only by +design, so that row cannot be deleted afterwards: **a deployment where somebody has done this should +treat that credential as disclosed and rotate it.** The address field now refuses a credential and +points at the token field instead. + +A deployment that reaches its MCP servers by ordinary vendor hostnames sees no difference. + ## 0.0.4 ### A click citing a ref this deployment cannot resolve is refused diff --git a/server/src/plugins/catalogue.ts b/server/src/plugins/catalogue.ts index c63140f8..3c81ef4d 100644 --- a/server/src/plugins/catalogue.ts +++ b/server/src/plugins/catalogue.ts @@ -276,7 +276,21 @@ export function customUrlRefusal(raw: string): string | null { return "An MCP server must be reached over https."; } - const host = url.hostname.toLowerCase(); + // Userinfo is not part of the host, so none of the host rules below would look at it, and what is + // typed here is stored verbatim: addCustomServer writes the string into mcp_servers.url and into + // the configuration.changed audit payload, whose redaction keys on the field name rather than the + // value. A secret written this way would sit in the trail in clear text. The refusal deliberately + // does not echo the URL back. + if (url.username || url.password) { + 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 + // the dot that the single-label test keys on, so the fully qualified form of every name this + // function refuses walks straight through it. + const host = url.hostname.toLowerCase().replace(/\.+$/, ""); // Bracketed IPv6 arrives with the brackets already stripped by URL, so the colon test catches it. if (host.includes(":") || /^[0-9.]+$/.test(host)) { @@ -289,6 +303,9 @@ export function customUrlRefusal(raw: string): string | null { host.endsWith(".internal") || host.endsWith(".local") || host.endsWith(".localdomain") || + // How a Kubernetes service is addressed from inside the cluster. It carries dots and none of + // the suffixes above, so without this it reads as an ordinary vendor name. + host.endsWith(".svc") || !host.includes(".") ) { return "That address is not reachable from outside this network."; diff --git a/server/tests/plugin-catalogue.test.ts b/server/tests/plugin-catalogue.test.ts index 5836ae6c..a122a5a7 100644 --- a/server/tests/plugin-catalogue.test.ts +++ b/server/tests/plugin-catalogue.test.ts @@ -232,6 +232,58 @@ describe("a URL an administrator typed", () => { expect(customUrlRefusal("https://printer.local/mcp")).not.toBeNull(); }); + test("the fully qualified spelling of those names is refused too", () => { + // A trailing dot is the root-anchored form of the same name and resolves to the same place, so + // every rule above has to see through it. It defeats them in two different ways: the suffix + // tests stop matching because the string now ends in the dot, and "database." acquires the dot + // that the single-label test keys on. + expect(customUrlRefusal("https://localhost./mcp")).not.toBeNull(); + expect(customUrlRefusal("https://database./mcp")).not.toBeNull(); + expect(customUrlRefusal("https://vault.internal./mcp")).not.toBeNull(); + expect(customUrlRefusal("https://printer.local./mcp")).not.toBeNull(); + expect( + customUrlRefusal("https://metadata.google.internal./computeMetadata/v1/"), + ).not.toBeNull(); + // More than one, because stripping a single dot leaves a string that still misses every rule. + expect(customUrlRefusal("https://localhost../mcp")).not.toBeNull(); + expect(customUrlRefusal("https://vault.internal.../mcp")).not.toBeNull(); + }); + + test("an in-cluster service name is refused", () => { + // .svc is how a Kubernetes service is addressed from inside the cluster. It has dots and none + // of the other suffixes, so it reads as an ordinary vendor name. + expect( + customUrlRefusal("https://kubernetes.default.svc/mcp"), + ).not.toBeNull(); + expect( + customUrlRefusal("https://kubernetes.default.svc.cluster.local/mcp"), + ).not.toBeNull(); + }); + + test("a credential in the URL is refused", () => { + // Userinfo is not part of the host, so every rule above passes it, and addCustomServer then + // 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 sensitive, so the secret + // would sit in the trail in clear text. + expect( + customUrlRefusal("https://oauth:s3cret@mcp.example.com/mcp"), + ).not.toBeNull(); + expect( + customUrlRefusal("https://token@mcp.example.com/mcp"), + ).not.toBeNull(); + }); + + test("refusing a credential in the URL does not repeat the credential", () => { + // The refusal is rendered to the administrator and can reach a log, so it must not carry the + // thing it exists to reject. + const refusal = customUrlRefusal( + "https://oauth:s3cret@mcp.example.com/mcp", + ); + expect(refusal).not.toBeNull(); + expect(refusal).not.toContain("s3cret"); + expect(refusal).not.toContain("oauth"); + }); + test("nonsense is refused rather than thrown", () => { expect(customUrlRefusal("not a url")).toBe("That is not a URL."); });