fix(attachments): download non-image attachments directly from S3

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 */

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" });
     }
   }),

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 =

packages/api/src/routers/card.ts

@@ -685,17 +685,34 @@ 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,
             s3Key: attachment.s3Key,
             originalFilename: attachment.originalFilename,
             size: attachment.size,
             url,
+            downloadUrl,
           };
         }),
       );

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),

packages/shared/src/utils/s3.ts

@@ -47,13 +47,19 @@ export async function generateDownloadUrl(
   bucket: string,
   key: string,
   expiresIn = 3600,
+  responseOverrides?: {
+    contentDisposition?: string;
+    contentType?: string;
+  },
 ) {
   const client = createS3Client();
   return getSignedUrl(
     client,
     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<string | null> {
   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''<value>`); 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;