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..22728686f1 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,97 @@ 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', + 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() }), + ); + 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..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(), @@ -20,6 +22,11 @@ const { mockGetTaskHumanOwnerUserIds: vi.fn(), })); +vi.mock('../tool-approval-enforcement', () => ({ + describeProxyToolApprovalBlock: () => '', + resolveProxyToolApprovalBlocks: mockResolveApprovalBlocks, +})); + vi.mock('@roomote/db/server', () => ({ db: { query: { @@ -173,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/__tests__/tool-approval-enforcement.test.ts b/apps/api/src/handlers/mcp/__tests__/tool-approval-enforcement.test.ts new file mode 100644 index 0000000000..71666d445d --- /dev/null +++ b/apps/api/src/handlers/mcp/__tests__/tool-approval-enforcement.test.ts @@ -0,0 +1,111 @@ +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('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', + 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..f97159fbf2 100644 --- a/apps/api/src/handlers/mcp/custom-mcp.ts +++ b/apps/api/src/handlers/mcp/custom-mcp.ts @@ -105,6 +105,8 @@ export function createCustomMcpProxy() { authHeader, extraHeaders, disabledToolNames: server.disabledTools, + toolApprovalIntegrationId: server.name, + toolApprovalPolicyScope: server.ownerUserId ? 'personal' : 'deployment', 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..b72c1e6451 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,14 @@ 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; + /** 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). @@ -1000,6 +1013,44 @@ 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, + policyScope: credentials.toolApprovalPolicyScope, + 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 +1124,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..d6aa292e7e --- /dev/null +++ b/apps/api/src/handlers/mcp/tool-approval-enforcement.ts @@ -0,0 +1,76 @@ +import { + isDeploymentExperimentEnabled, + listIntegrationToolPolicies, + listIntegrationToolUserPolicies, +} from '@roomote/db/server'; +import { resolveGoverningIntegrationToolPolicies } 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, 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; +}): Promise> { + const blocks = new Map(); + if (!(await isDeploymentExperimentEnabled('integrationToolApprovals'))) { + return blocks; + } + const actingUserId = + input.policyScope === 'deployment' + ? null + : await input.resolveActingUserId(); + const [deploymentPolicies, userPolicies] = await Promise.all([ + input.policyScope === 'personal' + ? Promise.resolve([]) + : listIntegrationToolPolicies(), + actingUserId + ? listIntegrationToolUserPolicies(actingUserId) + : Promise.resolve([]), + ]); + 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') { + blocks.set(toolName, 'needs_approval'); + } + } + return blocks; +} + +export function describeProxyToolApprovalBlock( + toolName: string, + block: ProxyToolApprovalBlock, +): string { + return block === 'reject' + ? `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.`; +} 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.

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..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,10 +415,37 @@ describe('resolveFastAgentToolApprovalRules', () => { }); }); + it('governs a custom server by its own policy layer only, since names can coincide', async () => { + const policy = (toolName: string) => ({ + policyId: toolName, + integrationId: 'mock-slack', + toolName, + mode: 'reject' as const, + updatedAt: '', + createdAt: '', + }); + 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-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..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,32 +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. - */ -export function mergeIntegrationToolPolicies( - deploymentPolicies: IntegrationToolPolicyMetadata[], - userPolicies: IntegrationToolPolicyMetadata[], -): IntegrationToolPolicyMetadata[] { - const merged = new Map( - deploymentPolicies.map((policy) => [ - integrationToolPolicyKey(policy.integrationId, policy.toolName), - policy, - ]), - ); - 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); - } - return [...merged.values()]; -} - /** * Compile configured policies into native session permission rules for the * integrations mounted this turn. Only tools the actor is actually @@ -308,7 +282,16 @@ export async function resolveFastAgentToolApprovalRules(input: { ]); const rules = buildIntegrationToolApprovalRules( input.integrations, - mergeIntegrationToolPolicies(policies, userPolicies), + // 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.test.ts b/packages/sdk/src/server/routers/mcp-connections.test.ts index 28a2920a31..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', }); }); @@ -1342,9 +1346,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..6a47370332 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; @@ -111,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; @@ -183,9 +190,13 @@ async function resolveMcpServerConfigs(options: { url: `${options.requestOrigin ?? ''}${HTTP_INTEGRATIONS_MCP_PATH}`, headers: {}, }; - if (!options.includeCacheRevision) { + // 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.includeSessionMetadata) { for (const server of Object.values(servers)) { delete server.cacheRevision; + delete server.toolApprovalPolicyScope; } } @@ -205,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, @@ -394,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. @@ -407,6 +422,7 @@ async function buildScopedCustomMcpServerConfigs( url: `${requestOrigin ?? ''}/api/mcp/development-fixtures`, headers: {}, cacheRevision: `${row.updatedAt?.getTime() ?? 0}`, + toolApprovalPolicyScope, }; continue; } @@ -433,6 +449,7 @@ async function buildScopedCustomMcpServerConfigs( url: requestOrigin ? `${requestOrigin}${proxyPath}` : proxyPath, headers: { 'X-MCP-Client': PRODUCT_NAME }, cacheRevision: `${row.updatedAt?.getTime() ?? 0}:${connectionUpdatedAt?.getTime() ?? ''}`, + toolApprovalPolicyScope, }; } 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`