From ece265c6e43ed559a667de65a48eef51038e2726 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Malfait?= Date: Mon, 9 Feb 2026 10:54:10 +0100 Subject: [PATCH] Harden local file storage driver path resolution (#17783) ## Summary - Normalize all file paths with `path.resolve` instead of `join` to properly handle `..` segments in file path inputs - Add `assertPathIsWithinStorage` guard on all write, delete, move, copy, and existence-check operations - Introduce `ACCESS_DENIED` exception code with i18n-ready user-friendly message - Read path already had realpath-based validation; updated its error code to `ACCESS_DENIED` for consistency ## Test plan - [x] Typecheck passes - [x] Lint passes - [x] Manual: verify file upload/download still works with valid paths - [x] Manual: verify `../` in file paths is rejected with ACCESS_DENIED Made with [Cursor](https://cursor.com) --------- Co-authored-by: Cursor Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: Etienne --- ...kfill-application-package-files.command.ts | 4 +- ...-17-migrate-workflow-code-steps.command.ts | 8 +- .../1-17/1-17-seed-workflow-v1-16.command.ts | 15 +- .../resolvers/application.resolver.ts | 2 +- .../services/application-sync.service.ts | 4 +- .../services/application.service.ts | 8 +- .../file-storage-driver.factory.spec.ts | 22 +- .../__tests__/file-storage.service.spec.ts | 111 ++------ .../validated-storage.driver.spec.ts | 236 ++++++++++++++++++ .../file-storage/drivers/local.driver.ts | 62 +++-- .../drivers/validated-storage.driver.ts | 113 +++++++++ .../file-storage-driver.factory.ts | 10 +- .../file-storage/file-storage.service.ts | 87 ++----- .../interfaces/file-storage-exception.ts | 3 + .../assert-storage-path-is-safe.util.spec.ts | 74 ++++++ .../utils/assert-storage-path-is-safe.util.ts | 31 +++ .../services/file-upload.service.ts | 2 +- .../file/files-field/files-field.service.ts | 4 +- .../file/services/file.service.spec.ts | 4 +- .../file/services/file.service.ts | 10 +- .../drivers/lambda.driver.ts | 6 +- .../drivers/local.driver.ts | 6 +- .../logic-function-executor.service.ts | 4 +- .../logic-function-source-builder.service.ts | 16 +- .../code-interpreter-tool.ts | 4 +- .../data/services/dev-seeder-data.service.ts | 2 +- ...-front-component-action-handler.service.ts | 2 +- ...e-logic-function-action-handler.service.ts | 4 +- ...e-logic-function-action-handler.service.ts | 2 +- ...e-logic-function-action-handler.service.ts | 4 +- 30 files changed, 614 insertions(+), 246 deletions(-) create mode 100644 packages/twenty-server/src/engine/core-modules/file-storage/drivers/__tests__/validated-storage.driver.spec.ts create mode 100644 packages/twenty-server/src/engine/core-modules/file-storage/drivers/validated-storage.driver.ts create mode 100644 packages/twenty-server/src/engine/core-modules/file-storage/utils/__tests__/assert-storage-path-is-safe.util.spec.ts create mode 100644 packages/twenty-server/src/engine/core-modules/file-storage/utils/assert-storage-path-is-safe.util.ts diff --git a/packages/twenty-server/src/database/commands/upgrade-version-command/1-17/1-17-backfill-application-package-files.command.ts b/packages/twenty-server/src/database/commands/upgrade-version-command/1-17/1-17-backfill-application-package-files.command.ts index 5df4d05275..83fa7cc883 100644 --- a/packages/twenty-server/src/database/commands/upgrade-version-command/1-17/1-17-backfill-application-package-files.command.ts +++ b/packages/twenty-server/src/database/commands/upgrade-version-command/1-17/1-17-backfill-application-package-files.command.ts @@ -180,7 +180,7 @@ export class BackfillApplicationPackageFilesCommand extends ActiveOrSuspendedWor toDelete: false, }; - const packageJsonFile = await this.fileStorageService.writeFile_v2({ + const packageJsonFile = await this.fileStorageService.writeFile({ sourceFile: packageJsonContent, mimeType: undefined, fileFolder: FileFolder.Dependencies, @@ -190,7 +190,7 @@ export class BackfillApplicationPackageFilesCommand extends ActiveOrSuspendedWor settings: dependencyFileSettings, }); - const yarnLockFile = await this.fileStorageService.writeFile_v2({ + const yarnLockFile = await this.fileStorageService.writeFile({ sourceFile: layer.yarnLock, mimeType: undefined, fileFolder: FileFolder.Dependencies, diff --git a/packages/twenty-server/src/database/commands/upgrade-version-command/1-17/1-17-migrate-workflow-code-steps.command.ts b/packages/twenty-server/src/database/commands/upgrade-version-command/1-17/1-17-migrate-workflow-code-steps.command.ts index 5bffcb73c1..f80c8a2eb3 100644 --- a/packages/twenty-server/src/database/commands/upgrade-version-command/1-17/1-17-migrate-workflow-code-steps.command.ts +++ b/packages/twenty-server/src/database/commands/upgrade-version-command/1-17/1-17-migrate-workflow-code-steps.command.ts @@ -245,13 +245,13 @@ export class MigrateWorkflowCodeStepsCommand extends ActiveOrSuspendedWorkspaces await fs.mkdir(builtTempDir, { recursive: true }); await fs.mkdir(sourceTempDir, { recursive: true }); - const builtSources = await this.fileStorageService.readFolder( + const builtSources = await this.fileStorageService.readFolderLegacy( oldPaths.built, ); await this.writeSourcesToLocalFolder(builtSources as Sources, builtTempDir); - const sourceSources = await this.fileStorageService.readFolder( + const sourceSources = await this.fileStorageService.readFolderLegacy( oldPaths.source, ); const flattened = @@ -272,7 +272,7 @@ export class MigrateWorkflowCodeStepsCommand extends ActiveOrSuspendedWorkspaces const builtTempDir = join(tempRoot, 'built'); const sourceTempDir = join(tempRoot, 'source'); - await this.fileStorageService.uploadFolder_v2({ + await this.fileStorageService.uploadFolder({ workspaceId, applicationUniversalIdentifier, fileFolder: FileFolder.BuiltLogicFunction, @@ -280,7 +280,7 @@ export class MigrateWorkflowCodeStepsCommand extends ActiveOrSuspendedWorkspaces localPath: builtTempDir, }); - await this.fileStorageService.uploadFolder_v2({ + await this.fileStorageService.uploadFolder({ workspaceId, applicationUniversalIdentifier, fileFolder: FileFolder.Source, diff --git a/packages/twenty-server/src/database/commands/upgrade-version-command/1-17/1-17-seed-workflow-v1-16.command.ts b/packages/twenty-server/src/database/commands/upgrade-version-command/1-17/1-17-seed-workflow-v1-16.command.ts index cd44f6eb83..d55e817a4b 100644 --- a/packages/twenty-server/src/database/commands/upgrade-version-command/1-17/1-17-seed-workflow-v1-16.command.ts +++ b/packages/twenty-server/src/database/commands/upgrade-version-command/1-17/1-17-seed-workflow-v1-16.command.ts @@ -486,8 +486,12 @@ export class SeedWorkflowV1_16Command extends ActiveOrSuspendedWorkspacesMigrati ); try { - await this.fileStorageService.delete({ folderPath: OLD_BUILT_FOLDER }); - await this.fileStorageService.delete({ folderPath: OLD_SOURCE_FOLDER }); + await this.fileStorageService.deleteLegacy({ + folderPath: OLD_BUILT_FOLDER, + }); + await this.fileStorageService.deleteLegacy({ + folderPath: OLD_SOURCE_FOLDER, + }); this.logger.log( `Cleaned old file storage: ${OLD_BUILT_FOLDER}, ${OLD_SOURCE_FOLDER}`, ); @@ -516,7 +520,10 @@ export class SeedWorkflowV1_16Command extends ActiveOrSuspendedWorkspacesMigrati }, }; - await this.fileStorageService.writeFolder(builtSources, builtFolder); - await this.fileStorageService.writeFolder(sourceSources, sourceFolder); + await this.fileStorageService.writeFolderLegacy(builtSources, builtFolder); + await this.fileStorageService.writeFolderLegacy( + sourceSources, + sourceFolder, + ); } } diff --git a/packages/twenty-server/src/engine/core-modules/application/resolvers/application.resolver.ts b/packages/twenty-server/src/engine/core-modules/application/resolvers/application.resolver.ts index 667d95cb26..4df4c32498 100644 --- a/packages/twenty-server/src/engine/core-modules/application/resolvers/application.resolver.ts +++ b/packages/twenty-server/src/engine/core-modules/application/resolvers/application.resolver.ts @@ -205,7 +205,7 @@ export class ApplicationResolver { const buffer = await streamToBuffer(createReadStream()); - return await this.fileStorageService.writeFile_v2({ + return await this.fileStorageService.writeFile({ sourceFile: buffer, mimeType: mimetype, fileFolder, diff --git a/packages/twenty-server/src/engine/core-modules/application/services/application-sync.service.ts b/packages/twenty-server/src/engine/core-modules/application/services/application-sync.service.ts index 0a26727454..b1623ba7f6 100644 --- a/packages/twenty-server/src/engine/core-modules/application/services/application-sync.service.ts +++ b/packages/twenty-server/src/engine/core-modules/application/services/application-sync.service.ts @@ -139,7 +139,7 @@ export class ApplicationSyncService { const packageJson = JSON.parse( ( await streamToBuffer( - await this.fileStorageService.readFile_v2({ + await this.fileStorageService.readFile({ applicationUniversalIdentifier: manifest.application.universalIdentifier, fileFolder: FileFolder.Source, @@ -187,7 +187,7 @@ export class ApplicationSyncService { ) { const yarnLockContent = ( await streamToBuffer( - await this.fileStorageService.readFile_v2({ + await this.fileStorageService.readFile({ applicationUniversalIdentifier: manifest.application.universalIdentifier, fileFolder: FileFolder.Source, diff --git a/packages/twenty-server/src/engine/core-modules/application/services/application.service.ts b/packages/twenty-server/src/engine/core-modules/application/services/application.service.ts index a238ca09c1..1055b08b76 100644 --- a/packages/twenty-server/src/engine/core-modules/application/services/application.service.ts +++ b/packages/twenty-server/src/engine/core-modules/application/services/application.service.ts @@ -359,7 +359,7 @@ export class ApplicationService { defaultPackageFields.yarnLockContent, ); - const packageJsonFile = await this.fileStorageService.writeFile_v2({ + const packageJsonFile = await this.fileStorageService.writeFile({ sourceFile: defaultPackageFields.packageJsonContent, mimeType: undefined, fileFolder: FileFolder.Dependencies, @@ -370,7 +370,7 @@ export class ApplicationService { queryRunner, }); - const yarnLockFile = await this.fileStorageService.writeFile_v2({ + const yarnLockFile = await this.fileStorageService.writeFile({ sourceFile: defaultPackageFields.yarnLockContent, mimeType: undefined, fileFolder: FileFolder.Dependencies, @@ -420,7 +420,7 @@ export class ApplicationService { yarnLockContent, ); - const packageJsonFile = await this.fileStorageService.writeFile_v2({ + const packageJsonFile = await this.fileStorageService.writeFile({ sourceFile: packageJsonContent, mimeType: undefined, fileFolder: FileFolder.Dependencies, @@ -430,7 +430,7 @@ export class ApplicationService { settings: { isTemporaryFile: false, toDelete: false }, }); - const yarnLockFile = await this.fileStorageService.writeFile_v2({ + const yarnLockFile = await this.fileStorageService.writeFile({ sourceFile: yarnLockContent, mimeType: undefined, fileFolder: FileFolder.Dependencies, diff --git a/packages/twenty-server/src/engine/core-modules/file-storage/__tests__/file-storage-driver.factory.spec.ts b/packages/twenty-server/src/engine/core-modules/file-storage/__tests__/file-storage-driver.factory.spec.ts index 5dbd02620c..07f3ae1027 100644 --- a/packages/twenty-server/src/engine/core-modules/file-storage/__tests__/file-storage-driver.factory.spec.ts +++ b/packages/twenty-server/src/engine/core-modules/file-storage/__tests__/file-storage-driver.factory.spec.ts @@ -78,7 +78,7 @@ describe('FileStorageDriverFactory', () => { }); describe('createDriver', () => { - it('should create LocalDriver for local storage', () => { + it('should create ValidatedStorageDriver wrapping LocalDriver for local storage', () => { const storagePath = '/tmp/storage'; jest @@ -93,10 +93,10 @@ describe('FileStorageDriverFactory', () => { const driver = factory['createDriver'](); expect(driver).toBeDefined(); - expect(driver.constructor.name).toBe('LocalDriver'); + expect(driver.constructor.name).toBe('ValidatedStorageDriver'); }); - it('should create S3Driver for S3 storage with access keys', () => { + it('should create ValidatedStorageDriver wrapping S3Driver for S3 storage with access keys', () => { jest .spyOn(twentyConfigService, 'get') .mockImplementation((key: string) => { @@ -121,10 +121,10 @@ describe('FileStorageDriverFactory', () => { const driver = factory['createDriver'](); expect(driver).toBeDefined(); - expect(driver.constructor.name).toBe('S3Driver'); + expect(driver.constructor.name).toBe('ValidatedStorageDriver'); }); - it('should create S3Driver for S3 storage without access keys (using provider chain)', () => { + it('should create ValidatedStorageDriver wrapping S3Driver for S3 storage without access keys (using provider chain)', () => { jest .spyOn(twentyConfigService, 'get') .mockImplementation((key: string) => { @@ -149,7 +149,7 @@ describe('FileStorageDriverFactory', () => { const driver = factory['createDriver'](); expect(driver).toBeDefined(); - expect(driver.constructor.name).toBe('S3Driver'); + expect(driver.constructor.name).toBe('ValidatedStorageDriver'); }); it('should throw error for invalid storage driver type', () => { @@ -177,7 +177,7 @@ describe('FileStorageDriverFactory', () => { const driver = factory.getCurrentDriver(); expect(driver).toBeDefined(); - expect(driver.constructor.name).toBe('LocalDriver'); + expect(driver.constructor.name).toBe('ValidatedStorageDriver'); }); it('should reuse driver when config key unchanged', () => { @@ -224,8 +224,8 @@ describe('FileStorageDriverFactory', () => { const driver2 = factory.getCurrentDriver(); expect(driver1).not.toBe(driver2); - expect(driver1.constructor.name).toBe('LocalDriver'); - expect(driver2.constructor.name).toBe('LocalDriver'); + expect(driver1.constructor.name).toBe('ValidatedStorageDriver'); + expect(driver2.constructor.name).toBe('ValidatedStorageDriver'); }); it('should create new driver when switching from local to S3', () => { @@ -265,8 +265,8 @@ describe('FileStorageDriverFactory', () => { const driver2 = factory.getCurrentDriver(); expect(driver1).not.toBe(driver2); - expect(driver1.constructor.name).toBe('LocalDriver'); - expect(driver2.constructor.name).toBe('S3Driver'); + expect(driver1.constructor.name).toBe('ValidatedStorageDriver'); + expect(driver2.constructor.name).toBe('ValidatedStorageDriver'); }); it('should throw error for unsupported storage type', () => { 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 894d3ddded..eea4a45917 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 @@ -74,7 +74,7 @@ describe('FileStorageService', () => { mockFileStorageDriverFactory.getCurrentDriver.mockReturnValue(mockDriver); }); - describe('writeFile', () => { + describe('writeFileLegacy', () => { it('should delegate to the current driver', async () => { const writeParams = { file: Buffer.from('test content'), @@ -85,7 +85,7 @@ describe('FileStorageService', () => { mockDriver.writeFile.mockResolvedValue(undefined); - await service.writeFile(writeParams); + await service.writeFileLegacy(writeParams); expect(fileStorageDriverFactory.getCurrentDriver).toHaveBeenCalled(); expect(mockDriver.writeFile).toHaveBeenCalledWith({ @@ -107,14 +107,14 @@ describe('FileStorageService', () => { mockDriver.writeFile.mockRejectedValue(error); - await expect(service.writeFile(writeParams)).rejects.toThrow( + await expect(service.writeFileLegacy(writeParams)).rejects.toThrow( 'Write failed', ); expect(fileStorageDriverFactory.getCurrentDriver).toHaveBeenCalled(); }); }); - describe('readFile', () => { + describe('readFileLegacy', () => { it('should delegate to the current driver', async () => { const readParams = { filePath: 'documents/test.txt', @@ -124,7 +124,7 @@ describe('FileStorageService', () => { mockDriver.readFile.mockResolvedValue(mockStream); - const result = await service.readFile(readParams); + const result = await service.readFileLegacy(readParams); expect(fileStorageDriverFactory.getCurrentDriver).toHaveBeenCalled(); expect(mockDriver.readFile).toHaveBeenCalledWith({ @@ -142,14 +142,14 @@ describe('FileStorageService', () => { mockDriver.readFile.mockRejectedValue(error); - await expect(service.readFile(readParams)).rejects.toThrow( + await expect(service.readFileLegacy(readParams)).rejects.toThrow( 'Read failed', ); expect(fileStorageDriverFactory.getCurrentDriver).toHaveBeenCalled(); }); }); - describe('delete', () => { + describe('deleteLegacy', () => { it('should delegate to the current driver with filename', async () => { const deleteParams = { folderPath: 'documents', @@ -158,7 +158,7 @@ describe('FileStorageService', () => { mockDriver.delete.mockResolvedValue(undefined); - await service.delete(deleteParams); + await service.deleteLegacy(deleteParams); expect(fileStorageDriverFactory.getCurrentDriver).toHaveBeenCalled(); expect(mockDriver.delete).toHaveBeenCalledWith(deleteParams); @@ -171,7 +171,7 @@ describe('FileStorageService', () => { mockDriver.delete.mockResolvedValue(undefined); - await service.delete(deleteParams); + await service.deleteLegacy(deleteParams); expect(fileStorageDriverFactory.getCurrentDriver).toHaveBeenCalled(); expect(mockDriver.delete).toHaveBeenCalledWith(deleteParams); @@ -187,7 +187,7 @@ describe('FileStorageService', () => { mockDriver.delete.mockRejectedValue(error); - await expect(service.delete(deleteParams)).rejects.toThrow( + await expect(service.deleteLegacy(deleteParams)).rejects.toThrow( 'Delete failed', ); expect(fileStorageDriverFactory.getCurrentDriver).toHaveBeenCalled(); @@ -195,38 +195,7 @@ describe('FileStorageService', () => { }); }); - describe('move', () => { - it('should delegate to the current driver', async () => { - const moveParams = { - from: { folderPath: 'documents', filename: 'test.txt' }, - to: { folderPath: 'archive', filename: 'archived-test.txt' }, - }; - - mockDriver.move.mockResolvedValue(undefined); - - await service.move(moveParams); - - expect(fileStorageDriverFactory.getCurrentDriver).toHaveBeenCalled(); - expect(mockDriver.move).toHaveBeenCalledWith(moveParams); - }); - - it('should handle move errors', async () => { - const moveParams = { - from: { folderPath: 'documents', filename: 'test.txt' }, - to: { folderPath: 'archive', filename: 'archived-test.txt' }, - }; - - const error = new Error('Move failed'); - - mockDriver.move.mockRejectedValue(error); - - await expect(service.move(moveParams)).rejects.toThrow('Move failed'); - expect(fileStorageDriverFactory.getCurrentDriver).toHaveBeenCalled(); - expect(mockDriver.move).toHaveBeenCalledWith(moveParams); - }); - }); - - describe('copy', () => { + describe('copyLegacy', () => { it('should delegate to the current driver', async () => { const copyParams = { from: { folderPath: 'documents', filename: 'test.txt' }, @@ -235,7 +204,7 @@ describe('FileStorageService', () => { mockDriver.copy.mockResolvedValue(undefined); - await service.copy(copyParams); + await service.copyLegacy(copyParams); expect(fileStorageDriverFactory.getCurrentDriver).toHaveBeenCalled(); expect(mockDriver.copy).toHaveBeenCalledWith(copyParams); @@ -251,59 +220,15 @@ describe('FileStorageService', () => { mockDriver.copy.mockRejectedValue(error); - await expect(service.copy(copyParams)).rejects.toThrow('Copy failed'); + await expect(service.copyLegacy(copyParams)).rejects.toThrow( + 'Copy failed', + ); expect(fileStorageDriverFactory.getCurrentDriver).toHaveBeenCalled(); expect(mockDriver.copy).toHaveBeenCalledWith(copyParams); }); }); - describe('checkFileExists', () => { - it('should delegate to the current driver and return true', async () => { - const checkParams = { - filePath: 'documents/test.txt', - }; - - mockDriver.checkFileExists.mockResolvedValue(true); - - const result = await service.checkFileExists(checkParams); - - expect(fileStorageDriverFactory.getCurrentDriver).toHaveBeenCalled(); - expect(mockDriver.checkFileExists).toHaveBeenCalledWith(checkParams); - expect(result).toBe(true); - }); - - it('should delegate to the current driver and return false', async () => { - const checkParams = { - filePath: 'documents/nonexistent.txt', - }; - - mockDriver.checkFileExists.mockResolvedValue(false); - - const result = await service.checkFileExists(checkParams); - - expect(fileStorageDriverFactory.getCurrentDriver).toHaveBeenCalled(); - expect(mockDriver.checkFileExists).toHaveBeenCalledWith(checkParams); - expect(result).toBe(false); - }); - - it('should handle checkFileExists errors', async () => { - const checkParams = { - filePath: 'documents/test.txt', - }; - - const error = new Error('Check failed'); - - mockDriver.checkFileExists.mockRejectedValue(error); - - await expect(service.checkFileExists(checkParams)).rejects.toThrow( - 'Check failed', - ); - expect(fileStorageDriverFactory.getCurrentDriver).toHaveBeenCalled(); - expect(mockDriver.checkFileExists).toHaveBeenCalledWith(checkParams); - }); - }); - - describe('checkFolderExists', () => { + describe('checkFolderExistsLegacy', () => { it('should delegate to the current driver and return true', async () => { const checkParams = { folderPath: 'documents', @@ -311,7 +236,7 @@ describe('FileStorageService', () => { mockDriver.checkFolderExists.mockResolvedValue(true); - const result = await service.checkFolderExists(checkParams); + const result = await service.checkFolderExistsLegacy(checkParams); expect(fileStorageDriverFactory.getCurrentDriver).toHaveBeenCalled(); expect(mockDriver.checkFolderExists).toHaveBeenCalledWith(checkParams); @@ -325,7 +250,7 @@ describe('FileStorageService', () => { mockDriver.checkFolderExists.mockResolvedValue(false); - const result = await service.checkFolderExists(checkParams); + const result = await service.checkFolderExistsLegacy(checkParams); expect(fileStorageDriverFactory.getCurrentDriver).toHaveBeenCalled(); expect(mockDriver.checkFolderExists).toHaveBeenCalledWith(checkParams); diff --git a/packages/twenty-server/src/engine/core-modules/file-storage/drivers/__tests__/validated-storage.driver.spec.ts b/packages/twenty-server/src/engine/core-modules/file-storage/drivers/__tests__/validated-storage.driver.spec.ts new file mode 100644 index 0000000000..745ef66afb --- /dev/null +++ b/packages/twenty-server/src/engine/core-modules/file-storage/drivers/__tests__/validated-storage.driver.spec.ts @@ -0,0 +1,236 @@ +import { Readable } from 'stream'; + +import { type StorageDriver } from 'src/engine/core-modules/file-storage/drivers/interfaces/storage-driver.interface'; +import { FileStorageExceptionCode } from 'src/engine/core-modules/file-storage/interfaces/file-storage-exception'; + +import { ValidatedStorageDriver } from 'src/engine/core-modules/file-storage/drivers/validated-storage.driver'; + +const createMockDriver = (): jest.Mocked => ({ + readFile: jest.fn().mockResolvedValue(Readable.from([])), + writeFile: jest.fn().mockResolvedValue(undefined), + downloadFolder: jest.fn().mockResolvedValue(undefined), + uploadFolder: jest.fn().mockResolvedValue(undefined), + downloadFile: jest.fn().mockResolvedValue(undefined), + delete: jest.fn().mockResolvedValue(undefined), + move: jest.fn().mockResolvedValue(undefined), + copy: jest.fn().mockResolvedValue(undefined), + checkFileExists: jest.fn().mockResolvedValue(true), + checkFolderExists: jest.fn().mockResolvedValue(true), +}); + +describe('ValidatedStorageDriver', () => { + let mockDelegate: jest.Mocked; + let driver: ValidatedStorageDriver; + + beforeEach(() => { + mockDelegate = createMockDriver(); + driver = new ValidatedStorageDriver(mockDelegate); + }); + + describe('delegates to the underlying driver for safe paths', () => { + it('should delegate readFile', async () => { + await driver.readFile({ filePath: 'folder/file.txt' }); + + expect(mockDelegate.readFile).toHaveBeenCalledWith({ + filePath: 'folder/file.txt', + }); + }); + + it('should delegate writeFile', async () => { + const params = { + filePath: 'folder/file.txt', + sourceFile: Buffer.from('data'), + mimeType: 'text/plain' as string | undefined, + }; + + await driver.writeFile(params); + + expect(mockDelegate.writeFile).toHaveBeenCalledWith(params); + }); + + it('should delegate downloadFolder', async () => { + await driver.downloadFolder({ + onStoragePath: 'folder', + localPath: '/tmp/local', + }); + + expect(mockDelegate.downloadFolder).toHaveBeenCalledWith({ + onStoragePath: 'folder', + localPath: '/tmp/local', + }); + }); + + it('should delegate uploadFolder', async () => { + await driver.uploadFolder({ + localPath: '/tmp/local', + onStoragePath: 'folder', + }); + + expect(mockDelegate.uploadFolder).toHaveBeenCalledWith({ + localPath: '/tmp/local', + onStoragePath: 'folder', + }); + }); + + it('should delegate delete', async () => { + await driver.delete({ folderPath: 'folder', filename: 'file.txt' }); + + expect(mockDelegate.delete).toHaveBeenCalledWith({ + folderPath: 'folder', + filename: 'file.txt', + }); + }); + + it('should delegate move', async () => { + const params = { + from: { folderPath: 'a', filename: 'f1.txt' }, + to: { folderPath: 'b', filename: 'f2.txt' }, + }; + + await driver.move(params); + + expect(mockDelegate.move).toHaveBeenCalledWith(params); + }); + + it('should delegate copy', async () => { + const params = { + from: { folderPath: 'a', filename: 'f1.txt' }, + to: { folderPath: 'b', filename: 'f2.txt' }, + }; + + await driver.copy(params); + + expect(mockDelegate.copy).toHaveBeenCalledWith(params); + }); + + it('should delegate checkFileExists', async () => { + await driver.checkFileExists({ filePath: 'folder/file.txt' }); + + expect(mockDelegate.checkFileExists).toHaveBeenCalledWith({ + filePath: 'folder/file.txt', + }); + }); + + it('should delegate checkFolderExists', async () => { + await driver.checkFolderExists({ folderPath: 'folder' }); + + expect(mockDelegate.checkFolderExists).toHaveBeenCalledWith({ + folderPath: 'folder', + }); + }); + }); + + describe('rejects path traversal attempts', () => { + it('should reject readFile with traversal', async () => { + await expect( + driver.readFile({ filePath: '../etc/passwd' }), + ).rejects.toMatchObject({ + code: FileStorageExceptionCode.ACCESS_DENIED, + }); + + expect(mockDelegate.readFile).not.toHaveBeenCalled(); + }); + + it('should reject writeFile with traversal', async () => { + await expect( + driver.writeFile({ + filePath: '../../evil', + sourceFile: Buffer.from('x'), + mimeType: undefined, + }), + ).rejects.toMatchObject({ + code: FileStorageExceptionCode.ACCESS_DENIED, + }); + + expect(mockDelegate.writeFile).not.toHaveBeenCalled(); + }); + + it('should reject delete with traversal in folderPath', async () => { + await expect( + driver.delete({ folderPath: '../secret' }), + ).rejects.toMatchObject({ + code: FileStorageExceptionCode.ACCESS_DENIED, + }); + + expect(mockDelegate.delete).not.toHaveBeenCalled(); + }); + + it('should reject delete with traversal in filename', async () => { + await expect( + driver.delete({ folderPath: 'folder', filename: '../../etc/passwd' }), + ).rejects.toMatchObject({ + code: FileStorageExceptionCode.ACCESS_DENIED, + }); + + expect(mockDelegate.delete).not.toHaveBeenCalled(); + }); + + it('should reject move with traversal', async () => { + await expect( + driver.move({ + from: { folderPath: '../secret' }, + to: { folderPath: 'dest' }, + }), + ).rejects.toMatchObject({ + code: FileStorageExceptionCode.ACCESS_DENIED, + }); + + expect(mockDelegate.move).not.toHaveBeenCalled(); + }); + + it('should reject copy with traversal', async () => { + await expect( + driver.copy({ + from: { folderPath: 'src' }, + to: { folderPath: '../../etc' }, + }), + ).rejects.toMatchObject({ + code: FileStorageExceptionCode.ACCESS_DENIED, + }); + + expect(mockDelegate.copy).not.toHaveBeenCalled(); + }); + + it('should reject downloadFolder with traversal', async () => { + await expect( + driver.downloadFolder({ + onStoragePath: '../../../etc', + localPath: '/tmp/local', + }), + ).rejects.toMatchObject({ + code: FileStorageExceptionCode.ACCESS_DENIED, + }); + + expect(mockDelegate.downloadFolder).not.toHaveBeenCalled(); + }); + }); + + describe('does NOT validate localPath parameters', () => { + it('should allow absolute localPath in downloadFolder', async () => { + await driver.downloadFolder({ + onStoragePath: 'folder', + localPath: '/tmp/any-path', + }); + + expect(mockDelegate.downloadFolder).toHaveBeenCalled(); + }); + + it('should allow absolute localPath in uploadFolder', async () => { + await driver.uploadFolder({ + localPath: '/tmp/any-path', + onStoragePath: 'folder', + }); + + expect(mockDelegate.uploadFolder).toHaveBeenCalled(); + }); + + it('should allow absolute localPath in downloadFile', async () => { + await driver.downloadFile({ + onStoragePath: 'folder/file.txt', + localPath: '/tmp/any-path', + }); + + expect(mockDelegate.downloadFile).toHaveBeenCalled(); + }); + }); +}); diff --git a/packages/twenty-server/src/engine/core-modules/file-storage/drivers/local.driver.ts b/packages/twenty-server/src/engine/core-modules/file-storage/drivers/local.driver.ts index 358f08d38d..4da02794e3 100644 --- a/packages/twenty-server/src/engine/core-modules/file-storage/drivers/local.driver.ts +++ b/packages/twenty-server/src/engine/core-modules/file-storage/drivers/local.driver.ts @@ -20,12 +20,23 @@ export class LocalDriver implements StorageDriver { this.options = options; } - private async createFolder(path: string) { - return fs.mkdir(path, { recursive: true }); + private async createFolder(folderPath: string) { + return fs.mkdir(folderPath, { recursive: true }); + } + + private assertRealPathIsWithinStorage(realPath: string): void { + const storageRoot = realpathSync(path.resolve(this.options.storagePath)); + + if (!realPath.startsWith(storageRoot + path.sep)) { + throw new FileStorageException( + 'Access denied', + FileStorageExceptionCode.ACCESS_DENIED, + ); + } } async readFile(params: { filePath: string }): Promise { - const joinedPath = join(`${this.options.storagePath}/`, params.filePath); + const joinedPath = join(this.options.storagePath, params.filePath); let filePath: string; try { @@ -36,14 +47,8 @@ export class LocalDriver implements StorageDriver { FileStorageExceptionCode.FILE_NOT_FOUND, ); } - const storageRoot = realpathSync(path.resolve(this.options.storagePath)); - if (!filePath.startsWith(storageRoot + path.sep)) { - throw new FileStorageException( - 'Access denied', - FileStorageExceptionCode.FILE_NOT_FOUND, - ); - } + this.assertRealPathIsWithinStorage(filePath); try { return createReadStream(filePath); @@ -64,7 +69,7 @@ export class LocalDriver implements StorageDriver { sourceFile: Buffer | Uint8Array | string; mimeType: string | undefined; }): Promise { - const filePath = `${this.options.storagePath}/${params.filePath}`; + const filePath = path.resolve(this.options.storagePath, params.filePath); const folderPath = dirname(filePath); await this.createFolder(folderPath); @@ -76,9 +81,12 @@ export class LocalDriver implements StorageDriver { onStoragePath: string; localPath: string; }): Promise { - await this.createFolder(dirname(params.localPath)); + const filePath = path.resolve( + this.options.storagePath, + params.onStoragePath, + ); - const filePath = join(`${this.options.storagePath}/`, params.onStoragePath); + await this.createFolder(dirname(params.localPath)); const content = await fs.readFile(filePath); @@ -89,8 +97,8 @@ export class LocalDriver implements StorageDriver { onStoragePath: string; localPath: string; }): Promise { - const rootFolderPath = join( - `${this.options.storagePath}/`, + const rootFolderPath = path.resolve( + this.options.storagePath, params.onStoragePath, ); @@ -146,8 +154,8 @@ export class LocalDriver implements StorageDriver { folderPath: string; filename?: string; }): Promise { - const filePath = join( - `${this.options.storagePath}/`, + const filePath = path.resolve( + this.options.storagePath, params.folderPath, params.filename || '', ); @@ -159,14 +167,14 @@ export class LocalDriver implements StorageDriver { from: { folderPath: string; filename?: string }; to: { folderPath: string; filename?: string }; }): Promise { - const fromPath = join( - `${this.options.storagePath}/`, + const fromPath = path.resolve( + this.options.storagePath, params.from.folderPath, params.from.filename || '', ); - const toPath = join( - `${this.options.storagePath}/`, + const toPath = path.resolve( + this.options.storagePath, params.to.folderPath, params.to.filename || '', ); @@ -194,13 +202,14 @@ export class LocalDriver implements StorageDriver { if (!params.from.filename && params.to.filename) { throw new Error('Cannot copy folder to file'); } - const fromPath = join( + + const fromPath = path.resolve( this.options.storagePath, params.from.folderPath, params.from.filename || '', ); - const toPath = join( + const toPath = path.resolve( this.options.storagePath, params.to.folderPath, params.to.filename || '', @@ -223,13 +232,16 @@ export class LocalDriver implements StorageDriver { } async checkFileExists(params: { filePath: string }): Promise { - const fullPath = join(this.options.storagePath, params.filePath); + const fullPath = path.resolve(this.options.storagePath, params.filePath); return existsSync(fullPath); } async checkFolderExists(params: { folderPath: string }): Promise { - const folderFullPath = join(this.options.storagePath, params.folderPath); + const folderFullPath = path.resolve( + this.options.storagePath, + params.folderPath, + ); return existsSync(folderFullPath); } diff --git a/packages/twenty-server/src/engine/core-modules/file-storage/drivers/validated-storage.driver.ts b/packages/twenty-server/src/engine/core-modules/file-storage/drivers/validated-storage.driver.ts new file mode 100644 index 0000000000..1537b5c528 --- /dev/null +++ b/packages/twenty-server/src/engine/core-modules/file-storage/drivers/validated-storage.driver.ts @@ -0,0 +1,113 @@ +import { type Readable } from 'stream'; + +import { type StorageDriver } from 'src/engine/core-modules/file-storage/drivers/interfaces/storage-driver.interface'; + +import { assertStoragePathIsSafe } from 'src/engine/core-modules/file-storage/utils/assert-storage-path-is-safe.util'; + +export class ValidatedStorageDriver implements StorageDriver { + constructor(private readonly delegate: StorageDriver) {} + + async readFile(params: { filePath: string }): Promise { + assertStoragePathIsSafe(params.filePath); + + return this.delegate.readFile(params); + } + + async writeFile(params: { + filePath: string; + sourceFile: Buffer | Uint8Array | string; + mimeType: string | undefined; + }): Promise { + assertStoragePathIsSafe(params.filePath); + + return this.delegate.writeFile(params); + } + + async downloadFolder(params: { + onStoragePath: string; + localPath: string; + }): Promise { + assertStoragePathIsSafe(params.onStoragePath); + + return this.delegate.downloadFolder(params); + } + + async uploadFolder(params: { + localPath: string; + onStoragePath: string; + }): Promise { + assertStoragePathIsSafe(params.onStoragePath); + + return this.delegate.uploadFolder(params); + } + + async downloadFile(params: { + onStoragePath: string; + localPath: string; + }): Promise { + assertStoragePathIsSafe(params.onStoragePath); + + return this.delegate.downloadFile(params); + } + + async delete(params: { + folderPath: string; + filename?: string; + }): Promise { + assertStoragePathIsSafe(params.folderPath); + + if (params.filename) { + assertStoragePathIsSafe(params.filename); + } + + return this.delegate.delete(params); + } + + async move(params: { + from: { folderPath: string; filename?: string }; + to: { folderPath: string; filename?: string }; + }): Promise { + assertStoragePathIsSafe(params.from.folderPath); + assertStoragePathIsSafe(params.to.folderPath); + + if (params.from.filename) { + assertStoragePathIsSafe(params.from.filename); + } + + if (params.to.filename) { + assertStoragePathIsSafe(params.to.filename); + } + + return this.delegate.move(params); + } + + async copy(params: { + from: { folderPath: string; filename?: string }; + to: { folderPath: string; filename?: string }; + }): Promise { + assertStoragePathIsSafe(params.from.folderPath); + assertStoragePathIsSafe(params.to.folderPath); + + if (params.from.filename) { + assertStoragePathIsSafe(params.from.filename); + } + + if (params.to.filename) { + assertStoragePathIsSafe(params.to.filename); + } + + return this.delegate.copy(params); + } + + async checkFileExists(params: { filePath: string }): Promise { + assertStoragePathIsSafe(params.filePath); + + return this.delegate.checkFileExists(params); + } + + async checkFolderExists(params: { folderPath: string }): Promise { + assertStoragePathIsSafe(params.folderPath); + + return this.delegate.checkFolderExists(params); + } +} diff --git a/packages/twenty-server/src/engine/core-modules/file-storage/file-storage-driver.factory.ts b/packages/twenty-server/src/engine/core-modules/file-storage/file-storage-driver.factory.ts index 743a883730..93acc0d933 100644 --- a/packages/twenty-server/src/engine/core-modules/file-storage/file-storage-driver.factory.ts +++ b/packages/twenty-server/src/engine/core-modules/file-storage/file-storage-driver.factory.ts @@ -7,6 +7,7 @@ import { StorageDriverType } from 'src/engine/core-modules/file-storage/interfac import { LocalDriver } from 'src/engine/core-modules/file-storage/drivers/local.driver'; import { S3Driver } from 'src/engine/core-modules/file-storage/drivers/s3.driver'; +import { ValidatedStorageDriver } from 'src/engine/core-modules/file-storage/drivers/validated-storage.driver'; import { DriverFactoryBase } from 'src/engine/core-modules/twenty-config/dynamic-factory.base'; import { ConfigVariablesGroup } from 'src/engine/core-modules/twenty-config/enums/config-variables-group.enum'; import { TwentyConfigService } from 'src/engine/core-modules/twenty-config/twenty-config.service'; @@ -40,14 +41,16 @@ export class FileStorageDriverFactory extends DriverFactoryBase { protected createDriver(): StorageDriver { const storageType = this.twentyConfigService.get('STORAGE_TYPE'); + let rawDriver: StorageDriver; switch (storageType) { case StorageDriverType.LOCAL: { const storagePath = this.twentyConfigService.get('STORAGE_LOCAL_PATH'); - return new LocalDriver({ + rawDriver = new LocalDriver({ storagePath: resolveAbsolutePath(storagePath), }); + break; } case StorageDriverType.S_3: { @@ -61,7 +64,7 @@ export class FileStorageDriverFactory extends DriverFactoryBase { 'STORAGE_S3_SECRET_ACCESS_KEY', ); - return new S3Driver({ + rawDriver = new S3Driver({ bucketName: bucketName ?? '', endpoint: endpoint, credentials: accessKeyId @@ -70,10 +73,13 @@ export class FileStorageDriverFactory extends DriverFactoryBase { forcePathStyle: true, region: region ?? '', }); + break; } default: throw new Error(`Invalid storage driver type: ${storageType}`); } + + return new ValidatedStorageDriver(rawDriver); } } 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 4e82c8226f..7569144499 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 @@ -40,10 +40,7 @@ export class FileStorageService { return `${workspaceId}/${applicationUniversalIdentifier}/${fileFolder}/${resourcePath}`; } - /** - * @deprecated Use writeFile_v2 instead - */ - writeFile(params: { + writeFileLegacy(params: { file: string | Buffer | Uint8Array; name: string; folder: string; @@ -60,7 +57,7 @@ export class FileStorageService { }); } - async writeFile_v2({ + async writeFile({ sourceFile, mimeType, fileFolder, @@ -130,16 +127,13 @@ export class FileStorageService { }); } - /** - * @deprecated Use readFile_v2 instead - */ - readFile(params: { filePath: string }): Promise { + readFileLegacy(params: { filePath: string }): Promise { const driver = this.fileStorageDriverFactory.getCurrentDriver(); return driver.readFile(params); } - readFile_v2(params: ResourceIdentifier): Promise { + readFile(params: ResourceIdentifier): Promise { const driver = this.fileStorageDriverFactory.getCurrentDriver(); const onStoragePath = this.buildOnStoragePath(params); @@ -147,16 +141,13 @@ export class FileStorageService { return driver.readFile({ filePath: onStoragePath }); } - /** - * @deprecated Use uploadFolder_v2 with local temp directory instead - */ - async writeFolder(sources: Sources, folderPath: string): Promise { + async writeFolderLegacy(sources: Sources, folderPath: string): Promise { for (const key of Object.keys(sources)) { if (isObject(sources[key])) { - await this.writeFolder(sources[key], join(folderPath, key)); + await this.writeFolderLegacy(sources[key], join(folderPath, key)); continue; } - await this.writeFile({ + await this.writeFileLegacy({ file: sources[key], name: key, folder: folderPath, @@ -165,10 +156,7 @@ export class FileStorageService { } } - /** - * @deprecated Use downloadFolder_v2 with local temp directory instead - */ - async readFolder( + async readFolderLegacy( folderPath: string, localTempPath?: string, ): Promise { @@ -203,7 +191,7 @@ export class FileStorageService { return sources; } - async readFolder_v2(params: ResourceIdentifier): Promise { + async readFolder(params: ResourceIdentifier): Promise { const driver = this.fileStorageDriverFactory.getCurrentDriver(); const onStoragePath = this.buildOnStoragePath(params); const tempDir = `/tmp/twenty-read-folder-${Date.now()}`; @@ -218,7 +206,7 @@ export class FileStorageService { return this.readLocalFolderToSources(tempDir); } - uploadFolder_v2( + uploadFolder( params: ResourceIdentifier & { localPath: string }, ): Promise { const driver = this.fileStorageDriverFactory.getCurrentDriver(); @@ -230,7 +218,7 @@ export class FileStorageService { }); } - downloadFolder_v2( + downloadFolder( params: ResourceIdentifier & { localPath: string }, ): Promise { const driver = this.fileStorageDriverFactory.getCurrentDriver(); @@ -242,7 +230,7 @@ export class FileStorageService { }); } - downloadFile_v2( + downloadFile( params: ResourceIdentifier & { localPath: string }, ): Promise { const driver = this.fileStorageDriverFactory.getCurrentDriver(); @@ -254,16 +242,16 @@ export class FileStorageService { }); } - /** - * @deprecated Use delete_v2 instead - */ - delete(params: { folderPath: string; filename?: string }): Promise { + deleteLegacy(params: { + folderPath: string; + filename?: string; + }): Promise { const driver = this.fileStorageDriverFactory.getCurrentDriver(); return driver.delete(params); } - delete_v2(params: ResourceIdentifier): Promise { + delete(params: ResourceIdentifier): Promise { const driver = this.fileStorageDriverFactory.getCurrentDriver(); const onStoragePath = this.buildOnStoragePath(params); @@ -296,10 +284,7 @@ export class FileStorageService { await this.fileRepository.delete(fileId); } - /** - * @deprecated Use copy_v2 instead - */ - copy(params: { + copyLegacy(params: { from: { folderPath: string; filename?: string }; to: { folderPath: string; filename?: string }; }): Promise { @@ -308,7 +293,7 @@ export class FileStorageService { return driver.copy(params); } - copy_v2({ + copy({ from, to, }: { @@ -367,19 +352,7 @@ export class FileStorageService { }); } - /** - * @deprecated Use move_v2 instead - */ - move(params: { - from: { folderPath: string; filename?: string }; - to: { folderPath: string; filename?: string }; - }): Promise { - const driver = this.fileStorageDriverFactory.getCurrentDriver(); - - return driver.move(params); - } - - move_v2({ + move({ from, to, }: { @@ -394,32 +367,20 @@ export class FileStorageService { }); } - /** - * @deprecated Use checkFileExists_v2 instead - */ - checkFileExists(params: { filePath: string }): Promise { + checkFolderExistsLegacy(params: { folderPath: string }): Promise { const driver = this.fileStorageDriverFactory.getCurrentDriver(); - return driver.checkFileExists(params); + return driver.checkFolderExists(params); } - checkFileExists_v2(params: ResourceIdentifier): Promise { + checkFileExists(params: ResourceIdentifier): Promise { const driver = this.fileStorageDriverFactory.getCurrentDriver(); const onStoragePath = this.buildOnStoragePath(params); return driver.checkFileExists({ filePath: onStoragePath }); } - /** - * @deprecated Use checkFolderExists_v2 instead - */ - checkFolderExists(params: { folderPath: string }): Promise { - const driver = this.fileStorageDriverFactory.getCurrentDriver(); - - return driver.checkFolderExists(params); - } - - checkFolderExists_v2(params: ResourceIdentifier): Promise { + checkFolderExists(params: ResourceIdentifier): Promise { const driver = this.fileStorageDriverFactory.getCurrentDriver(); const onStoragePath = this.buildOnStoragePath(params); diff --git a/packages/twenty-server/src/engine/core-modules/file-storage/interfaces/file-storage-exception.ts b/packages/twenty-server/src/engine/core-modules/file-storage/interfaces/file-storage-exception.ts index cd063112ff..ec328df5cc 100644 --- a/packages/twenty-server/src/engine/core-modules/file-storage/interfaces/file-storage-exception.ts +++ b/packages/twenty-server/src/engine/core-modules/file-storage/interfaces/file-storage-exception.ts @@ -6,6 +6,7 @@ import { CustomException } from 'src/utils/custom-exception'; export enum FileStorageExceptionCode { FILE_NOT_FOUND = 'FILE_NOT_FOUND', + ACCESS_DENIED = 'ACCESS_DENIED', } const getFileStorageExceptionUserFriendlyMessage = ( @@ -14,6 +15,8 @@ const getFileStorageExceptionUserFriendlyMessage = ( switch (code) { case FileStorageExceptionCode.FILE_NOT_FOUND: return msg`File not found.`; + case FileStorageExceptionCode.ACCESS_DENIED: + return msg`Access denied.`; default: assertUnreachable(code); } diff --git a/packages/twenty-server/src/engine/core-modules/file-storage/utils/__tests__/assert-storage-path-is-safe.util.spec.ts b/packages/twenty-server/src/engine/core-modules/file-storage/utils/__tests__/assert-storage-path-is-safe.util.spec.ts new file mode 100644 index 0000000000..be6ec67b3b --- /dev/null +++ b/packages/twenty-server/src/engine/core-modules/file-storage/utils/__tests__/assert-storage-path-is-safe.util.spec.ts @@ -0,0 +1,74 @@ +import { FileStorageExceptionCode } from 'src/engine/core-modules/file-storage/interfaces/file-storage-exception'; + +import { assertStoragePathIsSafe } from 'src/engine/core-modules/file-storage/utils/assert-storage-path-is-safe.util'; + +describe('assertStoragePathIsSafe', () => { + it('should accept valid relative paths', () => { + expect(() => assertStoragePathIsSafe('folder/file.txt')).not.toThrow(); + expect(() => + assertStoragePathIsSafe('workspace-123/profile-picture/avatar.png'), + ).not.toThrow(); + expect(() => assertStoragePathIsSafe('simple.txt')).not.toThrow(); + expect(() => + assertStoragePathIsSafe('a/b/c/d/e/deeply-nested-file.json'), + ).not.toThrow(); + }); + + it('should reject paths with .. traversal segments', () => { + expect(() => assertStoragePathIsSafe('../etc/passwd')).toThrow( + expect.objectContaining({ + code: FileStorageExceptionCode.ACCESS_DENIED, + }), + ); + expect(() => assertStoragePathIsSafe('folder/../../etc/passwd')).toThrow( + expect.objectContaining({ + code: FileStorageExceptionCode.ACCESS_DENIED, + }), + ); + expect(() => assertStoragePathIsSafe('..')).toThrow( + expect.objectContaining({ + code: FileStorageExceptionCode.ACCESS_DENIED, + }), + ); + }); + + it('should reject paths with null bytes', () => { + expect(() => assertStoragePathIsSafe('file\0.txt')).toThrow( + expect.objectContaining({ + code: FileStorageExceptionCode.ACCESS_DENIED, + }), + ); + expect(() => assertStoragePathIsSafe('folder/\0/file.txt')).toThrow( + expect.objectContaining({ + code: FileStorageExceptionCode.ACCESS_DENIED, + }), + ); + }); + + it('should reject absolute paths', () => { + expect(() => assertStoragePathIsSafe('/etc/passwd')).toThrow( + expect.objectContaining({ + code: FileStorageExceptionCode.ACCESS_DENIED, + }), + ); + expect(() => assertStoragePathIsSafe('/tmp/file.txt')).toThrow( + expect.objectContaining({ + code: FileStorageExceptionCode.ACCESS_DENIED, + }), + ); + }); + + it('should handle normalized traversal attempts', () => { + expect(() => assertStoragePathIsSafe('folder/../../../etc/passwd')).toThrow( + expect.objectContaining({ + code: FileStorageExceptionCode.ACCESS_DENIED, + }), + ); + }); + + it('should accept paths with dots that are not traversal', () => { + expect(() => assertStoragePathIsSafe('.hidden-file')).not.toThrow(); + expect(() => assertStoragePathIsSafe('folder/.gitignore')).not.toThrow(); + expect(() => assertStoragePathIsSafe('file.name.ext')).not.toThrow(); + }); +}); diff --git a/packages/twenty-server/src/engine/core-modules/file-storage/utils/assert-storage-path-is-safe.util.ts b/packages/twenty-server/src/engine/core-modules/file-storage/utils/assert-storage-path-is-safe.util.ts new file mode 100644 index 0000000000..1709e8ab90 --- /dev/null +++ b/packages/twenty-server/src/engine/core-modules/file-storage/utils/assert-storage-path-is-safe.util.ts @@ -0,0 +1,31 @@ +import path from 'path'; + +import { + FileStorageException, + FileStorageExceptionCode, +} from 'src/engine/core-modules/file-storage/interfaces/file-storage-exception'; + +export const assertStoragePathIsSafe = (storagePath: string): void => { + if (storagePath.includes('\0')) { + throw new FileStorageException( + 'Invalid storage path: contains null bytes', + FileStorageExceptionCode.ACCESS_DENIED, + ); + } + + if (path.isAbsolute(storagePath)) { + throw new FileStorageException( + 'Invalid storage path: absolute path not allowed', + FileStorageExceptionCode.ACCESS_DENIED, + ); + } + + const normalized = path.normalize(storagePath); + + if (normalized.split(path.sep).includes('..')) { + throw new FileStorageException( + 'Invalid storage path: path traversal detected', + FileStorageExceptionCode.ACCESS_DENIED, + ); + } +}; diff --git a/packages/twenty-server/src/engine/core-modules/file/file-upload/services/file-upload.service.ts b/packages/twenty-server/src/engine/core-modules/file/file-upload/services/file-upload.service.ts index 94cdb48e3f..bca2344112 100644 --- a/packages/twenty-server/src/engine/core-modules/file/file-upload/services/file-upload.service.ts +++ b/packages/twenty-server/src/engine/core-modules/file/file-upload/services/file-upload.service.ts @@ -40,7 +40,7 @@ export class FileUploadService { mimeType: string | undefined; folder: string; }) { - await this.fileStorage.writeFile({ + await this.fileStorage.writeFileLegacy({ file, name: filename, mimeType, diff --git a/packages/twenty-server/src/engine/core-modules/file/files-field/files-field.service.ts b/packages/twenty-server/src/engine/core-modules/file/files-field/files-field.service.ts index ba4ae219f2..7cc8d82e2c 100644 --- a/packages/twenty-server/src/engine/core-modules/file/files-field/files-field.service.ts +++ b/packages/twenty-server/src/engine/core-modules/file/files-field/files-field.service.ts @@ -80,7 +80,7 @@ export class FilesFieldService { }, }); - return await this.fileStorageService.writeFile_v2({ + return await this.fileStorageService.writeFile({ sourceFile: sanitizedFile, resourcePath: `${fieldMetadata.universalIdentifier}/${name}`, mimeType, @@ -153,7 +153,7 @@ export class FilesFieldService { }, }); - return await this.fileStorageService.readFile_v2({ + return await this.fileStorageService.readFile({ resourcePath: removeFileFolderFromFileEntityPath(file.path), fileFolder: FileFolder.FilesField, applicationUniversalIdentifier: application.universalIdentifier, diff --git a/packages/twenty-server/src/engine/core-modules/file/services/file.service.spec.ts b/packages/twenty-server/src/engine/core-modules/file/services/file.service.spec.ts index b9c05de8ad..d5fed4cb4a 100644 --- a/packages/twenty-server/src/engine/core-modules/file/services/file.service.spec.ts +++ b/packages/twenty-server/src/engine/core-modules/file/services/file.service.spec.ts @@ -21,7 +21,7 @@ describe('FileService', () => { { provide: FileStorageService, useValue: { - copy: jest.fn(), + copyLegacy: jest.fn(), }, }, { @@ -50,7 +50,7 @@ describe('FileService', () => { 'newWorkspaceId', ); - expect(fileStorageService.copy).toHaveBeenCalledWith({ + expect(fileStorageService.copyLegacy).toHaveBeenCalledWith({ from: { folderPath: 'workspace-workspaceId/path/to', filename: 'file', diff --git a/packages/twenty-server/src/engine/core-modules/file/services/file.service.ts b/packages/twenty-server/src/engine/core-modules/file/services/file.service.ts index a15d5af992..e54e7f38c0 100644 --- a/packages/twenty-server/src/engine/core-modules/file/services/file.service.ts +++ b/packages/twenty-server/src/engine/core-modules/file/services/file.service.ts @@ -33,7 +33,7 @@ export class FileService { ): Promise { const workspaceFolderPath = `workspace-${workspaceId}/${folderPath}`; - return await this.fileStorageService.readFile({ + return await this.fileStorageService.readFileLegacy({ filePath: `${workspaceFolderPath}/${filename}`, }); } @@ -85,7 +85,7 @@ export class FileService { }) { const workspaceFolderPath = `workspace-${workspaceId}/${folderPath}`; - return await this.fileStorageService.delete({ + return await this.fileStorageService.deleteLegacy({ folderPath: workspaceFolderPath, filename, }); @@ -95,7 +95,7 @@ export class FileService { const workspaceFolderPath = `workspace-${workspaceId}`; const isWorkspaceFolderFound = - await this.fileStorageService.checkFolderExists({ + await this.fileStorageService.checkFolderExistsLegacy({ folderPath: workspaceFolderPath, }); @@ -103,7 +103,7 @@ export class FileService { return; } - return await this.fileStorageService.delete({ + return await this.fileStorageService.deleteLegacy({ folderPath: workspaceFolderPath, }); } @@ -120,7 +120,7 @@ export class FileService { const toFilename = uuidV4() + extname(fromFilename); - await this.fileStorageService.copy({ + await this.fileStorageService.copyLegacy({ from: { folderPath: `${fromWorkspaceFolderPath}/${subFolder}`, filename: fromFilename, diff --git a/packages/twenty-server/src/engine/core-modules/logic-function/logic-function-drivers/drivers/lambda.driver.ts b/packages/twenty-server/src/engine/core-modules/logic-function/logic-function-drivers/drivers/lambda.driver.ts index 92ed463bc3..b6f0dbbbbe 100644 --- a/packages/twenty-server/src/engine/core-modules/logic-function/logic-function-drivers/drivers/lambda.driver.ts +++ b/packages/twenty-server/src/engine/core-modules/logic-function/logic-function-drivers/drivers/lambda.driver.ts @@ -150,14 +150,14 @@ export class LambdaDriver implements LogicFunctionExecutorDriver { inMemoryLayerFolderPath: string; }) { await Promise.all([ - this.fileStorageService.downloadFile_v2({ + this.fileStorageService.downloadFile({ workspaceId, applicationUniversalIdentifier, fileFolder: FileFolder.Dependencies, resourcePath: 'package.json', localPath: join(inMemoryLayerFolderPath, 'package.json'), }), - this.fileStorageService.downloadFile_v2({ + this.fileStorageService.downloadFile({ workspaceId, applicationUniversalIdentifier, fileFolder: FileFolder.Dependencies, @@ -368,7 +368,7 @@ export class LambdaDriver implements LogicFunctionExecutorDriver { const compiledCode = ( await streamToBuffer( - await this.fileStorageService.readFile_v2({ + await this.fileStorageService.readFile({ workspaceId: flatLogicFunction.workspaceId, applicationUniversalIdentifier, fileFolder: FileFolder.BuiltLogicFunction, diff --git a/packages/twenty-server/src/engine/core-modules/logic-function/logic-function-drivers/drivers/local.driver.ts b/packages/twenty-server/src/engine/core-modules/logic-function/logic-function-drivers/drivers/local.driver.ts index 625a6174bd..4de4084c3a 100644 --- a/packages/twenty-server/src/engine/core-modules/logic-function/logic-function-drivers/drivers/local.driver.ts +++ b/packages/twenty-server/src/engine/core-modules/logic-function/logic-function-drivers/drivers/local.driver.ts @@ -47,14 +47,14 @@ export class LocalDriver implements LogicFunctionExecutorDriver { inMemoryLayerFolderPath: string; }) { await Promise.all([ - this.fileStorageService.downloadFile_v2({ + this.fileStorageService.downloadFile({ workspaceId, applicationUniversalIdentifier, fileFolder: FileFolder.Dependencies, resourcePath: 'package.json', localPath: join(inMemoryLayerFolderPath, 'package.json'), }), - this.fileStorageService.downloadFile_v2({ + this.fileStorageService.downloadFile({ workspaceId, applicationUniversalIdentifier, fileFolder: FileFolder.Dependencies, @@ -122,7 +122,7 @@ export class LocalDriver implements LogicFunctionExecutorDriver { const baseFolderPath = dirname(flatLogicFunction.builtHandlerPath); - await this.fileStorageService.downloadFolder_v2({ + await this.fileStorageService.downloadFolder({ workspaceId: flatLogicFunction.workspaceId, applicationUniversalIdentifier, fileFolder: FileFolder.BuiltLogicFunction, diff --git a/packages/twenty-server/src/engine/core-modules/logic-function/logic-function-executor/services/logic-function-executor.service.ts b/packages/twenty-server/src/engine/core-modules/logic-function/logic-function-executor/services/logic-function-executor.service.ts index 8d565b7e2d..073322c823 100644 --- a/packages/twenty-server/src/engine/core-modules/logic-function/logic-function-executor/services/logic-function-executor.service.ts +++ b/packages/twenty-server/src/engine/core-modules/logic-function/logic-function-executor/services/logic-function-executor.service.ts @@ -281,13 +281,13 @@ export class LogicFunctionExecutorService flatApplication: FlatApplication; applicationUniversalIdentifier: string; }): Promise { - const packageJsonExists = await this.fileStorageService.checkFileExists_v2({ + const packageJsonExists = await this.fileStorageService.checkFileExists({ workspaceId: flatApplication.workspaceId, applicationUniversalIdentifier, fileFolder: FileFolder.Dependencies, resourcePath: 'package.json', }); - const yarnLockExists = await this.fileStorageService.checkFileExists_v2({ + const yarnLockExists = await this.fileStorageService.checkFileExists({ workspaceId: flatApplication.workspaceId, applicationUniversalIdentifier, fileFolder: FileFolder.Dependencies, diff --git a/packages/twenty-server/src/engine/core-modules/logic-function/logic-function-source-builder/logic-function-source-builder.service.ts b/packages/twenty-server/src/engine/core-modules/logic-function/logic-function-source-builder/logic-function-source-builder.service.ts index 14ea6905af..1d08ab0fc1 100644 --- a/packages/twenty-server/src/engine/core-modules/logic-function/logic-function-source-builder/logic-function-source-builder.service.ts +++ b/packages/twenty-server/src/engine/core-modules/logic-function/logic-function-source-builder/logic-function-source-builder.service.ts @@ -128,7 +128,7 @@ export class LogicFunctionSourceBuilderService { const sourceFile = sourceFiles[0]; const builtFile = builtFiles[0]; - await this.fileStorageService.writeFile_v2({ + await this.fileStorageService.writeFile({ workspaceId, applicationUniversalIdentifier, fileFolder: FileFolder.Source, @@ -141,7 +141,7 @@ export class LogicFunctionSourceBuilderService { }, }); - await this.fileStorageService.writeFile_v2({ + await this.fileStorageService.writeFile({ workspaceId, applicationUniversalIdentifier, fileFolder: FileFolder.BuiltLogicFunction, @@ -181,7 +181,7 @@ export class LogicFunctionSourceBuilderService { const baseFolderPath = getLogicFunctionBaseFolderPath(sourceHandlerPath); - await this.fileStorageService.uploadFolder_v2({ + await this.fileStorageService.uploadFolder({ workspaceId, applicationUniversalIdentifier, fileFolder: FileFolder.Source, @@ -206,7 +206,7 @@ export class LogicFunctionSourceBuilderService { const baseFolderPath = getLogicFunctionBaseFolderPath(sourceHandlerPath); - await this.fileStorageService.downloadFolder_v2({ + await this.fileStorageService.downloadFolder({ workspaceId, applicationUniversalIdentifier, fileFolder: FileFolder.Source, @@ -231,7 +231,7 @@ export class LogicFunctionSourceBuilderService { const builtFile = await fs.readFile(builtBundleFilePath, 'utf-8'); - await this.fileStorageService.writeFile_v2({ + await this.fileStorageService.writeFile({ workspaceId, applicationUniversalIdentifier, fileFolder: FileFolder.BuiltLogicFunction, @@ -260,7 +260,7 @@ export class LogicFunctionSourceBuilderService { const baseFolderPath = getLogicFunctionBaseFolderPath(sourceHandlerPath); try { - return await this.fileStorageService.readFolder_v2({ + return await this.fileStorageService.readFolder({ workspaceId, applicationUniversalIdentifier, fileFolder: FileFolder.Source, @@ -297,7 +297,7 @@ export class LogicFunctionSourceBuilderService { const toBuiltBaseFolderPath = getLogicFunctionBaseFolderPath(toBuiltHandlerPath); - await this.fileStorageService.copy_v2({ + await this.fileStorageService.copy({ from: { workspaceId, applicationUniversalIdentifier, @@ -312,7 +312,7 @@ export class LogicFunctionSourceBuilderService { }, }); - await this.fileStorageService.copy_v2({ + await this.fileStorageService.copy({ from: { workspaceId, applicationUniversalIdentifier, diff --git a/packages/twenty-server/src/engine/core-modules/tool/tools/code-interpreter-tool/code-interpreter-tool.ts b/packages/twenty-server/src/engine/core-modules/tool/tools/code-interpreter-tool/code-interpreter-tool.ts index 76978a5f18..51b628e2be 100644 --- a/packages/twenty-server/src/engine/core-modules/tool/tools/code-interpreter-tool/code-interpreter-tool.ts +++ b/packages/twenty-server/src/engine/core-modules/tool/tools/code-interpreter-tool/code-interpreter-tool.ts @@ -353,7 +353,7 @@ export class CodeInterpreterTool implements Tool { const sanitizedFilename = path.basename(file.filename); try { - await this.fileStorageService.writeFile({ + await this.fileStorageService.writeFileLegacy({ file: file.content, name: sanitizedFilename, mimeType: file.mimeType, @@ -402,7 +402,7 @@ export class CodeInterpreterTool implements Tool { } try { - await this.fileStorageService.writeFile({ + await this.fileStorageService.writeFileLegacy({ file: file.content, name: sanitizedFilename, mimeType: file.mimeType, diff --git a/packages/twenty-server/src/engine/workspace-manager/dev-seeder/data/services/dev-seeder-data.service.ts b/packages/twenty-server/src/engine/workspace-manager/dev-seeder/data/services/dev-seeder-data.service.ts index 73f8c4aac5..d9deadd581 100644 --- a/packages/twenty-server/src/engine/workspace-manager/dev-seeder/data/services/dev-seeder-data.service.ts +++ b/packages/twenty-server/src/engine/workspace-manager/dev-seeder/data/services/dev-seeder-data.service.ts @@ -429,7 +429,7 @@ export class DevSeederDataService { const filePath = join(sampleFilesDir, filename); const fileBuffer = await readFile(filePath); - await this.fileStorageService.writeFile({ + await this.fileStorageService.writeFileLegacy({ file: fileBuffer, name: filename, folder: `workspace-${workspaceId}/attachment`, diff --git a/packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-runner/action-handlers/front-component/services/create-front-component-action-handler.service.ts b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-runner/action-handlers/front-component/services/create-front-component-action-handler.service.ts index bc80484b88..4c60caa19e 100644 --- a/packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-runner/action-handlers/front-component/services/create-front-component-action-handler.service.ts +++ b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-runner/action-handlers/front-component/services/create-front-component-action-handler.service.ts @@ -68,7 +68,7 @@ export class CreateFrontComponentActionHandlerService extends WorkspaceMigration applicationUniversalIdentifier: string; builtComponentPath: string; }): Promise { - const builtExists = await this.fileStorageService.checkFileExists_v2({ + const builtExists = await this.fileStorageService.checkFileExists({ workspaceId, applicationUniversalIdentifier, fileFolder: FileFolder.BuiltFrontComponent, diff --git a/packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-runner/action-handlers/logic-function/services/create-logic-function-action-handler.service.ts b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-runner/action-handlers/logic-function/services/create-logic-function-action-handler.service.ts index 63434af765..7786f73a04 100644 --- a/packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-runner/action-handlers/logic-function/services/create-logic-function-action-handler.service.ts +++ b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-runner/action-handlers/logic-function/services/create-logic-function-action-handler.service.ts @@ -40,13 +40,13 @@ export class CreateLogicFunctionActionHandlerService extends WorkspaceMigrationR const applicationUniversalIdentifier = flatApplication.universalIdentifier; const [sourceExists, builtExists] = await Promise.all([ - this.fileStorageService.checkFileExists_v2({ + this.fileStorageService.checkFileExists({ workspaceId, applicationUniversalIdentifier, fileFolder: FileFolder.Source, resourcePath: logicFunction.sourceHandlerPath, }), - this.fileStorageService.checkFileExists_v2({ + this.fileStorageService.checkFileExists({ workspaceId, applicationUniversalIdentifier, fileFolder: FileFolder.BuiltLogicFunction, diff --git a/packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-runner/action-handlers/logic-function/services/delete-logic-function-action-handler.service.ts b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-runner/action-handlers/logic-function/services/delete-logic-function-action-handler.service.ts index af1be0cacb..32a244428c 100644 --- a/packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-runner/action-handlers/logic-function/services/delete-logic-function-action-handler.service.ts +++ b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-runner/action-handlers/logic-function/services/delete-logic-function-action-handler.service.ts @@ -59,7 +59,7 @@ export class DeleteLogicFunctionActionHandlerService extends WorkspaceMigrationR const applicationUniversalIdentifier = flatApplication.universalIdentifier; - await this.fileStorageService.delete_v2({ + await this.fileStorageService.delete({ workspaceId, applicationUniversalIdentifier, fileFolder: FileFolder.BuiltLogicFunction, diff --git a/packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-runner/action-handlers/logic-function/services/update-logic-function-action-handler.service.ts b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-runner/action-handlers/logic-function/services/update-logic-function-action-handler.service.ts index 65d139cfff..d57751718e 100644 --- a/packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-runner/action-handlers/logic-function/services/update-logic-function-action-handler.service.ts +++ b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-runner/action-handlers/logic-function/services/update-logic-function-action-handler.service.ts @@ -73,13 +73,13 @@ export class UpdateLogicFunctionActionHandlerService extends WorkspaceMigrationR applicationUniversalIdentifier: string; }): Promise { const [sourceExists, builtExists] = await Promise.all([ - this.fileStorageService.checkFileExists_v2({ + this.fileStorageService.checkFileExists({ workspaceId: flatLogicFunction.workspaceId, applicationUniversalIdentifier, fileFolder: FileFolder.Source, resourcePath: flatLogicFunction.sourceHandlerPath, }), - this.fileStorageService.checkFileExists_v2({ + this.fileStorageService.checkFileExists({ workspaceId: flatLogicFunction.workspaceId, applicationUniversalIdentifier, fileFolder: FileFolder.BuiltLogicFunction,