From 351a69a9727d6e02820d6a0b6d921ee0569df0df Mon Sep 17 00:00:00 2001
From: Marco Beretta <81851188+berry-13@users.noreply.github.com>
Date: Wed, 29 Jul 2026 04:16:03 +0200
Subject: [PATCH] fix(files): scope deleteFiles by id and owner instead of
 replacing one with the other

Passing a user replaced the id filter rather than narrowing it, so
deleteFiles([oneId], user) deleted every file that user owned. Reads exactly
like the opposite of what it did, and the import's asset cleanup called it
that way - releasing a single unreferenced attachment would have wiped the
account's files while leaving their storage objects behind.

Passing no ids with a user still means everything that user owns, which is
how account deletion calls it, and an unscoped call now refuses rather than
emptying the collection.
---
 .../data-schemas/src/methods/file.spec.ts     | 71 +++++++++++++++++++
 packages/data-schemas/src/methods/file.ts     | 29 ++++++--
 2 files changed, 96 insertions(+), 4 deletions(-)

diff --git a/packages/data-schemas/src/methods/file.spec.ts b/packages/data-schemas/src/methods/file.spec.ts
index 71b12ff5aa..b1439d7d6a 100644
--- a/packages/data-schemas/src/methods/file.spec.ts
+++ b/packages/data-schemas/src/methods/file.spec.ts
@@ -1207,6 +1207,77 @@ describe('File Methods', () => {
       expect(remaining).toHaveLength(1);
       expect(remaining![0].user?.toString()).toBe(otherUserId.toString());
     });
+
+    /**
+     * `user` narrows the id filter; it does not replace it. When it replaced
+     * it, a caller removing a single file by id silently deleted every file
+     * that user owned.
+     */
+    it('should delete only the named file when both ids and a user are given', async () => {
+      const userId = new mongoose.Types.ObjectId();
+      const doomed = uuidv4();
+      const spared = uuidv4();
+
+      await fileMethods.createFile({
+        file_id: doomed,
+        user: userId,
+        filename: 'doomed.txt',
+        filepath: '/uploads/doomed.txt',
+        type: 'text/plain',
+        bytes: 100,
+      });
+      await fileMethods.createFile({
+        file_id: spared,
+        user: userId,
+        filename: 'spared.txt',
+        filepath: '/uploads/spared.txt',
+        type: 'text/plain',
+        bytes: 100,
+      });
+
+      const result = await fileMethods.deleteFiles([doomed], userId.toString());
+
+      expect(result.deletedCount).toBe(1);
+      const remaining = await fileMethods.getFiles({});
+      expect(remaining).toHaveLength(1);
+      expect(remaining![0].file_id).toBe(spared);
+    });
+
+    it("should not delete another user's file that happens to share an id", async () => {
+      const userId = new mongoose.Types.ObjectId();
+      const otherUserId = new mongoose.Types.ObjectId();
+      const sharedId = uuidv4();
+
+      await fileMethods.createFile({
+        file_id: sharedId,
+        user: otherUserId,
+        filename: 'theirs.txt',
+        filepath: '/uploads/theirs.txt',
+        type: 'text/plain',
+        bytes: 100,
+      });
+
+      const result = await fileMethods.deleteFiles([sharedId], userId.toString());
+
+      expect(result.deletedCount).toBe(0);
+      expect(await fileMethods.getFiles({})).toHaveLength(1);
+    });
+
+    it('should refuse to delete anything when given neither ids nor a user', async () => {
+      await fileMethods.createFile({
+        file_id: uuidv4(),
+        user: new mongoose.Types.ObjectId(),
+        filename: 'safe.txt',
+        filepath: '/uploads/safe.txt',
+        type: 'text/plain',
+        bytes: 100,
+      });
+
+      const result = await fileMethods.deleteFiles([]);
+
+      expect(result.deletedCount).toBe(0);
+      expect(await fileMethods.getFiles({})).toHaveLength(1);
+    });
   });
 
   describe('batchUpdateFiles', () => {
diff --git a/packages/data-schemas/src/methods/file.ts b/packages/data-schemas/src/methods/file.ts
index 2e1d9159a5..37f3f3d8df 100644
--- a/packages/data-schemas/src/methods/file.ts
+++ b/packages/data-schemas/src/methods/file.ts
@@ -456,14 +456,35 @@ export function createFileMethods(mongoose: typeof import('mongoose')): {
    * @returns A promise that resolves to the result of the deletion operation
    */
   async function deleteFiles(
-    file_ids: string[],
+    file_ids: string[] | null | undefined,
     user?: string,
   ): Promise<{ deletedCount?: number }> {
     const File = mongoose.models.File as Model<IMongoFile>;
-    let deleteQuery: FilterQuery<IMongoFile> = { file_id: { $in: file_ids } };
-    if (user) {
-      deleteQuery = { user: user };
+    const hasIds = Array.isArray(file_ids) && file_ids.length > 0;
+
+    /**
+     * `user` narrows the id filter rather than replacing it.
+     *
+     * Replacing it made `deleteFiles([oneId], user)` delete every file that
+     * user owns, which reads exactly like the opposite of what it does — a
+     * caller removing one file would silently wipe the account's attachments.
+     * Passing no ids (or an empty array) with a user still means "everything
+     * this user owns", which is how account deletion calls it.
+     */
+    const deleteQuery: FilterQuery<IMongoFile> = {};
+    if (hasIds) {
+      deleteQuery.file_id = { $in: file_ids as string[] };
     }
+    if (user) {
+      deleteQuery.user = user;
+    }
+
+    /** An unscoped `deleteMany({})` would empty the collection. */
+    if (!hasIds && !user) {
+      logger.warn('[deleteFiles] Refusing to delete with neither file ids nor a user');
+      return { deletedCount: 0 };
+    }
+
     return File.deleteMany(deleteQuery);
   }