From 5b310b3fe750bea82c09fa78f03cfd7172889ac8 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sun, 23 Aug 2026 17:12:04 -0500 Subject: [PATCH 1/3] See through the fully qualified spelling of a name we already refuse 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. --- server/src/plugins/catalogue.ts | 10 +++++++++- server/tests/plugin-catalogue.test.ts | 28 +++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/server/src/plugins/catalogue.ts b/server/src/plugins/catalogue.ts index c63140f8..02733d0c 100644 --- a/server/src/plugins/catalogue.ts +++ b/server/src/plugins/catalogue.ts @@ -276,7 +276,12 @@ export function customUrlRefusal(raw: string): string | null { return "An MCP server must be reached over https."; } - const host = url.hostname.toLowerCase(); + // 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 +294,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..46a9a9c8 100644 --- a/server/tests/plugin-catalogue.test.ts +++ b/server/tests/plugin-catalogue.test.ts @@ -232,6 +232,34 @@ 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("nonsense is refused rather than thrown", () => { expect(customUrlRefusal("not a url")).toBe("That is not a URL."); }); From 10e0afa687ae03fe886e0f9add5cb54d3946029b Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sun, 23 Aug 2026 17:12:18 -0500 Subject: [PATCH 2/3] Refuse a credential typed into the MCP server address 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. --- server/src/plugins/catalogue.ts | 9 +++++++++ server/tests/plugin-catalogue.test.ts | 24 ++++++++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/server/src/plugins/catalogue.ts b/server/src/plugins/catalogue.ts index 02733d0c..3c81ef4d 100644 --- a/server/src/plugins/catalogue.ts +++ b/server/src/plugins/catalogue.ts @@ -276,6 +276,15 @@ export function customUrlRefusal(raw: string): string | null { return "An MCP server must be reached over https."; } + // 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 diff --git a/server/tests/plugin-catalogue.test.ts b/server/tests/plugin-catalogue.test.ts index 46a9a9c8..a122a5a7 100644 --- a/server/tests/plugin-catalogue.test.ts +++ b/server/tests/plugin-catalogue.test.ts @@ -260,6 +260,30 @@ describe("a URL an administrator typed", () => { ).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."); }); From 79694994c23548a22c58b15e4a8915f0ce3d5b0a Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sun, 23 Aug 2026 17:24:40 -0500 Subject: [PATCH 3/3] Note the tightened MCP server address check in the changelog 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. --- CHANGELOG.md | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) 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