From 6827460cf4b1d0aa84bfcfecae51a389ad6aba85 Mon Sep 17 00:00:00 2001
From: Sonarly Claude Code <claude-code@sonarly.com>
Date: Sun, 3 May 2026 11:40:40 +0000
Subject: [PATCH] fix: use applicationUniversalIdentifier in deleteByFileId S3
 path construction
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

https://sonarly.com/issue/33714?type=bug

Files uploaded to S3 via the new Files v2 API are never actually deleted from S3 storage because `deleteByFileId` constructs the S3 key using `applicationId` (UUID) instead of `applicationUniversalIdentifier` (string identifier like "twenty-standard"), creating a path mismatch with how files were written.

Fix: Fixed `deleteByFileId` in `FileStorageService` to construct the correct S3 storage path.

**The bug:** Files are written to S3 at path `{workspaceId}/{applicationUniversalIdentifier}/{fileFolder}/{resourcePath}` (via `buildOnStoragePath`), but `deleteByFileId` was constructing the delete path as `{workspaceId}/{applicationId}/{file.path}` — using the application's UUID (`applicationId`) instead of its string identifier (`applicationUniversalIdentifier`). Since S3 `DeleteObject` silently succeeds on non-existent keys, files were never actually removed.

**The fix:** Added an `applicationRepository.findOneOrFail()` lookup to resolve the application's `universalIdentifier` from `file.applicationId`, then reused the existing `buildOnStoragePath` method to construct the correct S3 key — the same pattern used by every other method in this service (`writeFile`, `readFile`, `delete`, etc.). The driver's `delete` call now uses `dirname`/`basename` of the full storage path, matching the convention used in the `copy` method.

**Test:** Added a unit test to `file-storage.service.spec.ts` that verifies `deleteByFileId` looks up the application by ID and constructs the S3 path using `applicationUniversalIdentifier`, not `applicationId`.
---
 .../__tests__/file-storage.service.spec.ts    | 47 +++++++++++++++++++
 .../file-storage/file-storage.service.ts      | 21 ++++++++-
 2 files changed, 66 insertions(+), 2 deletions(-)

diff --git a/packages/twenty-server/src/engine/core-modules/file-storage/__tests__/file-storage.service.spec.ts b/packages/twenty-server/src/engine/core-modules/file-storage/__tests__/file-storage.service.spec.ts
index 3f4a036c686..2e86f3c8f8f 100644
--- a/packages/twenty-server/src/engine/core-modules/file-storage/__tests__/file-storage.service.spec.ts
+++ b/packages/twenty-server/src/engine/core-modules/file-storage/__tests__/file-storage.service.spec.ts
@@ -16,6 +16,8 @@ describe('FileStorageService', () => {
 
   const mockFileRepository = {
     save: jest.fn(),
+    findOneOrFail: jest.fn(),
+    delete: jest.fn(),
   };
 
   const mockApplicationRepository = {
@@ -193,6 +195,51 @@ describe('FileStorageService', () => {
       });
     });
 
+    describe('deleteByFileId', () => {
+      it('should build the correct storage path using applicationUniversalIdentifier', async () => {
+        const fileId = 'file-uuid';
+        const workspaceId = 'ws-uuid';
+        const applicationId = 'app-uuid';
+        const applicationUniversalIdentifier = 'twenty-standard';
+
+        mockFileRepository.findOneOrFail.mockResolvedValue({
+          id: fileId,
+          workspaceId,
+          applicationId,
+          path: 'files-field/field-uid/file-uuid.pdf',
+        });
+
+        mockApplicationRepository.findOneOrFail.mockResolvedValue({
+          id: applicationId,
+          universalIdentifier: applicationUniversalIdentifier,
+          workspaceId,
+        });
+
+        mockDriver.delete.mockResolvedValue(undefined);
+        mockFileRepository.delete.mockResolvedValue(undefined);
+
+        await service.deleteByFileId({
+          fileId,
+          workspaceId,
+          fileFolder: 'files-field' as any,
+        });
+
+        expect(mockApplicationRepository.findOneOrFail).toHaveBeenCalledWith({
+          where: {
+            id: applicationId,
+            workspaceId,
+          },
+        });
+
+        expect(mockDriver.delete).toHaveBeenCalledWith({
+          folderPath: `${workspaceId}/${applicationUniversalIdentifier}/files-field/field-uid`,
+          filename: 'file-uuid.pdf',
+        });
+
+        expect(mockFileRepository.delete).toHaveBeenCalledWith(fileId);
+      });
+    });
+
     describe('checkFolderExistsLegacy', () => {
       it('should delegate to the current driver and return true', async () => {
         const checkParams = {
diff --git a/packages/twenty-server/src/engine/core-modules/file-storage/file-storage.service.ts b/packages/twenty-server/src/engine/core-modules/file-storage/file-storage.service.ts
index c27390d4412..0585d539d77 100644
--- a/packages/twenty-server/src/engine/core-modules/file-storage/file-storage.service.ts
+++ b/packages/twenty-server/src/engine/core-modules/file-storage/file-storage.service.ts
@@ -224,11 +224,28 @@ export class FileStorageService {
         path: Like(`${fileFolder}/%`),
       },
     });
+
+    const application = await this.applicationRepository.findOneOrFail({
+      where: {
+        id: file.applicationId,
+        workspaceId,
+      },
+    });
+
+    const resourcePath = file.path.replace(`${fileFolder}/`, '');
+
+    const onStoragePath = this.buildOnStoragePath({
+      workspaceId,
+      applicationUniversalIdentifier: application.universalIdentifier,
+      fileFolder,
+      resourcePath,
+    });
+
     const driver = this.fileStorageDriverFactory.getCurrentDriver();
 
     await driver.delete({
-      folderPath: `${file.workspaceId}/${file.applicationId}`,
-      filename: file.path,
+      folderPath: dirname(onStoragePath),
+      filename: basename(onStoragePath),
     });
 
     await this.fileRepository.delete(fileId);