diff --git a/server/src/__tests__/attachment-types.test.ts b/server/src/__tests__/attachment-types.test.ts index 5a430102..7dfc34d2 100644 --- a/server/src/__tests__/attachment-types.test.ts +++ b/server/src/__tests__/attachment-types.test.ts @@ -1,8 +1,11 @@ import { describe, it, expect } from "vitest"; import { - parseAllowedTypes, - matchesContentType, DEFAULT_ALLOWED_TYPES, + INLINE_ATTACHMENT_TYPES, + isInlineAttachmentContentType, + matchesContentType, + normalizeContentType, + parseAllowedTypes, } from "../attachment-types.js"; describe("parseAllowedTypes", () => { @@ -95,3 +98,28 @@ describe("matchesContentType", () => { expect(matchesContentType("application/zip", patterns)).toBe(true); }); }); + +describe("normalizeContentType", () => { + it("lowercases and trims explicit types", () => { + expect(normalizeContentType(" Application/Zip ")).toBe("application/zip"); + }); + + it("falls back to octet-stream when the type is missing", () => { + expect(normalizeContentType(undefined)).toBe("application/octet-stream"); + expect(normalizeContentType("")).toBe("application/octet-stream"); + }); +}); + +describe("isInlineAttachmentContentType", () => { + it("allows the configured inline-safe types", () => { + for (const contentType of ["image/png", "image/svg+xml", "application/pdf", "text/plain"]) { + expect(isInlineAttachmentContentType(contentType)).toBe(true); + } + }); + + it("rejects potentially unsafe or binary download types", () => { + expect(INLINE_ATTACHMENT_TYPES).not.toContain("text/html"); + expect(isInlineAttachmentContentType("text/html")).toBe(false); + expect(isInlineAttachmentContentType("application/zip")).toBe(false); + }); +}); diff --git a/server/src/__tests__/issue-attachment-routes.test.ts b/server/src/__tests__/issue-attachment-routes.test.ts new file mode 100644 index 00000000..42a6b4e6 --- /dev/null +++ b/server/src/__tests__/issue-attachment-routes.test.ts @@ -0,0 +1,175 @@ +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"; +import type { StorageService } from "../storage/types.js"; + +const mockIssueService = vi.hoisted(() => ({ + getById: vi.fn(), + getByIdentifier: vi.fn(), + createAttachment: vi.fn(), + getAttachmentById: vi.fn(), +})); + +const mockLogActivity = vi.hoisted(() => vi.fn(async () => undefined)); + +vi.mock("../services/index.js", () => ({ + accessService: () => ({ + canUser: vi.fn(), + hasPermission: vi.fn(), + }), + agentService: () => ({ + getById: vi.fn(), + }), + documentService: () => ({}), + 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 () => ["company-1"]), + }), + issueApprovalService: () => ({}), + issueService: () => mockIssueService, + logActivity: mockLogActivity, + projectService: () => ({}), + routineService: () => ({ + syncRunStatusForIssue: vi.fn(async () => undefined), + }), + workProductService: () => ({}), +})); + +function createStorageService(): StorageService { + return { + provider: "local_disk", + putFile: vi.fn(async (input) => ({ + provider: "local_disk", + objectKey: `${input.namespace}/${input.originalFilename ?? "upload"}`, + contentType: input.contentType, + byteSize: input.body.length, + sha256: "sha256-sample", + originalFilename: input.originalFilename, + })), + getObject: vi.fn(async () => ({ + stream: Readable.from(Buffer.from("test")), + contentLength: 4, + })), + headObject: vi.fn(), + deleteObject: vi.fn(), + }; +} + +function createApp(storage: StorageService) { + const app = express(); + app.use((req, _res, next) => { + (req as any).actor = { + type: "board", + userId: "local-board", + companyIds: ["company-1"], + source: "local_implicit", + isInstanceAdmin: false, + }; + next(); + }); + app.use("/api", issueRoutes({} as any, storage)); + app.use(errorHandler); + return app; +} + +function makeAttachment(contentType: string, originalFilename: string) { + const now = new Date("2026-01-01T00:00:00.000Z"); + return { + id: "attachment-1", + companyId: "company-1", + issueId: "11111111-1111-4111-8111-111111111111", + issueCommentId: null, + assetId: "asset-1", + provider: "local_disk", + objectKey: `issues/issue-1/${originalFilename}`, + contentType, + byteSize: 4, + sha256: "sha256-sample", + originalFilename, + createdByAgentId: null, + createdByUserId: "local-board", + createdAt: now, + updatedAt: now, + }; +} + +describe("issue attachment routes", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("accepts zip uploads for issue attachments", async () => { + const storage = createStorageService(); + mockIssueService.getById.mockResolvedValue({ + id: "11111111-1111-4111-8111-111111111111", + companyId: "company-1", + identifier: "PAP-1", + }); + mockIssueService.createAttachment.mockResolvedValue(makeAttachment("application/zip", "bundle.zip")); + + const res = await request(createApp(storage)) + .post("/api/companies/company-1/issues/11111111-1111-4111-8111-111111111111/attachments") + .attach("file", Buffer.from("zip"), { filename: "bundle.zip", contentType: "application/zip" }); + + expect(res.status).toBe(201); + const putFileCall = vi.mocked(storage.putFile).mock.calls[0]?.[0]; + expect(putFileCall).toMatchObject({ + companyId: "company-1", + namespace: "issues/11111111-1111-4111-8111-111111111111", + originalFilename: "bundle.zip", + contentType: "application/zip", + }); + expect(Buffer.isBuffer(putFileCall?.body)).toBe(true); + expect(mockIssueService.createAttachment).toHaveBeenCalledWith( + expect.objectContaining({ + issueId: "11111111-1111-4111-8111-111111111111", + contentType: "application/zip", + originalFilename: "bundle.zip", + }), + ); + expect(res.body.contentType).toBe("application/zip"); + }); + + it("serves html attachments as downloads with nosniff", async () => { + const storage = createStorageService(); + mockIssueService.getAttachmentById.mockResolvedValue(makeAttachment("text/html", "report.html")); + + const res = await request(createApp(storage)).get("/api/attachments/attachment-1/content"); + + expect(res.status).toBe(200); + expect(res.headers["content-disposition"]).toBe('attachment; filename="report.html"'); + expect(res.headers["x-content-type-options"]).toBe("nosniff"); + }); + + it("keeps image attachments inline for previews", async () => { + const storage = createStorageService(); + mockIssueService.getAttachmentById.mockResolvedValue(makeAttachment("image/png", "preview.png")); + + const res = await request(createApp(storage)).get("/api/attachments/attachment-1/content"); + + expect(res.status).toBe(200); + expect(res.headers["content-disposition"]).toBe('inline; filename="preview.png"'); + }); +}); diff --git a/server/src/attachment-types.ts b/server/src/attachment-types.ts index b9349179..9c02b86d 100644 --- a/server/src/attachment-types.ts +++ b/server/src/attachment-types.ts @@ -1,10 +1,10 @@ /** * Shared attachment content-type configuration. * - * By default only image types are allowed. Set the + * By default a curated set of image/document/text types are allowed. Set the * `PAPERCLIP_ALLOWED_ATTACHMENT_TYPES` environment variable to a * comma-separated list of MIME types or wildcard patterns to expand the - * allowed set. + * allowed set for routes that use this allowlist. * * Examples: * PAPERCLIP_ALLOWED_ATTACHMENT_TYPES=image/*,application/pdf @@ -29,6 +29,17 @@ export const DEFAULT_ALLOWED_TYPES: readonly string[] = [ "text/html", ]; +export const DEFAULT_ATTACHMENT_CONTENT_TYPE = "application/octet-stream"; +export const SVG_CONTENT_TYPE = "image/svg+xml"; +export const INLINE_ATTACHMENT_TYPES: readonly string[] = [ + "image/*", + "application/pdf", + "text/plain", + "text/markdown", + "application/json", + "text/csv", +]; + /** * Parse a comma-separated list of MIME type patterns into a normalised array. * Returns the default image-only list when the input is empty or undefined. @@ -59,6 +70,15 @@ export function matchesContentType(contentType: string, allowedPatterns: string[ }); } +export function normalizeContentType(contentType: string | null | undefined): string { + const normalized = (contentType ?? "").trim().toLowerCase(); + return normalized || DEFAULT_ATTACHMENT_CONTENT_TYPE; +} + +export function isInlineAttachmentContentType(contentType: string): boolean { + return matchesContentType(contentType, [...INLINE_ATTACHMENT_TYPES]); +} + // ---------- Module-level singletons read once at startup ---------- const allowedPatterns: string[] = parseAllowedTypes( diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 4617b7ed..9dfe26db 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -47,7 +47,12 @@ import { logger } from "../middleware/logger.js"; import { forbidden, HttpError, unauthorized } from "../errors.js"; import { assertCompanyAccess, getActorInfo } from "./authz.js"; import { shouldWakeAssigneeOnCheckout } from "./issues-checkout-wakeup.js"; -import { isAllowedContentType, MAX_ATTACHMENT_BYTES } from "../attachment-types.js"; +import { + isInlineAttachmentContentType, + MAX_ATTACHMENT_BYTES, + normalizeContentType, + SVG_CONTENT_TYPE, +} from "../attachment-types.js"; import { queueIssueAssignmentWakeup } from "../services/issue-assignment-wakeup.js"; const MAX_ISSUE_COMMENT_LIMIT = 500; @@ -2108,11 +2113,7 @@ export function issueRoutes( res.status(400).json({ error: "Missing file field 'file'" }); return; } - const contentType = (file.mimetype || "").toLowerCase(); - if (!isAllowedContentType(contentType)) { - res.status(422).json({ error: `Unsupported attachment type: ${contentType || "unknown"}` }); - return; - } + const contentType = normalizeContentType(file.mimetype); if (file.buffer.length <= 0) { res.status(422).json({ error: "Attachment is empty" }); return; @@ -2176,11 +2177,17 @@ export function issueRoutes( assertCompanyAccess(req, attachment.companyId); const object = await storage.getObject(attachment.companyId, attachment.objectKey); - res.setHeader("Content-Type", attachment.contentType || object.contentType || "application/octet-stream"); + const responseContentType = normalizeContentType(attachment.contentType || object.contentType); + res.setHeader("Content-Type", responseContentType); res.setHeader("Content-Length", String(attachment.byteSize || object.contentLength || 0)); res.setHeader("Cache-Control", "private, max-age=60"); + res.setHeader("X-Content-Type-Options", "nosniff"); + if (responseContentType === SVG_CONTENT_TYPE) { + res.setHeader("Content-Security-Policy", "sandbox; default-src 'none'; img-src 'self' data:; style-src 'unsafe-inline'"); + } const filename = attachment.originalFilename ?? "attachment"; - res.setHeader("Content-Disposition", `inline; filename=\"${filename.replaceAll("\"", "")}\"`); + const disposition = isInlineAttachmentContentType(responseContentType) ? "inline" : "attachment"; + res.setHeader("Content-Disposition", `${disposition}; filename=\"${filename.replaceAll("\"", "")}\"`); object.stream.on("error", (err) => { next(err); diff --git a/ui/src/pages/IssueDetail.tsx b/ui/src/pages/IssueDetail.tsx index d16a3103..1ab5b578 100644 --- a/ui/src/pages/IssueDetail.tsx +++ b/ui/src/pages/IssueDetail.tsx @@ -1121,7 +1121,6 @@ export function IssueDetail() {