From ae67de9fcac238f394aca931acc0f57169970cb3 Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Mon, 21 Sep 2026 16:35:56 -0500 Subject: [PATCH 1/5] feat: enforce per-tool approval policies at the integration proxy --- .../handlers/mcp/__tests__/custom-mcp.test.ts | 84 +++++++++++++++++ .../mcp/__tests__/integration-mcp.test.ts | 5 + .../tool-approval-enforcement.test.ts | 92 +++++++++++++++++++ apps/api/src/handlers/mcp/custom-mcp.ts | 1 + apps/api/src/handlers/mcp/integration-mcp.ts | 1 + apps/api/src/handlers/mcp/linear.ts | 6 +- apps/api/src/handlers/mcp/proxy-utils.ts | 53 ++++++++++- .../handlers/mcp/tool-approval-enforcement.ts | 73 +++++++++++++++ ...rationToolApprovalsExperimentalSetting.tsx | 10 +- 9 files changed, 319 insertions(+), 6 deletions(-) create mode 100644 apps/api/src/handlers/mcp/__tests__/tool-approval-enforcement.test.ts create mode 100644 apps/api/src/handlers/mcp/tool-approval-enforcement.ts diff --git a/apps/api/src/handlers/mcp/__tests__/custom-mcp.test.ts b/apps/api/src/handlers/mcp/__tests__/custom-mcp.test.ts index 196377b327..f52b041a29 100644 --- a/apps/api/src/handlers/mcp/__tests__/custom-mcp.test.ts +++ b/apps/api/src/handlers/mcp/__tests__/custom-mcp.test.ts @@ -12,6 +12,7 @@ const { mockFindCustomServer, mockFindConnection, mockGetValidAccessToken, + mockResolveApprovalBlocks, } = vi.hoisted(() => ({ mockEnv: { R_CUSTOM_MCP_ALLOWED_PRIVATE_CIDRS: undefined as string | undefined, @@ -21,6 +22,7 @@ const { mockFindCustomServer: vi.fn(), mockFindConnection: vi.fn(), mockGetValidAccessToken: vi.fn(), + mockResolveApprovalBlocks: vi.fn(async () => new Map()), })); vi.mock('@roomote/env', () => ({ @@ -30,6 +32,11 @@ vi.mock('@roomote/env', () => ({ value === true, })); +vi.mock('../tool-approval-enforcement', async (importOriginal) => ({ + ...(await importOriginal()), + resolveProxyToolApprovalBlocks: mockResolveApprovalBlocks, +})); + vi.mock('@roomote/db/server', () => ({ db: { query: { @@ -451,6 +458,83 @@ describe('createCustomMcpProxy', () => { expect(body.result.tools.map((tool) => tool.name)).toEqual(['safe_tool']); }); + describe('tool approval policies', () => { + afterEach(() => { + mockResolveApprovalBlocks.mockImplementation(async () => new Map()); + }); + + it('resolves policies under the server name for the calling token', async () => { + mockFindCustomServer.mockResolvedValue( + buildServerRow({ url: upstreamUrl() }), + ); + await postMcp(createApp(), initializeRequest); + expect(mockResolveApprovalBlocks).toHaveBeenCalledWith( + expect.objectContaining({ + integrationId: 'internal-tools', + tokenType: 'run', + }), + ); + }); + + it('refuses a blocked tool call with the policy reason and never contacts the upstream', async () => { + mockFindCustomServer.mockResolvedValue( + buildServerRow({ url: upstreamUrl() }), + ); + mockResolveApprovalBlocks.mockResolvedValue( + new Map([['dangerous_tool', 'needs_approval']]), + ); + + const response = await postMcp(createApp(), { + jsonrpc: '2.0', + id: 2, + method: 'tools/call', + params: { name: 'dangerous_tool', arguments: {} }, + }); + + expect(response.status).toBe(403); + const body = (await response.json()) as { error: { message: string } }; + expect(body.error.message).toContain('needs approval'); + expect(lastUpstreamHeaders).toBeNull(); + }); + + it('hides blocked tools from tools/list', async () => { + mockFindCustomServer.mockResolvedValue( + buildServerRow({ url: upstreamUrl() }), + ); + mockResolveApprovalBlocks.mockResolvedValue( + new Map([['dangerous_tool', 'reject']]), + ); + + const response = await postMcp(createApp(), { + jsonrpc: '2.0', + id: 3, + method: 'tools/list', + params: {}, + }); + const body = (await response.json()) as { + result: { tools: { name: string }[] }; + }; + expect(body.result.tools.map((tool) => tool.name)).toEqual(['safe_tool']); + }); + + it('fails closed when the policies cannot be read', async () => { + mockFindCustomServer.mockResolvedValue( + buildServerRow({ url: upstreamUrl() }), + ); + mockResolveApprovalBlocks.mockRejectedValue(new Error('db down')); + + const response = await postMcp(createApp(), { + jsonrpc: '2.0', + id: 2, + method: 'tools/call', + params: { name: 'safe_tool', arguments: {} }, + }); + + expect(response.status).toBe(500); + expect(lastUpstreamHeaders).toBeNull(); + }); + }); + it('refuses upstream redirects instead of following them', async () => { mockFindCustomServer.mockResolvedValue( buildServerRow({ url: upstreamUrl('/redirect') }), diff --git a/apps/api/src/handlers/mcp/__tests__/integration-mcp.test.ts b/apps/api/src/handlers/mcp/__tests__/integration-mcp.test.ts index d21eb82488..19d31cfe39 100644 --- a/apps/api/src/handlers/mcp/__tests__/integration-mcp.test.ts +++ b/apps/api/src/handlers/mcp/__tests__/integration-mcp.test.ts @@ -20,6 +20,11 @@ const { mockGetTaskHumanOwnerUserIds: vi.fn(), })); +vi.mock('../tool-approval-enforcement', () => ({ + describeProxyToolApprovalBlock: () => '', + resolveProxyToolApprovalBlocks: async () => new Map(), +})); + vi.mock('@roomote/db/server', () => ({ db: { query: { diff --git a/apps/api/src/handlers/mcp/__tests__/tool-approval-enforcement.test.ts b/apps/api/src/handlers/mcp/__tests__/tool-approval-enforcement.test.ts new file mode 100644 index 0000000000..8c905ab4f7 --- /dev/null +++ b/apps/api/src/handlers/mcp/__tests__/tool-approval-enforcement.test.ts @@ -0,0 +1,92 @@ +const { mockExperiment, mockDeployment, mockUser } = vi.hoisted(() => ({ + mockExperiment: vi.fn(async () => true), + mockDeployment: vi.fn(async () => [] as unknown[]), + mockUser: vi.fn(async () => [] as unknown[]), +})); + +vi.mock('@roomote/db/server', () => ({ + isDeploymentExperimentEnabled: mockExperiment, + listIntegrationToolPolicies: mockDeployment, + listIntegrationToolUserPolicies: mockUser, +})); + +import { resolveProxyToolApprovalBlocks } from '../tool-approval-enforcement'; + +const policy = ( + integrationId: string, + toolName: string, + mode: 'ask' | 'reject', +) => ({ integrationId, toolName, mode }); + +describe('resolveProxyToolApprovalBlocks', () => { + beforeEach(() => { + vi.clearAllMocks(); + mockExperiment.mockResolvedValue(true); + mockDeployment.mockResolvedValue([ + policy('linear', 'delete_issue', 'reject'), + policy('linear', 'save_issue', 'ask'), + policy('other', 'save_issue', 'reject'), + ]); + mockUser.mockResolvedValue([]); + }); + + it('blocks reject for every caller and ask only for task runs', async () => { + const task = await resolveProxyToolApprovalBlocks({ + integrationId: 'linear', + tokenType: 'run', + resolveActingUserId: async () => 'user-1', + }); + expect(Object.fromEntries(task)).toEqual({ + delete_issue: 'reject', + save_issue: 'needs_approval', + }); + + // A Session already decided its native ask before the call got here. + const session = await resolveProxyToolApprovalBlocks({ + integrationId: 'linear', + tokenType: 'auth', + resolveActingUserId: async () => 'user-1', + }); + expect(Object.fromEntries(session)).toEqual({ delete_issue: 'reject' }); + }); + + it("applies the stricter of the deployment and the acting user's policy", async () => { + mockUser.mockResolvedValue([ + policy('linear', 'save_issue', 'reject'), + policy('linear', 'delete_issue', 'ask'), + policy('linear', 'list_issues', 'ask'), + ]); + const blocks = await resolveProxyToolApprovalBlocks({ + integrationId: 'linear', + tokenType: 'run', + resolveActingUserId: async () => 'user-1', + }); + expect(mockUser).toHaveBeenCalledWith('user-1'); + expect(Object.fromEntries(blocks)).toEqual({ + delete_issue: 'reject', + save_issue: 'reject', + list_issues: 'needs_approval', + }); + }); + + it('reads no personal policies for a run without a human actor', async () => { + await resolveProxyToolApprovalBlocks({ + integrationId: 'linear', + tokenType: 'run', + resolveActingUserId: async () => null, + }); + expect(mockUser).not.toHaveBeenCalled(); + }); + + it('blocks nothing and reads nothing while the experiment is off', async () => { + mockExperiment.mockResolvedValue(false); + const blocks = await resolveProxyToolApprovalBlocks({ + integrationId: 'linear', + tokenType: 'run', + resolveActingUserId: async () => 'user-1', + }); + expect(blocks.size).toBe(0); + expect(mockDeployment).not.toHaveBeenCalled(); + expect(mockUser).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/api/src/handlers/mcp/custom-mcp.ts b/apps/api/src/handlers/mcp/custom-mcp.ts index 7648483d54..dd466b4131 100644 --- a/apps/api/src/handlers/mcp/custom-mcp.ts +++ b/apps/api/src/handlers/mcp/custom-mcp.ts @@ -105,6 +105,7 @@ export function createCustomMcpProxy() { authHeader, extraHeaders, disabledToolNames: server.disabledTools, + toolApprovalIntegrationId: server.name, upstream: server.url, }; }, diff --git a/apps/api/src/handlers/mcp/integration-mcp.ts b/apps/api/src/handlers/mcp/integration-mcp.ts index 829c9bef75..578a7b7b50 100644 --- a/apps/api/src/handlers/mcp/integration-mcp.ts +++ b/apps/api/src/handlers/mcp/integration-mcp.ts @@ -192,6 +192,7 @@ export function createIntegrationMcpProxy( return { ...credentials, ...resolvedToolPolicy, + toolApprovalIntegrationId: integration.id, }; }, }); diff --git a/apps/api/src/handlers/mcp/linear.ts b/apps/api/src/handlers/mcp/linear.ts index 0b23adf3fb..4cbe5649c6 100644 --- a/apps/api/src/handlers/mcp/linear.ts +++ b/apps/api/src/handlers/mcp/linear.ts @@ -54,7 +54,11 @@ export function createLinearMcp(options?: { ); } - return { authHeader: linearAccessToken, disabledToolNames }; + return { + authHeader: linearAccessToken, + disabledToolNames, + toolApprovalIntegrationId: 'linear', + }; }, }); } diff --git a/apps/api/src/handlers/mcp/proxy-utils.ts b/apps/api/src/handlers/mcp/proxy-utils.ts index 5c836fde21..7eea2c7a71 100644 --- a/apps/api/src/handlers/mcp/proxy-utils.ts +++ b/apps/api/src/handlers/mcp/proxy-utils.ts @@ -19,6 +19,11 @@ import { import type { Variables } from '../../types'; import { fetchWithLongLivedStreamDispatcher } from '../long-lived-fetch'; import { createLoggedProxyResponseBody } from '../proxy-response-stream'; +import { + describeProxyToolApprovalBlock, + resolveProxyToolApprovalBlocks, + type ProxyToolApprovalBlock, +} from './tool-approval-enforcement'; type JsonRpcRequestId = string | number | null; @@ -345,6 +350,12 @@ interface ResolvedCredentials { */ allowedToolNames?: readonly string[] | null; disabledToolNames?: readonly string[] | null; + /** + * The id this integration's per-tool approval policies are keyed on (the + * built-in integration id, or a custom server's name). Set it to have the + * proxy enforce those policies; see `tool-approval-enforcement.ts`. + */ + toolApprovalIntegrationId?: string; /** * Per-request upstream URL. Required when the proxy was constructed without * a static `upstream` (custom servers resolve theirs from the database). @@ -1000,6 +1011,43 @@ export function createMcpProxy(config: McpProxyConfig) { ); } + // Approval-blocked tools are hidden and refused exactly like disabled + // ones; only the refusal message differs. + let toolApprovalBlocks = new Map(); + if (credentials.toolApprovalIntegrationId) { + try { + toolApprovalBlocks = await resolveProxyToolApprovalBlocks({ + integrationId: credentials.toolApprovalIntegrationId, + tokenType: auth.tokenType, + resolveActingUserId: () => resolveTaskOrSessionUserIdOrNull(auth), + }); + } catch (error) { + // Fail closed: an unreadable policy must not let a gated tool run. + console.error( + formatSingleLineLog(`${logPrefix} Failed to resolve tool approvals`, { + requestId, + method, + path, + error: error instanceof Error ? error.message : String(error), + }), + ); + return jsonRpcErrorResponse( + 500, + -32603, + `Failed to resolve ${name} tool approval policies`, + ); + } + if (toolApprovalBlocks.size > 0) { + credentials = { + ...credentials, + disabledToolNames: [ + ...(credentials.disabledToolNames ?? []), + ...toolApprovalBlocks.keys(), + ], + }; + } + } + const effectiveUpstream = credentials.upstream ?? upstream; if (!effectiveUpstream) { @@ -1073,10 +1121,13 @@ export function createMcpProxy(config: McpProxyConfig) { }, ), ); + const approvalBlock = toolApprovalBlocks.get(toolName); return jsonRpcErrorResponse( 403, -32000, - `${name} MCP tool "${toolName}" is not allowed on this endpoint`, + approvalBlock + ? describeProxyToolApprovalBlock(toolName, approvalBlock) + : `${name} MCP tool "${toolName}" is not allowed on this endpoint`, getJsonRpcRequestId(parsedBody), ); } diff --git a/apps/api/src/handlers/mcp/tool-approval-enforcement.ts b/apps/api/src/handlers/mcp/tool-approval-enforcement.ts new file mode 100644 index 0000000000..be41eacf43 --- /dev/null +++ b/apps/api/src/handlers/mcp/tool-approval-enforcement.ts @@ -0,0 +1,73 @@ +import { + isDeploymentExperimentEnabled, + listIntegrationToolPolicies, + listIntegrationToolUserPolicies, +} from '@roomote/db/server'; +import { + resolveStricterIntegrationToolPolicyMode, + type IntegrationToolPolicyMode, +} from '@roomote/types'; + +export type ProxyToolApprovalBlock = 'reject' | 'needs_approval'; + +/** + * Experiment-gated (`integrationToolApprovals`) enforcement of per-tool + * approval policies at the integration proxy, the one boundary every caller + * crosses. A Session enforces `ask` natively before the call ever gets here, + * but a task's agent has a shell next to its MCP configuration, so nothing + * inside the sandbox can be the boundary for a task. + * + * - `reject` blocks the tool for every caller. + * - `ask` blocks it for task runs, which have no approval flow yet, so a + * gated call can never be handed to a task to run unasked. Session calls + * pass: their native ask was already decided by the Session owner. + * + * The stricter of the deployment policy and the acting user's personal + * policy applies. Returns no blocks while the experiment is off. + */ +export async function resolveProxyToolApprovalBlocks(input: { + integrationId: string; + tokenType: 'run' | 'auth'; + /** Looked up only while the experiment is on. */ + resolveActingUserId: () => Promise; +}): Promise> { + const blocks = new Map(); + if (!(await isDeploymentExperimentEnabled('integrationToolApprovals'))) { + return blocks; + } + const actingUserId = await input.resolveActingUserId(); + const [deploymentPolicies, userPolicies] = await Promise.all([ + listIntegrationToolPolicies(), + actingUserId + ? listIntegrationToolUserPolicies(actingUserId) + : Promise.resolve([]), + ]); + const modes = new Map(); + for (const policy of [...deploymentPolicies, ...userPolicies]) { + if (policy.integrationId !== input.integrationId) continue; + modes.set( + policy.toolName, + resolveStricterIntegrationToolPolicyMode( + modes.get(policy.toolName), + policy.mode, + ), + ); + } + for (const [toolName, mode] of modes) { + if (mode === 'reject') { + blocks.set(toolName, 'reject'); + } else if (mode === 'ask' && input.tokenType === 'run') { + blocks.set(toolName, 'needs_approval'); + } + } + return blocks; +} + +export function describeProxyToolApprovalBlock( + toolName: string, + block: ProxyToolApprovalBlock, +): string { + return block === 'reject' + ? `Tool "${toolName}" is blocked by this deployment's tool approval policy.` + : `Tool "${toolName}" needs approval before it runs, and tasks cannot request approval yet. Ask the user to run it from a Session instead.`; +} diff --git a/apps/web/src/components/settings/IntegrationToolApprovalsExperimentalSetting.tsx b/apps/web/src/components/settings/IntegrationToolApprovalsExperimentalSetting.tsx index cd802da57e..aebacdd205 100644 --- a/apps/web/src/components/settings/IntegrationToolApprovalsExperimentalSetting.tsx +++ b/apps/web/src/components/settings/IntegrationToolApprovalsExperimentalSetting.tsx @@ -29,10 +29,12 @@ export function IntegrationToolApprovalsExperimentalSetting() { in Settings → Integrations offers Always allow (default), Ask first, and Reject per tool. Ask first pauses each call until the Session owner allows it once, stops the asks for the rest of that Session, or - rejects it; Reject blocks it outright. Session owners can also ask to - be asked about any tool from its call in the transcript. Tools left at - the default run exactly as before. Policies are deployment-wide and - apply from the next session turn. + rejects it; Reject blocks it outright, in Sessions and tasks alike. + Tasks cannot request approval yet, so an Ask first tool is unavailable + to them. Session owners can also ask to be asked about any tool from + its call in the transcript. Tools left at the default run exactly as + before. Policies are deployment-wide and apply from the next session + turn.

From e9acf4711b5da5a833fa79040fdf612a8f7e54bd Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Mon, 21 Sep 2026 16:55:34 -0500 Subject: [PATCH 2/5] fix: govern a custom server by its own policy layer, since names can coincide --- .../handlers/mcp/__tests__/custom-mcp.test.ts | 14 ++++++ .../tool-approval-enforcement.test.ts | 19 ++++++++ apps/api/src/handlers/mcp/custom-mcp.ts | 1 + apps/api/src/handlers/mcp/proxy-utils.ts | 3 ++ .../handlers/mcp/tool-approval-enforcement.ts | 17 +++++-- .../fast-agent-tool-approvals.test.ts | 30 +++++++++++++ .../fast-agent/fast-agent-conversation.ts | 2 + .../fast-agent-integration-broker.ts | 8 ++++ .../fast-agent/fast-agent-tool-approvals.ts | 45 ++++++++++++++----- .../server/routers/mcp-connections.test.ts | 2 + .../sdk/src/server/routers/mcp-connections.ts | 13 ++++++ 11 files changed, 139 insertions(+), 15 deletions(-) diff --git a/apps/api/src/handlers/mcp/__tests__/custom-mcp.test.ts b/apps/api/src/handlers/mcp/__tests__/custom-mcp.test.ts index f52b041a29..22728686f1 100644 --- a/apps/api/src/handlers/mcp/__tests__/custom-mcp.test.ts +++ b/apps/api/src/handlers/mcp/__tests__/custom-mcp.test.ts @@ -471,11 +471,25 @@ describe('createCustomMcpProxy', () => { expect(mockResolveApprovalBlocks).toHaveBeenCalledWith( expect.objectContaining({ integrationId: 'internal-tools', + policyScope: 'deployment', tokenType: 'run', }), ); }); + it("resolves a personal server under its owner's policies only", async () => { + mockFindCustomServer.mockResolvedValue( + buildServerRow({ url: upstreamUrl(), ownerUserId: 'user-1' }), + ); + await postMcp(createApp(createAuthToken()), initializeRequest); + expect(mockResolveApprovalBlocks).toHaveBeenLastCalledWith( + expect.objectContaining({ + integrationId: 'internal-tools', + policyScope: 'personal', + }), + ); + }); + it('refuses a blocked tool call with the policy reason and never contacts the upstream', async () => { mockFindCustomServer.mockResolvedValue( buildServerRow({ url: upstreamUrl() }), diff --git a/apps/api/src/handlers/mcp/__tests__/tool-approval-enforcement.test.ts b/apps/api/src/handlers/mcp/__tests__/tool-approval-enforcement.test.ts index 8c905ab4f7..71666d445d 100644 --- a/apps/api/src/handlers/mcp/__tests__/tool-approval-enforcement.test.ts +++ b/apps/api/src/handlers/mcp/__tests__/tool-approval-enforcement.test.ts @@ -69,6 +69,25 @@ describe('resolveProxyToolApprovalBlocks', () => { }); }); + it('governs a custom server by its own layer only, since names can coincide', async () => { + mockDeployment.mockResolvedValue([ + policy('tools', 'shared_only', 'reject'), + ]); + mockUser.mockResolvedValue([policy('tools', 'personal_only', 'reject')]); + const resolve = (policyScope?: 'deployment' | 'personal') => + resolveProxyToolApprovalBlocks({ + integrationId: 'tools', + policyScope, + tokenType: 'run', + resolveActingUserId: async () => 'user-1', + }).then((blocks) => [...blocks.keys()].sort()); + + expect(await resolve('deployment')).toEqual(['shared_only']); + expect(await resolve('personal')).toEqual(['personal_only']); + // Built-in integrations take both layers. + expect(await resolve()).toEqual(['personal_only', 'shared_only']); + }); + it('reads no personal policies for a run without a human actor', async () => { await resolveProxyToolApprovalBlocks({ integrationId: 'linear', diff --git a/apps/api/src/handlers/mcp/custom-mcp.ts b/apps/api/src/handlers/mcp/custom-mcp.ts index dd466b4131..f97159fbf2 100644 --- a/apps/api/src/handlers/mcp/custom-mcp.ts +++ b/apps/api/src/handlers/mcp/custom-mcp.ts @@ -106,6 +106,7 @@ export function createCustomMcpProxy() { extraHeaders, disabledToolNames: server.disabledTools, toolApprovalIntegrationId: server.name, + toolApprovalPolicyScope: server.ownerUserId ? 'personal' : 'deployment', upstream: server.url, }; }, diff --git a/apps/api/src/handlers/mcp/proxy-utils.ts b/apps/api/src/handlers/mcp/proxy-utils.ts index 7eea2c7a71..b72c1e6451 100644 --- a/apps/api/src/handlers/mcp/proxy-utils.ts +++ b/apps/api/src/handlers/mcp/proxy-utils.ts @@ -356,6 +356,8 @@ interface ResolvedCredentials { * proxy enforce those policies; see `tool-approval-enforcement.ts`. */ toolApprovalIntegrationId?: string; + /** The one policy layer governing a custom server; unset takes both. */ + toolApprovalPolicyScope?: 'deployment' | 'personal'; /** * Per-request upstream URL. Required when the proxy was constructed without * a static `upstream` (custom servers resolve theirs from the database). @@ -1018,6 +1020,7 @@ export function createMcpProxy(config: McpProxyConfig) { try { toolApprovalBlocks = await resolveProxyToolApprovalBlocks({ integrationId: credentials.toolApprovalIntegrationId, + policyScope: credentials.toolApprovalPolicyScope, tokenType: auth.tokenType, resolveActingUserId: () => resolveTaskOrSessionUserIdOrNull(auth), }); diff --git a/apps/api/src/handlers/mcp/tool-approval-enforcement.ts b/apps/api/src/handlers/mcp/tool-approval-enforcement.ts index be41eacf43..4da0e2c370 100644 --- a/apps/api/src/handlers/mcp/tool-approval-enforcement.ts +++ b/apps/api/src/handlers/mcp/tool-approval-enforcement.ts @@ -23,10 +23,16 @@ export type ProxyToolApprovalBlock = 'reject' | 'needs_approval'; * pass: their native ask was already decided by the Session owner. * * The stricter of the deployment policy and the acting user's personal - * policy applies. Returns no blocks while the experiment is off. + * policy applies, within the layers that govern the integration. Returns no blocks while the experiment is off. */ export async function resolveProxyToolApprovalBlocks(input: { integrationId: string; + /** + * A custom server is governed by one layer only, matching where its + * policies are edited: `deployment` for a shared server, `personal` for its + * owner's. Their names can coincide. Unset (built-ins) takes both. + */ + policyScope?: 'deployment' | 'personal'; tokenType: 'run' | 'auth'; /** Looked up only while the experiment is on. */ resolveActingUserId: () => Promise; @@ -35,9 +41,14 @@ export async function resolveProxyToolApprovalBlocks(input: { if (!(await isDeploymentExperimentEnabled('integrationToolApprovals'))) { return blocks; } - const actingUserId = await input.resolveActingUserId(); + const actingUserId = + input.policyScope === 'deployment' + ? null + : await input.resolveActingUserId(); const [deploymentPolicies, userPolicies] = await Promise.all([ - listIntegrationToolPolicies(), + input.policyScope === 'personal' + ? Promise.resolve([]) + : listIntegrationToolPolicies(), actingUserId ? listIntegrationToolUserPolicies(actingUserId) : Promise.resolve([]), diff --git a/packages/cloud-agents/src/server/fast-agent/__tests__/fast-agent-tool-approvals.test.ts b/packages/cloud-agents/src/server/fast-agent/__tests__/fast-agent-tool-approvals.test.ts index b22cf11237..5718e9a9c7 100644 --- a/packages/cloud-agents/src/server/fast-agent/__tests__/fast-agent-tool-approvals.test.ts +++ b/packages/cloud-agents/src/server/fast-agent/__tests__/fast-agent-tool-approvals.test.ts @@ -416,6 +416,36 @@ describe('resolveFastAgentToolApprovalRules', () => { }); }); + it('governs a custom server by its own policy layer only, since names can coincide', () => { + const policy = (toolName: string) => ({ + policyId: toolName, + integrationId: 'tools', + toolName, + mode: 'reject' as const, + updatedAt: '', + createdAt: '', + }); + const merged = (scope?: 'deployment' | 'personal') => + mergeIntegrationToolPolicies( + [policy('shared_only')], + [policy('personal_only')], + [ + { + id: 'tools', + toolApprovalPolicyScope: scope, + tools: [], + } as unknown as FastAgentIntegration, + ], + ) + .map((entry) => entry.toolName) + .sort(); + + expect(merged('deployment')).toEqual(['shared_only']); + expect(merged('personal')).toEqual(['personal_only']); + // Built-in integrations take both layers. + expect(merged()).toEqual(['personal_only', 'shared_only']); + }); + it('reads no personal policies without a Session owner', async () => { await resolveFastAgentToolApprovalRules({ integrations }); expect(listIntegrationToolUserPolicies).not.toHaveBeenCalled(); diff --git a/packages/cloud-agents/src/server/fast-agent/fast-agent-conversation.ts b/packages/cloud-agents/src/server/fast-agent/fast-agent-conversation.ts index acfbd58fd7..e6a46b58d3 100644 --- a/packages/cloud-agents/src/server/fast-agent/fast-agent-conversation.ts +++ b/packages/cloud-agents/src/server/fast-agent/fast-agent-conversation.ts @@ -178,6 +178,8 @@ export type FastAgentMcpServerConfig = { disabledTools?: string[]; /** Opaque, non-secret revision used to invalidate process-local tool catalogs. */ cacheRevision?: string; + /** Which approval policies govern a custom server; unset for built-ins. */ + toolApprovalPolicyScope?: 'deployment' | 'personal'; }; /** Structured input request issued with the Fast-native request_user_input tool. */ diff --git a/packages/cloud-agents/src/server/fast-agent/fast-agent-integration-broker.ts b/packages/cloud-agents/src/server/fast-agent/fast-agent-integration-broker.ts index ac961d9f31..06f3a0ddc1 100644 --- a/packages/cloud-agents/src/server/fast-agent/fast-agent-integration-broker.ts +++ b/packages/cloud-agents/src/server/fast-agent/fast-agent-integration-broker.ts @@ -58,6 +58,8 @@ export type FastAgentIntegration = { name: string; description: string; dataPolicy?: 'shared' | 'private'; + /** Which approval policies govern a custom server; unset for built-ins. */ + toolApprovalPolicyScope?: 'deployment' | 'personal'; instructions?: string; tools: McpToolDefinition[]; endpoint?: { @@ -546,6 +548,9 @@ export async function listFastAgentIntegrations( config, }), disabledTools: normalizeDisabledToolNames(config.disabledTools ?? []), + ...(config.toolApprovalPolicyScope + ? { toolApprovalPolicyScope: config.toolApprovalPolicyScope } + : {}), })); if (githubInstallation && !configuredServers.github) { @@ -652,6 +657,9 @@ export async function listFastAgentIntegrations( name: result.value.name, description: result.value.description, dataPolicy: result.value.dataPolicy, + ...(result.value.toolApprovalPolicyScope + ? { toolApprovalPolicyScope: result.value.toolApprovalPolicyScope } + : {}), instructions: isMemory ? createMemoryMcpInstructions(result.value.id, { primary: primaryMemory, diff --git a/packages/cloud-agents/src/server/fast-agent/fast-agent-tool-approvals.ts b/packages/cloud-agents/src/server/fast-agent/fast-agent-tool-approvals.ts index db01013ee8..3a39e63218 100644 --- a/packages/cloud-agents/src/server/fast-agent/fast-agent-tool-approvals.ts +++ b/packages/cloud-agents/src/server/fast-agent/fast-agent-tool-approvals.ts @@ -92,24 +92,45 @@ function listMountedIntegrationTools( * Layer the Session owner's personal policies on the deployment ones. The * stricter mode wins per tool, so a personal policy can gate or block calls * in the owner's Sessions but never loosen what an admin configured. + * + * A custom server is governed by one layer only, matching where its policies + * are edited: a shared server takes the deployment's, a personal server its + * owner's. Their names can coincide, so a policy written for one must never + * reach the other. Built-in integrations take both layers. */ export function mergeIntegrationToolPolicies( deploymentPolicies: IntegrationToolPolicyMetadata[], userPolicies: IntegrationToolPolicyMetadata[], + integrations: FastAgentIntegration[] = [], ): IntegrationToolPolicyMetadata[] { - const merged = new Map( - deploymentPolicies.map((policy) => [ - integrationToolPolicyKey(policy.integrationId, policy.toolName), - policy, + const scopeById = new Map( + integrations.map((integration) => [ + integration.id, + integration.toolApprovalPolicyScope, ]), ); - for (const policy of userPolicies) { - const key = integrationToolPolicyKey(policy.integrationId, policy.toolName); - const mode = resolveStricterIntegrationToolPolicyMode( - merged.get(key)?.mode, - policy.mode, - ); - if (mode === policy.mode) merged.set(key, policy); + const governs = ( + layer: 'deployment' | 'personal', + policy: IntegrationToolPolicyMetadata, + ) => (scopeById.get(policy.integrationId) ?? layer) === layer; + + const merged = new Map(); + for (const [layer, policies] of [ + ['deployment', deploymentPolicies], + ['personal', userPolicies], + ] as const) { + for (const policy of policies) { + if (!governs(layer, policy)) continue; + const key = integrationToolPolicyKey( + policy.integrationId, + policy.toolName, + ); + const mode = resolveStricterIntegrationToolPolicyMode( + merged.get(key)?.mode, + policy.mode, + ); + if (mode === policy.mode) merged.set(key, policy); + } } return [...merged.values()]; } @@ -308,7 +329,7 @@ export async function resolveFastAgentToolApprovalRules(input: { ]); const rules = buildIntegrationToolApprovalRules( input.integrations, - mergeIntegrationToolPolicies(policies, userPolicies), + mergeIntegrationToolPolicies(policies, userPolicies, input.integrations), sessionOverrides, ); return { rules, hash: hashIntegrationToolApprovalRules(rules) }; diff --git a/packages/sdk/src/server/routers/mcp-connections.test.ts b/packages/sdk/src/server/routers/mcp-connections.test.ts index 28a2920a31..3d8808a878 100644 --- a/packages/sdk/src/server/routers/mcp-connections.test.ts +++ b/packages/sdk/src/server/routers/mcp-connections.test.ts @@ -1342,9 +1342,11 @@ describe('custom MCP server delivery', () => { }), ).toEqual({ ...expected, + // Session-only metadata the worker delivery above never carries. 'http-integrations': { ...expected['http-integrations'], cacheRevision: '0:', + toolApprovalPolicyScope: 'deployment', }, }); }, diff --git a/packages/sdk/src/server/routers/mcp-connections.ts b/packages/sdk/src/server/routers/mcp-connections.ts index 7d3561685d..5f4dac7f93 100644 --- a/packages/sdk/src/server/routers/mcp-connections.ts +++ b/packages/sdk/src/server/routers/mcp-connections.ts @@ -84,6 +84,13 @@ type ResolvedMcpServerConfig = { headers: Record; disabledTools?: string[]; cacheRevision?: string; + /** + * Which per-tool approval policies govern a custom server: a shared server + * takes the deployment's, a personal one its owner's. A name can exist in + * both scopes, so the name alone cannot tell them apart. Unset for + * built-in integrations, where both layers apply. + */ + toolApprovalPolicyScope?: 'deployment' | 'personal'; }; type ResolvedMcpServerConfigs = Record; @@ -183,9 +190,13 @@ async function resolveMcpServerConfigs(options: { url: `${options.requestOrigin ?? ''}${HTTP_INTEGRATIONS_MCP_PATH}`, headers: {}, }; + // A worker writes what it receives into an agent's MCP configuration, so + // it gets the connection fields only. The cache revision and the approval + // policy scope are control-plane metadata for Roomote's Session runtime. if (!options.includeCacheRevision) { for (const server of Object.values(servers)) { delete server.cacheRevision; + delete server.toolApprovalPolicyScope; } } @@ -433,6 +444,8 @@ async function buildScopedCustomMcpServerConfigs( url: requestOrigin ? `${requestOrigin}${proxyPath}` : proxyPath, headers: { 'X-MCP-Client': PRODUCT_NAME }, cacheRevision: `${row.updatedAt?.getTime() ?? 0}:${connectionUpdatedAt?.getTime() ?? ''}`, + toolApprovalPolicyScope: + scope.visibility === 'owner' ? 'personal' : 'deployment', }; } From ba2bab3306f2b6184ec55aec1fa685476ca8be4b Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Mon, 21 Sep 2026 17:02:27 -0500 Subject: [PATCH 3/5] fix: carry the approval policy scope on the seeded development server too --- packages/sdk/src/server/routers/mcp-connections.test.ts | 4 ++++ packages/sdk/src/server/routers/mcp-connections.ts | 8 ++++++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/packages/sdk/src/server/routers/mcp-connections.test.ts b/packages/sdk/src/server/routers/mcp-connections.test.ts index 3d8808a878..75d7bd958f 100644 --- a/packages/sdk/src/server/routers/mcp-connections.test.ts +++ b/packages/sdk/src/server/routers/mcp-connections.test.ts @@ -406,6 +406,9 @@ describe('mcpConnectionsRouter.getMcpServerConfigs', () => { expect(result['intercom']?.url).toBe( 'https://api.preview.roomote.run/api/mcp/custom/11111111-1111-4111-8111-111111111111', ); + // The name is shared, so the scope is what says whose approval policies + // govern the server that won. + expect(result['intercom']?.toolApprovalPolicyScope).toBe('personal'); expect(mockFindPersonalServers).toHaveBeenCalledTimes(1); }); @@ -476,6 +479,7 @@ describe('mcpConnectionsRouter.getMcpServerConfigs', () => { url: 'https://api.preview.roomote.run/api/mcp/development-fixtures', headers: {}, cacheRevision: '1789516800000', + toolApprovalPolicyScope: 'deployment', }); }); diff --git a/packages/sdk/src/server/routers/mcp-connections.ts b/packages/sdk/src/server/routers/mcp-connections.ts index 5f4dac7f93..5050b4115b 100644 --- a/packages/sdk/src/server/routers/mcp-connections.ts +++ b/packages/sdk/src/server/routers/mcp-connections.ts @@ -405,6 +405,10 @@ async function buildScopedCustomMcpServerConfigs( ): Promise { const servers: ResolvedMcpServerConfigs = {}; const rows = await customMcpServerStore(scope).list({ enabledOnly: true }); + // Every entry built here belongs to this one scope, whichever branch + // builds it. + const toolApprovalPolicyScope = + scope.visibility === 'owner' ? 'personal' : 'deployment'; for (const row of rows) { // stdio servers ride the worker merge path via getCustomStdioMcpServers. @@ -418,6 +422,7 @@ async function buildScopedCustomMcpServerConfigs( url: `${requestOrigin ?? ''}/api/mcp/development-fixtures`, headers: {}, cacheRevision: `${row.updatedAt?.getTime() ?? 0}`, + toolApprovalPolicyScope, }; continue; } @@ -444,8 +449,7 @@ async function buildScopedCustomMcpServerConfigs( url: requestOrigin ? `${requestOrigin}${proxyPath}` : proxyPath, headers: { 'X-MCP-Client': PRODUCT_NAME }, cacheRevision: `${row.updatedAt?.getTime() ?? 0}:${connectionUpdatedAt?.getTime() ?? ''}`, - toolApprovalPolicyScope: - scope.visibility === 'owner' ? 'personal' : 'deployment', + toolApprovalPolicyScope, }; } From 5e30763a194c858af21c4225d5e7a9e8de7c924e Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Mon, 21 Sep 2026 17:11:20 -0500 Subject: [PATCH 4/5] refactor: one shared rule for combining policy layers, and proxy opt-in tests --- .../mcp/__tests__/integration-mcp.test.ts | 23 ++++- .../src/handlers/mcp/__tests__/linear.test.ts | 93 +++++++++++++++++++ .../handlers/mcp/tool-approval-enforcement.ts | 24 ++--- .../fast-agent-tool-approvals.test.ts | 42 ++++----- .../fast-agent/fast-agent-tool-approvals.ts | 60 +++--------- .../sdk/src/server/routers/mcp-connections.ts | 6 +- ...integration-tool-policy-strictness.test.ts | 74 +++++++++++---- .../types/src/integration-tool-approvals.ts | 48 +++++++++- 8 files changed, 260 insertions(+), 110 deletions(-) create mode 100644 apps/api/src/handlers/mcp/__tests__/linear.test.ts diff --git a/apps/api/src/handlers/mcp/__tests__/integration-mcp.test.ts b/apps/api/src/handlers/mcp/__tests__/integration-mcp.test.ts index 19d31cfe39..babe2f1314 100644 --- a/apps/api/src/handlers/mcp/__tests__/integration-mcp.test.ts +++ b/apps/api/src/handlers/mcp/__tests__/integration-mcp.test.ts @@ -11,7 +11,9 @@ const { mockGetValidAccessToken, mockDecrypt, mockGetTaskHumanOwnerUserIds, + mockResolveApprovalBlocks, } = vi.hoisted(() => ({ + mockResolveApprovalBlocks: vi.fn(async () => new Map()), mockFindTaskRun: vi.fn(), mockFindConnection: vi.fn(), mockFindEnablement: vi.fn(), @@ -22,7 +24,7 @@ const { vi.mock('../tool-approval-enforcement', () => ({ describeProxyToolApprovalBlock: () => '', - resolveProxyToolApprovalBlocks: async () => new Map(), + resolveProxyToolApprovalBlocks: mockResolveApprovalBlocks, })); vi.mock('@roomote/db/server', () => ({ @@ -178,6 +180,25 @@ describe('createIntegrationMcpProxy acting-user scoping', () => { expect(response.status).toBe(200); }); + it('enforces approval policies under the integration id, across both policy layers', async () => { + mockFindTaskRun.mockResolvedValue({ id: 42, actingUserId: null }); + mockFindConnection.mockResolvedValue({ id: 'conn-1', userId: null }); + stubUpstreamFetch(); + + await postMcp( + createApp('supermemory', createRunToken()), + createInitializeRequest(1), + ); + + expect(mockResolveApprovalBlocks).toHaveBeenLastCalledWith( + expect.objectContaining({ + integrationId: 'supermemory', + policyScope: undefined, + tokenType: 'run', + }), + ); + }); + it('serves a deployment-scoped integration on a run with a human actor', async () => { mockFindTaskRun.mockResolvedValue({ id: 42, actingUserId: 'user-1' }); mockFindConnection.mockResolvedValue({ id: 'conn-1', userId: null }); diff --git a/apps/api/src/handlers/mcp/__tests__/linear.test.ts b/apps/api/src/handlers/mcp/__tests__/linear.test.ts new file mode 100644 index 0000000000..711f8bd456 --- /dev/null +++ b/apps/api/src/handlers/mcp/__tests__/linear.test.ts @@ -0,0 +1,93 @@ +import { Hono } from 'hono'; +import type { RunTokenContext } from '@roomote/types'; + +import type { Variables } from '../../../types'; + +const { mockResolveApprovalBlocks } = vi.hoisted(() => ({ + mockResolveApprovalBlocks: vi.fn(async () => new Map()), +})); + +vi.mock('../tool-approval-enforcement', () => ({ + describeProxyToolApprovalBlock: () => 'blocked by policy', + resolveProxyToolApprovalBlocks: mockResolveApprovalBlocks, +})); + +vi.mock('@roomote/db/server', () => ({ + db: { + query: { + deploymentMcpEnablements: { + findFirst: vi.fn(async () => ({ disabledTools: null })), + }, + taskRuns: { findFirst: vi.fn(async () => ({ id: 42 })) }, + }, + }, + deploymentMcpEnablements: { mcpId: 'mcpId', enabled: 'enabled' }, + taskRuns: { id: 'id' }, + eq: vi.fn(), + and: vi.fn(), + getTaskHumanOwnerUserIds: vi.fn(async () => []), +})); + +vi.mock('@roomote/sdk/server', () => ({ + findLinearDeploymentMcpConnection: vi.fn(async () => ({ id: 'conn-1' })), + getValidAccessToken: vi.fn(async () => 'linear-token'), +})); + +import { createLinearMcp } from '../linear'; + +const runToken: RunTokenContext = { + runId: 42, + userId: null, + principal: 'deployment', + tokenType: 'run', + version: 1, +}; + +function createApp() { + const app = new Hono<{ Variables: Variables }>(); + app.use('*', async (c, next) => { + c.set('authContext', runToken); + await next(); + }); + app.route('/linear', createLinearMcp()); + return app; +} + +function post(body: unknown) { + return createApp().request('/linear', { + method: 'POST', + headers: { + accept: 'application/json, text/event-stream', + 'content-type': 'application/json', + }, + body: JSON.stringify(body), + }); +} + +describe('createLinearMcp tool approval policies', () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.unstubAllGlobals(); + }); + + it('refuses a policy-blocked tool under the linear id without contacting Linear', async () => { + const fetchMock = vi.fn(); + vi.stubGlobal('fetch', fetchMock); + mockResolveApprovalBlocks.mockResolvedValue( + new Map([['save_issue', 'needs_approval']]), + ); + + const response = await post({ + jsonrpc: '2.0', + id: 1, + method: 'tools/call', + params: { name: 'save_issue', arguments: {} }, + }); + + expect(mockResolveApprovalBlocks).toHaveBeenCalledWith( + expect.objectContaining({ integrationId: 'linear', tokenType: 'run' }), + ); + expect(response.status).toBe(403); + expect(fetchMock).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/api/src/handlers/mcp/tool-approval-enforcement.ts b/apps/api/src/handlers/mcp/tool-approval-enforcement.ts index 4da0e2c370..80ecd68bfa 100644 --- a/apps/api/src/handlers/mcp/tool-approval-enforcement.ts +++ b/apps/api/src/handlers/mcp/tool-approval-enforcement.ts @@ -3,10 +3,7 @@ import { listIntegrationToolPolicies, listIntegrationToolUserPolicies, } from '@roomote/db/server'; -import { - resolveStricterIntegrationToolPolicyMode, - type IntegrationToolPolicyMode, -} from '@roomote/types'; +import { resolveGoverningIntegrationToolPolicies } from '@roomote/types'; export type ProxyToolApprovalBlock = 'reject' | 'needs_approval'; @@ -53,18 +50,13 @@ export async function resolveProxyToolApprovalBlocks(input: { ? listIntegrationToolUserPolicies(actingUserId) : Promise.resolve([]), ]); - const modes = new Map(); - for (const policy of [...deploymentPolicies, ...userPolicies]) { - if (policy.integrationId !== input.integrationId) continue; - modes.set( - policy.toolName, - resolveStricterIntegrationToolPolicyMode( - modes.get(policy.toolName), - policy.mode, - ), - ); - } - for (const [toolName, mode] of modes) { + const governing = resolveGoverningIntegrationToolPolicies({ + deploymentPolicies, + userPolicies, + scopeOf: () => input.policyScope, + }); + for (const { integrationId, toolName, mode } of governing) { + if (integrationId !== input.integrationId) continue; if (mode === 'reject') { blocks.set(toolName, 'reject'); } else if (mode === 'ask' && input.tokenType === 'run') { diff --git a/packages/cloud-agents/src/server/fast-agent/__tests__/fast-agent-tool-approvals.test.ts b/packages/cloud-agents/src/server/fast-agent/__tests__/fast-agent-tool-approvals.test.ts index 5718e9a9c7..8e90c64ffa 100644 --- a/packages/cloud-agents/src/server/fast-agent/__tests__/fast-agent-tool-approvals.test.ts +++ b/packages/cloud-agents/src/server/fast-agent/__tests__/fast-agent-tool-approvals.test.ts @@ -40,7 +40,6 @@ import { extractApprovalCallArgs, hashIntegrationToolApprovalRules, integrationToolApprovalRulesToConfig, - mergeIntegrationToolPolicies, resolveFastAgentToolApprovalRules, resolveFastAgentToolApprovalSession, shouldDisposeInstanceForToolApprovalRules, @@ -416,40 +415,37 @@ describe('resolveFastAgentToolApprovalRules', () => { }); }); - it('governs a custom server by its own policy layer only, since names can coincide', () => { + it('governs a custom server by its own policy layer only, since names can coincide', async () => { const policy = (toolName: string) => ({ policyId: toolName, - integrationId: 'tools', + integrationId: 'mock-slack', toolName, mode: 'reject' as const, updatedAt: '', createdAt: '', }); - const merged = (scope?: 'deployment' | 'personal') => - mergeIntegrationToolPolicies( - [policy('shared_only')], - [policy('personal_only')], - [ - { - id: 'tools', - toolApprovalPolicyScope: scope, - tools: [], - } as unknown as FastAgentIntegration, - ], - ) - .map((entry) => entry.toolName) - .sort(); - - expect(merged('deployment')).toEqual(['shared_only']); - expect(merged('personal')).toEqual(['personal_only']); - // Built-in integrations take both layers. - expect(merged()).toEqual(['personal_only', 'shared_only']); + vi.mocked(listIntegrationToolPolicies).mockResolvedValueOnce([ + policy('read_channel'), + ]); + vi.mocked(listIntegrationToolUserPolicies).mockResolvedValueOnce([ + policy('post_message'), + ]); + const resolved = await resolveFastAgentToolApprovalRules({ + integrations: [ + { ...integrations[0]!, toolApprovalPolicyScope: 'personal' }, + ], + ownerUserId: 'owner-id', + }); + // A shared server of the same name has the deployment policy; this + // personal one must only see its owner's. + expect(resolved?.rules.map((rule) => rule.permission)).toEqual([ + codeModeToolKey('mock-slack', 'post_message'), + ]); }); it('reads no personal policies without a Session owner', async () => { await resolveFastAgentToolApprovalRules({ integrations }); expect(listIntegrationToolUserPolicies).not.toHaveBeenCalled(); - expect(mergeIntegrationToolPolicies([], [])).toEqual([]); }); it('is inactive without the experiment', async () => { diff --git a/packages/cloud-agents/src/server/fast-agent/fast-agent-tool-approvals.ts b/packages/cloud-agents/src/server/fast-agent/fast-agent-tool-approvals.ts index 3a39e63218..cfad967f88 100644 --- a/packages/cloud-agents/src/server/fast-agent/fast-agent-tool-approvals.ts +++ b/packages/cloud-agents/src/server/fast-agent/fast-agent-tool-approvals.ts @@ -20,7 +20,7 @@ import { import { integrationToolPolicyKey, resolveEffectiveIntegrationToolMode, - resolveStricterIntegrationToolPolicyMode, + resolveGoverningIntegrationToolPolicies, type IntegrationToolApprovalMetadata, type IntegrationToolPolicyMetadata, type IntegrationToolSessionOverrideMetadata, @@ -88,53 +88,6 @@ function listMountedIntegrationTools( }); } -/** - * Layer the Session owner's personal policies on the deployment ones. The - * stricter mode wins per tool, so a personal policy can gate or block calls - * in the owner's Sessions but never loosen what an admin configured. - * - * A custom server is governed by one layer only, matching where its policies - * are edited: a shared server takes the deployment's, a personal server its - * owner's. Their names can coincide, so a policy written for one must never - * reach the other. Built-in integrations take both layers. - */ -export function mergeIntegrationToolPolicies( - deploymentPolicies: IntegrationToolPolicyMetadata[], - userPolicies: IntegrationToolPolicyMetadata[], - integrations: FastAgentIntegration[] = [], -): IntegrationToolPolicyMetadata[] { - const scopeById = new Map( - integrations.map((integration) => [ - integration.id, - integration.toolApprovalPolicyScope, - ]), - ); - const governs = ( - layer: 'deployment' | 'personal', - policy: IntegrationToolPolicyMetadata, - ) => (scopeById.get(policy.integrationId) ?? layer) === layer; - - const merged = new Map(); - for (const [layer, policies] of [ - ['deployment', deploymentPolicies], - ['personal', userPolicies], - ] as const) { - for (const policy of policies) { - if (!governs(layer, policy)) continue; - const key = integrationToolPolicyKey( - policy.integrationId, - policy.toolName, - ); - const mode = resolveStricterIntegrationToolPolicyMode( - merged.get(key)?.mode, - policy.mode, - ); - if (mode === policy.mode) merged.set(key, policy); - } - } - return [...merged.values()]; -} - /** * Compile configured policies into native session permission rules for the * integrations mounted this turn. Only tools the actor is actually @@ -329,7 +282,16 @@ export async function resolveFastAgentToolApprovalRules(input: { ]); const rules = buildIntegrationToolApprovalRules( input.integrations, - mergeIntegrationToolPolicies(policies, userPolicies, input.integrations), + // The Session owner's personal policies layer on the deployment ones; + // see `resolveGoverningIntegrationToolPolicies` for the rule. + resolveGoverningIntegrationToolPolicies({ + deploymentPolicies: policies, + userPolicies, + scopeOf: (integrationId) => + input.integrations.find( + (integration) => integration.id === integrationId, + )?.toolApprovalPolicyScope, + }), sessionOverrides, ); return { rules, hash: hashIntegrationToolApprovalRules(rules) }; diff --git a/packages/sdk/src/server/routers/mcp-connections.ts b/packages/sdk/src/server/routers/mcp-connections.ts index 5050b4115b..6a47370332 100644 --- a/packages/sdk/src/server/routers/mcp-connections.ts +++ b/packages/sdk/src/server/routers/mcp-connections.ts @@ -118,7 +118,7 @@ async function resolveMcpServerConfigs(options: { auth: Parameters[0]; requestOrigin: string | null; includeRoomoteMemberTools?: boolean; - includeCacheRevision?: boolean; + includeSessionMetadata?: boolean; quiet?: boolean; }): Promise { const logInfo: InfoLogger = options.quiet ? () => {} : console.info; @@ -193,7 +193,7 @@ async function resolveMcpServerConfigs(options: { // A worker writes what it receives into an agent's MCP configuration, so // it gets the connection fields only. The cache revision and the approval // policy scope are control-plane metadata for Roomote's Session runtime. - if (!options.includeCacheRevision) { + if (!options.includeSessionMetadata) { for (const server of Object.values(servers)) { delete server.cacheRevision; delete server.toolApprovalPolicyScope; @@ -216,7 +216,7 @@ export async function resolveUserMcpServerConfigs(options: { auth: { userId: options.userId }, requestOrigin: getRequestOrigin({ url: options.apiBaseUrl }), includeRoomoteMemberTools: options.includeRoomoteMemberTools, - includeCacheRevision: true, + includeSessionMetadata: true, // This runs on every Fast turn; the per-connection info stream is worker // config-fetch debugging noise at that frequency. quiet: true, diff --git a/packages/types/src/__tests__/integration-tool-policy-strictness.test.ts b/packages/types/src/__tests__/integration-tool-policy-strictness.test.ts index 43334894d0..5b57c583e2 100644 --- a/packages/types/src/__tests__/integration-tool-policy-strictness.test.ts +++ b/packages/types/src/__tests__/integration-tool-policy-strictness.test.ts @@ -1,23 +1,63 @@ import { describe, expect, it } from 'vitest'; -import { resolveStricterIntegrationToolPolicyMode } from '../integration-tool-approvals'; +import { resolveGoverningIntegrationToolPolicies } from '../integration-tool-approvals'; -describe('resolveStricterIntegrationToolPolicyMode', () => { +const policy = ( + integrationId: string, + toolName: string, + mode: 'allow' | 'ask' | 'reject', +) => ({ integrationId, toolName, mode }); + +const modes = (policies: ReturnType[]): Record => + Object.fromEntries(policies.map((entry) => [entry.toolName, entry.mode])); + +describe('resolveGoverningIntegrationToolPolicies', () => { it('lets a personal policy tighten but never loosen the deployment one', () => { - expect(resolveStricterIntegrationToolPolicyMode(undefined, 'ask')).toBe( - 'ask', - ); - expect(resolveStricterIntegrationToolPolicyMode('ask', 'reject')).toBe( - 'reject', - ); - expect(resolveStricterIntegrationToolPolicyMode('ask', 'allow')).toBe( - 'ask', - ); - expect(resolveStricterIntegrationToolPolicyMode('reject', 'ask')).toBe( - 'reject', - ); - expect( - resolveStricterIntegrationToolPolicyMode(undefined, undefined), - ).toBeUndefined(); + const governing = resolveGoverningIntegrationToolPolicies({ + deploymentPolicies: [ + policy('linear', 'save_issue', 'ask'), + policy('linear', 'delete_issue', 'reject'), + policy('linear', 'list_issues', 'ask'), + ], + userPolicies: [ + policy('linear', 'save_issue', 'reject'), + policy('linear', 'delete_issue', 'ask'), + policy('linear', 'list_issues', 'allow'), + policy('linear', 'get_issue', 'ask'), + ], + scopeOf: () => undefined, + }); + expect(modes(governing)).toEqual({ + save_issue: 'reject', + delete_issue: 'reject', + list_issues: 'ask', + get_issue: 'ask', + }); + }); + + it('governs a custom server by its own layer only, since names can coincide', () => { + const resolve = (scope?: 'deployment' | 'personal') => + Object.keys( + modes( + resolveGoverningIntegrationToolPolicies({ + deploymentPolicies: [policy('tools', 'shared_only', 'reject')], + userPolicies: [policy('tools', 'personal_only', 'reject')], + scopeOf: () => scope, + }), + ), + ).sort(); + + expect(resolve('deployment')).toEqual(['shared_only']); + expect(resolve('personal')).toEqual(['personal_only']); + expect(resolve()).toEqual(['personal_only', 'shared_only']); + }); + + it('never lets distinct integration and tool pairs share one entry', () => { + const governing = resolveGoverningIntegrationToolPolicies({ + deploymentPolicies: [policy('a', 'bc', 'ask')], + userPolicies: [policy('ab', 'c', 'reject')], + scopeOf: () => undefined, + }); + expect(governing).toHaveLength(2); }); }); diff --git a/packages/types/src/integration-tool-approvals.ts b/packages/types/src/integration-tool-approvals.ts index c549682f34..0ea4de0c87 100644 --- a/packages/types/src/integration-tool-approvals.ts +++ b/packages/types/src/integration-tool-approvals.ts @@ -124,7 +124,7 @@ const INTEGRATION_TOOL_POLICY_MODE_STRICTNESS: Record< > = { allow: 0, ask: 1, reject: 2 }; /** The stricter of a tool's deployment policy and the requester's own. */ -export function resolveStricterIntegrationToolPolicyMode( +function resolveStricterIntegrationToolPolicyMode( deploymentMode: IntegrationToolPolicyMode | undefined, userMode: IntegrationToolPolicyMode | undefined, ): IntegrationToolPolicyMode | undefined { @@ -136,6 +136,52 @@ export function resolveStricterIntegrationToolPolicyMode( : deploymentMode; } +/** Which policy layers govern an integration; unset means both. */ +export type IntegrationToolPolicyScope = 'deployment' | 'personal'; + +type IntegrationToolPolicyEntry = Pick< + IntegrationToolPolicyMetadata, + 'integrationId' | 'toolName' | 'mode' +>; + +/** + * The one rule for combining policy layers, shared by every enforcement + * point (the Session runtime and the integration proxy): per tool, the + * stricter mode among the layers that govern its integration. + * + * A custom server is governed by one layer only, matching where its policies + * are edited: a shared server takes the deployment's, a personal server its + * owner's. Their names can coincide, so a policy written for one must never + * reach the other. Built-in integrations (no scope) take both layers. + */ +export function resolveGoverningIntegrationToolPolicies< + T extends IntegrationToolPolicyEntry, +>(input: { + deploymentPolicies: T[]; + userPolicies: T[]; + scopeOf: (integrationId: string) => IntegrationToolPolicyScope | undefined; +}): T[] { + const governing = new Map(); + for (const [layer, policies] of [ + ['deployment', input.deploymentPolicies], + ['personal', input.userPolicies], + ] as const) { + for (const policy of policies) { + if ((input.scopeOf(policy.integrationId) ?? layer) !== layer) continue; + const key = integrationToolPolicyKey( + policy.integrationId, + policy.toolName, + ); + const mode = resolveStricterIntegrationToolPolicyMode( + governing.get(key)?.mode, + policy.mode, + ); + if (mode === policy.mode) governing.set(key, policy); + } + } + return [...governing.values()]; +} + /** * The mode a tool actually runs under in one session. `policyMode` is the * stricter of the deployment and personal policy. A deployment `reject` From e1c1396973f8e6287337084da8febaa06a00f459 Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Mon, 21 Sep 2026 17:11:56 -0500 Subject: [PATCH 5/5] fix: a blocked-tool message that is also true for a personal policy --- apps/api/src/handlers/mcp/tool-approval-enforcement.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/api/src/handlers/mcp/tool-approval-enforcement.ts b/apps/api/src/handlers/mcp/tool-approval-enforcement.ts index 80ecd68bfa..d6aa292e7e 100644 --- a/apps/api/src/handlers/mcp/tool-approval-enforcement.ts +++ b/apps/api/src/handlers/mcp/tool-approval-enforcement.ts @@ -71,6 +71,6 @@ export function describeProxyToolApprovalBlock( block: ProxyToolApprovalBlock, ): string { return block === 'reject' - ? `Tool "${toolName}" is blocked by this deployment's tool approval policy.` + ? `Tool "${toolName}" is blocked by a tool approval policy.` : `Tool "${toolName}" needs approval before it runs, and tasks cannot request approval yet. Ask the user to run it from a Session instead.`; }