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.
This commit is contained in:
Marco Beretta 2026-07-29 04:16:03 +02:00
parent 5e9aae7784
commit 351a69a972
No known key found for this signature in database
GPG key ID: D918033D8E74CC11
2 changed files with 96 additions and 4 deletions

View file

@ -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', () => {

View file

@ -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);
}