diff --git a/packages/shared/src/validators/work-product.test.ts b/packages/shared/src/validators/work-product.test.ts index 214a9637..82ccb711 100644 --- a/packages/shared/src/validators/work-product.test.ts +++ b/packages/shared/src/validators/work-product.test.ts @@ -16,4 +16,26 @@ describe("attachmentArtifactWorkProductMetadataSchema", () => { expect(parsed.contentType).toBe("video/mp4"); expect(parsed.downloadPath).toContain("download=1"); }); + + it("rejects off-route or scriptable paths", () => { + const parsed = attachmentArtifactWorkProductMetadataSchema.safeParse({ + attachmentId: "11111111-1111-4111-8111-111111111111", + contentType: "video/mp4", + byteSize: 1234, + contentPath: "https://evil.example/video.mp4", + openPath: "javascript:alert(1)", + downloadPath: "/api/attachments/11111111-1111-4111-8111-111111111111/content", + originalFilename: "demo.mp4", + }); + + expect(parsed.success).toBe(false); + if (parsed.success) { + throw new Error("Expected invalid attachment artifact metadata"); + } + expect(parsed.error.issues.map((issue) => issue.path.join("."))).toEqual([ + "contentPath", + "openPath", + "downloadPath", + ]); + }); }); diff --git a/packages/shared/src/validators/work-product.ts b/packages/shared/src/validators/work-product.ts index ca8d3852..ac31b476 100644 --- a/packages/shared/src/validators/work-product.ts +++ b/packages/shared/src/validators/work-product.ts @@ -1,5 +1,9 @@ import { z } from "zod"; +function attachmentContentPath(attachmentId: string): string { + return `/api/attachments/${attachmentId}/content`; +} + export const issueWorkProductTypeSchema = z.enum([ "preview_url", "runtime_service", @@ -37,6 +41,29 @@ export const attachmentArtifactWorkProductMetadataSchema = z.object({ openPath: z.string().min(1), downloadPath: z.string().min(1), originalFilename: z.string().optional().nullable(), +}).superRefine((value, ctx) => { + const contentPath = attachmentContentPath(value.attachmentId); + if (value.contentPath !== contentPath) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ["contentPath"], + message: "contentPath must point to the same-origin attachment content route", + }); + } + if (value.openPath !== contentPath) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ["openPath"], + message: "openPath must point to the same-origin attachment content route", + }); + } + if (value.downloadPath !== `${contentPath}?download=1`) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ["downloadPath"], + message: "downloadPath must point to the same-origin attachment download route", + }); + } }); export type AttachmentArtifactWorkProductMetadata = z.infer; diff --git a/server/src/__tests__/issue-attachment-routes.test.ts b/server/src/__tests__/issue-attachment-routes.test.ts index 04f93a68..1386c245 100644 --- a/server/src/__tests__/issue-attachment-routes.test.ts +++ b/server/src/__tests__/issue-attachment-routes.test.ts @@ -13,6 +13,11 @@ const mockIssueService = vi.hoisted(() => ({ const mockCompanyService = vi.hoisted(() => ({ getById: vi.fn(), })); +const mockWorkProductService = vi.hoisted(() => ({ + createForIssue: vi.fn(), + getById: vi.fn(), + update: vi.fn(), +})); const mockLogActivity = vi.hoisted(() => vi.fn(async () => undefined)); @@ -97,7 +102,7 @@ function registerRouteMocks() { routineService: () => ({ syncRunStatusForIssue: vi.fn(async () => undefined), }), - workProductService: () => ({}), + workProductService: () => mockWorkProductService, })); } @@ -144,6 +149,7 @@ async function createApp(storage: StorageService, options?: { companyIds?: strin vi.importActual("../routes/issues.js"), ]); const app = express(); + app.use(express.json()); app.use((req, _res, next) => { (req as any).actor = { type: "board", @@ -219,6 +225,9 @@ describe("issue attachment routes", () => { id: "company-1", attachmentMaxBytes: 1024 * 1024 * 1024, }); + mockWorkProductService.createForIssue.mockReset(); + mockWorkProductService.getById.mockReset(); + mockWorkProductService.update.mockReset(); }); it("accepts zip uploads for issue attachments", async () => { @@ -429,4 +438,160 @@ describe("issue attachment routes", () => { expect(res.status).toBe(403); expect(storage.getObject).not.toHaveBeenCalled(); }); + + it("canonicalizes paperclip artifact metadata before creating a work product", async () => { + const storage = createStorageService(); + const issue = { + id: "11111111-1111-4111-8111-111111111111", + companyId: "company-1", + identifier: "PAP-1", + projectId: null, + }; + mockIssueService.getById.mockResolvedValue(issue); + mockIssueService.getAttachmentById.mockResolvedValue({ + ...makeAttachment("video/mp4", "clip.mp4"), + id: "22222222-2222-4222-8222-222222222222", + byteSize: 6, + issueId: issue.id, + }); + mockWorkProductService.createForIssue.mockResolvedValue({ + id: "work-product-1", + issueId: issue.id, + companyId: issue.companyId, + type: "artifact", + provider: "paperclip", + title: "Clip", + metadata: null, + }); + + const app = await createApp(storage); + const res = await request(app) + .post(`/api/issues/${issue.id}/work-products`) + .send({ + type: "artifact", + provider: "paperclip", + title: "Clip", + metadata: { + attachmentId: "22222222-2222-4222-8222-222222222222", + contentType: "video/mp4", + byteSize: 6, + contentPath: "https://evil.example/clip.mp4", + openPath: "javascript:alert(1)", + downloadPath: "javascript:alert(2)", + originalFilename: "clip.mp4", + }, + }); + + expect(res.status).toBe(201); + expect(mockWorkProductService.createForIssue).toHaveBeenCalledWith( + issue.id, + issue.companyId, + expect.objectContaining({ + type: "artifact", + provider: "paperclip", + metadata: { + attachmentId: "22222222-2222-4222-8222-222222222222", + contentType: "video/mp4", + byteSize: 6, + contentPath: "/api/attachments/22222222-2222-4222-8222-222222222222/content", + openPath: "/api/attachments/22222222-2222-4222-8222-222222222222/content", + downloadPath: "/api/attachments/22222222-2222-4222-8222-222222222222/content?download=1", + originalFilename: "clip.mp4", + }, + }), + ); + }); + + it("rejects paperclip artifact metadata that references another issue's attachment", async () => { + const storage = createStorageService(); + const issue = { + id: "11111111-1111-4111-8111-111111111111", + companyId: "company-1", + identifier: "PAP-1", + projectId: null, + }; + mockIssueService.getById.mockResolvedValue(issue); + mockIssueService.getAttachmentById.mockResolvedValue({ + ...makeAttachment("video/mp4", "clip.mp4"), + id: "22222222-2222-4222-8222-222222222222", + issueId: "different-issue", + }); + + const app = await createApp(storage); + const res = await request(app) + .post(`/api/issues/${issue.id}/work-products`) + .send({ + type: "artifact", + provider: "paperclip", + title: "Clip", + metadata: { + attachmentId: "22222222-2222-4222-8222-222222222222", + }, + }); + + expect(res.status).toBe(422); + expect(res.body.error).toBe("Attachment artifact must reference an attachment on the same issue"); + expect(mockWorkProductService.createForIssue).not.toHaveBeenCalled(); + }); + + it("canonicalizes paperclip artifact metadata on work product updates", async () => { + const storage = createStorageService(); + const issue = { + id: "11111111-1111-4111-8111-111111111111", + companyId: "company-1", + identifier: "PAP-1", + projectId: null, + }; + mockWorkProductService.getById.mockResolvedValue({ + id: "work-product-1", + issueId: issue.id, + companyId: issue.companyId, + type: "artifact", + provider: "paperclip", + title: "Clip", + metadata: null, + }); + mockIssueService.getById.mockResolvedValue(issue); + mockIssueService.getAttachmentById.mockResolvedValue({ + ...makeAttachment("video/webm", "clip.webm"), + id: "22222222-2222-4222-8222-222222222222", + issueId: issue.id, + byteSize: 8, + }); + mockWorkProductService.update.mockResolvedValue({ + id: "work-product-1", + issueId: issue.id, + companyId: issue.companyId, + type: "artifact", + provider: "paperclip", + title: "Clip", + metadata: null, + }); + + const app = await createApp(storage); + const res = await request(app) + .patch("/api/work-products/work-product-1") + .send({ + metadata: { + attachmentId: "22222222-2222-4222-8222-222222222222", + openPath: "javascript:alert(1)", + }, + }); + + expect(res.status).toBe(200); + expect(mockWorkProductService.update).toHaveBeenCalledWith( + "work-product-1", + expect.objectContaining({ + metadata: { + attachmentId: "22222222-2222-4222-8222-222222222222", + contentType: "video/webm", + byteSize: 8, + contentPath: "/api/attachments/22222222-2222-4222-8222-222222222222/content", + openPath: "/api/attachments/22222222-2222-4222-8222-222222222222/content", + downloadPath: "/api/attachments/22222222-2222-4222-8222-222222222222/content?download=1", + originalFilename: "clip.webm", + }, + }), + ); + }); }); diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 6f1c6a23..b5b7251f 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -17,6 +17,7 @@ import { import { addIssueCommentSchema, acceptIssueThreadInteractionSchema, + attachmentArtifactWorkProductMetadataSchema, cancelIssueThreadInteractionSchema, companySearchQuerySchema, createIssueAttachmentMetadataSchema, @@ -181,6 +182,26 @@ function applyCreateIssueStatusDefault(req: Request, res: Response, next: () => next(); } +function buildAttachmentContentPath(attachmentId: string): string { + return `/api/attachments/${attachmentId}/content`; +} + +function requiresPaperclipAttachmentMetadata(input: { + type?: unknown; + provider?: unknown; +}, fallback?: { + type?: string | null; + provider?: string | null; +}) { + const type = typeof input.type === "string" ? input.type : fallback?.type ?? null; + const provider = typeof input.provider === "string" ? input.provider : fallback?.provider ?? null; + return type === "artifact" && provider === "paperclip"; +} + +const attachmentArtifactMetadataInputSchema = z.object({ + attachmentId: z.string().uuid(), +}).passthrough(); + function buildCreateIssueActivityStatusDetails( issue: { assigneeAgentId: string | null; status: string }, res: Response, @@ -1233,6 +1254,38 @@ export function issueRoutes( }, "failed to wake assignee on document annotation comment")); } + async function canonicalizePaperclipArtifactMetadata(input: { + issue: { id: string; companyId: string }; + metadata: Record | null | undefined; + }) { + const parsed = attachmentArtifactMetadataInputSchema.safeParse(input.metadata); + if (!parsed.success) { + throw unprocessable("Invalid attachment artifact metadata", { + code: "invalid_attachment_artifact_metadata", + details: parsed.error.issues, + }); + } + + const attachment = await svc.getAttachmentById(parsed.data.attachmentId); + if (!attachment || attachment.companyId !== input.issue.companyId || attachment.issueId !== input.issue.id) { + throw unprocessable("Attachment artifact must reference an attachment on the same issue", { + code: "invalid_attachment_artifact_metadata", + attachmentId: parsed.data.attachmentId, + }); + } + + const contentPath = buildAttachmentContentPath(attachment.id); + return attachmentArtifactWorkProductMetadataSchema.parse({ + attachmentId: attachment.id, + contentType: normalizeContentType(attachment.contentType), + byteSize: attachment.byteSize, + contentPath, + openPath: contentPath, + downloadPath: `${contentPath}?download=1`, + originalFilename: attachment.originalFilename ?? null, + }); + } + async function assertIssueEnvironmentSelection( companyId: string, environmentId: string | null | undefined, @@ -3245,10 +3298,17 @@ export function issueRoutes( assertCompanyAccess(req, issue.companyId); if (!(await assertAgentIssueMutationAllowed(req, res, issue))) return; if (!(await assertDeliverableMutationAllowedByRunContext(req, res, issue))) return; - const product = await workProductsSvc.createForIssue(issue.id, issue.companyId, { + const createInput = { ...req.body, projectId: req.body.projectId ?? issue.projectId ?? null, - }); + }; + if (requiresPaperclipAttachmentMetadata(createInput)) { + createInput.metadata = await canonicalizePaperclipArtifactMetadata({ + issue, + metadata: req.body.metadata ?? null, + }); + } + const product = await workProductsSvc.createForIssue(issue.id, issue.companyId, createInput); if (!product) { res.status(422).json({ error: "Invalid work product payload" }); return; @@ -3289,7 +3349,19 @@ export function issueRoutes( } if (!(await assertAgentIssueMutationAllowed(req, res, issue))) return; if (!(await assertDeliverableMutationAllowedByRunContext(req, res, issue))) return; - const product = await workProductsSvc.update(id, req.body); + const patch = { ...req.body }; + if (requiresPaperclipAttachmentMetadata(patch, existing)) { + if (patch.metadata !== undefined) { + patch.metadata = await canonicalizePaperclipArtifactMetadata({ + issue, + metadata: patch.metadata ?? null, + }); + } else if (!requiresPaperclipAttachmentMetadata(existing)) { + res.status(422).json({ error: "Attachment-backed artifact metadata is required" }); + return; + } + } + const product = await workProductsSvc.update(id, patch); if (!product) { res.status(404).json({ error: "Work product not found" }); return; diff --git a/ui/src/lib/issue-output.test.ts b/ui/src/lib/issue-output.test.ts index 15bb3508..a7f681ea 100644 --- a/ui/src/lib/issue-output.test.ts +++ b/ui/src/lib/issue-output.test.ts @@ -100,6 +100,7 @@ describe("getIssueOutputs", () => { const result = getIssueOutputs([ makeWorkProduct({ id: "pr-1", type: "pull_request" }), makeWorkProduct({ id: "doc-1", type: "document" }), + makeWorkProduct({ id: "artifact-1", type: "artifact", provider: "custom", metadata: videoMetadata() }), ]); expect(result.count).toBe(0); expect(result.primary).toBeNull(); diff --git a/ui/src/lib/issue-output.ts b/ui/src/lib/issue-output.ts index 9e396ab6..b3ae947f 100644 --- a/ui/src/lib/issue-output.ts +++ b/ui/src/lib/issue-output.ts @@ -123,7 +123,7 @@ function toTime(value: string | Date): number { * marked primary) comes first, then remaining artifacts by most-recent. */ export function getIssueOutputs(workProducts: IssueWorkProduct[] | null | undefined): IssueOutputs { - const artifacts = (workProducts ?? []).filter((wp) => wp.type === "artifact"); + const artifacts = (workProducts ?? []).filter((wp) => wp.type === "artifact" && wp.provider === "paperclip"); const items: IssueOutputItem[] = artifacts.map((wp) => { const parsed = attachmentArtifactWorkProductMetadataSchema.safeParse(wp.metadata);