fix(storage): validate fileIds, batch deletions, correct test comment

Three review fixes on the orphan-delete path:

- Validate every entry in the request's `fileIds` array against the
  same `^[\w-]{1,200}$` regex used elsewhere. Without this, a non-
  string or path-traversal-shaped id slipped past the array-shape
  check and would only blow up inside the Prisma / S3 calls below.
- Batch the cleanup. The previous loop did per-file
  `s3File.findUnique` + `deleteS3Object` + `s3File.delete` serially.
  For large selections that's N+1 round trips and a stalled HTTP
  request. Use a single `findMany` keyed by the fileId set, parallel
  S3 deletes (concurrency 8) with `Promise.allSettled` so one S3
  failure doesn't drop subsequent deletions, and a single
  `deleteMany` for the rows.
- The diff test's `totalCanvasRefs: 2` came with the comment "active
  only", but the route counts every canvas-referenced fileId
  including soft-deleted ones (`file-a` active + `file-b` deleted).
  Fix the comment so it documents the real semantic.

Change-Id: Id61e9a2d6f17ccc6e6c8922e6b8e6549b32c73f4
Co-developed-by: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: OhYee

backend/src/__tests__/storage.integration.ts

@@ -321,8 +321,12 @@ describe("Storage management routes", () => {
         .set("Authorization", `Bearer ${ownerToken}`);
 
       expect(res.status).toBe(200);
+      // totalCanvasRefs counts every canvas-referenced fileId (active +
+      // soft-deleted), so file-a (active) and file-b (deleted) both
+      // contribute. activeCanvasRefs in the per-row payload below is
+      // what restricts to non-deleted refs.
       expect(res.body.summary).toMatchObject({
-        totalCanvasRefs: 2, // active only
+        totalCanvasRefs: 2,
         totalSqliteFiles: 3,
       });
 

backend/src/routes/storage.ts

@@ -317,12 +317,28 @@ export const registerStorageRoutes = (
       if (!userId) return res.status(401).json({ error: "Unauthorized" });
 
       const { id } = req.params;
-      const { confirmName, fileIds } = req.body ?? {};
+      const { confirmName, fileIds: rawFileIds } = req.body ?? {};
 
-      if (!Array.isArray(fileIds) || fileIds.length === 0) {
+      if (!Array.isArray(rawFileIds) || rawFileIds.length === 0) {
         return res.status(400).json({ error: "fileIds must be a non-empty array" });
       }
 
+      // Validate every entry: same regex as the rest of the codebase
+      // (security.ts sanitiser, /files/:fileId route, processFilesForS3).
+      // Without this, a non-string or path-traversal-shaped id would
+      // explode inside the Prisma / S3 calls below.
+      const VALID_FILE_ID = /^[\w-]{1,200}$/;
+      const invalidIds = rawFileIds.filter(
+        (fid) => typeof fid !== "string" || !VALID_FILE_ID.test(fid),
+      );
+      if (invalidIds.length > 0) {
+        return res.status(400).json({
+          error: "fileIds contains invalid entries",
+          invalidFileIds: invalidIds,
+        });
+      }
+      const fileIds = rawFileIds as string[];
+
       const drawing = await prisma.drawing.findFirst({
         where: { id, userId },
       });
@@ -341,46 +357,61 @@ export const registerStorageRoutes = (
 
       // Safety: reject if any fileId is still referenced by a non-deleted element
       const activeRefs = collectReferencedFileIds(elements, false);
-      const blockedIds = (fileIds as string[]).filter((fid) =>
-        activeRefs.has(fid)
-      );
+      const blockedIds = fileIds.filter((fid) => activeRefs.has(fid));
       if (blockedIds.length > 0) {
         return res.status(400).json({
           error: "Cannot delete files referenced by active elements",
           blockedFileIds: blockedIds,
         });
       }
 
-      let deletedCount = 0;
-      let errorCount = 0;
-
-      // S3File rows are scoped (drawingId, fileId), and each drawing
-      // has its own S3 object under its own prefix path — deleting
-      // here cannot strand a sibling drawing.
-      for (const fileId of fileIds as string[]) {
-        try {
-          if (isS3Enabled()) {
-            const s3Record = await prisma.s3File.findUnique({
-              where: { drawingId_fileId: { drawingId: id, fileId } },
-            });
-            if (s3Record) {
-              await deleteS3Object(s3Record.s3Key);
-              await prisma.s3File.delete({
-                where: { drawingId_fileId: { drawingId: id, fileId } },
-              });
-            }
-          }
+      // Batched S3 + DB cleanup. S3File rows are scoped
+      // (drawingId, fileId), and each drawing has its own S3 object
+      // under its own prefix path — deletion here cannot strand a
+      // sibling drawing. Doing N+1 sequential lookups + deletes per
+      // file would tie up the request unnecessarily for large
+      // selections.
+      let s3ObjectsDeleted = 0;
+      let s3DeleteErrors = 0;
 
-          delete files[fileId];
-          deletedCount++;
-        } catch (err: any) {
-          console.error(
-            `[storage/orphans] Failed to delete fileId=${fileId}`,
-            err
+      if (isS3Enabled()) {
+        const s3Records = await prisma.s3File.findMany({
+          where: { drawingId: id, fileId: { in: fileIds } },
+        });
+
+        const S3_DELETE_CONCURRENCY = 8;
+        for (let i = 0; i < s3Records.length; i += S3_DELETE_CONCURRENCY) {
+          const batch = s3Records.slice(i, i + S3_DELETE_CONCURRENCY);
+          const results = await Promise.allSettled(
+            batch.map((rec) => deleteS3Object(rec.s3Key)),
           );
-          errorCount++;
+          for (let j = 0; j < results.length; j++) {
+            const result = results[j];
+            if (result.status === "fulfilled") {
+              s3ObjectsDeleted++;
+            } else {
+              console.error(
+                `[storage/orphans] Failed to delete S3 object: ${batch[j].s3Key}`,
+                result.reason,
+              );
+              s3DeleteErrors++;
+            }
+          }
         }
+
+        await prisma.s3File.deleteMany({
+          where: { drawingId: id, fileId: { in: fileIds } },
+        });
+      }
+
+      // Update the drawing's files JSON regardless of S3 outcomes —
+      // the JSON entry is what the editor reads, and once trimmed it
+      // can't be restored from the bucket alone.
+      for (const fileId of fileIds) {
+        delete files[fileId];
       }
+      const deletedCount = fileIds.length;
+      const errorCount = s3DeleteErrors;
 
       // Also remove deleted elements that reference the orphaned files,
       // so the files disappear from the diff completely.