From dea534539747e0befcd6bceb5d13e18da54db64a Mon Sep 17 00:00:00 2001 From: "Bob.Hou" Date: Sun, 23 Aug 2026 01:18:06 -0400 Subject: [PATCH] fix(mcp): honor the mcp:connect carve-out in transport route guards (#11139) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Validated on the combined batch board over tip 17897cc3: gates clean (changelog, file-size 159 frozen, complexity 2628<=2774, cognitive 1184<=1223, dead-code 409<=416, provider-consistency 267/350/0), typecheck:core clean, 83/83 PR suites + neighbors green (mcp-route-scope-carveout 15 tests with bug-injection proof, management-auth-hardening per-route call-shape pinning). The route layer now honors the #9159 mcp:connect carve-out exactly like the policy layer — audit/audit-stats deliberately stay manage-only, oma_ tokens still require admin. Thank you @HouMinXi! --- src/app/api/mcp/sse/route.ts | 4 +- src/app/api/mcp/status/route.ts | 2 +- src/app/api/mcp/stream/route.ts | 6 +- src/app/api/mcp/tools/route.ts | 2 +- src/lib/api/requireManagementAuth.ts | 28 +- tests/unit/management-auth-hardening.test.ts | 50 +++- tests/unit/mcp-route-scope-carveout.test.ts | 265 +++++++++++++++++++ 7 files changed, 345 insertions(+), 12 deletions(-) create mode 100644 tests/unit/mcp-route-scope-carveout.test.ts diff --git a/src/app/api/mcp/sse/route.ts b/src/app/api/mcp/sse/route.ts index 57988e36ee..33dc0b250f 100644 --- a/src/app/api/mcp/sse/route.ts +++ b/src/app/api/mcp/sse/route.ts @@ -30,7 +30,7 @@ async function guardEnabled(): Promise { } export async function GET(request: NextRequest) { - const authError = await requireManagementAuth(request); + const authError = await requireManagementAuth(request, { acceptMcpConnectScope: true }); if (authError) return authError; const blocked = await guardEnabled(); if (blocked) return blocked; @@ -38,7 +38,7 @@ export async function GET(request: NextRequest) { } export async function POST(request: NextRequest) { - const authError = await requireManagementAuth(request); + const authError = await requireManagementAuth(request, { acceptMcpConnectScope: true }); if (authError) return authError; const blocked = await guardEnabled(); if (blocked) return blocked; diff --git a/src/app/api/mcp/status/route.ts b/src/app/api/mcp/status/route.ts index e392b67efb..ecf08ab93f 100644 --- a/src/app/api/mcp/status/route.ts +++ b/src/app/api/mcp/status/route.ts @@ -14,7 +14,7 @@ import { getCachedSettings } from "@/lib/db/settings"; import { requireManagementAuth } from "@/lib/api/requireManagementAuth"; export async function GET(request: Request) { - const authError = await requireManagementAuth(request); + const authError = await requireManagementAuth(request, { acceptMcpConnectScope: true }); if (authError) return authError; try { const [heartbeat, stats, lastCallPage, settings] = await Promise.all([ diff --git a/src/app/api/mcp/stream/route.ts b/src/app/api/mcp/stream/route.ts index c985524985..d07333ae1b 100644 --- a/src/app/api/mcp/stream/route.ts +++ b/src/app/api/mcp/stream/route.ts @@ -33,7 +33,7 @@ async function guardEnabled(): Promise { } export async function POST(request: NextRequest) { - const authError = await requireManagementAuth(request); + const authError = await requireManagementAuth(request, { acceptMcpConnectScope: true }); if (authError) return authError; const blocked = await guardEnabled(); if (blocked) return blocked; @@ -41,7 +41,7 @@ export async function POST(request: NextRequest) { } export async function GET(request: NextRequest) { - const authError = await requireManagementAuth(request); + const authError = await requireManagementAuth(request, { acceptMcpConnectScope: true }); if (authError) return authError; const blocked = await guardEnabled(); if (blocked) return blocked; @@ -49,7 +49,7 @@ export async function GET(request: NextRequest) { } export async function DELETE(request: NextRequest) { - const authError = await requireManagementAuth(request); + const authError = await requireManagementAuth(request, { acceptMcpConnectScope: true }); if (authError) return authError; const blocked = await guardEnabled(); if (blocked) return blocked; diff --git a/src/app/api/mcp/tools/route.ts b/src/app/api/mcp/tools/route.ts index c9f5cc0227..fc5c1acf1e 100644 --- a/src/app/api/mcp/tools/route.ts +++ b/src/app/api/mcp/tools/route.ts @@ -3,7 +3,7 @@ import { MCP_TOOLS, MCP_TOOL_MAP } from "@omniroute/open-sse/mcp-server/schemas/ import { requireManagementAuth } from "@/lib/api/requireManagementAuth"; export async function GET(request: Request) { - const authError = await requireManagementAuth(request); + const authError = await requireManagementAuth(request, { acceptMcpConnectScope: true }); if (authError) return authError; try { return NextResponse.json({ diff --git a/src/lib/api/requireManagementAuth.ts b/src/lib/api/requireManagementAuth.ts index 34ebc25cbf..33112c127b 100644 --- a/src/lib/api/requireManagementAuth.ts +++ b/src/lib/api/requireManagementAuth.ts @@ -8,7 +8,9 @@ import { isTrustedLoopbackInternalServiceRequest } from "@/lib/api/internalServi import { AUTHZ_HEADER_AUTH_KIND, AUTHZ_HEADER_AUTH_LABEL } from "@/server/authz/headers"; import { MANAGE_SCOPE, + MCP_CONNECT_SCOPE, hasManageScope as hasManageScopeShared, + hasMcpConnectOrManageScope, } from "@/shared/constants/managementScopes"; export { MANAGE_SCOPE }; @@ -26,6 +28,13 @@ export function hasManageScope(scopes: string[] = []): boolean { interface RequireManagementAuthOptions { alwaysRequireAuth?: boolean; invalidApiKeyStatus?: 401 | 403; + /** + * Accept the narrow `mcp:connect` scope in the API-key branch, mirroring the + * #9159 carve-out the central managementPolicy already applies to /api/mcp/* + * paths. Only the MCP transport routes (stream/sse/status/tools) may enable + * this — every other management route stays manage/admin-only. + */ + acceptMcpConnectScope?: boolean; } function invalidManagementTokenResponse(options: RequireManagementAuthOptions): Response { @@ -117,11 +126,26 @@ export async function requireManagementAuth( }); } - if (meta && hasManageScope(meta.scopes)) return null; + // API-key branch: with acceptMcpConnectScope (MCP transport routes) the + // #9159 carve-out applies — hasMcpConnectOrManageScope accepts manage, + // admin, and mcp:connect. Without it, the guard stays manage-only. A null + // meta (valid key, metadata unavailable — deleted mid-request) falls + // through to the same 403 as the default path for every caller, keeping + // the error contract uniform. + if ( + meta && + (options.acceptMcpConnectScope + ? hasMcpConnectOrManageScope(meta.scopes) + : hasManageScope(meta.scopes)) + ) { + return null; + } return createErrorResponse({ status: 403, - message: "API key lacks 'manage' scope. Enable it in the API Keys dashboard.", + message: options.acceptMcpConnectScope + ? `API key lacks '${MCP_CONNECT_SCOPE}' (or 'manage') scope. Enable it in the API Keys dashboard.` + : "API key lacks 'manage' scope. Enable it in the API Keys dashboard.", type: "invalid_request", }); } diff --git a/tests/unit/management-auth-hardening.test.ts b/tests/unit/management-auth-hardening.test.ts index 85116bb96e..2d50666548 100644 --- a/tests/unit/management-auth-hardening.test.ts +++ b/tests/unit/management-auth-hardening.test.ts @@ -215,6 +215,9 @@ test("MCP transport and inspection routes require management authentication", () // route must self-enforce. requireManagementAuth covers: CLI machine // token (loopback), dashboard session cookie, and manage-scope API key — // matching the documented bypasses in LOCAL_ONLY_MANAGE_SCOPE_BYPASS_PREFIXES. + // The transport routes (stream/sse/status/tools) pass + // { acceptMcpConnectScope: true }, the route-layer half of the #9159 + // mcp:connect carve-out; audit/audit/stats stay manage-only. const routePaths = [ "src/app/api/mcp/status/route.ts", "src/app/api/mcp/tools/route.ts", @@ -227,12 +230,53 @@ test("MCP transport and inspection routes require management authentication", () for (const routePath of routePaths) { const content = fs.readFileSync(routePath, "utf8"); assert.ok(content.includes('from "@/lib/api/requireManagementAuth"'), routePath); - assert.ok( - content.includes("const authError = await requireManagementAuth(request);"), - routePath + // Transport routes must use the carve-out form; audit routes must use + // the bare form. The per-route shape is pinned exactly — reverting a + // transport route to the bare call fails here. + const isAudit = routePath.includes("/audit"); + const hasOption = content.includes( + "const authError = await requireManagementAuth(request, { acceptMcpConnectScope: true });" ); + // Audit routes must use the BARE call — any second argument at all + // (even an unrelated option) widens their auth surface. + const hasAnyOption = /requireManagementAuth\(request,\s*\{/.test(content); + const hasBare = content.includes("const authError = await requireManagementAuth(request);"); + assert.equal( + hasOption, + !isAudit, + `${routePath} carve-out shape mismatch (hasOption=${hasOption}, audit=${isAudit})` + ); + if (isAudit) { + assert.ok(hasBare, `${routePath} must keep the bare guard call`); + assert.ok(!hasAnyOption, `${routePath} must not pass ANY options to the guard (manage-only)`); + } assert.ok(content.includes("if (authError) return authError;"), routePath); } + + // Carve-out hygiene: only the four transport routes may accept + // mcp:connect; the audit inspection routes remain manage-only. + for (const routePath of [ + "src/app/api/mcp/audit/route.ts", + "src/app/api/mcp/audit/stats/route.ts", + ]) { + const content = fs.readFileSync(routePath, "utf8"); + assert.ok( + !content.includes("acceptMcpConnectScope"), + `${routePath} must stay manage-only (no mcp:connect carve-out)` + ); + } + for (const routePath of [ + "src/app/api/mcp/status/route.ts", + "src/app/api/mcp/tools/route.ts", + "src/app/api/mcp/sse/route.ts", + "src/app/api/mcp/stream/route.ts", + ]) { + const content = fs.readFileSync(routePath, "utf8"); + assert.ok( + content.includes("acceptMcpConnectScope: true"), + `${routePath} must enable the mcp:connect carve-out` + ); + } }); test("management routes sanitize error.message before returning it to clients", () => { diff --git a/tests/unit/mcp-route-scope-carveout.test.ts b/tests/unit/mcp-route-scope-carveout.test.ts new file mode 100644 index 0000000000..d64dbc7922 --- /dev/null +++ b/tests/unit/mcp-route-scope-carveout.test.ts @@ -0,0 +1,265 @@ +// #9159 route-layer follow-up — the /api/mcp/* transport routes call +// requireManagementAuth() whose API-key branch only accepts the `manage` +// scope. The policy layer (managementPolicy) already carves out mcp:connect +// for /api/mcp/* paths, but the route handler's own check runs independently +// and rejects mcp:connect-only keys with 403 before the MCP transport ever +// starts — stranding MCP-only clients (remote search gateways) with keys far +// broader than the least-privilege design intended. These tests pin the route +// layer to the same carve-out contract the policy layer already enforces. +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omr-mcp-route-scope-")); +process.env.DATA_DIR = TEST_DATA_DIR; +process.env.API_KEY_SECRET = "test-secret"; +process.env.OMNIROUTE_DISABLE_REDIS_AUTH_CACHE = "1"; + +const core = await import("../../src/lib/db/core.ts"); +const apiKeysDb = await import("../../src/lib/db/apiKeys.ts"); +const settingsDb = await import("../../src/lib/db/settings.ts"); +const accessTokensDb = await import("../../src/lib/db/accessTokens.ts"); +const { requireManagementAuth } = await import("../../src/lib/api/requireManagementAuth.ts"); +const { MCP_CONNECT_SCOPE } = await import("../../src/shared/constants/managementScopes.ts"); + +const ORIGINAL_JWT = process.env.JWT_SECRET; +const ORIGINAL_INITIAL = process.env.INITIAL_PASSWORD; + +function reset() { + core.resetDbInstance(); + apiKeysDb.resetApiKeyState(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); + fs.mkdirSync(TEST_DATA_DIR, { recursive: true }); + delete process.env.JWT_SECRET; + delete process.env.INITIAL_PASSWORD; +} + +test.beforeEach(() => { + reset(); +}); + +test.after(() => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); + if (ORIGINAL_JWT === undefined) delete process.env.JWT_SECRET; + else process.env.JWT_SECRET = ORIGINAL_JWT; + if (ORIGINAL_INITIAL === undefined) delete process.env.INITIAL_PASSWORD; + else process.env.INITIAL_PASSWORD = ORIGINAL_INITIAL; +}); + +async function seedAuthRequired() { + process.env.JWT_SECRET = "test-jwt-secret-for-mcp-route-scope"; + process.env.INITIAL_PASSWORD = "initial-pass"; + await settingsDb.updateSettings({ requireLogin: true }); +} + +async function seedKey(scopes: string[], machineId: string): Promise { + const created = await apiKeysDb.createApiKey(`test-${scopes.join("-")}`, machineId, scopes); + // createApiKey returns the raw key only at creation time. + return created.key; +} + +function mcpRequest(key: string, pathname = "/api/mcp/stream", method = "POST"): Request { + return new Request(`http://localhost:20128${pathname}`, { + method, + headers: { + Authorization: `Bearer ${key}`, + "Content-Type": "application/json", + Accept: "application/json, text/event-stream", + }, + }); +} + +test("mcp:connect-only key passes the route-layer guard on /api/mcp/stream", async () => { + await seedAuthRequired(); + const key = await seedKey([MCP_CONNECT_SCOPE], "machine-route-mcp-connect"); + const err = await requireManagementAuth(mcpRequest(key), { + acceptMcpConnectScope: true, + }); + assert.equal(err, null, "mcp:connect key must pass the MCP route guard"); +}); + +test("mcp:connect-only key passes the guard for GET transport routes too", async () => { + await seedAuthRequired(); + const key = await seedKey([MCP_CONNECT_SCOPE], "machine-route-mcp-get"); + const err = await requireManagementAuth(mcpRequest(key, "/api/mcp/status", "GET"), { + acceptMcpConnectScope: true, + }); + assert.equal(err, null, "guard is method-agnostic (GET status route)"); +}); + +test("mcp:connect-only key passes the guard on the sse and tools transport routes", async () => { + await seedAuthRequired(); + const key = await seedKey([MCP_CONNECT_SCOPE], "machine-route-mcp-sse-tools"); + for (const [path, method] of [ + ["/api/mcp/sse", "GET"], + ["/api/mcp/tools", "GET"], + ] as const) { + const err = await requireManagementAuth(mcpRequest(key, path, method), { + acceptMcpConnectScope: true, + }); + assert.equal(err, null, `mcp:connect key must pass ${method} ${path}`); + } +}); + +test("manage-only routes keep the historical 403 message for insufficient scope", async () => { + await seedAuthRequired(); + const key = await seedKey(["execute:search"], "machine-route-manage-msg"); + const err = await requireManagementAuth(mcpRequest(key, "/api/providers")); + assert.ok(err !== null && err instanceof Response); + assert.equal(err.status, 403); + const body = (await err.json()) as { error?: { message?: string } | string }; + const message = typeof body.error === "string" ? body.error : (body.error?.message ?? ""); + assert.match(message, /API key lacks 'manage' scope\./, "default guard message unchanged"); +}); + +test("mcp:connect-only rejection without the option pins the default 403 message", async () => { + await seedAuthRequired(); + const key = await seedKey([MCP_CONNECT_SCOPE], "machine-route-default-msg"); + const err = await requireManagementAuth(mcpRequest(key, "/api/providers")); + assert.ok(err !== null && err instanceof Response); + assert.equal(err.status, 403); + const body = (await err.json()) as { error?: { message?: string } | string }; + const message = typeof body.error === "string" ? body.error : (body.error?.message ?? ""); + assert.match(message, /API key lacks 'manage' scope\./); +}); + +test("admin-only key passes the MCP carve-out", async () => { + await seedAuthRequired(); + const key = await seedKey(["admin"], "machine-route-admin-only"); + const err = await requireManagementAuth(mcpRequest(key), { + acceptMcpConnectScope: true, + }); + assert.equal(err, null, "admin scope is accepted by hasMcpConnectOrManageScope"); +}); + +test("a key with an empty scopes array is rejected", async () => { + await seedAuthRequired(); + const key = await seedKey([], "machine-route-empty-scopes"); + const err = await requireManagementAuth(mcpRequest(key), { + acceptMcpConnectScope: true, + }); + assert.ok(err !== null && err instanceof Response, "no scopes -> 403"); + assert.equal(err.status, 403); + const body = (await err.json()) as { error?: { message?: string } | string }; + const message = typeof body.error === "string" ? body.error : (body.error?.message ?? ""); + assert.ok(message.length > 0, `unexpected error envelope: ${JSON.stringify(body)}`); + assert.match(message, /mcp:connect/); +}); + +test("admin + mcp:connect scopes pass the carve-out (no precedence bug)", async () => { + await seedAuthRequired(); + const key = await seedKey(["admin", MCP_CONNECT_SCOPE], "machine-route-admin-connect"); + const err = await requireManagementAuth(mcpRequest(key), { + acceptMcpConnectScope: true, + }); + assert.equal(err, null, "admin+mcp:connect combination must pass"); +}); + +test("mcp:connect-only key is rejected by the DEFAULT guard (manage-only routes unchanged)", async () => { + await seedAuthRequired(); + const key = await seedKey([MCP_CONNECT_SCOPE], "machine-route-mcp-default"); + const err = await requireManagementAuth(mcpRequest(key, "/api/providers")); + assert.ok(err !== null, "without the option the guard stays manage-only"); + assert.equal(err?.status, 403); +}); + +test("scope-less key is rejected with an actionable message mentioning mcp:connect", async () => { + await seedAuthRequired(); + const key = await seedKey(["execute:search"], "machine-route-search-only"); + const err = await requireManagementAuth(mcpRequest(key), { + acceptMcpConnectScope: true, + }); + assert.ok(err !== null, "insufficient scope must be rejected"); + assert.ok(err instanceof Response); + const body = (await err.json()) as { error?: { message?: string } | string }; + const message = typeof body.error === "string" ? body.error : (body.error?.message ?? ""); + assert.ok(message.length > 0, `unexpected error envelope: ${JSON.stringify(body)}`); + // Pin the full actionable guidance, not just the scope token. + assert.match( + message, + /API key lacks 'mcp:connect' \(or 'manage'\) scope\./, + "403 message must name the required scopes" + ); +}); + +test("manage-scope key keeps working with the MCP option enabled", async () => { + await seedAuthRequired(); + const key = await seedKey(["manage", MCP_CONNECT_SCOPE], "machine-route-manage"); + const err = await requireManagementAuth(mcpRequest(key), { + acceptMcpConnectScope: true, + }); + assert.equal(err, null, "manage+connect key must pass the MCP route guard"); +}); + +test("an unauthenticated request is still rejected with the MCP option enabled", async () => { + await seedAuthRequired(); + const noAuth = new Request("http://localhost:20128/api/mcp/stream", { + method: "POST", + headers: { Accept: "application/json, text/event-stream" }, + }); + const err = await requireManagementAuth(noAuth, { + acceptMcpConnectScope: true, + }); + assert.ok(err !== null, "no credential must be rejected"); + assert.ok(err instanceof Response); + assert.equal(err.status, 401, "scope carve-out must not bypass authentication"); +}); + +test("when auth is not required (no JWT_SECRET), the guard passes everything — carve-out included", async () => { + // Deployment without requireLogin: every management route is open; the + // carve-out option changing nothing here is the pre-existing contract. + delete process.env.JWT_SECRET; + delete process.env.INITIAL_PASSWORD; + await settingsDb.updateSettings({ requireLogin: false }); + const key = await seedKey([], "machine-route-noauth"); + const err = await requireManagementAuth(mcpRequest(key), { + acceptMcpConnectScope: true, + }); + assert.equal(err, null, "auth-disabled deployment keeps the open-door contract"); +}); + +// Design decision (owner 2026-06-19, shared with the policy layer's +// inferRequiredScope in src/server/authz/accessScopes.ts): /api/mcp sits in +// ADMIN_SCOPE_PREFIXES, so an oma_ access token needs `admin` regardless of +// acceptMcpConnectScope. The carve-out is an API-key-only feature — pinning +// it here so a future "fix" that routes oma_ tokens through the carve-out +// fails loudly. +test("oma_ access token with only mcp:connect scope is rejected (admin required by owner policy)", async () => { + await seedAuthRequired(); + const token = accessTokensDb.createAccessToken({ + name: "mcp-oma-test", + scope: MCP_CONNECT_SCOPE, + expiresAt: null, + }); + const req = new Request("http://localhost:20128/api/mcp/stream", { + method: "POST", + headers: { + Authorization: `Bearer ${token.secret}`, + "Content-Type": "application/json", + }, + }); + const err = await requireManagementAuth(req, { acceptMcpConnectScope: true }); + assert.ok(err !== null && err instanceof Response, "oma_ + mcp:connect must be rejected"); + assert.equal(err.status, 403); +}); + +test("oma_ access token with admin scope passes the MCP guard", async () => { + await seedAuthRequired(); + const token = accessTokensDb.createAccessToken({ + name: "mcp-oma-admin", + scope: "admin", + expiresAt: null, + }); + const req = new Request("http://localhost:20128/api/mcp/stream", { + method: "POST", + headers: { + Authorization: `Bearer ${token.secret}`, + "Content-Type": "application/json", + }, + }); + const err = await requireManagementAuth(req, { acceptMcpConnectScope: true }); + assert.equal(err, null, "oma_ + admin must pass"); +});