From 4a27bb064c034811be0fde14e14d027b78d5be89 Mon Sep 17 00:00:00 2001 From: Dotta Date: Mon, 20 Apr 2026 08:22:52 -0500 Subject: [PATCH] Enforce active checkout ownership on issue mutations Co-Authored-By: Paperclip --- packages/shared/src/constants.ts | 1 + ...ue-agent-mutation-ownership-routes.test.ts | 356 ++++++++++++++++++ server/src/routes/issues.ts | 83 +++- ui/src/pages/CompanyAccess.tsx | 1 + 4 files changed, 435 insertions(+), 6 deletions(-) create mode 100644 server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts diff --git a/packages/shared/src/constants.ts b/packages/shared/src/constants.ts index 035e7bb7edb..c3f0dbd9d0a 100644 --- a/packages/shared/src/constants.ts +++ b/packages/shared/src/constants.ts @@ -432,6 +432,7 @@ export const PERMISSION_KEYS = [ "users:manage_permissions", "tasks:assign", "tasks:assign_scope", + "tasks:manage_active_checkouts", "joins:approve", ] as const; export type PermissionKey = (typeof PERMISSION_KEYS)[number]; diff --git a/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts b/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts new file mode 100644 index 00000000000..44fa50d92a2 --- /dev/null +++ b/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts @@ -0,0 +1,356 @@ +import { Readable } from "node:stream"; +import express from "express"; +import request from "supertest"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { errorHandler } from "../middleware/index.js"; +import { issueRoutes } from "../routes/issues.js"; + +const issueId = "11111111-1111-4111-8111-111111111111"; +const companyId = "22222222-2222-4222-8222-222222222222"; +const ownerAgentId = "33333333-3333-4333-8333-333333333333"; +const peerAgentId = "44444444-4444-4444-8444-444444444444"; +const ownerRunId = "55555555-5555-4555-8555-555555555555"; + +const mockIssueService = vi.hoisted(() => ({ + addComment: vi.fn(), + assertCheckoutOwner: vi.fn(), + getAttachmentById: vi.fn(), + getByIdentifier: vi.fn(), + getById: vi.fn(), + getRelationSummaries: vi.fn(), + getWakeableParentAfterChildCompletion: vi.fn(), + listAttachments: vi.fn(), + listWakeableBlockedDependents: vi.fn(), + remove: vi.fn(), + removeAttachment: vi.fn(), + update: vi.fn(), + findMentionedAgents: vi.fn(), +})); + +const mockAccessService = vi.hoisted(() => ({ + canUser: vi.fn(), + hasPermission: vi.fn(), +})); + +const mockAgentService = vi.hoisted(() => ({ + getById: vi.fn(), + resolveByReference: vi.fn(), +})); + +const mockDocumentService = vi.hoisted(() => ({ + upsertIssueDocument: vi.fn(), +})); + +const mockWorkProductService = vi.hoisted(() => ({ + getById: vi.fn(), + update: vi.fn(), +})); + +const mockStorageService = vi.hoisted(() => ({ + provider: "local_disk", + putFile: vi.fn(), + getObject: vi.fn(), + headObject: vi.fn(), + deleteObject: vi.fn(), +})); + +vi.mock("@paperclipai/shared/telemetry", () => ({ + trackAgentTaskCompleted: vi.fn(), + trackErrorHandlerCrash: vi.fn(), +})); + +vi.mock("../telemetry.js", () => ({ + getTelemetryClient: vi.fn(() => ({ track: vi.fn() })), +})); + +vi.mock("../services/index.js", () => ({ + accessService: () => mockAccessService, + agentService: () => mockAgentService, + documentService: () => mockDocumentService, + executionWorkspaceService: () => ({}), + feedbackService: () => ({ + listIssueVotesForUser: vi.fn(async () => []), + saveIssueVote: vi.fn(async () => ({ vote: null, consentEnabledNow: false, sharingEnabled: false })), + }), + goalService: () => ({}), + heartbeatService: () => ({ + wakeup: vi.fn(async () => undefined), + reportRunActivity: vi.fn(async () => undefined), + getRun: vi.fn(async () => null), + getActiveRunForAgent: vi.fn(async () => null), + cancelRun: vi.fn(async () => null), + }), + instanceSettingsService: () => ({ + get: vi.fn(async () => ({ + id: "instance-settings-1", + general: { + censorUsernameInLogs: false, + feedbackDataSharingPreference: "prompt", + }, + })), + listCompanyIds: vi.fn(async () => [companyId]), + }), + issueApprovalService: () => ({}), + issueService: () => mockIssueService, + logActivity: vi.fn(async () => undefined), + projectService: () => ({}), + routineService: () => ({ + syncRunStatusForIssue: vi.fn(async () => undefined), + }), + workProductService: () => mockWorkProductService, +})); + +function makeIssue(overrides: Record = {}) { + return { + id: issueId, + companyId, + status: "in_progress", + priority: "high", + projectId: null, + goalId: null, + parentId: null, + assigneeAgentId: ownerAgentId, + assigneeUserId: null, + createdByUserId: "board-user", + identifier: "PAP-1649", + title: "Owned active issue", + executionPolicy: null, + executionState: null, + hiddenAt: null, + ...overrides, + }; +} + +function makeAgent(id: string, overrides: Record = {}) { + return { + id, + companyId, + role: "engineer", + reportsTo: null, + permissions: { canCreateAgents: false }, + ...overrides, + }; +} + +function createApp(actor: Record) { + const app = express(); + app.use(express.json()); + app.use((req, _res, next) => { + (req as any).actor = actor; + next(); + }); + app.use("/api", issueRoutes({} as any, mockStorageService as any)); + app.use(errorHandler); + return app; +} + +function peerActor(overrides: Record = {}) { + return { + type: "agent", + agentId: peerAgentId, + companyId, + source: "agent_key", + runId: "66666666-6666-4666-8666-666666666666", + ...overrides, + }; +} + +function ownerActor() { + return { + type: "agent", + agentId: ownerAgentId, + companyId, + source: "agent_key", + runId: ownerRunId, + }; +} + +function boardActor() { + return { + type: "board", + userId: "board-user", + companyIds: [companyId], + source: "local_implicit", + isInstanceAdmin: false, + }; +} + +describe("agent issue mutation checkout ownership", () => { + beforeEach(() => { + vi.clearAllMocks(); + mockAccessService.canUser.mockResolvedValue(true); + mockAccessService.hasPermission.mockResolvedValue(false); + mockAgentService.getById.mockImplementation(async (id: string) => { + if (id === ownerAgentId) return makeAgent(ownerAgentId); + if (id === peerAgentId) return makeAgent(peerAgentId); + return null; + }); + mockAgentService.resolveByReference.mockResolvedValue({ ambiguous: false, agent: null }); + mockIssueService.getById.mockResolvedValue(makeIssue()); + mockIssueService.getByIdentifier.mockResolvedValue(null); + mockIssueService.assertCheckoutOwner.mockResolvedValue({ adoptedFromRunId: null }); + mockIssueService.getRelationSummaries.mockResolvedValue({ blockedBy: [], blocks: [] }); + mockIssueService.listWakeableBlockedDependents.mockResolvedValue([]); + mockIssueService.getWakeableParentAfterChildCompletion.mockResolvedValue(null); + mockIssueService.findMentionedAgents.mockResolvedValue([]); + mockIssueService.update.mockImplementation(async (_id: string, patch: Record) => ({ + ...makeIssue(), + ...patch, + })); + mockIssueService.addComment.mockResolvedValue({ + id: "77777777-7777-4777-8777-777777777777", + issueId, + companyId, + body: "comment", + }); + mockIssueService.listAttachments.mockResolvedValue([]); + mockIssueService.remove.mockResolvedValue(makeIssue({ status: "cancelled" })); + mockIssueService.getAttachmentById.mockResolvedValue({ + id: "attachment-1", + issueId, + companyId, + objectKey: "issues/attachment-1/report.txt", + contentType: "text/plain", + byteSize: 6, + originalFilename: "report.txt", + }); + mockIssueService.removeAttachment.mockResolvedValue({ + id: "attachment-1", + issueId, + companyId, + objectKey: "issues/attachment-1/report.txt", + }); + mockDocumentService.upsertIssueDocument.mockResolvedValue({ + created: false, + document: { + id: "document-1", + key: "plan", + title: "Plan", + format: "markdown", + latestRevisionNumber: 2, + }, + }); + mockWorkProductService.getById.mockResolvedValue({ + id: "product-1", + issueId, + companyId, + type: "artifact", + }); + mockWorkProductService.update.mockResolvedValue({ + id: "product-1", + issueId, + companyId, + type: "artifact", + title: "Updated", + }); + mockStorageService.putFile.mockResolvedValue({ + provider: "local_disk", + objectKey: "issues/upload.txt", + contentType: "text/plain", + byteSize: 6, + sha256: "sha256", + originalFilename: "upload.txt", + }); + mockStorageService.getObject.mockResolvedValue({ + stream: Readable.from(Buffer.from("report")), + contentLength: 6, + }); + mockStorageService.deleteObject.mockResolvedValue(undefined); + }); + + it.each([ + ["patch", (app: express.Express) => request(app).patch(`/api/issues/${issueId}`).send({ title: "Blocked" })], + ["delete", (app: express.Express) => request(app).delete(`/api/issues/${issueId}`)], + ["comment", (app: express.Express) => request(app).post(`/api/issues/${issueId}/comments`).send({ body: "blocked" })], + [ + "document upsert", + (app: express.Express) => + request(app).put(`/api/issues/${issueId}/documents/plan`).send({ format: "markdown", body: "# blocked" }), + ], + ["work product update", (app: express.Express) => request(app).patch("/api/work-products/product-1").send({ title: "Blocked" })], + [ + "attachment upload", + (app: express.Express) => + request(app) + .post(`/api/companies/${companyId}/issues/${issueId}/attachments`) + .attach("file", Buffer.from("report"), { filename: "report.txt", contentType: "text/plain" }), + ], + ["attachment delete", (app: express.Express) => request(app).delete("/api/attachments/attachment-1")], + ])("rejects peer agent %s on another agent's active checkout", async (_name, sendRequest) => { + const res = await sendRequest(createApp(peerActor())); + + expect(res.status, JSON.stringify(res.body)).toBe(409); + expect(res.body.error).toBe("Issue is checked out by another agent"); + expect(mockIssueService.assertCheckoutOwner).not.toHaveBeenCalled(); + expect(mockIssueService.update).not.toHaveBeenCalled(); + expect(mockIssueService.addComment).not.toHaveBeenCalled(); + expect(mockDocumentService.upsertIssueDocument).not.toHaveBeenCalled(); + expect(mockWorkProductService.update).not.toHaveBeenCalled(); + expect(mockStorageService.putFile).not.toHaveBeenCalled(); + expect(mockStorageService.deleteObject).not.toHaveBeenCalled(); + }); + + it("allows the checked-out owner with the matching run id to patch and update documents", async () => { + const app = createApp(ownerActor()); + + await request(app).patch(`/api/issues/${issueId}`).send({ title: "Updated" }).expect(200); + await request(app) + .put(`/api/issues/${issueId}/documents/plan`) + .send({ format: "markdown", body: "# updated" }) + .expect(200); + + expect(mockIssueService.assertCheckoutOwner).toHaveBeenCalledWith(issueId, ownerAgentId, ownerRunId); + expect(mockIssueService.update).toHaveBeenCalled(); + expect(mockDocumentService.upsertIssueDocument).toHaveBeenCalledWith( + expect.objectContaining({ + issueId, + key: "plan", + createdByAgentId: ownerAgentId, + createdByRunId: ownerRunId, + }), + ); + }); + + it("preserves board mutations on active checkouts", async () => { + const app = createApp(boardActor()); + + await request(app).patch(`/api/issues/${issueId}`).send({ title: "Board update" }).expect(200); + await request(app) + .put(`/api/issues/${issueId}/documents/plan`) + .send({ format: "markdown", body: "# board" }) + .expect(200); + + expect(mockIssueService.assertCheckoutOwner).not.toHaveBeenCalled(); + expect(mockIssueService.update).toHaveBeenCalled(); + expect(mockDocumentService.upsertIssueDocument).toHaveBeenCalled(); + }); + + it("allows agents with the active-checkout management grant to mutate active checkouts", async () => { + mockAccessService.hasPermission.mockImplementation(async ( + _companyId: string, + _principalType: string, + principalId: string, + permissionKey: string, + ) => principalId === peerAgentId && permissionKey === "tasks:manage_active_checkouts"); + + const res = await request(createApp(peerActor())).patch(`/api/issues/${issueId}`).send({ title: "Managed update" }); + + expect(res.status).toBe(200); + expect(mockIssueService.assertCheckoutOwner).not.toHaveBeenCalled(); + expect(mockIssueService.update).toHaveBeenCalled(); + }); + + it("allows same-company agent mutations when the issue is not in progress", async () => { + mockIssueService.getById.mockResolvedValue(makeIssue({ status: "todo", assigneeAgentId: ownerAgentId })); + mockIssueService.update.mockImplementation(async (_id: string, patch: Record) => ({ + ...makeIssue({ status: "todo", assigneeAgentId: ownerAgentId }), + ...patch, + })); + + const res = await request(createApp(peerActor())).patch(`/api/issues/${issueId}`).send({ title: "Todo update" }); + + expect(res.status).toBe(200); + expect(mockIssueService.assertCheckoutOwner).not.toHaveBeenCalled(); + expect(mockIssueService.update).toHaveBeenCalled(); + }); +}); diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 03a35a47b9a..49807016398 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -407,7 +407,37 @@ export function issueRoutes( return null; } - async function assertAgentRunCheckoutOwnership( + async function hasActiveCheckoutManagementOverride( + actorAgentId: string, + companyId: string, + assigneeAgentId: string | null, + ) { + const allowedByGrant = await access.hasPermission( + companyId, + "agent", + actorAgentId, + "tasks:manage_active_checkouts", + ); + if (allowedByGrant) return true; + + const actorAgent = await agentsSvc.getById(actorAgentId); + if (!actorAgent || actorAgent.companyId !== companyId) return false; + if (canCreateAgentsLegacy(actorAgent)) return true; + + // Reporting-chain managers may intervene in an agent's active checkout + // without taking the task over. Peers must own the checkout/run first. + let cursor = assigneeAgentId; + for (let depth = 0; cursor && depth < 50; depth += 1) { + const assignee = await agentsSvc.getById(cursor); + if (!assignee || assignee.companyId !== companyId) return false; + if (assignee.reportsTo === actorAgentId) return true; + cursor = assignee.reportsTo; + } + + return false; + } + + async function assertAgentIssueMutationAllowed( req: Request, res: Response, issue: { id: string; companyId: string; status: string; assigneeAgentId: string | null }, @@ -418,9 +448,23 @@ export function issueRoutes( res.status(403).json({ error: "Agent authentication required" }); return false; } - if (issue.status !== "in_progress" || issue.assigneeAgentId !== actorAgentId) { + if (issue.status !== "in_progress") { return true; } + if (issue.assigneeAgentId !== actorAgentId) { + if (await hasActiveCheckoutManagementOverride(actorAgentId, issue.companyId, issue.assigneeAgentId)) { + return true; + } + res.status(409).json({ + error: "Issue is checked out by another agent", + details: { + issueId: issue.id, + assigneeAgentId: issue.assigneeAgentId, + actorAgentId, + }, + }); + return false; + } const runId = requireAgentRunId(req, res); if (!runId) return false; const ownership = await svc.assertCheckoutOwner(issue.id, actorAgentId, runId); @@ -909,6 +953,7 @@ export function issueRoutes( return; } assertCompanyAccess(req, issue.companyId); + if (!(await assertAgentIssueMutationAllowed(req, res, issue))) return; const keyParsed = issueDocumentKeySchema.safeParse(String(req.params.key ?? "").trim().toLowerCase()); if (!keyParsed.success) { res.status(400).json({ error: "Invalid document key", details: keyParsed.error.issues }); @@ -980,6 +1025,7 @@ export function issueRoutes( return; } assertCompanyAccess(req, issue.companyId); + if (!(await assertAgentIssueMutationAllowed(req, res, issue))) return; const keyParsed = issueDocumentKeySchema.safeParse(String(req.params.key ?? "").trim().toLowerCase()); if (!keyParsed.success) { res.status(400).json({ error: "Invalid document key", details: keyParsed.error.issues }); @@ -1068,6 +1114,7 @@ export function issueRoutes( return; } assertCompanyAccess(req, issue.companyId); + if (!(await assertAgentIssueMutationAllowed(req, res, issue))) return; const product = await workProductsSvc.createForIssue(issue.id, issue.companyId, { ...req.body, projectId: req.body.projectId ?? issue.projectId ?? null, @@ -1099,6 +1146,12 @@ export function issueRoutes( return; } assertCompanyAccess(req, existing.companyId); + const issue = await svc.getById(existing.issueId); + if (!issue) { + res.status(404).json({ error: "Issue not found" }); + return; + } + if (!(await assertAgentIssueMutationAllowed(req, res, issue))) return; const product = await workProductsSvc.update(id, req.body); if (!product) { res.status(404).json({ error: "Work product not found" }); @@ -1127,6 +1180,12 @@ export function issueRoutes( return; } assertCompanyAccess(req, existing.companyId); + const issue = await svc.getById(existing.issueId); + if (!issue) { + res.status(404).json({ error: "Issue not found" }); + return; + } + if (!(await assertAgentIssueMutationAllowed(req, res, issue))) return; const removed = await workProductsSvc.remove(id); if (!removed) { res.status(404).json({ error: "Work product not found" }); @@ -1294,6 +1353,8 @@ export function issueRoutes( res.status(404).json({ error: "Issue not found" }); return; } + assertCompanyAccess(req, issue.companyId); + if (!(await assertAgentIssueMutationAllowed(req, res, issue))) return; if (!(await assertCanManageIssueApprovalLinks(req, res, issue.companyId))) return; const actor = getActorInfo(req); @@ -1326,6 +1387,8 @@ export function issueRoutes( res.status(404).json({ error: "Issue not found" }); return; } + assertCompanyAccess(req, issue.companyId); + if (!(await assertAgentIssueMutationAllowed(req, res, issue))) return; if (!(await assertCanManageIssueApprovalLinks(req, res, issue.companyId))) return; await issueApprovalsSvc.unlink(id, approvalId); @@ -1457,7 +1520,7 @@ export function issueRoutes( } assertCompanyAccess(req, existing.companyId); assertNoAgentHostWorkspaceCommandMutation(req, collectIssueWorkspaceCommandPaths(req.body)); - if (!(await assertAgentRunCheckoutOwnership(req, res, existing))) return; + if (!(await assertAgentIssueMutationAllowed(req, res, existing))) return; const actor = getActorInfo(req); const isClosed = isClosedIssueStatus(existing.status); @@ -2053,6 +2116,7 @@ export function issueRoutes( return; } assertCompanyAccess(req, existing.companyId); + if (!(await assertAgentIssueMutationAllowed(req, res, existing))) return; const attachments = await svc.listAttachments(id); const issue = await svc.remove(id); @@ -2166,7 +2230,7 @@ export function issueRoutes( return; } assertCompanyAccess(req, existing.companyId); - if (!(await assertAgentRunCheckoutOwnership(req, res, existing))) return; + if (!(await assertAgentIssueMutationAllowed(req, res, existing))) return; const actorRunId = requireAgentRunId(req, res); if (req.actor.type === "agent" && !actorRunId) return; @@ -2255,7 +2319,7 @@ export function issueRoutes( return; } assertCompanyAccess(req, issue.companyId); - if (!(await assertAgentRunCheckoutOwnership(req, res, issue))) return; + if (!(await assertAgentIssueMutationAllowed(req, res, issue))) return; const comment = await svc.getComment(commentId); if (!comment || comment.issueId !== id) { @@ -2400,7 +2464,7 @@ export function issueRoutes( return; } assertCompanyAccess(req, issue.companyId); - if (!(await assertAgentRunCheckoutOwnership(req, res, issue))) return; + if (!(await assertAgentIssueMutationAllowed(req, res, issue))) return; const closedExecutionWorkspace = await getClosedIssueExecutionWorkspace(issue); if (closedExecutionWorkspace) { respondClosedIssueExecutionWorkspace(res, closedExecutionWorkspace); @@ -2730,6 +2794,7 @@ export function issueRoutes( res.status(422).json({ error: "Issue does not belong to company" }); return; } + if (!(await assertAgentIssueMutationAllowed(req, res, issue))) return; try { await runSingleFileUpload(req, res); @@ -2840,6 +2905,12 @@ export function issueRoutes( return; } assertCompanyAccess(req, attachment.companyId); + const issue = await svc.getById(attachment.issueId); + if (!issue) { + res.status(404).json({ error: "Issue not found" }); + return; + } + if (!(await assertAgentIssueMutationAllowed(req, res, issue))) return; try { await storage.deleteObject(attachment.companyId, attachment.objectKey); diff --git a/ui/src/pages/CompanyAccess.tsx b/ui/src/pages/CompanyAccess.tsx index 51ff8d69c7f..b1ba9aa5813 100644 --- a/ui/src/pages/CompanyAccess.tsx +++ b/ui/src/pages/CompanyAccess.tsx @@ -33,6 +33,7 @@ const permissionLabels: Record = { "users:manage_permissions": "Manage members and grants", "tasks:assign": "Assign tasks", "tasks:assign_scope": "Assign scoped tasks", + "tasks:manage_active_checkouts": "Manage active task checkouts", "joins:approve": "Approve join requests", };