diff --git a/.changeset/secure-toolkit-workspace-policies.md b/.changeset/secure-toolkit-workspace-policies.md new file mode 100644 index 0000000000..9382c718e2 --- /dev/null +++ b/.changeset/secure-toolkit-workspace-policies.md @@ -0,0 +1,5 @@ +--- +"@executor-js/sdk": patch +--- + +Enforce workspace approval and block policies when tools run through a toolkit-scoped executor. diff --git a/apps/cloud/src/mcp/session-build-semaphore.test.ts b/apps/cloud/src/mcp/session-build-semaphore.test.ts index 3d4ad76343..584b65ee0e 100644 --- a/apps/cloud/src/mcp/session-build-semaphore.test.ts +++ b/apps/cloud/src/mcp/session-build-semaphore.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it, beforeEach } from "@effect/vitest"; +import { describe, expect, it, beforeEach, afterEach, vi } from "@effect/vitest"; import { acquireBuildSlot, @@ -13,6 +13,10 @@ describe("session-build-semaphore", () => { resetBuildSlotsForTest(); }); + afterEach(() => { + vi.useRealTimers(); + }); + it("grants up to the cap immediately, with no wait", async () => { const results = await Promise.all([ acquireBuildSlot().promise, @@ -214,6 +218,7 @@ describe("session-build-semaphore", () => { }); it("proceeds without a slot when the queue wait exceeds the timeout, and does not count it as active", async () => { + vi.useFakeTimers(); await Promise.all([ acquireBuildSlot().promise, acquireBuildSlot().promise, @@ -223,6 +228,10 @@ describe("session-build-semaphore", () => { expect(currentActiveBuildsForTest()).toBe(4); const timedOutHandle = acquireBuildSlot(10); + await vi.advanceTimersByTimeAsync(9); + expect(currentQueueLengthForTest()).toBe(1); + expect(currentActiveBuildsForTest()).toBe(4); + await vi.advanceTimersByTimeAsync(1); const result = await timedOutHandle.promise; expect(result).toEqual({ acquired: false, waitMs: expect.any(Number), timedOut: true }); diff --git a/e2e/scenarios/toolkits-mcp.test.ts b/e2e/scenarios/toolkits-mcp.test.ts index b84eed1e15..4dee74e7b2 100644 --- a/e2e/scenarios/toolkits-mcp.test.ts +++ b/e2e/scenarios/toolkits-mcp.test.ts @@ -769,3 +769,73 @@ scenario( }), ), ); + +scenario( + "Toolkits · workspace approval and block survive a toolkit approve rule", + { timeout: 240_000 }, + Effect.gen(function* () { + const target = yield* Target; + const mcp = yield* Mcp; + const apiSurface = yield* Api; + const identity = yield* target.newIdentity(); + const client = yield* apiSurface.client(api, identity); + const name = unique("workspace-policy-kit"); + const createdPattern = `${unique("workspace-gated-result")}.*`; + const toolPattern = "executor.coreTools.policies.create"; + yield* Effect.gen(function* () { + const toolkit = yield* client.toolkits.create({ payload: { owner: "org", name } }); + yield* client.toolkits.createConnection({ + params: { toolkitId: toolkit.id }, + payload: { pattern: toolPattern }, + }); + yield* client.toolkits.createPolicy({ + params: { toolkitId: toolkit.id }, + payload: { pattern: toolPattern, action: "approve" }, + }); + yield* client.policies.create({ + payload: { owner: "org", pattern: toolPattern, action: "require_approval" }, + }); + const session = mcp.session(identity, { url: toolkitUrl(target.baseUrl, toolkit.slug) }); + const paused = yield* session.call("execute", { + code: createPolicyCode({ pattern: createdPattern, action: "block" }), + }); + expect(paused.text).toContain("Execution paused"); + expect((yield* client.policies.list()).some((p) => p.pattern === createdPattern)).toBe(false); + const resumed = yield* session.approvePaused(paused.text); + expect(resumed.ok).toBe(true); + expect((yield* client.policies.list()).some((p) => p.pattern === createdPattern)).toBe(true); + yield* client.policies.create({ + payload: { owner: "org", pattern: toolPattern, action: "block" }, + }); + const blockedPattern = `${createdPattern}blocked`; + const blockedSession = mcp.session(identity, { + url: toolkitUrl(target.baseUrl, toolkit.slug), + }); + const blocked = yield* blockedSession.call("execute", { + code: createPolicyCode({ pattern: blockedPattern, action: "block" }), + }); + expect(blocked.text).not.toContain("Execution paused"); + expect((yield* client.policies.list()).some((p) => p.pattern === blockedPattern)).toBe(false); + }).pipe( + Effect.ensuring( + Effect.gen(function* () { + const listed = yield* client.toolkits.list(); + yield* Effect.forEach( + listed.toolkits.filter((t) => t.name === name), + (t) => client.toolkits.remove({ params: { toolkitId: t.id } }), + { discard: true }, + ); + const policies = yield* client.policies.list(); + yield* Effect.forEach( + policies.filter( + (p) => p.pattern === toolPattern || p.pattern.startsWith(createdPattern), + ), + (p) => + client.policies.remove({ params: { policyId: p.id }, payload: { owner: p.owner } }), + { discard: true }, + ); + }).pipe(Effect.ignore), + ), + ); + }), +); diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index efaf119212..7303596daa 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -153,6 +153,7 @@ import { } from "./oauth-client"; import type { FirstPartyOAuthClientConfig } from "./oauth-client"; import { + combineEffectivePolicies, comparePolicyRow, isValidPattern, matchPattern, @@ -5290,6 +5291,7 @@ export const createExecutor = EffectivePolicy; + readonly workspaceRows: readonly ToolPolicyRow[]; }; const compareProviderPolicyRule = ( @@ -5331,53 +5334,63 @@ export const createExecutor = => - activeToolPolicyProvider - ? // Batched per-operation resolver: fetch all policy + connection state - // once, then resolve every tool in this operation against that - // snapshot. Avoids the per-tool resolve N+1 on the list surface. - activeToolPolicyProvider.prepare - ? activeToolPolicyProvider.prepare().pipe( - Effect.map((resolve) => ({ - kind: "prepared" as const, - resolve, - })), - ) - : activeToolPolicyProvider.resolve - ? Effect.succeed({ - kind: "provider" as const, - provider: activeToolPolicyProvider, - rules: null, - }) - : activeToolPolicyProvider.list().pipe( - Effect.map((rules) => ({ - kind: "provider" as const, - provider: activeToolPolicyProvider!, - rules, - })), - ) - : core - .findMany("tool_policy", {}) - .pipe(Effect.map((rows) => ({ kind: "global" as const, rows }))); + Effect.gen(function* () { + // Fetch workspace policy once per operation, then reuse the provider's + // prepared snapshot for every tool on the list surface. + const workspaceRows = yield* core.findMany("tool_policy", {}); + if (!activeToolPolicyProvider) { + return { kind: "global" as const, rows: workspaceRows }; + } + if (activeToolPolicyProvider.prepare) { + const resolve = yield* activeToolPolicyProvider.prepare(); + return { kind: "prepared" as const, resolve, workspaceRows }; + } + if (activeToolPolicyProvider.resolve) { + return { + kind: "provider" as const, + provider: activeToolPolicyProvider, + rules: null, + workspaceRows, + }; + } + const rules = yield* activeToolPolicyProvider.list(); + return { + kind: "provider" as const, + provider: activeToolPolicyProvider, + rules, + workspaceRows, + }; + }); const resolvePolicyFromRuleSet = ( toolId: string, ruleSet: ActivePolicyRuleSet, defaultRequiresApproval?: boolean, ): Effect.Effect => - ruleSet.kind === "prepared" - ? Effect.succeed(ruleSet.resolve({ toolId, defaultRequiresApproval })) - : ruleSet.kind === "provider" - ? ruleSet.provider.resolve - ? ruleSet.provider.resolve({ toolId, defaultRequiresApproval }) - : Effect.succeed(resolveProviderPolicyFromRules(toolId, ruleSet.rules ?? [])) - : Effect.succeed( - resolveEffectivePolicy( - toolId, - ruleSet.rows, - ownerRankForRow, - defaultRequiresApproval, - ), - ); + Effect.gen(function* () { + if (ruleSet.kind === "global") { + return resolveEffectivePolicy( + toolId, + ruleSet.rows, + ownerRankForRow, + defaultRequiresApproval, + ); + } + + const workspacePolicy = resolveEffectivePolicy( + toolId, + ruleSet.workspaceRows, + ownerRankForRow, + defaultRequiresApproval, + ); + const providerPolicy = + ruleSet.kind === "prepared" + ? ruleSet.resolve({ toolId, defaultRequiresApproval }) + : ruleSet.provider.resolve + ? yield* ruleSet.provider.resolve({ toolId, defaultRequiresApproval }) + : resolveProviderPolicyFromRules(toolId, ruleSet.rules ?? []); + return combineEffectivePolicies(providerPolicy, workspacePolicy); + }); // ------------------------------------------------------------------ // Tools (read surface) @@ -5954,7 +5967,6 @@ export const createExecutor = => Effect.gen(function* () { const parsed = parseToolAddress(String(address)); - const policyRows = yield* core.findMany("tool_policy", {}); const toolId = parsed ? `${parsed.integration}.${parsed.owner}.${parsed.connection}.${parsed.tool}` : String(address); @@ -5975,7 +5987,8 @@ export const createExecutor = { }); }); +describe("combineEffectivePolicies", () => { + const user = (action: "approve" | "require_approval" | "block", pattern: string) => ({ + action, + source: "user" as const, + pattern, + }); + const pluginDefault = (action: "approve" | "require_approval") => ({ + action, + source: "plugin-default" as const, + }); + + it("keeps a provider capability-boundary block", () => { + expect(combineEffectivePolicies(user("block", "*"), user("approve", "sample.*"))).toEqual( + user("block", "*"), + ); + }); + + it("keeps a workspace block", () => { + expect( + combineEffectivePolicies(user("approve", "sample.ctl.read"), user("block", "sample.*")), + ).toEqual(user("block", "sample.*")); + }); + + it("keeps workspace approval when the toolkit approves", () => { + expect( + combineEffectivePolicies( + user("approve", "sample.ctl.read"), + user("require_approval", "sample.*"), + ), + ).toEqual(user("require_approval", "sample.*")); + }); + + it("uses an explicit rule over a plugin default", () => { + expect( + combineEffectivePolicies( + user("approve", "sample.ctl.read"), + pluginDefault("require_approval"), + ), + ).toEqual(user("approve", "sample.ctl.read")); + }); + + it("uses the more restrictive result when both are plugin defaults", () => { + expect( + combineEffectivePolicies(pluginDefault("approve"), pluginDefault("require_approval")), + ).toEqual(pluginDefault("require_approval")); + }); +}); + // --------------------------------------------------------------------------- // Executor integration — v2 surface. A test plugin produces per-connection // tools via `resolveTools`; policies are owner-scoped; tools are addressed by @@ -695,6 +744,100 @@ describe("active tool-policy provider", () => { expect(Predicate.isTagged("ToolBlockedError")(blocked.failure)).toBe(true); }), ); + + it.effect("enforces workspace require_approval over provider approve", () => + Effect.gen(function* () { + const executor = yield* makeTestExecutor({ + plugins: [staticPlugin, policyProviderPlugin] as const, + }); + yield* executor.policies.create({ + owner: "org", + pattern: "toolkit-fixture.ctl.allowed", + action: "require_approval", + }); + + const calls = { count: 0 }; + const result = yield* executor.execute( + ToolAddress.make("toolkit-fixture.ctl.allowed"), + {}, + { onElicitation: recordingHandler(calls) }, + ); + expect(result).toBe("allowed"); + expect(calls.count).toBe(1); + }), + ); + + it.effect("enforces workspace block over provider approve", () => + Effect.gen(function* () { + const executor = yield* makeTestExecutor({ + plugins: [staticPlugin, policyProviderPlugin] as const, + }); + yield* executor.policies.create({ + owner: "org", + pattern: "toolkit-fixture.ctl.allowed", + action: "block", + }); + + expect(yield* executor.tools.list()).toHaveLength(0); + const policy = yield* executor.policies.resolve( + ToolAddress.make("toolkit-fixture.ctl.allowed"), + ); + expect(policy.action).toBe("block"); + + const result = yield* Effect.result( + executor.execute(ToolAddress.make("toolkit-fixture.ctl.allowed"), {}), + ); + expect(Result.isFailure(result)).toBe(true); + if (!Result.isFailure(result)) return; + expect(Predicate.isTagged("ToolBlockedError")(result.failure)).toBe(true); + }), + ); + + it.effect("combines a prepared provider with workspace policies", () => + Effect.gen(function* () { + const preparedProviderPlugin = definePlugin(() => ({ + id: "prepared-policy-provider" as const, + storage: () => ({}), + toolPolicyProvider: () => ({ + list: () => Effect.succeed([]), + prepare: () => + Effect.succeed((input: { readonly toolId: string }) => + input.toolId === "toolkit-fixture.ctl.allowed" + ? { + action: "approve" as const, + source: "user" as const, + pattern: "toolkit-fixture.ctl.allowed", + } + : { action: "block" as const, source: "user" as const, pattern: "*" }, + ), + }), + }))(); + const executor = yield* makeTestExecutor({ + plugins: [staticPlugin, preparedProviderPlugin] as const, + }); + yield* executor.policies.create({ + owner: "org", + pattern: "toolkit-fixture.ctl.allowed", + action: "require_approval", + }); + + const calls = { count: 0 }; + yield* executor.execute( + ToolAddress.make("toolkit-fixture.ctl.allowed"), + {}, + { onElicitation: recordingHandler(calls) }, + ); + expect(calls.count).toBe(1); + + const hidden = yield* Effect.result( + executor.execute(ToolAddress.make("toolkit-fixture.ctl.hidden"), {}), + ); + expect(Result.isFailure(hidden)).toBe(true); + expect( + (yield* executor.policies.resolve(ToolAddress.make("toolkit-fixture.ctl.hidden"))).action, + ).toBe("block"); + }), + ); }); describe("approve / require_approval interaction with annotations", () => { diff --git a/packages/core/sdk/src/policies.ts b/packages/core/sdk/src/policies.ts index 8620d9c6d3..b2bd5c0e0d 100644 --- a/packages/core/sdk/src/policies.ts +++ b/packages/core/sdk/src/policies.ts @@ -198,6 +198,22 @@ const moreRestrictive = ( return candidateRank > currentRank ? candidate : current; }; +export const combineEffectivePolicies = ( + providerPolicy: EffectivePolicy, + workspacePolicy: EffectivePolicy, +): EffectivePolicy => { + if (providerPolicy.action === "block") return providerPolicy; + if (workspacePolicy.action === "block") return workspacePolicy; + + if (providerPolicy.source === "user" && workspacePolicy.source === "user") { + return moreRestrictive(providerPolicy, workspacePolicy); + } + + if (workspacePolicy.source === "user") return workspacePolicy; + if (providerPolicy.source === "user") return providerPolicy; + return moreRestrictive(providerPolicy, workspacePolicy); +}; + export const resolveToolPolicy = ( toolId: string, policies: readonly ToolPolicyRow[], diff --git a/packages/plugins/toolkits/src/server.test.ts b/packages/plugins/toolkits/src/server.test.ts index bab67eb0e9..03a8e704c6 100644 --- a/packages/plugins/toolkits/src/server.test.ts +++ b/packages/plugins/toolkits/src/server.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from "@effect/vitest"; -import { Effect, Predicate, Result } from "effect"; -import { makeTestExecutor } from "@executor-js/sdk/testing"; +import { Effect, Predicate, Result, Schema } from "effect"; +import { createExecutor, definePlugin, tool, ToolAddress } from "@executor-js/sdk"; +import { makeTestExecutor, makeTestWorkspaceHarness } from "@executor-js/sdk/testing"; import { toolkitsPlugin } from "./server"; @@ -199,4 +200,112 @@ describe("toolkitsPlugin", () => { ).toContain("google_docs.org.* approve"); }), ); + + it.effect("enforces workspace policies in a toolkit-scoped executor", () => + Effect.gen(function* () { + const samplePlugin = definePlugin(() => ({ + id: "sample" as const, + storage: () => ({}), + staticIntegrations: () => [ + { + kind: "control" as const, + id: "sample.ctl", + name: "Sample Control", + tools: [ + tool({ + name: "readTool", + description: "read tool", + inputSchema: Schema.toStandardSchemaV1( + Schema.toStandardJSONSchemaV1(Schema.Struct({})), + ), + execute: () => Effect.succeed("read-data"), + }), + tool({ + name: "deleteTool", + description: "delete tool", + inputSchema: Schema.toStandardSchemaV1( + Schema.toStandardJSONSchemaV1(Schema.Struct({})), + ), + execute: () => Effect.succeed("deleted"), + }), + tool({ + name: "outsideTool", + description: "outside toolkit", + inputSchema: Schema.toStandardSchemaV1( + Schema.toStandardJSONSchemaV1(Schema.Struct({})), + ), + execute: () => Effect.succeed("outside"), + }), + ], + }, + ], + }))(); + const harness = yield* makeTestWorkspaceHarness({ + plugins: [toolkitsPlugin(), samplePlugin] as const, + }); + const setup = harness.executor; + const toolkit = yield* setup.toolkits.create({ owner: "org", name: "Test Kit" }); + yield* setup.toolkits.createConnection(toolkit.id, { + pattern: "sample.ctl.readTool", + }); + yield* setup.toolkits.createConnection(toolkit.id, { + pattern: "sample.ctl.deleteTool", + }); + yield* setup.toolkits.createPolicy(toolkit.id, { + pattern: "sample.ctl.readTool", + action: "approve", + }); + yield* setup.policies.create({ + owner: "org", + pattern: "sample.ctl.readTool", + action: "require_approval", + }); + yield* setup.policies.create({ + owner: "org", + pattern: "sample.ctl.deleteTool", + action: "block", + }); + + const scoped = yield* Effect.acquireRelease( + createExecutor({ + ...harness.config, + plugins: [toolkitsPlugin({ activeToolkitSlug: toolkit.slug }), samplePlugin] as const, + }), + (executor) => executor.close().pipe(Effect.ignore), + ); + const tools = yield* scoped.tools.list(); + expect(tools.map((entry) => String(entry.address))).toEqual(["sample.ctl.readTool"]); + expect((yield* scoped.policies.resolve(ToolAddress.make("sample.ctl.readTool"))).action).toBe( + "require_approval", + ); + expect( + (yield* scoped.policies.resolve(ToolAddress.make("sample.ctl.deleteTool"))).action, + ).toBe("block"); + expect( + (yield* scoped.policies.resolve(ToolAddress.make("sample.ctl.outsideTool"))).action, + ).toBe("block"); + + let elicited = false; + expect( + yield* scoped.execute( + ToolAddress.make("sample.ctl.readTool"), + {}, + { + onElicitation: () => { + elicited = true; + return Effect.succeed({ action: "accept" as const }); + }, + }, + ), + ).toBe("read-data"); + expect(elicited).toBe(true); + + const blocked = yield* Effect.result( + scoped.execute(ToolAddress.make("sample.ctl.deleteTool"), {}), + ); + expect(Result.isFailure(blocked)).toBe(true); + if (!Result.isFailure(blocked)) return; + expect(Predicate.isTagged("ToolBlockedError")(blocked.failure)).toBe(true); + }), + ); });