From 048c3578e3d4f5a873e08f8c440b07a845f29028 Mon Sep 17 00:00:00 2001 From: Dotta Date: Tue, 11 Aug 2026 21:25:13 +0000 Subject: [PATCH] fix(server): enforce MCP wizard action scope Prevent connection installs from adding connection-wide allow entries, keep wizard-managed app profiles additive across binding precedence, and repair legacy over-grants with a data migration. Add decision-level regression coverage for generic and curated MCP paths. Co-Authored-By: Paperclip --- ...26_remove_app_connection_wide_includes.sql | 10 ++ packages/db/src/migrations/meta/_journal.json | 9 +- .../__tests__/generic-mcp-connection.test.ts | 59 +++++++++--- .../src/__tests__/tool-access-service.test.ts | 33 ++++++- server/src/services/tool-access-policy.ts | 18 +++- server/src/services/tool-access.ts | 47 +++++----- .../tool-profile-binding-precedence.test.ts | 93 +++++++++++++++++++ .../tool-profile-binding-precedence.ts | 43 +++++++++ 8 files changed, 268 insertions(+), 44 deletions(-) create mode 100644 packages/db/src/migrations/0226_remove_app_connection_wide_includes.sql create mode 100644 server/src/services/tool-profile-binding-precedence.test.ts diff --git a/packages/db/src/migrations/0226_remove_app_connection_wide_includes.sql b/packages/db/src/migrations/0226_remove_app_connection_wide_includes.sql new file mode 100644 index 00000000000..dfd61362534 --- /dev/null +++ b/packages/db/src/migrations/0226_remove_app_connection_wide_includes.sql @@ -0,0 +1,10 @@ +DELETE FROM "tool_profile_entries" AS "entry" +USING "tool_profiles" AS "profile" +WHERE "entry"."profile_id" = "profile"."id" + AND "entry"."company_id" = "profile"."company_id" + AND "entry"."selector_type" = 'connection' + AND "entry"."effect" = 'include' + AND "entry"."connection_id" IS NOT NULL + AND "profile"."profile_key" = 'app:' || "entry"."connection_id"::text + AND "profile"."metadata" ->> 'source' IN ('app_gallery_finish', 'tool_connection_install') + AND "profile"."metadata" ->> 'connectionId' = "entry"."connection_id"::text; diff --git a/packages/db/src/migrations/meta/_journal.json b/packages/db/src/migrations/meta/_journal.json index f06b8e1d559..a69e2adb3f9 100644 --- a/packages/db/src/migrations/meta/_journal.json +++ b/packages/db/src/migrations/meta/_journal.json @@ -1569,6 +1569,13 @@ "when": 1787101144413, "tag": "0225_drop_claude_setup_token_sessions", "breakpoints": true + }, + { + "idx": 226, + "version": "7", + "when": 1787101145413, + "tag": "0226_remove_app_connection_wide_includes", + "breakpoints": true } ] -} \ No newline at end of file +} diff --git a/server/src/__tests__/generic-mcp-connection.test.ts b/server/src/__tests__/generic-mcp-connection.test.ts index e9d0d446468..bcd99b7a52a 100644 --- a/server/src/__tests__/generic-mcp-connection.test.ts +++ b/server/src/__tests__/generic-mcp-connection.test.ts @@ -1270,15 +1270,26 @@ describeEmbeddedPostgres("generic remote MCP connections", () => { const company = await createCompany(db); const service = toolAccessService(db); const policy = toolAccessPolicyService(db); - const [agent] = await db.insert(agents).values({ - companyId: company.id, - name: `Generic MCP agent ${randomUUID()}`, - role: "engineer", - status: "active", - adapterType: "process", - adapterConfig: {}, - runtimeConfig: {}, - }).returning(); + const [agent, outsideAgent] = await db.insert(agents).values([ + { + companyId: company.id, + name: `Generic MCP agent ${randomUUID()}`, + role: "engineer", + status: "active", + adapterType: "process", + adapterConfig: {}, + runtimeConfig: {}, + }, + { + companyId: company.id, + name: `Outside MCP agent ${randomUUID()}`, + role: "engineer", + status: "active", + adapterType: "process", + adapterConfig: {}, + runtimeConfig: {}, + }, + ]).returning(); const connected = await service.connectGalleryApp(company.id, { link: MCP_URL, name: "Fixture governance" }); const readEntry = connected.catalog.find((entry) => entry.toolName === "list_insights")!; @@ -1297,9 +1308,9 @@ describeEmbeddedPostgres("generic remote MCP connections", () => { expect.objectContaining({ targetType: "agent", targetId: agent!.id }), ]); - const decisionInput = (catalogEntryId: string, toolName: string) => ({ + const decisionInput = (catalogEntryId: string, toolName: string, agentId = agent!.id) => ({ companyId: company.id, - actor: { actorType: "agent" as const, actorId: agent!.id, agentId: agent!.id }, + actor: { actorType: "agent" as const, actorId: agentId, agentId }, request: { connectionId: connected.connectionId, catalogEntryId, toolName }, }); @@ -1310,6 +1321,19 @@ describeEmbeddedPostgres("generic remote MCP connections", () => { .resolves.toMatchObject({ allowed: true, reasonCode: "allow_profile" }); await expect(policy.decide(decisionInput(writeEntry.id, "create_insight"))) .resolves.toMatchObject({ allowed: false, reasonCode: "deny_default" }); + await expect(policy.decide(decisionInput(readEntry.id, "list_insights", outsideAgent!.id))) + .resolves.toMatchObject({ allowed: false, reasonCode: "deny_default" }); + + // Simulate a profile written by the legacy install path. Saving installs + // must self-heal this over-broad entry as well as avoiding it for new apps. + await db.insert(toolProfileEntries).values({ + companyId: company.id, + profileId: finished.profile.id, + selectorType: "connection", + effect: "include", + applicationId: connected.connection.applicationId, + connectionId: connected.connectionId, + }); // Installation targets the chosen agent, same as a curated connection. await service.putConnectionInstalls(connected.connectionId, { @@ -1318,6 +1342,19 @@ describeEmbeddedPostgres("generic remote MCP connections", () => { const installs = await db.select().from(toolConnectionInstalls) .where(eq(toolConnectionInstalls.connectionId, connected.connectionId)); expect(installs).toEqual([expect.objectContaining({ targetType: "agent", targetId: agent!.id })]); + const installedProfileEntries = await db.select().from(toolProfileEntries) + .where(eq(toolProfileEntries.profileId, finished.profile.id)); + expect(installedProfileEntries).not.toEqual(expect.arrayContaining([ + expect.objectContaining({ + selectorType: "connection", + connectionId: connected.connectionId, + effect: "include", + }), + ])); + await expect(policy.decide(decisionInput(readEntry.id, "list_insights"))) + .resolves.toMatchObject({ allowed: true, reasonCode: "allow_profile" }); + await expect(policy.decide(decisionInput(writeEntry.id, "create_insight"))) + .resolves.toMatchObject({ allowed: false, reasonCode: "deny_default" }); // Revoke: archiving the connection removes access but keeps the trail. await service.archiveConnection(connected.connectionId, company.id); diff --git a/server/src/__tests__/tool-access-service.test.ts b/server/src/__tests__/tool-access-service.test.ts index 0b76eb60cb5..4402d1debbb 100644 --- a/server/src/__tests__/tool-access-service.test.ts +++ b/server/src/__tests__/tool-access-service.test.ts @@ -5384,6 +5384,20 @@ describeEmbeddedPostgres("tool access service", () => { expect.objectContaining({ id: duplicateEntry.id, riskLevel: "write", isWrite: true }), ])); + // A narrower profile must not make an app shared with "All agents" + // disappear. App action selection is an additive capability assignment; + // ordinary profile precedence still governs non-app defaults. + const existingAgentProfile = await service.createProfile(company.id, { + profileKey: `existing-agent-profile-${randomUUID()}`, + name: "Existing agent defaults", + defaultAction: "deny", + }); + await service.bindProfile( + existingAgentProfile.id, + { targetType: "agent", targetId: agent.id }, + { actorType: "user", actorId: "board" }, + ); + await expect(service.finishGalleryAppConnection(otherCompany.id, connection.id, { enabledCatalogEntryIds: [fetchEntry.id], askFirstCatalogEntryIds: [], @@ -7040,11 +7054,14 @@ describeEmbeddedPostgres("tool access service", () => { expect(unusedRow!.lastUsedAt).toBeNull(); }); - it("syncs installs, auto-extends agent access, and exposes install state", async () => { + it("syncs installs without widening action access or calling the remote tool", async () => { const company = await createCompany(db); const agent = await createAgent(db, company.id); const { connection } = await createRemoteToolFixture(db, company.id); - const app = createRouteApp(db); + const fetchMock = vi.spyOn(globalThis, "fetch"); + const app = createRouteApp(db, undefined, createToolGatewayService(db, { + toolActionSigningSecret: "test-secret", + })); const put = await request(app) .put(`/api/tool-connections/${connection.id}/installs`) @@ -7071,7 +7088,17 @@ describeEmbeddedPostgres("tool access service", () => { const effective = await toolAccessService(db).getEffectiveProfilesForAgent(company.id, agent.id); expect(effective.installedConnections.map((item) => item.id)).toEqual([connection.id]); - expect(effective.allowedTools.some((tool) => tool.connectionId === connection.id)).toBe(true); + expect(effective.allowedTools.some((tool) => tool.connectionId === connection.id)).toBe(false); + + const deniedCall = await request(app) + .post(`/api/tool-connections/${connection.id}/test-calls`) + .send({ agentId: agent.id, toolName: "send_email", parameters: { to: "a@example.com" } }) + .expect(200); + expect(deniedCall.body).toMatchObject({ + decision: "off", + error: { reasonCode: "deny_default" }, + }); + expect(fetchMock).not.toHaveBeenCalled(); const get = await request(app).get(`/api/tool-connections/${connection.id}`); expect(get.status).toBe(200); diff --git a/server/src/services/tool-access-policy.ts b/server/src/services/tool-access-policy.ts index ae5f9a46db6..311c15976e4 100644 --- a/server/src/services/tool-access-policy.ts +++ b/server/src/services/tool-access-policy.ts @@ -42,7 +42,10 @@ import type { } from "@paperclipai/shared"; import { toolPolicyConditionsSchema } from "@paperclipai/shared"; import { badRequest, conflict, notFound, unprocessable } from "../errors.js"; -import { narrowestScopeBindings, profileIdsInBindingOrder } from "./tool-profile-binding-precedence.js"; +import { + effectiveToolProfileBindings, + profileIdsInBindingOrder, +} from "./tool-profile-binding-precedence.js"; import { recordToolRuntimeAuditWriteFailure } from "./tool-runtime-metrics.js"; type ToolAccessContext = { @@ -1002,11 +1005,16 @@ export function toolAccessPolicyService(db: Db) { async function effectiveProfiles(ctx: ToolAccessContext) { const bindings = await db.select().from(toolProfileBindings).where(eq(toolProfileBindings.companyId, ctx.companyId)); - const activeBindings = narrowestScopeBindings(bindings.filter((binding) => targetMatches(binding, ctx))); - if (activeBindings.length === 0) return { profiles: [], entries: [] as Array }; + const matchingBindings = bindings.filter((binding) => targetMatches(binding, ctx)); + if (matchingBindings.length === 0) return { profiles: [], entries: [] as Array }; + const candidateProfileIds = profileIdsInBindingOrder(matchingBindings); + const candidateProfiles = await db.select().from(toolProfiles).where(and( + eq(toolProfiles.companyId, ctx.companyId), + inArray(toolProfiles.id, candidateProfileIds), + )); + const activeBindings = effectiveToolProfileBindings(matchingBindings, candidateProfiles, ctx.connectionId); const profileIds = profileIdsInBindingOrder(activeBindings); - const profiles = await db.select().from(toolProfiles).where(and(eq(toolProfiles.companyId, ctx.companyId), inArray(toolProfiles.id, profileIds))); - const profilesById = new Map(profiles.map((profile) => [profile.id, profile])); + const profilesById = new Map(candidateProfiles.map((profile) => [profile.id, profile])); const activeProfiles = profileIds .map((profileId) => profilesById.get(profileId) ?? null) .filter((profile): profile is typeof toolProfiles.$inferSelect => Boolean(profile && profile.status === "active")); diff --git a/server/src/services/tool-access.ts b/server/src/services/tool-access.ts index 29cd107945f..03df823afbd 100644 --- a/server/src/services/tool-access.ts +++ b/server/src/services/tool-access.ts @@ -130,7 +130,11 @@ import { guardedRemoteHttpFetch, type GuardedRemoteHttpFetchOptions } from "./re import { secretService } from "./secrets.js"; import { toolAccessPolicyService } from "./tool-access-policy.js"; import { readSignedToolArgumentsPayload } from "./tool-content-guards.js"; -import { narrowestScopeBindings, profileIdsInBindingOrder } from "./tool-profile-binding-precedence.js"; +import { + effectiveToolProfileBindings, + narrowestScopeBindings, + profileIdsInBindingOrder, +} from "./tool-profile-binding-precedence.js"; import { recordToolRuntimeAuditWriteFailure, TOOL_RUNTIME_AUDIT_WRITE_FAILURE_METRIC } from "./tool-runtime-metrics.js"; import { createToolRuntimeSupervisor, ToolRuntimeSupervisorError } from "./tool-runtime-supervisor.js"; @@ -2875,7 +2879,7 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} } async function appProfileForConnection( - dbClient: Pick, + dbClient: Pick, connection: typeof toolConnections.$inferSelect, ) { const profileKey = `app:${connection.id}`; @@ -2895,26 +2899,19 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} metadata: { source: "tool_connection_install", connectionId: connection.id }, }).returning(); } - const [existingEntry] = await dbClient - .select({ id: toolProfileEntries.id }) - .from(toolProfileEntries) + // Installation controls where a connection is exposed, not which actions + // it grants. The app wizard's catalog-entry includes are the authority for + // action selection, so remove the legacy connection-wide include that used + // to silently turn every installed action on. + await dbClient + .delete(toolProfileEntries) .where(and( eq(toolProfileEntries.companyId, connection.companyId), eq(toolProfileEntries.profileId, profile.id), eq(toolProfileEntries.selectorType, "connection"), + eq(toolProfileEntries.effect, "include"), eq(toolProfileEntries.connectionId, connection.id), - )) - .limit(1); - if (!existingEntry) { - await dbClient.insert(toolProfileEntries).values({ - companyId: connection.companyId, - profileId: profile.id, - selectorType: "connection", - effect: "include", - applicationId: connection.applicationId, - connectionId: connection.id, - }); - } + )); return profile; } @@ -8260,11 +8257,11 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} .from(toolProfileBindings) .where(eq(toolProfileBindings.companyId, companyId)) .orderBy(asc(toolProfileBindings.priority), asc(toolProfileBindings.createdAt)); - const bindings = narrowestScopeBindings(allBindings.filter((binding) => + const matchingBindings = allBindings.filter((binding) => (binding.targetType === "company" && binding.targetId === companyId) || (binding.targetType === "agent" && binding.targetId === agentId) - )); - if (bindings.length === 0) { + ); + if (matchingBindings.length === 0) { return { agentId, profiles: [], @@ -8275,12 +8272,14 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} installedConnections: await resolveInstalledConnectionsForAgent(companyId, agentId), }; } - const profileIds = profileIdsInBindingOrder(bindings); - const profiles = await db + const candidateProfileIds = profileIdsInBindingOrder(matchingBindings); + const candidateProfiles = await db .select() .from(toolProfiles) - .where(and(eq(toolProfiles.companyId, companyId), inArray(toolProfiles.id, profileIds))); - const profilesById = new Map(profiles.map((profile) => [profile.id, profile])); + .where(and(eq(toolProfiles.companyId, companyId), inArray(toolProfiles.id, candidateProfileIds))); + const bindings = effectiveToolProfileBindings(matchingBindings, candidateProfiles); + const profileIds = profileIdsInBindingOrder(bindings); + const profilesById = new Map(candidateProfiles.map((profile) => [profile.id, profile])); const activeProfiles = profileIds .map((profileId) => profilesById.get(profileId) ?? null) .filter((profile): profile is typeof toolProfiles.$inferSelect => Boolean(profile && profile.status === "active")); diff --git a/server/src/services/tool-profile-binding-precedence.test.ts b/server/src/services/tool-profile-binding-precedence.test.ts new file mode 100644 index 00000000000..971fc465a4f --- /dev/null +++ b/server/src/services/tool-profile-binding-precedence.test.ts @@ -0,0 +1,93 @@ +import { describe, expect, it } from "vitest"; +import { + effectiveToolProfileBindings, + narrowestScopeBindings, +} from "./tool-profile-binding-precedence.js"; + +const createdAt = new Date("2026-08-11T00:00:00.000Z"); + +describe("tool profile binding precedence", () => { + it("keeps ordinary profiles at the narrowest matching scope", () => { + const companyBinding = { + profileId: "company-profile", + targetType: "company" as const, + targetId: "company-1", + priority: 100, + createdAt, + }; + const agentBinding = { + profileId: "agent-profile", + targetType: "agent" as const, + targetId: "agent-1", + priority: 100, + createdAt, + }; + + expect(narrowestScopeBindings([companyBinding, agentBinding])).toEqual([agentBinding]); + expect(effectiveToolProfileBindings( + [companyBinding, agentBinding], + [ + { id: "company-profile", profileKey: "company-default", metadata: {} }, + { id: "agent-profile", profileKey: "agent-default", metadata: {} }, + ], + "connection-1", + )).toEqual([agentBinding]); + }); + + it("carries the wizard-managed app assignment alongside a narrower profile", () => { + const appBinding = { + profileId: "app-profile", + targetType: "company" as const, + targetId: "company-1", + priority: 100, + createdAt, + }; + const agentBinding = { + profileId: "agent-profile", + targetType: "agent" as const, + targetId: "agent-1", + priority: 100, + createdAt, + }; + + expect(effectiveToolProfileBindings( + [appBinding, agentBinding], + [ + { + id: "app-profile", + profileKey: "app:connection-1", + metadata: { source: "app_gallery_finish", connectionId: "connection-1" }, + }, + { id: "agent-profile", profileKey: "agent-default", metadata: {} }, + ], + "connection-1", + )).toEqual([agentBinding, appBinding]); + }); + + it("does not overlay a wizard profile onto another connection", () => { + const appBinding = { + profileId: "app-profile", + targetType: "company" as const, + targetId: "company-1", + priority: 100, + createdAt, + }; + const agentBinding = { + profileId: "agent-profile", + targetType: "agent" as const, + targetId: "agent-1", + priority: 100, + createdAt, + }; + + expect(effectiveToolProfileBindings( + [appBinding, agentBinding], + [{ + id: "app-profile", + profileKey: "app:connection-1", + metadata: { source: "app_gallery_finish", connectionId: "connection-1" }, + }], + "connection-2", + )).toEqual([agentBinding]); + }); +}); diff --git a/server/src/services/tool-profile-binding-precedence.ts b/server/src/services/tool-profile-binding-precedence.ts index 0236841fed5..c67e1192b68 100644 --- a/server/src/services/tool-profile-binding-precedence.ts +++ b/server/src/services/tool-profile-binding-precedence.ts @@ -3,10 +3,17 @@ import type { ToolProfileBindingTargetType } from "@paperclipai/shared"; type BindingLike = { profileId: string; targetType: ToolProfileBindingTargetType; + targetId: string; priority: number; createdAt: Date | string; }; +type ProfileLike = { + id: string; + profileKey: string; + metadata: unknown; +}; + const TOOL_PROFILE_SCOPE_PRECEDENCE: Record = { // Named gateways bind one concrete MCP endpoint instance, so they should // override broader run, agent, and company defaults when both match. @@ -48,3 +55,39 @@ export function profileIdsInBindingOrder; + if (metadata.source !== "app_gallery_finish" || typeof metadata.connectionId !== "string") return false; + if (profile.profileKey !== `app:${metadata.connectionId}`) return false; + return connectionId === undefined || connectionId === null || metadata.connectionId === connectionId; +} + +/** + * App-wizard assignments are additive capabilities: choosing an app for all + * agents (or for one agent) must not disappear merely because that agent also + * has a narrower general-purpose profile. Ordinary profiles still use the + * narrowest-scope rule; only the profile managed by the app wizard is carried + * alongside that winning tier. + */ +export function effectiveToolProfileBindings( + bindings: T[], + profiles: ProfileLike[], + connectionId?: string | null, +): T[] { + const appProfileIds = new Set( + profiles.filter((profile) => isWizardAppProfile(profile, connectionId)).map((profile) => profile.id), + ); + const selected = [ + ...narrowestScopeBindings(bindings), + ...bindings.filter((binding) => appProfileIds.has(binding.profileId)), + ]; + const seen = new Set(); + return selected.filter((binding) => { + const key = `${binding.targetType}:${binding.targetId}:${binding.profileId}`; + if (seen.has(key)) return false; + seen.add(key); + return true; + }); +}