mirror of
https://github.com/alkimake/paperclip.git
synced 2026-06-14 01:50:39 +09:00
Harden issue artifact metadata
This commit is contained in:
Dotta
parent
96d266109b
commit
bbf77fcb69
6 changed files with 292 additions and 5 deletions
|
|
@ -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<typeof import("../routes/issues.js")>("../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",
|
||||
},
|
||||
}),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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<string, unknown> | 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;
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue