From 045d481cd1cddb71a82e8b7f9371bc3f0e03fb49 Mon Sep 17 00:00:00 2001 From: bitfathers94 <237535319+bitfathers94@users.noreply.github.com> Date: Fri, 31 Jul 2026 10:01:24 +0000 Subject: [PATCH] fix(mcp): exclude admin tools from a self-host discovery card when the admin surface is off toolsForDeployment only filtered on availability, so a self-hosted deployment's .well-known documents advertised the five admin tools even when LOOPOVER_MCP_ADMIN_ENABLED is unset and /mcp never registers them. Mirror the same registration gate createServer uses, read it at request time alongside the deployment, and fold it into the discovery memo key so the two documents don't leak into each other on one isolate. --- src/api/routes.ts | 8 +- src/mcp/discovery-routes.ts | 26 ++++-- src/mcp/server.ts | 6 +- test/integration/mcp-discovery-routes.test.ts | 82 +++++++++++++++++++ test/unit/mcp-discovery-surfaces.test.ts | 14 ++-- 5 files changed, 120 insertions(+), 16 deletions(-) diff --git a/src/api/routes.ts b/src/api/routes.ts index 33025838bd..6c8b99b5cd 100644 --- a/src/api/routes.ts +++ b/src/api/routes.ts @@ -176,7 +176,7 @@ import { validateOrbRelayEnrollment, } from "../orb/relay"; import { computeFleetAnalytics } from "../orb/analytics"; -import { handleMcpRequest } from "../mcp/server"; +import { handleMcpRequest, isMcpAdminEnabled } from "../mcp/server"; import { simulateOpenPrPressureSchema } from "../mcp/server"; import { simulateOpenPrPressure, type OpenPrPressureInput } from "../services/open-pr-pressure-scenarios"; import { DISCOVERY_PATHS, discoveryDocumentsFor, respondWithDocument, toolsForDeployment } from "../mcp/discovery-routes"; @@ -644,11 +644,15 @@ export function createApp() { // app (src/server.ts serves this very Hono instance), so the deployment has to be read at request // time rather than assumed. const deployment = isSelfHostedReviewRuntime(c.env) ? "selfhost" : "cloud"; + // #10039: same request-time read `createServer()` uses to gate admin-tool REGISTRATION, so a + // self-host card never advertises the five admin tools when the flag that would register them is off. + const adminEnabled = isMcpAdminEnabled(c.env); const documents = discoveryDocumentsFor({ version: LATEST_RECOMMENDED_MCP_VERSION, deployment, + adminEnabled, baseUrl: c.env.PUBLIC_API_ORIGIN ?? new URL(c.req.url).origin, - tools: toolsForDeployment(deployment), + tools: toolsForDeployment(deployment, adminEnabled), }); return respondWithDocument(documents[path]!, c.req.header("if-none-match") ?? null); }); diff --git a/src/mcp/discovery-routes.ts b/src/mcp/discovery-routes.ts index 819c5de702..fe1ebef646 100644 --- a/src/mcp/discovery-routes.ts +++ b/src/mcp/discovery-routes.ts @@ -8,7 +8,8 @@ // mounts the SAME routes over its own availability-filtered tool list. That is what makes a self-hosted // card truthful rather than a copy of the cloud one — a `cloud`-only tool is absent from a self-host card // because it is absent from that deployment's list, not because a second implementation remembered to -// exclude it. +// exclude it. The same truthfulness applies to a tool that is available but not REGISTERED (#10039's +// admin category): a self-host card omits it too, because it is absent from what `/mcp` actually serves. import { buildAgentToolsIndex, buildAnthropicTools, @@ -26,19 +27,31 @@ export type DiscoveryContext = { deployment: DiscoveryDeployment; baseUrl: string; tools: readonly McpToolDefinition[]; + /** Whether THIS deployment currently registers the "admin" category (#10039's `isMcpAdminEnabled`). + * Carried on the context (rather than re-derived from `tools`) so it can also feed the memo key below. */ + adminEnabled: boolean; }; /** - * The tools a deployment truthfully serves: `both` plus its own kind. + * The tools a deployment truthfully serves: `both` plus its own kind, minus whatever it does not actually + * REGISTER (#10039). * * Locality is deliberately NOT filtered here. A `local-git` tool is still part of the catalog a client * discovers — the remote simply expects the caller to supply the branch metadata rather than reading a * checkout — so hiding it would under-describe the server. + * + * `availability` is not the only thing that decides whether a deployment serves a tool: "admin" is + * registered in `createServer()` only when `isMcpAdminEnabled(env)` is true, so a card built without regard + * to that flag would advertise five tools `/mcp` refuses as unknown on a default self-host deployment. This + * mirrors that same registration condition rather than re-deriving a category allowlist, so a second + * conditionally-registered category needs only a change here, not a new hardcoded filter at each caller. */ -export function toolsForDeployment(deployment: DiscoveryDeployment): McpToolDefinition[] { +export function toolsForDeployment(deployment: DiscoveryDeployment, adminEnabled: boolean): McpToolDefinition[] { // `both` is not listed: the registry's filter treats it as the ABSENCE of a restriction, so it already // satisfies either constraint. Naming it here would read as if it were a third deployment. - return listToolDefinitions({ availability: [deployment] }); + const tools = listToolDefinitions({ availability: [deployment] }); + if (adminEnabled) return tools; + return tools.filter((tool) => tool.category !== "admin"); } /** @@ -100,7 +113,10 @@ export function respondWithDocument(document: DiscoveryDocument, ifNoneMatch: st const DOCUMENT_CACHE = new Map>(); export function discoveryDocumentsFor(context: DiscoveryContext): Record { - const key = `${context.deployment}|${context.version}|${context.baseUrl}`; + // `adminEnabled` rides along: without it, the first request on an isolate would pick a tool list and the + // memo would keep serving it to every later request on the same (deployment, version, baseUrl), even one + // that arrives after the flag flips. + const key = `${context.deployment}|${context.version}|${context.baseUrl}|${context.adminEnabled}`; let documents = DOCUMENT_CACHE.get(key); if (!documents) { documents = buildDiscoveryDocuments(context); diff --git a/src/mcp/server.ts b/src/mcp/server.ts index d279e6854a..a0a60a1aa7 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -785,8 +785,10 @@ void _INTERNAL_JOB_MESSAGE_TYPES_ARE_REAL; /** Master opt-in for the "admin" tool category (#7721), default OFF. Same truthy-string convention as every * other LOOPOVER_* flag in this repo. Gates tool REGISTRATION in createServer() below; each admin tool - * handler additionally requires actor === "mcp-admin" at call time regardless of this flag. */ -function isMcpAdminEnabled(env: Env): boolean { + * handler additionally requires actor === "mcp-admin" at call time regardless of this flag. Exported so the + * `.well-known` discovery routes (#10039) can mirror this exact registration gate instead of growing a + * second copy of the truthy-string regex. */ +export function isMcpAdminEnabled(env: Env): boolean { return /^(1|true|yes|on)$/i.test((env.LOOPOVER_MCP_ADMIN_ENABLED ?? "").trim()); } diff --git a/test/integration/mcp-discovery-routes.test.ts b/test/integration/mcp-discovery-routes.test.ts index 4f98cfc224..88ce85c956 100644 --- a/test/integration/mcp-discovery-routes.test.ts +++ b/test/integration/mcp-discovery-routes.test.ts @@ -122,3 +122,85 @@ describe("discovery routes (#9526)", () => { expect(card.remotes[0]!.url).toBe("https://api.loopover.ai/mcp"); }); }); + +// #10039: a self-host card must describe only what THIS deployment's /mcp actually registers. "admin" is +// the one category gated behind LOOPOVER_MCP_ADMIN_ENABLED (createServer's isMcpAdminEnabled) rather than +// availability alone, so a card built from availability filtering only would advertise five tools the +// server refuses as unknown on a default (flag-unset) self-host deployment. +describe("admin-tool exclusion from a self-host card when the admin surface is not enabled (#10039)", () => { + const ADMIN_TOOL_NAMES = [ + "loopover_admin_get_config", + "loopover_admin_write_config", + "loopover_admin_list_config_backups", + "loopover_admin_trigger_redeploy", + "loopover_admin_rotate_secret", + ]; + + it("lists none of the five admin tools when LOOPOVER_MCP_ADMIN_ENABLED is unset on a self-host env", async () => { + const env = createTestEnv(); + expect(env.SELFHOST_TRANSIENT_CACHE, "the test env is the self-host runtime").toBeTruthy(); + expect(env.LOOPOVER_MCP_ADMIN_ENABLED, "the flag defaults off").toBeFalsy(); + + const card = (await (await app.fetch(new Request("https://api.loopover.ai/.well-known/mcp.json"), env)).json()) as { + tools: Array<{ name: string }>; + }; + const index = (await (await app.fetch(new Request("https://api.loopover.ai/.well-known/agent-tools/index.json"), env)).json()) as { + tools: Array<{ name: string }>; + }; + const cardNames = card.tools.map((tool) => tool.name); + const indexNames = index.tools.map((tool) => tool.name); + for (const name of ADMIN_TOOL_NAMES) { + expect(cardNames, `${name} must not be on the flag-off self-host card`).not.toContain(name); + expect(indexNames, `${name} must not be on the flag-off self-host index`).not.toContain(name); + } + }); + + it("lists all five admin tools when LOOPOVER_MCP_ADMIN_ENABLED=1 on the same self-host env", async () => { + const env = createTestEnv({ LOOPOVER_MCP_ADMIN_ENABLED: "1" }); + expect(env.SELFHOST_TRANSIENT_CACHE, "the test env is the self-host runtime").toBeTruthy(); + + const card = (await (await app.fetch(new Request("https://api.loopover.ai/.well-known/mcp.json"), env)).json()) as { + tools: Array<{ name: string }>; + }; + const index = (await (await app.fetch(new Request("https://api.loopover.ai/.well-known/agent-tools/index.json"), env)).json()) as { + tools: Array<{ name: string }>; + }; + const cardNames = card.tools.map((tool) => tool.name); + const indexNames = index.tools.map((tool) => tool.name); + for (const name of ADMIN_TOOL_NAMES) { + expect(cardNames, `${name} must be on the flag-on self-host card`).toContain(name); + expect(indexNames, `${name} must be on the flag-on self-host index`).toContain(name); + } + }); + + it("the flag-on and flag-off documents do not leak through the memo, with no reset needed", async () => { + // Same module instance, same (deployment, version, baseUrl) -- only the flag differs. Deliberately does + // NOT call resetDiscoveryCacheForTesting between the two requests: that would mask a memo key that + // forgot to carry the flag, since a fresh cache always misses regardless. + const off = await app.fetch(new Request("https://api.loopover.ai/.well-known/mcp.json"), createTestEnv()); + const on = await app.fetch(new Request("https://api.loopover.ai/.well-known/mcp.json"), createTestEnv({ LOOPOVER_MCP_ADMIN_ENABLED: "1" })); + + const offBody = await off.text(); + const onBody = await on.text(); + expect(onBody).not.toBe(offBody); + expect(on.headers.get("etag")).not.toBe(off.headers.get("etag")); + + const offNames = (JSON.parse(offBody) as { tools: Array<{ name: string }> }).tools.map((tool) => tool.name); + const onNames = (JSON.parse(onBody) as { tools: Array<{ name: string }> }).tools.map((tool) => tool.name); + for (const name of ADMIN_TOOL_NAMES) { + expect(offNames).not.toContain(name); + expect(onNames).toContain(name); + } + }); + + it("does not affect the cloud deployment's documents, which never listed the selfhost-only admin tools", async () => { + const cloudEnv = { ...createTestEnv({ LOOPOVER_MCP_ADMIN_ENABLED: "1" }), SELFHOST_TRANSIENT_CACHE: undefined } as unknown as Env; + const card = (await (await app.fetch(new Request("https://api.loopover.ai/.well-known/mcp.json"), cloudEnv)).json()) as { + deployment: string; + tools: Array<{ name: string }>; + }; + expect(card.deployment).toBe("cloud"); + const names = card.tools.map((tool) => tool.name); + for (const name of ADMIN_TOOL_NAMES) expect(names).not.toContain(name); + }); +}); diff --git a/test/unit/mcp-discovery-surfaces.test.ts b/test/unit/mcp-discovery-surfaces.test.ts index b31555086c..d4decbff26 100644 --- a/test/unit/mcp-discovery-surfaces.test.ts +++ b/test/unit/mcp-discovery-surfaces.test.ts @@ -30,7 +30,7 @@ import { buildOpenApiSpec } from "../../src/openapi/spec"; // that is answering, it is stable enough to cache, and it never under- or over-states the tool set. const TOOLS = listToolDefinitions({ availability: ["cloud"] }); -const CONTEXT = { version: "3.15.2", deployment: "cloud" as const, baseUrl: "https://api.loopover.ai", tools: TOOLS }; +const CONTEXT = { version: "3.15.2", deployment: "cloud" as const, baseUrl: "https://api.loopover.ai", tools: TOOLS, adminEnabled: false }; beforeEach(() => { resetDiscoveryCacheForTesting(); @@ -38,8 +38,8 @@ beforeEach(() => { describe("availability filtering (#9526)", () => { it("a cloud card excludes selfhost-only tools, and a selfhost card excludes cloud-only ones", () => { - const cloud = new Set(toolsForDeployment("cloud").map((tool) => tool.name)); - const selfhost = new Set(toolsForDeployment("selfhost").map((tool) => tool.name)); + const cloud = new Set(toolsForDeployment("cloud", false).map((tool) => tool.name)); + const selfhost = new Set(toolsForDeployment("selfhost", false).map((tool) => tool.name)); // The registry's availability filter is INCLUSIVE (`both` satisfies any constraint), so "only" has to // be derived from the raw field rather than by filtering. @@ -54,8 +54,8 @@ describe("availability filtering (#9526)", () => { it("both deployments carry every `both` tool", () => { const shared = listToolDefinitions().filter((tool) => tool.availability === "both").map((tool) => tool.name); - const cloud = new Set(toolsForDeployment("cloud").map((tool) => tool.name)); - const selfhost = new Set(toolsForDeployment("selfhost").map((tool) => tool.name)); + const cloud = new Set(toolsForDeployment("cloud", false).map((tool) => tool.name)); + const selfhost = new Set(toolsForDeployment("selfhost", false).map((tool) => tool.name)); for (const name of shared) { expect(cloud.has(name)).toBe(true); expect(selfhost.has(name)).toBe(true); @@ -67,7 +67,7 @@ describe("availability filtering (#9526)", () => { // reading a checkout. Hiding them would under-describe the server. const localGit = listToolDefinitions({ locality: ["local-git"] }).filter((tool) => tool.availability === "both").map((tool) => tool.name); expect(localGit.length).toBeGreaterThan(0); - const cloud = new Set(toolsForDeployment("cloud").map((tool) => tool.name)); + const cloud = new Set(toolsForDeployment("cloud", false).map((tool) => tool.name)); for (const name of localGit) expect(cloud.has(name)).toBe(true); }); }); @@ -215,7 +215,7 @@ describe("the per-origin memo (#9526)", () => { it("keeps deployments separate, so a self-host card is not a copy of the cloud one", () => { const cloud = JSON.parse(discoveryDocumentsFor(CONTEXT)["/.well-known/mcp.json"]!.body); const selfhost = JSON.parse( - discoveryDocumentsFor({ ...CONTEXT, deployment: "selfhost", tools: toolsForDeployment("selfhost") })["/.well-known/mcp.json"]!.body, + discoveryDocumentsFor({ ...CONTEXT, deployment: "selfhost", tools: toolsForDeployment("selfhost", false) })["/.well-known/mcp.json"]!.body, ); expect(selfhost.deployment).toBe("selfhost"); expect(selfhost.tools.length).not.toBe(cloud.tools.length);