diff --git a/apps/web/next.config.js b/apps/web/next.config.js index 034a01aaa..7e4aabc94 100644 --- a/apps/web/next.config.js +++ b/apps/web/next.config.js @@ -36,6 +36,7 @@ const config = { "@kan/shared", "@kan/auth", "@kan/stripe", + "@kan/logger", ], /** We already do linting and typechecking as separate tasks in CI */ diff --git a/apps/web/src/pages/api/download/attatchment.ts b/apps/web/src/pages/api/download/attatchment.ts deleted file mode 100644 index c69dd2dfc..000000000 --- a/apps/web/src/pages/api/download/attatchment.ts +++ /dev/null @@ -1,75 +0,0 @@ -import type { NextApiRequest, NextApiResponse } from "next"; - -import { withApiLogging } from "@kan/api/utils/apiLogging"; -import { withRateLimit } from "@kan/api/utils/rateLimit"; - -import { env } from "~/env"; - -export default withRateLimit( - { points: 100, duration: 60 }, - withApiLogging(async (req: NextApiRequest, res: NextApiResponse) => { - if (req.method !== "GET") { - return res.status(405).json({ message: "Method not allowed" }); - } - - const { url, filename } = req.query; - - if (!url || typeof url !== "string") { - return res.status(400).json({ message: "url parameter is required" }); - } - - const s3Endpoint = env.S3_ENDPOINT; - - if (s3Endpoint) { - let parsed: URL; - try { - parsed = new URL(url); - } catch { - return res.status(400).json({ message: "Invalid URL" }); - } - - const hostname = parsed.hostname.toLowerCase(); - let allowedHost: string; - try { - allowedHost = new URL(s3Endpoint).hostname.toLowerCase(); - } catch { - return res - .status(500) - .json({ message: "Storage endpoint misconfigured" }); - } - - if (hostname !== allowedHost && !hostname.endsWith(`.${allowedHost}`)) { - return res.status(403).json({ message: "URL not allowed" }); - } - } - - try { - const downloadFilename = - typeof filename === "string" - ? encodeURIComponent(filename) - : "attachment"; - - const upstream = await fetch(url); - - if (!upstream.ok) { - return res - .status(upstream.status) - .json({ message: "Failed to fetch attachment" }); - } - - const contentType = - upstream.headers.get("Content-Type") ?? "application/octet-stream"; - - res.setHeader("Content-Type", contentType); - res.setHeader( - "Content-Disposition", - `attachment; filename="${downloadFilename}"; filename*=UTF-8''${downloadFilename}`, - ); - - const buffer = await upstream.arrayBuffer(); - return res.send(Buffer.from(buffer)); - } catch (error) { - return res.status(500).json({ message: "Failed to download attachment" }); - } - }), -); diff --git a/apps/web/src/pages/api/upload/attachment.ts b/apps/web/src/pages/api/upload/attachment.ts index 7c52d1c61..7582665e4 100644 --- a/apps/web/src/pages/api/upload/attachment.ts +++ b/apps/web/src/pages/api/upload/attachment.ts @@ -9,9 +9,12 @@ import * as cardRepo from "@kan/db/repository/card.repo"; import * as cardActivityRepo from "@kan/db/repository/cardActivity.repo"; import * as cardAttachmentRepo from "@kan/db/repository/cardAttachment.repo"; import { createS3Client, generateUID } from "@kan/shared/utils"; +import { createLogger } from "@kan/logger"; import { env } from "~/env"; +const log = createLogger("attachment-upload"); + // FIXME: Respect the environment variable: NEXT_API_BODY_SIZE_LIMIT const MAX_SIZE_BYTES = 50 * 1024 * 1024; // 50MB @@ -134,6 +137,15 @@ export default withRateLimit( return res.status(200).json({ attachment }); } catch (error) { + log.error( + { + err: error, + cardPublicId: req.query.cardPublicId, + contentType: req.headers["content-type"], + contentLength: req.headers["content-length"], + }, + "Attachment upload failed", + ); return res.status(500).json({ error: "Internal server error" }); } }), diff --git a/apps/web/src/views/card/components/AttachmentThumbnails.tsx b/apps/web/src/views/card/components/AttachmentThumbnails.tsx index bd93249c5..c1519066c 100644 --- a/apps/web/src/views/card/components/AttachmentThumbnails.tsx +++ b/apps/web/src/views/card/components/AttachmentThumbnails.tsx @@ -19,6 +19,7 @@ interface Attachment { publicId: string; contentType: string; url: string | null; + downloadUrl: string | null; originalFilename: string | null; s3Key: string; size?: number | null; @@ -137,7 +138,11 @@ export function AttachmentThumbnails({ }; const handleDownload = (attachment: Attachment) => { - if (!attachment.url) { + // downloadUrl carries Content-Disposition: attachment baked into the + // signed URL — required because the HTML `download` attribute is + // ignored cross-origin, so the response header is what actually forces + // the save dialog when the browser hits S3 directly. + if (!attachment.downloadUrl) { showPopup({ header: t`Download failed`, message: t`No download URL available for this attachment.`, @@ -146,13 +151,14 @@ export function AttachmentThumbnails({ return; } - const downloadUrl = `/api/download/attatchment?url=${encodeURIComponent(attachment.url)}&filename=${encodeURIComponent(attachment.originalFilename ?? "attachment")}`; - const link = document.createElement("a"); - link.href = downloadUrl; + link.href = attachment.downloadUrl; + link.download = attachment.originalFilename ?? "attachment"; + link.rel = "noopener"; link.style.display = "none"; document.body.appendChild(link); link.click(); + document.body.removeChild(link); }; const selectedAttachment = diff --git a/packages/api/src/routers/card.ts b/packages/api/src/routers/card.ts index 751b4fd96..f2d38f82c 100644 --- a/packages/api/src/routers/card.ts +++ b/packages/api/src/routers/card.ts @@ -685,10 +685,26 @@ export const cardRouter = createTRPCRouter({ code: "NOT_FOUND", }); - // Generate URLs for all attachments + // Each attachment carries two presigned URLs: + // url — inline view (used by image thumbnails / lightbox) + // downloadUrl — Content-Disposition: attachment baked into the + // signed URL, so the browser saves the file with its + // original name when downloaded directly from S3. + // Two URLs (not one with a flag) because the same attachment is used + // for two semantically distinct actions; collapsing them caused an + // image-lightbox download regression that flagged in cage-match. const attachmentsWithUrls = await Promise.all( result.attachments.map(async (attachment) => { - const url = await generateAttachmentUrl(attachment.s3Key); + const [url, downloadUrl] = await Promise.all([ + generateAttachmentUrl(attachment.s3Key, { + disposition: "inline", + }), + generateAttachmentUrl(attachment.s3Key, { + disposition: "attachment", + originalFilename: attachment.originalFilename, + contentType: attachment.contentType, + }), + ]); return { publicId: attachment.publicId, contentType: attachment.contentType, @@ -696,6 +712,7 @@ export const cardRouter = createTRPCRouter({ originalFilename: attachment.originalFilename, size: attachment.size, url, + downloadUrl, }; }), ); diff --git a/packages/api/src/schemas/card.ts b/packages/api/src/schemas/card.ts index 2a13b28cb..f60a53869 100644 --- a/packages/api/src/schemas/card.ts +++ b/packages/api/src/schemas/card.ts @@ -57,6 +57,7 @@ export const cardDetailSchema = z.object({ originalFilename: z.string().nullable(), size: z.number().nullable(), url: z.string().nullable(), + downloadUrl: z.string().nullable(), }), ), checklists: z.array(checklistResponseSchema), diff --git a/packages/shared/src/utils/s3.ts b/packages/shared/src/utils/s3.ts index a90fb8855..3ea565d71 100644 --- a/packages/shared/src/utils/s3.ts +++ b/packages/shared/src/utils/s3.ts @@ -47,6 +47,10 @@ export async function generateDownloadUrl( bucket: string, key: string, expiresIn = 3600, + responseOverrides?: { + contentDisposition?: string; + contentType?: string; + }, ) { const client = createS3Client(); return getSignedUrl( @@ -54,6 +58,8 @@ export async function generateDownloadUrl( new GetObjectCommand({ Bucket: bucket, Key: key, + ResponseContentDisposition: responseOverrides?.contentDisposition, + ResponseContentType: responseOverrides?.contentType, }), { expiresIn }, ); @@ -106,7 +112,12 @@ export async function generateAvatarUrl( */ export async function generateAttachmentUrl( attachmentKey: string | null | undefined, - expiresIn = 86400, // 24 hours + options: { + expiresIn?: number; + disposition?: "inline" | "attachment"; + originalFilename?: string | null; + contentType?: string | null; + } = {}, ): Promise { if (!attachmentKey) { return null; @@ -117,8 +128,38 @@ export async function generateAttachmentUrl( return null; } + const expiresIn = options.expiresIn ?? 86400; + + const responseOverrides: { + contentDisposition?: string; + contentType?: string; + } = {}; + + if (options.disposition === "attachment" && options.originalFilename) { + // RFC 5987: ascii fallback + UTF-8 encoded form for non-ascii filenames. + // encodeURIComponent leaves "'" unescaped, but RFC 5987 requires it + // (it's the delimiter inside `filename*=UTF-8''`); patch it. + const safeAscii = options.originalFilename + .replace(/[^\x20-\x7E]/g, "_") + .replace(/[\\"]/g, ""); + const utf8 = encodeURIComponent(options.originalFilename).replace( + /'/g, + "%27", + ); + responseOverrides.contentDisposition = `attachment; filename="${safeAscii}"; filename*=UTF-8''${utf8}`; + } + + if (options.contentType) { + responseOverrides.contentType = options.contentType; + } + try { - return await generateDownloadUrl(bucket, attachmentKey, expiresIn); + return await generateDownloadUrl( + bucket, + attachmentKey, + expiresIn, + Object.keys(responseOverrides).length > 0 ? responseOverrides : undefined, + ); } catch { // If URL generation fails, return null return null;