diff --git a/server/src/__tests__/tool-access-service.test.ts b/server/src/__tests__/tool-access-service.test.ts index 22a0c05990..291cec311a 100644 --- a/server/src/__tests__/tool-access-service.test.ts +++ b/server/src/__tests__/tool-access-service.test.ts @@ -4115,7 +4115,7 @@ describeEmbeddedPostgres("tool access service", () => { ]); }); - it("cancels stale pending action requests with invalid signatures before listing the review queue", async () => { + it("cancels invalid-signature pending action requests but keeps unsigned in-flight ones out of the review queue without cancelling them", async () => { vi.stubEnv("PAPERCLIP_TOOL_ACTION_SIGNING_SECRET", "current-secret"); const company = await createCompany(db); const [application] = await db.insert(toolApplications).values({ @@ -4207,7 +4207,10 @@ describeEmbeddedPostgres("tool access service", () => { expect(list.map((item) => item.request.id)).toEqual([validRequest.id]); expect(statusById.get(validRequest.id)).toBe("pending"); - expect(statusById.get(missingSignatureRequest.id)).toBe("cancelled"); + // An unsigned request is still being created; the read hides it but keeps it + // pending, so the creator can finish signing and the later approve succeeds. + expect(statusById.get(missingSignatureRequest.id)).toBe("pending"); + // A request signed with a rotated/old secret is unverifiable and is cancelled. expect(statusById.get(oldSecretRequest.id)).toBe("cancelled"); }); diff --git a/server/src/__tests__/tool-gateway.test.ts b/server/src/__tests__/tool-gateway.test.ts index 5a7d4cc0a3..8ce669a407 100644 --- a/server/src/__tests__/tool-gateway.test.ts +++ b/server/src/__tests__/tool-gateway.test.ts @@ -2495,6 +2495,207 @@ rl.on("line", (line) => { } }); + it("expires an abandoned unsigned ask-first request so a later retry can proceed", async () => { + // The gateway builds an ask-first request in two steps inside one call: it + // inserts the row with a null signature and a null expiry, then signs the + // row. If the gateway stops between the two steps, the row stays pending + // and unsigned forever, and the review queue hides it. A later retry of the + // same tool call must not replay that dead row. It must expire the row and + // create a fresh, signable request. + const company = await createCompany(db); + const agent = await createAgent(db, company.id); + const { run } = await createIssueAndRun(db, company.id, agent.id); + const fake = await startFakeRemoteMcpServer((fakeRequest) => ({ + body: { + jsonrpc: "2.0", + id: fakeRequest.body?.id, + result: { content: [{ type: "text", text: "should not run while pending approval" }] }, + }, + })); + + try { + const remoteTool = await createRemoteMcpTool(db, company.id, { + applicationKey: "abandoned-unsigned-app", + toolName: "kv_set", + url: fake.url, + }); + const remoteToolName = expectedConnectedToolName({ + applicationKey: remoteTool.application.applicationKey, + connectionId: remoteTool.connection.id, + toolName: remoteTool.catalogEntry.toolName, + }); + await allowToolsForAgent(db, company.id, agent.id, [remoteToolName]); + await db.insert(toolPolicies).values({ + companyId: company.id, + name: "Review abandoned connected writes", + policyType: "require_approval", + selectors: { connectionId: remoteTool.connection.id }, + priority: 10, + }); + + const gateway = createTestToolGatewayService(db); + const session = await gateway.createSession({ companyId: company.id, agentId: agent.id, runId: run.id }); + await gateway.executeTool({ + sessionToken: session.token, + tool: remoteToolName, + parameters: { key: "abandoned", value: "original" }, + }).then( + () => { + throw new Error("Expected connected MCP call to require approval"); + }, + (error) => expectGatewayError(error, 409, "approval_required"), + ); + + const [firstRequest] = await db + .select() + .from(toolActionRequests) + .where(eq(toolActionRequests.companyId, company.id)); + + // Rewind the row to the abandoned state: created long ago, pending, never + // signed. The old createdAt proves the create stopped, not that a parallel + // create still runs, so a later retry can expire the row. + await db + .update(toolActionRequests) + .set({ + signedArguments: null, + expiresAt: null, + interactionId: null, + createdAt: new Date(Date.now() - 10 * 60 * 1000), + updatedAt: new Date(), + }) + .where(eq(toolActionRequests.id, firstRequest.id)); + + // Retry the same tool call. The gateway must not replay the dead row. + await gateway.executeTool({ + sessionToken: session.token, + tool: remoteToolName, + parameters: { key: "abandoned", value: "original" }, + }).then( + () => { + throw new Error("Expected retry to require a fresh approval"); + }, + (error) => expectGatewayError(error, 409, "approval_required"), + ); + + // The abandoned row is expired, not replayed as a live approval. + const [afterRetry] = await db + .select() + .from(toolActionRequests) + .where(eq(toolActionRequests.id, firstRequest.id)); + expect(afterRetry.status).toBe("expired"); + + // The retry created a fresh, signed request that the queue can show. + const allRequests = await db + .select() + .from(toolActionRequests) + .where(eq(toolActionRequests.companyId, company.id)); + const freshRequest = allRequests.find((request) => request.id !== firstRequest.id); + expect(freshRequest).toBeTruthy(); + expect(freshRequest!.status).toBe("pending"); + expect(freshRequest!.signedArguments).not.toBeNull(); + expect(freshRequest!.expiresAt).not.toBeNull(); + + // The tool never executed while the approval stayed pending. + expect(fake.requests).toHaveLength(0); + } finally { + await fake.close(); + } + }); + + it("does not expire a recent unsigned ask-first request that a parallel create still owns", async () => { + // The create signs the row in two steps. A concurrent matching call can see + // the row after the insert but before the sign. A recent createdAt means a + // parallel create still runs, so the concurrent call must replay the live + // request. It must not expire the row and must not create a duplicate. + const company = await createCompany(db); + const agent = await createAgent(db, company.id); + const { run } = await createIssueAndRun(db, company.id, agent.id); + const fake = await startFakeRemoteMcpServer((fakeRequest) => ({ + body: { + jsonrpc: "2.0", + id: fakeRequest.body?.id, + result: { content: [{ type: "text", text: "should not run while pending approval" }] }, + }, + })); + + try { + const remoteTool = await createRemoteMcpTool(db, company.id, { + applicationKey: "inflight-unsigned-app", + toolName: "kv_set", + url: fake.url, + }); + const remoteToolName = expectedConnectedToolName({ + applicationKey: remoteTool.application.applicationKey, + connectionId: remoteTool.connection.id, + toolName: remoteTool.catalogEntry.toolName, + }); + await allowToolsForAgent(db, company.id, agent.id, [remoteToolName]); + await db.insert(toolPolicies).values({ + companyId: company.id, + name: "Review inflight connected writes", + policyType: "require_approval", + selectors: { connectionId: remoteTool.connection.id }, + priority: 10, + }); + + const gateway = createTestToolGatewayService(db); + const session = await gateway.createSession({ companyId: company.id, agentId: agent.id, runId: run.id }); + await gateway.executeTool({ + sessionToken: session.token, + tool: remoteToolName, + parameters: { key: "inflight", value: "original" }, + }).then( + () => { + throw new Error("Expected connected MCP call to require approval"); + }, + (error) => expectGatewayError(error, 409, "approval_required"), + ); + + const [firstRequest] = await db + .select() + .from(toolActionRequests) + .where(eq(toolActionRequests.companyId, company.id)); + + // Rewind to the in-flight window: unsigned, but created just now. This is + // the state a concurrent matching call sees while the create still runs. + await db + .update(toolActionRequests) + .set({ signedArguments: null, expiresAt: null, interactionId: null, updatedAt: new Date() }) + .where(eq(toolActionRequests.id, firstRequest.id)); + + // The concurrent matching call replays the live request; it does not expire. + await gateway.executeTool({ + sessionToken: session.token, + tool: remoteToolName, + parameters: { key: "inflight", value: "original" }, + }).then( + () => { + throw new Error("Expected the concurrent call to require approval"); + }, + (error) => expectGatewayError(error, 409, "approval_required"), + ); + + // The row stays pending and is not expired. + const [afterRetry] = await db + .select() + .from(toolActionRequests) + .where(eq(toolActionRequests.id, firstRequest.id)); + expect(afterRetry.status).toBe("pending"); + + // No duplicate request was created for the same tool call. + const allRequests = await db + .select() + .from(toolActionRequests) + .where(eq(toolActionRequests.companyId, company.id)); + expect(allRequests).toHaveLength(1); + + // The tool never executed while the approval stayed pending. + expect(fake.requests).toHaveLength(0); + } finally { + await fake.close(); + } + }); + it("requires re-review when an approved connected MCP replay credential latest version changed", async () => { const company = await createCompany(db); const agent = await createAgent(db, company.id); diff --git a/server/src/__tests__/tool-review-queue-unsigned-request.test.ts b/server/src/__tests__/tool-review-queue-unsigned-request.test.ts new file mode 100644 index 0000000000..4c8f392d82 --- /dev/null +++ b/server/src/__tests__/tool-review-queue-unsigned-request.test.ts @@ -0,0 +1,157 @@ +// Regression: the tool review queue must not cancel an ask-first action request +// that the gateway has created but not signed yet. +// +// The gateway builds a require-approval action request in two steps: it first +// inserts the row with a null signature, then signs the row a moment later. A +// review-queue read (listActionRequests) that lands inside that window must hide +// the unsigned row from the queue, but must leave it pending. If the read +// cancels the row, the following approve call fails with action_not_pending. +import { randomUUID } from "node:crypto"; +import { eq } from "drizzle-orm"; +import { afterAll, beforeAll, describe, expect, it, vi } from "vitest"; +import { + companies, + createDb, + toolApplications, + toolActionRequests, + toolCatalogEntries, + toolConnections, + toolInvocations, +} from "@paperclipai/db"; +import { toolAccessService } from "../services/tool-access.js"; +import { createToolGatewayService } from "../services/tool-gateway.js"; +import { canonicalToolArguments, signToolArguments } from "../services/tool-content-guards.js"; +import { + getEmbeddedPostgresTestSupport, + startEmbeddedPostgresTestDatabase, +} from "./helpers/embedded-postgres.js"; + +const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport(); +const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip; +const signingSecret = "review-queue-regression-secret"; + +describeEmbeddedPostgres("tool review queue vs unsigned ask-first request", () => { + let db!: ReturnType; + let tempDb: Awaited> | null = null; + + beforeAll(async () => { + tempDb = await startEmbeddedPostgresTestDatabase("paperclip-review-queue-unsigned-"); + db = createDb(tempDb.connectionString); + }, 20_000); + + afterAll(async () => { + await tempDb?.cleanup(); + }); + + it("hides an unsigned pending request from the queue, keeps it pending, and lets approve succeed once it is signed", async () => { + vi.stubEnv("PAPERCLIP_TOOL_ACTION_SIGNING_SECRET", signingSecret); + const [company] = await db.insert(companies).values({ + name: `Review Queue ${randomUUID()}`, + issuePrefix: `RQ${randomUUID().slice(0, 6).toUpperCase()}`, + }).returning(); + const [application] = await db.insert(toolApplications).values({ + companyId: company.id, + name: `Review Queue app ${randomUUID()}`, + type: "mcp_http", + status: "active", + }).returning(); + const [connection] = await db.insert(toolConnections).values({ + companyId: company.id, + applicationId: application.id, + name: `Review Queue connection ${randomUUID()}`, + uid: `test/${randomUUID()}`, + transport: "mcp_remote", + status: "active", + enabled: true, + config: { url: "https://fixture.example/mcp" }, + }).returning(); + const [catalogEntry] = await db.insert(toolCatalogEntries).values({ + companyId: company.id, + applicationId: application.id, + connectionId: connection.id, + name: "sheets:update_cell", + toolName: "sheets:update_cell", + title: "Update sheet cell", + riskLevel: "write", + isWrite: true, + status: "active", + versionHash: "v1", + schemaHash: "s1", + }).returning(); + + const parameters = { cell: "B1", value: "first" }; + const canonicalArguments = canonicalToolArguments(parameters); + + // A test-origin ask-first invocation, exactly as recordInvocation records it. + const [invocation] = await db.insert(toolInvocations).values({ + companyId: company.id, + actorType: "user", + actorId: "board", + agentId: null, + runId: null, + issueId: null, + applicationId: application.id, + connectionId: connection.id, + catalogEntryId: catalogEntry.id, + toolName: "sheets:update_cell", + argumentsHash: "args-hash", + argumentsSummary: { summary: canonicalArguments, sha256: "args-hash", sizeBytes: canonicalArguments.length }, + policyDecision: "require_approval", + approvalState: "pending", + status: "awaiting_approval", + }).returning(); + + // Step one of the two-step create: the row exists, pending, not yet signed. + const [actionRequest] = await db.insert(toolActionRequests).values({ + companyId: company.id, + invocationId: invocation.id, + status: "pending", + canonicalArgumentsHash: "args-hash", + canonicalArgumentsSummary: { summary: canonicalArguments, sha256: "args-hash", sizeBytes: canonicalArguments.length }, + signedArguments: null, + }).returning(); + + // A concurrent review-queue read lands inside the create window. + const listedDuringCreate = await toolAccessService(db).listActionRequests(company.id, "pending"); + expect(listedDuringCreate.map((item) => item.request.id)).not.toContain(actionRequest.id); + + const [afterRead] = await db + .select() + .from(toolActionRequests) + .where(eq(toolActionRequests.id, actionRequest.id)); + expect(afterRead.status).toBe("pending"); + + // Step two of the create: the gateway signs the row. + const signedArguments = signToolArguments({ + invocationId: invocation.id, + toolName: invocation.toolName, + canonicalArguments, + executionOnApprove: true, + signingSecret, + }); + await db + .update(toolActionRequests) + .set({ signedArguments, updatedAt: new Date() }) + .where(eq(toolActionRequests.id, actionRequest.id)); + + // The queue now shows the signed request. + const listedAfterSign = await toolAccessService(db).listActionRequests(company.id, "pending"); + expect(listedAfterSign.map((item) => item.request.id)).toContain(actionRequest.id); + + // Approve no longer races a cancelled row. + const gateway = createToolGatewayService(db, { toolActionSigningSecret: signingSecret }); + await expect( + gateway.approveActionRequest({ + companyId: company.id, + actionRequestId: actionRequest.id, + actor: { userId: "board" }, + }), + ).resolves.toBeTruthy(); + + const [afterApprove] = await db + .select() + .from(toolActionRequests) + .where(eq(toolActionRequests.id, actionRequest.id)); + expect(["approved", "executed", "failed"]).toContain(afterApprove.status); + }); +}); diff --git a/server/src/services/tool-access.ts b/server/src/services/tool-access.ts index 3ef102ab92..90e0bab144 100644 --- a/server/src/services/tool-access.ts +++ b/server/src/services/tool-access.ts @@ -6749,21 +6749,37 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} const invocationById = new Map(invocations.map((invocation) => [invocation.id, invocation])); let visibleRequests = requests; if (status === "pending") { - const invalidRequestIds = requests - .filter((request) => { - const invocation = invocationById.get(request.invocationId); - if (!invocation) return true; - try { - return !readSignedToolArgumentsPayload({ - signedArguments: request.signedArguments, - invocationId: invocation.id, - toolName: invocation.toolName, - }); - } catch { - return true; - } - }) - .map((request) => request.id); + // A pending request that the creator has not signed yet is still being + // set up. The gateway creates the row (signedArguments = null) and signs + // it in a second step, so a review-queue read can observe the row inside + // that window. Hide such a request from the queue, but do not cancel it — + // cancelling here races the two-step create and makes the later approve + // fail with action_not_pending. Only cancel a request that carries a + // signature we cannot verify (secret rotation or tampering). + const unsignedRequestIds = new Set(); + const invalidRequestIds: string[] = []; + for (const request of requests) { + const invocation = invocationById.get(request.invocationId); + if (!invocation) { + invalidRequestIds.push(request.id); + continue; + } + if (request.signedArguments === null) { + unsignedRequestIds.add(request.id); + continue; + } + let readable = false; + try { + readable = Boolean(readSignedToolArgumentsPayload({ + signedArguments: request.signedArguments, + invocationId: invocation.id, + toolName: invocation.toolName, + })); + } catch { + readable = false; + } + if (!readable) invalidRequestIds.push(request.id); + } if (invalidRequestIds.length > 0) { await db .update(toolActionRequests) @@ -6773,8 +6789,10 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} eq(toolActionRequests.status, "pending"), inArray(toolActionRequests.id, invalidRequestIds), )); - const invalidIds = new Set(invalidRequestIds); - visibleRequests = requests.filter((request) => !invalidIds.has(request.id)); + } + const hiddenIds = new Set([...invalidRequestIds, ...unsignedRequestIds]); + if (hiddenIds.size > 0) { + visibleRequests = requests.filter((request) => !hiddenIds.has(request.id)); } } if (visibleRequests.length === 0) return []; diff --git a/server/src/services/tool-gateway.ts b/server/src/services/tool-gateway.ts index dcb7680694..5c86958d3a 100644 --- a/server/src/services/tool-gateway.ts +++ b/server/src/services/tool-gateway.ts @@ -85,6 +85,14 @@ const DEFAULT_TOOL_TIMEOUT_MS = 10_000; // `tool_timeout` even though the approval succeeded. Give approved executions // the full permitted headroom instead. const APPROVED_EXECUTION_TIMEOUT_MS = 60_000; +// The gateway creates an ask-first request in two steps: it inserts the row +// with a null signature, then it signs the row and sets the expiry. A concurrent +// matching call can observe the row in this window. A null signature alone does +// not prove the create stopped; the create can still run in another request. Only +// treat an unsigned row as abandoned after this grace time from createdAt. This +// grace must exceed the normal sign path (approval-snapshot fetch + interaction +// create) so a live create keeps its own row. +const UNSIGNED_ACTION_REQUEST_ABANDON_MS = 2 * 60 * 1000; const MAX_REMOTE_MCP_RESPONSE_BYTES = 1_000_000; const ACTIVE_GATEWAY_RUN_STATUSES = new Set(["running"]); const uuidPattern = /^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i; @@ -1687,7 +1695,13 @@ export function createToolGatewayService( { agentId: input.session.agentId }, ); - await db + // Sign the row only while it is still pending. A concurrent matching call can + // expire this row when the create runs longer than the abandon grace time. + // Guard the update with the pending status so the sign step never resurrects + // an expired row. When the guard matches no row, the row is already resolved. + // Do not emit an approval card for a dead row; throw a retriable conflict so + // the retry finds the live request or creates a fresh, signable one. + const signedRows = await db .update(toolActionRequests) .set({ interactionId: interaction.id, @@ -1699,7 +1713,32 @@ export function createToolGatewayService( expiresAt, updatedAt: new Date(), }) - .where(eq(toolActionRequests.id, actionRequest.id)); + .where(and(eq(toolActionRequests.id, actionRequest.id), eq(toolActionRequests.status, "pending"))) + .returning({ id: toolActionRequests.id }); + if (signedRows.length === 0) { + await db + .update(toolInvocations) + .set({ + status: "failed", + approvalState: "expired", + errorCode: "approval_request_superseded", + errorMessage: "The approval request was resolved before it could be signed", + completedAt: new Date(), + updatedAt: new Date(), + }) + .where(eq(toolInvocations.id, input.invocation.id)); + throw new ToolGatewayHttpError( + 409, + "The approval request was resolved before it could be signed", + "approval_request_superseded", + { + invocationId: input.invocation.id, + actionRequestId: actionRequest.id, + tool: input.tool.name, + instructions: "A parallel call already handled this approval. Retry the same call now to reach the live approval request.", + }, + ); + } await writeToolCallEvent({ invocationId: input.invocation.id, @@ -4325,11 +4364,28 @@ export function createToolGatewayService( .orderBy(desc(toolActionRequests.createdAt)) .limit(1); if (!match) return null; - if ( - match.actionRequest.status === "pending" - && match.actionRequest.expiresAt - && match.actionRequest.expiresAt.getTime() <= Date.now() - ) { + // The gateway builds an ask-first request in two steps inside one call: it + // inserts the row with a null signature and a null expiry, then signs the + // row and sets the expiry. A concurrent matching call can observe the row in + // this window. A null signature does not prove the create stopped, because a + // parallel create can still be signing the same row right now. Only treat an + // unsigned row as abandoned after the grace time from createdAt has passed; + // before that, return the match so the retry replays approval_required and + // does not create a duplicate request or expire a live row. After the grace + // time an unsigned row stays pending forever and the review queue hides it, + // so it can never be approved. Do not replay it as a live approval. Expire + // the row and let the retry create a fresh, signable request. A null expiry + // alone (without this guard) also makes the getTime() check below unsafe. + const pendingRequest = match.actionRequest; + const pendingUnsigned = + pendingRequest.status === "pending" + && pendingRequest.signedArguments === null + && Date.now() - pendingRequest.createdAt.getTime() >= UNSIGNED_ACTION_REQUEST_ABANDON_MS; + const pendingExpired = + pendingRequest.status === "pending" + && pendingRequest.expiresAt !== null + && pendingRequest.expiresAt.getTime() <= Date.now(); + if (pendingUnsigned || pendingExpired) { const now = new Date(); await db.update(toolActionRequests).set({ status: "expired", resolvedAt: now, updatedAt: now }).where(and( eq(toolActionRequests.id, match.actionRequest.id),