From 895bb58fc6f982fc268a11004e5978bf98bdf01f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Malfait?= Date: Wed, 25 Mar 2026 16:15:15 +0100 Subject: [PATCH] feat: add S3 presigned URL redirect for file downloads (#18864) ## Summary - When `STORAGE_S3_PRESIGNED_URL_BASE` is configured, the file controller returns a **302 redirect** to a presigned S3 URL instead of proxying every byte through the server. This eliminates server bandwidth and CPU overhead for S3-backed deployments. - For local storage or S3 without a public endpoint, behavior is unchanged (stream + pipe with security headers). - Added `getPresignedUrl` to the `StorageDriver` interface (required method returning `string | null`), with implementations in S3Driver (uses a separate presign client with the public endpoint), LocalDriver (returns `null`), and ValidatedStorageDriver (path traversal protection + delegation). - Added a unified `getFileResponseById` method in `FileService` that performs a single DB lookup and returns either a redirect URL or a stream, avoiding double lookups. - Extracted `getContentDisposition` from the header util so both the proxy path and presigned URL path share the same inline/attachment allowlist. - Added MinIO service to `docker-compose.dev.yml` (optional `s3` profile) for local S3 testing. - Documented S3 presigned URL setup, CORS, and `nosniff` requirements in the self-hosting docs. ## Test plan - [x] All 63 unit tests pass across 5 test suites (util, S3 driver, validated driver, file storage service, controller) - [x] `npx nx typecheck twenty-server` passes - [ ] Manual E2E test with MinIO: `docker compose --profile s3 up -d`, configure S3 env vars, verify `curl -I` returns 302 with `Location` header pointing to MinIO - [ ] Verify local storage (no `STORAGE_S3_PRESIGNED_URL_BASE`) still streams files with 200 + security headers - [ ] Verify public assets endpoint still proxies (no redirect) Made with [Cursor](https://cursor.com) --- packages/twenty-docker/.env.example | 2 + packages/twenty-docker/twenty/Dockerfile | 9 +- .../self-host/capabilities/setup.mdx | 10 ++ .../__tests__/file-storage.service.spec.ts | 42 +++++++ .../drivers/__tests__/s3.driver.spec.ts | 115 ++++++++++++++++++ .../validated-storage.driver.spec.ts | 30 +++++ .../interfaces/storage-driver.interface.ts | 7 ++ .../file-storage/drivers/local.driver.ts | 4 + .../file-storage/drivers/s3.driver.ts | 41 ++++++- .../drivers/validated-storage.driver.ts | 11 ++ .../file-storage-driver.factory.ts | 8 ++ .../file-storage/file-storage.service.ts | 18 +++ .../file/controllers/file.controller.spec.ts | 46 +++++-- .../file/controllers/file.controller.ts | 23 ++-- .../file/services/file.service.ts | 48 ++++++++ .../file/types/file-response.type.ts | 5 + .../set-file-response-headers.utils.spec.ts | 90 ++++++++++++++ .../utils/get-content-disposition.utils.ts | 22 ++++ .../utils/set-file-response-headers.utils.ts | 24 +--- .../twenty-config/config-variables.ts | 42 ++++++- 20 files changed, 550 insertions(+), 47 deletions(-) create mode 100644 packages/twenty-server/src/engine/core-modules/file-storage/drivers/__tests__/s3.driver.spec.ts create mode 100644 packages/twenty-server/src/engine/core-modules/file/types/file-response.type.ts create mode 100644 packages/twenty-server/src/engine/core-modules/file/utils/__tests__/set-file-response-headers.utils.spec.ts create mode 100644 packages/twenty-server/src/engine/core-modules/file/utils/get-content-disposition.utils.ts diff --git a/packages/twenty-docker/.env.example b/packages/twenty-docker/.env.example index cea1b527ee..4ac34c337d 100644 --- a/packages/twenty-docker/.env.example +++ b/packages/twenty-docker/.env.example @@ -16,3 +16,5 @@ STORAGE_TYPE=local # STORAGE_S3_REGION=eu-west3 # STORAGE_S3_NAME=my-bucket # STORAGE_S3_ENDPOINT= +# STORAGE_S3_ACCESS_KEY_ID= +# STORAGE_S3_SECRET_ACCESS_KEY= diff --git a/packages/twenty-docker/twenty/Dockerfile b/packages/twenty-docker/twenty/Dockerfile index c80b02e3c9..42394747ee 100644 --- a/packages/twenty-docker/twenty/Dockerfile +++ b/packages/twenty-docker/twenty/Dockerfile @@ -17,6 +17,7 @@ COPY ./packages/twenty-ui/package.json /app/packages/twenty-ui/ COPY ./packages/twenty-shared/package.json /app/packages/twenty-shared/ COPY ./packages/twenty-front/package.json /app/packages/twenty-front/ COPY ./packages/twenty-sdk/package.json /app/packages/twenty-sdk/ +COPY ./packages/twenty-client-sdk/package.json /app/packages/twenty-client-sdk/ RUN yarn && yarn cache clean && npx nx reset @@ -27,6 +28,7 @@ COPY ./packages/twenty-emails /app/packages/twenty-emails COPY ./packages/twenty-shared /app/packages/twenty-shared COPY ./packages/twenty-ui /app/packages/twenty-ui COPY ./packages/twenty-sdk /app/packages/twenty-sdk +COPY ./packages/twenty-client-sdk /app/packages/twenty-client-sdk COPY ./packages/twenty-server /app/packages/twenty-server RUN npx nx run twenty-server:lingui:extract && \ @@ -47,7 +49,7 @@ RUN npx esbuild packages/twenty-server/scripts/setup-db.ts \ RUN find /app/packages/twenty-server/dist -name '*.d.ts' -delete \ && rm -rf /app/packages/twenty-server/dist/packages/twenty-server/test -RUN yarn workspaces focus --production twenty-emails twenty-shared twenty-sdk twenty-server +RUN yarn workspaces focus --production twenty-emails twenty-shared twenty-sdk twenty-client-sdk twenty-server FROM common-deps AS twenty-front-build @@ -58,6 +60,7 @@ COPY ./packages/twenty-front /app/packages/twenty-front COPY ./packages/twenty-ui /app/packages/twenty-ui COPY ./packages/twenty-shared /app/packages/twenty-shared COPY ./packages/twenty-sdk /app/packages/twenty-sdk +COPY ./packages/twenty-client-sdk /app/packages/twenty-client-sdk RUN npx nx run twenty-front:lingui:extract && \ npx nx run twenty-front:lingui:compile # To skip the memory-intensive frontend build, pre-build on the host: @@ -106,6 +109,8 @@ COPY --chown=1000 --from=twenty-server-build /app/packages/twenty-shared/dist /a COPY --chown=1000 --from=twenty-server-build /app/packages/twenty-emails/package.json /app/packages/twenty-emails/ COPY --chown=1000 --from=twenty-server-build /app/packages/twenty-emails/dist /app/packages/twenty-emails/dist COPY --chown=1000 --from=twenty-server-build /app/packages/twenty-sdk/package.json /app/packages/twenty-sdk/ +COPY --chown=1000 --from=twenty-server-build /app/packages/twenty-client-sdk/package.json /app/packages/twenty-client-sdk/ +COPY --chown=1000 --from=twenty-server-build /app/packages/twenty-client-sdk/dist /app/packages/twenty-client-sdk/dist COPY --chown=1000 --from=twenty-server-build /app/packages/twenty-ui/package.json /app/packages/twenty-ui/ COPY --chown=1000 --from=twenty-server-build /app/packages/twenty-front/package.json /app/packages/twenty-front/ @@ -180,6 +185,8 @@ COPY --from=twenty-server-build /app/packages/twenty-shared/dist /app/packages/t COPY --from=twenty-server-build /app/packages/twenty-emails/package.json /app/packages/twenty-emails/ COPY --from=twenty-server-build /app/packages/twenty-emails/dist /app/packages/twenty-emails/dist COPY --from=twenty-server-build /app/packages/twenty-sdk/package.json /app/packages/twenty-sdk/ +COPY --from=twenty-server-build /app/packages/twenty-client-sdk/package.json /app/packages/twenty-client-sdk/ +COPY --from=twenty-server-build /app/packages/twenty-client-sdk/dist /app/packages/twenty-client-sdk/dist COPY --from=twenty-server-build /app/packages/twenty-ui/package.json /app/packages/twenty-ui/ COPY --from=twenty-server-build /app/packages/twenty-front/package.json /app/packages/twenty-front/ diff --git a/packages/twenty-docs/developers/self-host/capabilities/setup.mdx b/packages/twenty-docs/developers/self-host/capabilities/setup.mdx index 975d59e28f..f109b644eb 100644 --- a/packages/twenty-docs/developers/self-host/capabilities/setup.mdx +++ b/packages/twenty-docs/developers/self-host/capabilities/setup.mdx @@ -289,6 +289,16 @@ yarn command:prod cron:workflow:automated-cron-trigger **Environment-only mode:** If you set `IS_CONFIG_VARIABLES_IN_DB_ENABLED=false`, add these variables to your `.env` file instead. +## S3 Storage + + +By default, Twenty stores uploaded files on the local filesystem. For production deployments, use S3 or an S3-compatible service (MinIO, DigitalOcean Spaces, etc.) to ensure files persist across container restarts and scale across multiple server instances. + + +Set `STORAGE_TYPE=S_3` and configure the `STORAGE_S3_*` variables through the admin panel or `.env`. See the [config-variables.ts reference](https://github.com/twentyhq/twenty/blob/main/packages/twenty-server/src/engine/core-modules/twenty-config/config-variables.ts) for the full list of S3 variables. + +When using S3 with CORS-dependent features (e.g. in-browser file downloads), make sure your bucket allows your Twenty frontend origin in its CORS configuration. + ## Logic Functions & Code Interpreter Twenty supports logic functions for workflows and the code interpreter for AI data analysis. Both run user-provided code and require explicit configuration for security. 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 1e23516ae7..3f4a036c68 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 @@ -67,6 +67,7 @@ describe('FileStorageService', () => { uploadFolder: jest.fn(), checkFileExists: jest.fn(), checkFolderExists: jest.fn(), + getPresignedUrl: jest.fn(), }; mockFileStorageDriverFactory.getCurrentDriver.mockReturnValue(mockDriver); @@ -151,6 +152,47 @@ describe('FileStorageService', () => { }); }); + describe('getPresignedUrl', () => { + it('should delegate to the driver and return the URL', async () => { + mockDriver.getPresignedUrl.mockResolvedValue( + 'https://s3.example.com/signed', + ); + + mockApplicationRepository.findOneOrFail.mockResolvedValue({ + universalIdentifier: 'app-uid', + }); + + const result = await service.getPresignedUrl({ + resourcePath: 'file.txt', + fileFolder: 'workflow' as any, + applicationUniversalIdentifier: 'app-uid', + workspaceId: 'ws-id', + responseContentType: 'image/png', + responseContentDisposition: 'inline', + }); + + expect(result).toBe('https://s3.example.com/signed'); + expect(mockDriver.getPresignedUrl).toHaveBeenCalled(); + }); + + it('should return null when driver returns null', async () => { + mockDriver.getPresignedUrl.mockResolvedValue(null); + + mockApplicationRepository.findOneOrFail.mockResolvedValue({ + universalIdentifier: 'app-uid', + }); + + const result = await service.getPresignedUrl({ + resourcePath: 'file.txt', + fileFolder: 'workflow' as any, + applicationUniversalIdentifier: 'app-uid', + workspaceId: 'ws-id', + }); + + expect(result).toBeNull(); + }); + }); + 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/drivers/__tests__/s3.driver.spec.ts b/packages/twenty-server/src/engine/core-modules/file-storage/drivers/__tests__/s3.driver.spec.ts new file mode 100644 index 0000000000..e899b4d35c --- /dev/null +++ b/packages/twenty-server/src/engine/core-modules/file-storage/drivers/__tests__/s3.driver.spec.ts @@ -0,0 +1,115 @@ +import { GetObjectCommand } from '@aws-sdk/client-s3'; +import { getSignedUrl } from '@aws-sdk/s3-request-presigner'; + +import { S3Driver } from 'src/engine/core-modules/file-storage/drivers/s3.driver'; + +jest.mock('@aws-sdk/client-s3', () => { + const actual = jest.requireActual('@aws-sdk/client-s3'); + + return { + ...actual, + S3: jest.fn().mockImplementation(() => ({})), + }; +}); + +jest.mock('@aws-sdk/s3-request-presigner', () => ({ + getSignedUrl: jest.fn(), +})); + +describe('S3Driver.getPresignedUrl', () => { + afterEach(() => { + jest.clearAllMocks(); + }); + + it('should return null when presigning is not enabled', async () => { + const driver = new S3Driver({ + bucketName: 'test-bucket', + region: 'us-east-1', + endpoint: 'http://localhost:9000', + }); + + const result = await driver.getPresignedUrl({ + filePath: 'some/file.png', + }); + + expect(result).toBeNull(); + expect(getSignedUrl).not.toHaveBeenCalled(); + }); + + it('should presign with the main client when enabled without endpoint override', async () => { + (getSignedUrl as jest.Mock).mockResolvedValue( + 'https://s3.us-east-1.amazonaws.com/test-bucket/file.png?X-Amz-Signature=abc', + ); + + const driver = new S3Driver({ + bucketName: 'test-bucket', + region: 'us-east-1', + presignEnabled: true, + }); + + const result = await driver.getPresignedUrl({ + filePath: 'file.png', + responseContentType: 'image/png', + responseContentDisposition: 'inline', + }); + + expect(result).toBe( + 'https://s3.us-east-1.amazonaws.com/test-bucket/file.png?X-Amz-Signature=abc', + ); + expect(getSignedUrl).toHaveBeenCalledWith( + expect.anything(), + expect.any(GetObjectCommand), + { expiresIn: 900 }, + ); + }); + + it('should presign with a separate client when endpoint override is provided', async () => { + (getSignedUrl as jest.Mock).mockResolvedValue( + 'https://public.s3.com/test-bucket/some/file.png?X-Amz-Signature=abc', + ); + + const driver = new S3Driver({ + bucketName: 'test-bucket', + region: 'us-east-1', + endpoint: 'http://internal-minio:9000', + presignEnabled: true, + presignEndpoint: 'https://public.s3.com', + }); + + const result = await driver.getPresignedUrl({ + filePath: 'some/file.png', + responseContentType: 'image/png', + responseContentDisposition: 'inline', + }); + + expect(result).toBe( + 'https://public.s3.com/test-bucket/some/file.png?X-Amz-Signature=abc', + ); + expect(getSignedUrl).toHaveBeenCalledWith( + expect.anything(), + expect.any(GetObjectCommand), + { expiresIn: 900 }, + ); + }); + + it('should use custom expiry when provided', async () => { + (getSignedUrl as jest.Mock).mockResolvedValue('https://signed.url'); + + const driver = new S3Driver({ + bucketName: 'test-bucket', + region: 'us-east-1', + presignEnabled: true, + }); + + await driver.getPresignedUrl({ + filePath: 'file.txt', + expiresInSeconds: 3600, + }); + + expect(getSignedUrl).toHaveBeenCalledWith( + expect.anything(), + expect.any(GetObjectCommand), + { expiresIn: 3600 }, + ); + }); +}); 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 index 745ef66afb..e6f993d35d 100644 --- 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 @@ -16,6 +16,7 @@ const createMockDriver = (): jest.Mocked => ({ copy: jest.fn().mockResolvedValue(undefined), checkFileExists: jest.fn().mockResolvedValue(true), checkFolderExists: jest.fn().mockResolvedValue(true), + getPresignedUrl: jest.fn().mockResolvedValue(null), }); describe('ValidatedStorageDriver', () => { @@ -118,6 +119,25 @@ describe('ValidatedStorageDriver', () => { folderPath: 'folder', }); }); + + it('should delegate getPresignedUrl', async () => { + mockDelegate.getPresignedUrl.mockResolvedValue( + 'https://s3.example.com/signed', + ); + + const result = await driver.getPresignedUrl({ + filePath: 'folder/file.txt', + responseContentType: 'image/png', + responseContentDisposition: 'inline', + }); + + expect(result).toBe('https://s3.example.com/signed'); + expect(mockDelegate.getPresignedUrl).toHaveBeenCalledWith({ + filePath: 'folder/file.txt', + responseContentType: 'image/png', + responseContentDisposition: 'inline', + }); + }); }); describe('rejects path traversal attempts', () => { @@ -203,6 +223,16 @@ describe('ValidatedStorageDriver', () => { expect(mockDelegate.downloadFolder).not.toHaveBeenCalled(); }); + + it('should reject getPresignedUrl with traversal', async () => { + await expect( + driver.getPresignedUrl({ filePath: '../../../etc/passwd' }), + ).rejects.toMatchObject({ + code: FileStorageExceptionCode.ACCESS_DENIED, + }); + + expect(mockDelegate.getPresignedUrl).not.toHaveBeenCalled(); + }); }); describe('does NOT validate localPath parameters', () => { diff --git a/packages/twenty-server/src/engine/core-modules/file-storage/drivers/interfaces/storage-driver.interface.ts b/packages/twenty-server/src/engine/core-modules/file-storage/drivers/interfaces/storage-driver.interface.ts index 3a938dbc13..f4aee0b57a 100644 --- a/packages/twenty-server/src/engine/core-modules/file-storage/drivers/interfaces/storage-driver.interface.ts +++ b/packages/twenty-server/src/engine/core-modules/file-storage/drivers/interfaces/storage-driver.interface.ts @@ -34,4 +34,11 @@ export interface StorageDriver { checkFileExists(params: { filePath: string }): Promise; checkFolderExists(params: { folderPath: string }): Promise; + + getPresignedUrl(params: { + filePath: string; + expiresInSeconds?: number; + responseContentType?: string; + responseContentDisposition?: string; + }): Promise; } 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 f21b1017bb..efd686f166 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 @@ -300,6 +300,10 @@ export class LocalDriver implements StorageDriver { return existsSync(fullPath); } + async getPresignedUrl(): Promise { + return null; + } + async checkFolderExists(params: { folderPath: string }): Promise { const folderFullPath = path.resolve( this.options.storagePath, diff --git a/packages/twenty-server/src/engine/core-modules/file-storage/drivers/s3.driver.ts b/packages/twenty-server/src/engine/core-modules/file-storage/drivers/s3.driver.ts index a45347939b..e9031ff3a4 100644 --- a/packages/twenty-server/src/engine/core-modules/file-storage/drivers/s3.driver.ts +++ b/packages/twenty-server/src/engine/core-modules/file-storage/drivers/s3.driver.ts @@ -20,6 +20,7 @@ import { S3, type S3ClientConfig, } from '@aws-sdk/client-s3'; +import { getSignedUrl } from '@aws-sdk/s3-request-presigner'; import { isDefined } from 'twenty-shared/utils'; import { type StorageDriver } from 'src/engine/core-modules/file-storage/drivers/interfaces/storage-driver.interface'; @@ -32,15 +33,25 @@ export interface S3DriverOptions extends S3ClientConfig { bucketName: string; endpoint?: string; region: string; + presignEnabled?: boolean; + presignEndpoint?: string; } export class S3Driver implements StorageDriver { private s3Client: S3; + private presignClient: S3 | undefined; private bucketName: string; private readonly logger = new Logger(S3Driver.name); constructor(options: S3DriverOptions) { - const { bucketName, region, endpoint, ...s3Options } = options; + const { + bucketName, + region, + endpoint, + presignEnabled, + presignEndpoint, + ...s3Options + } = options; if (!bucketName || !region) { return; @@ -48,6 +59,12 @@ export class S3Driver implements StorageDriver { this.s3Client = new S3({ ...s3Options, region, endpoint }); this.bucketName = bucketName; + + if (presignEnabled) { + this.presignClient = presignEndpoint + ? new S3({ ...s3Options, region, endpoint: presignEndpoint }) + : this.s3Client; + } } public get client(): S3 { @@ -363,6 +380,28 @@ export class S3Driver implements StorageDriver { } } + async getPresignedUrl(params: { + filePath: string; + expiresInSeconds?: number; + responseContentType?: string; + responseContentDisposition?: string; + }): Promise { + if (!this.presignClient) { + return null; + } + + const command = new GetObjectCommand({ + Bucket: this.bucketName, + Key: params.filePath, + ResponseContentType: params.responseContentType, + ResponseContentDisposition: params.responseContentDisposition, + }); + + return getSignedUrl(this.presignClient, command, { + expiresIn: params.expiresInSeconds ?? 900, + }); + } + async checkBucketExists(args: HeadBucketCommandInput) { try { await this.s3Client.headBucket(args); 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 index 1537b5c528..f36e944098 100644 --- 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 @@ -99,6 +99,17 @@ export class ValidatedStorageDriver implements StorageDriver { return this.delegate.copy(params); } + async getPresignedUrl(params: { + filePath: string; + expiresInSeconds?: number; + responseContentType?: string; + responseContentDisposition?: string; + }): Promise { + assertStoragePathIsSafe(params.filePath); + + return this.delegate.getPresignedUrl(params); + } + async checkFileExists(params: { filePath: string }): Promise { assertStoragePathIsSafe(params.filePath); 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 93acc0d933..c9738a2b58 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 @@ -63,10 +63,18 @@ export class FileStorageDriverFactory extends DriverFactoryBase { const secretAccessKey = this.twentyConfigService.get( 'STORAGE_S3_SECRET_ACCESS_KEY', ); + const presignEnabled = this.twentyConfigService.get( + 'STORAGE_S3_PRESIGNED_URL_ENABLED', + ); + const presignEndpointOverride = this.twentyConfigService.get( + 'STORAGE_S3_PRESIGNED_URL_BASE', + ); rawDriver = new S3Driver({ bucketName: bucketName ?? '', endpoint: endpoint, + presignEnabled, + presignEndpoint: presignEndpointOverride || undefined, credentials: accessKeyId ? { accessKeyId, secretAccessKey } : fromNodeProviderChain({ clientConfig: { region } }), 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 1ab6159994..c27390d441 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 @@ -114,6 +114,24 @@ export class FileStorageService { }); } + async getPresignedUrl( + params: ResourceIdentifier & { + expiresInSeconds?: number; + responseContentType?: string; + responseContentDisposition?: string; + }, + ): Promise { + const driver = this.fileStorageDriverFactory.getCurrentDriver(); + const onStoragePath = this.buildOnStoragePath(params); + + return driver.getPresignedUrl({ + filePath: onStoragePath, + expiresInSeconds: params.expiresInSeconds, + responseContentType: params.responseContentType, + responseContentDisposition: params.responseContentDisposition, + }); + } + readFile(params: ResourceIdentifier): Promise { const driver = this.fileStorageDriverFactory.getCurrentDriver(); diff --git a/packages/twenty-server/src/engine/core-modules/file/controllers/file.controller.spec.ts b/packages/twenty-server/src/engine/core-modules/file/controllers/file.controller.spec.ts index 33e8c356a7..e3a488d128 100644 --- a/packages/twenty-server/src/engine/core-modules/file/controllers/file.controller.spec.ts +++ b/packages/twenty-server/src/engine/core-modules/file/controllers/file.controller.spec.ts @@ -34,6 +34,7 @@ const createMockStream = (): Readable => { const createMockResponse = () => ({ setHeader: jest.fn(), + redirect: jest.fn(), }); describe('FileController', () => { @@ -56,6 +57,7 @@ describe('FileController', () => { useValue: { getFileStreamById: jest.fn(), getFileStreamByPath: jest.fn(), + getFileResponseById: jest.fn(), }, }, ], @@ -79,10 +81,38 @@ describe('FileController', () => { }); describe('getFileById', () => { - it('should call fileService.getFileStreamById and pipe the result with headers', async () => { + it('should 302 redirect when presigned URL is available', async () => { + jest.spyOn(fileService, 'getFileResponseById').mockResolvedValue({ + type: 'redirect', + presignedUrl: 'https://s3.example.com/file?signed=abc', + }); + + const mockRequest = { workspaceId: 'workspace-id' } as any; + const mockResponse = createMockResponse() as any; + + await controller.getFileById( + mockResponse, + mockRequest, + FileFolder.Workflow, + 'file-123', + ); + + expect(fileService.getFileResponseById).toHaveBeenCalledWith({ + fileId: 'file-123', + workspaceId: 'workspace-id', + fileFolder: FileFolder.Workflow, + }); + expect(mockResponse.redirect).toHaveBeenCalledWith( + 'https://s3.example.com/file?signed=abc', + ); + expect(mockResponse.setHeader).not.toHaveBeenCalled(); + }); + + it('should stream with headers when no presigned URL (local driver)', async () => { const mockStream = createMockStream(); - jest.spyOn(fileService, 'getFileStreamById').mockResolvedValue({ + jest.spyOn(fileService, 'getFileResponseById').mockResolvedValue({ + type: 'stream', stream: mockStream, mimeType: 'image/png', }); @@ -97,11 +127,6 @@ describe('FileController', () => { 'file-123', ); - expect(fileService.getFileStreamById).toHaveBeenCalledWith({ - fileId: 'file-123', - workspaceId: 'workspace-id', - fileFolder: FileFolder.CorePicture, - }); expect(mockResponse.setHeader).toHaveBeenCalledWith( 'Content-Type', 'image/png', @@ -120,7 +145,8 @@ describe('FileController', () => { it('should force attachment disposition for non-safe MIME types', async () => { const mockStream = createMockStream(); - jest.spyOn(fileService, 'getFileStreamById').mockResolvedValue({ + jest.spyOn(fileService, 'getFileResponseById').mockResolvedValue({ + type: 'stream', stream: mockStream, mimeType: 'text/html', }); @@ -147,7 +173,7 @@ describe('FileController', () => { it('should throw FileException with FILE_NOT_FOUND when file is not found', async () => { jest - .spyOn(fileService, 'getFileStreamById') + .spyOn(fileService, 'getFileResponseById') .mockRejectedValue( new FileStorageException( 'File not found', @@ -172,7 +198,7 @@ describe('FileController', () => { it('should throw FileException with INTERNAL_SERVER_ERROR for unexpected errors', async () => { jest - .spyOn(fileService, 'getFileStreamById') + .spyOn(fileService, 'getFileResponseById') .mockRejectedValue(new Error('Storage unavailable')); const mockRequest = { workspaceId: 'workspace-id' } as any; diff --git a/packages/twenty-server/src/engine/core-modules/file/controllers/file.controller.ts b/packages/twenty-server/src/engine/core-modules/file/controllers/file.controller.ts index a163021eef..6811b21c71 100644 --- a/packages/twenty-server/src/engine/core-modules/file/controllers/file.controller.ts +++ b/packages/twenty-server/src/engine/core-modules/file/controllers/file.controller.ts @@ -96,22 +96,29 @@ export class FileController { const workspaceId = (req as any)?.workspaceId; try { - const { stream, mimeType } = await this.fileService.getFileStreamById({ + const fileResponse = await this.fileService.getFileResponseById({ fileId, workspaceId, fileFolder, }); - setFileResponseHeaders(res, mimeType); + if (fileResponse.type === 'redirect') { + return res.redirect(fileResponse.presignedUrl); + } - stream.on('error', () => { - throw new FileException( - 'Error streaming file from storage', - FileExceptionCode.INTERNAL_SERVER_ERROR, - ); + setFileResponseHeaders(res, fileResponse.mimeType); + + fileResponse.stream.on('error', () => { + if (!res.headersSent) { + res.status(500).send('Error streaming file from storage'); + + return; + } + + res.destroy(); }); - stream.pipe(res); + fileResponse.stream.pipe(res); } catch (error) { if ( error instanceof FileStorageException && 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 e007c07f12..79be22c723 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 @@ -19,6 +19,8 @@ import { import { FileStorageService } from 'src/engine/core-modules/file-storage/file-storage.service'; import { FileEntity } from 'src/engine/core-modules/file/entities/file.entity'; import { removeFileFolderFromFileEntityPath } from 'src/engine/core-modules/file/utils/remove-file-folder-from-file-entity-path.utils'; +import { type FileResponse } from 'src/engine/core-modules/file/types/file-response.type'; +import { getContentDisposition } from 'src/engine/core-modules/file/utils/get-content-disposition.utils'; import { JwtWrapperService } from 'src/engine/core-modules/jwt/services/jwt-wrapper.service'; import { TwentyConfigService } from 'src/engine/core-modules/twenty-config/twenty-config.service'; import { streamToBuffer } from 'src/utils/stream-to-buffer'; @@ -111,6 +113,52 @@ export class FileService { }; } + async getFileResponseById(params: { + fileId: string; + workspaceId: string; + fileFolder: FileFolder; + }): Promise { + const file = await this.fileRepository.findOneOrFail({ + where: { + id: params.fileId, + workspaceId: params.workspaceId, + path: Like(`${params.fileFolder}/%`), + }, + }); + + const application = await this.applicationRepository.findOneOrFail({ + where: { + id: file.applicationId, + workspaceId: params.workspaceId, + }, + }); + + const mimeType = file.mimeType ?? 'application/octet-stream'; + const resourceIdentifier = { + resourcePath: removeFileFolderFromFileEntityPath(file.path), + fileFolder: params.fileFolder, + applicationUniversalIdentifier: application.universalIdentifier, + workspaceId: params.workspaceId, + }; + + const presignedUrl = await this.fileStorageService.getPresignedUrl({ + ...resourceIdentifier, + expiresInSeconds: this.twentyConfigService.get( + 'STORAGE_S3_PRESIGNED_URL_EXPIRES_IN', + ), + responseContentType: mimeType, + responseContentDisposition: getContentDisposition(mimeType), + }); + + if (presignedUrl) { + return { type: 'redirect', presignedUrl }; + } + + const stream = await this.fileStorageService.readFile(resourceIdentifier); + + return { type: 'stream', stream, mimeType }; + } + async getFileContentById({ fileId, workspaceId, diff --git a/packages/twenty-server/src/engine/core-modules/file/types/file-response.type.ts b/packages/twenty-server/src/engine/core-modules/file/types/file-response.type.ts new file mode 100644 index 0000000000..a4bd210fec --- /dev/null +++ b/packages/twenty-server/src/engine/core-modules/file/types/file-response.type.ts @@ -0,0 +1,5 @@ +import { type Readable } from 'stream'; + +export type FileResponse = + | { type: 'redirect'; presignedUrl: string } + | { type: 'stream'; stream: Readable; mimeType: string }; diff --git a/packages/twenty-server/src/engine/core-modules/file/utils/__tests__/set-file-response-headers.utils.spec.ts b/packages/twenty-server/src/engine/core-modules/file/utils/__tests__/set-file-response-headers.utils.spec.ts new file mode 100644 index 0000000000..d601f86fcd --- /dev/null +++ b/packages/twenty-server/src/engine/core-modules/file/utils/__tests__/set-file-response-headers.utils.spec.ts @@ -0,0 +1,90 @@ +import { getContentDisposition } from 'src/engine/core-modules/file/utils/get-content-disposition.utils'; +import { setFileResponseHeaders } from 'src/engine/core-modules/file/utils/set-file-response-headers.utils'; + +const createMockResponse = () => ({ + setHeader: jest.fn(), +}); + +describe('setFileResponseHeaders', () => { + it('should set Content-Type from mimeType', () => { + const res = createMockResponse(); + + setFileResponseHeaders(res as any, 'image/png'); + + expect(res.setHeader).toHaveBeenCalledWith('Content-Type', 'image/png'); + }); + + it('should fall back to application/octet-stream for empty mimeType', () => { + const res = createMockResponse(); + + setFileResponseHeaders(res as any, ''); + + expect(res.setHeader).toHaveBeenCalledWith( + 'Content-Type', + 'application/octet-stream', + ); + }); + + it('should always set X-Content-Type-Options: nosniff', () => { + const res = createMockResponse(); + + setFileResponseHeaders(res as any, 'text/html'); + + expect(res.setHeader).toHaveBeenCalledWith( + 'X-Content-Type-Options', + 'nosniff', + ); + }); + + it.each([ + 'image/jpeg', + 'image/png', + 'image/gif', + 'image/webp', + 'application/pdf', + 'text/plain', + 'video/mp4', + 'audio/mpeg', + ])('should set Content-Disposition: inline for safe type %s', (mimeType) => { + const res = createMockResponse(); + + setFileResponseHeaders(res as any, mimeType); + + expect(res.setHeader).toHaveBeenCalledWith('Content-Disposition', 'inline'); + }); + + it.each([ + 'text/html', + 'image/svg+xml', + 'application/xml', + 'application/octet-stream', + 'application/javascript', + ])( + 'should set Content-Disposition: attachment for unsafe type %s', + (mimeType) => { + const res = createMockResponse(); + + setFileResponseHeaders(res as any, mimeType); + + expect(res.setHeader).toHaveBeenCalledWith( + 'Content-Disposition', + 'attachment', + ); + }, + ); +}); + +describe('getContentDisposition', () => { + it('should return inline for safe MIME types', () => { + expect(getContentDisposition('image/png')).toBe('inline'); + expect(getContentDisposition('application/pdf')).toBe('inline'); + }); + + it('should return attachment for unsafe MIME types', () => { + expect(getContentDisposition('text/html')).toBe('attachment'); + expect(getContentDisposition('application/xml')).toBe('attachment'); + expect(getContentDisposition('application/octet-stream')).toBe( + 'attachment', + ); + }); +}); diff --git a/packages/twenty-server/src/engine/core-modules/file/utils/get-content-disposition.utils.ts b/packages/twenty-server/src/engine/core-modules/file/utils/get-content-disposition.utils.ts new file mode 100644 index 0000000000..6a7f477a67 --- /dev/null +++ b/packages/twenty-server/src/engine/core-modules/file/utils/get-content-disposition.utils.ts @@ -0,0 +1,22 @@ +const INLINE_SAFE_MIME_TYPES = new Set([ + 'image/jpeg', + 'image/png', + 'image/gif', + 'image/webp', + 'image/avif', + 'image/bmp', + 'image/tiff', + 'application/pdf', + 'text/plain', + 'audio/mpeg', + 'audio/wav', + 'audio/ogg', + 'video/mp4', + 'video/webm', + 'video/ogg', + 'image/x-icon', +]); + +export const getContentDisposition = (mimeType: string): string => { + return INLINE_SAFE_MIME_TYPES.has(mimeType) ? 'inline' : 'attachment'; +}; diff --git a/packages/twenty-server/src/engine/core-modules/file/utils/set-file-response-headers.utils.ts b/packages/twenty-server/src/engine/core-modules/file/utils/set-file-response-headers.utils.ts index 6c80c23ab5..026340fa49 100644 --- a/packages/twenty-server/src/engine/core-modules/file/utils/set-file-response-headers.utils.ts +++ b/packages/twenty-server/src/engine/core-modules/file/utils/set-file-response-headers.utils.ts @@ -1,31 +1,11 @@ import { type Response } from 'express'; -const INLINE_SAFE_MIME_TYPES = new Set([ - 'image/jpeg', - 'image/png', - 'image/gif', - 'image/webp', - 'image/avif', - 'image/bmp', - 'image/tiff', - 'application/pdf', - 'text/plain', - 'audio/mpeg', - 'audio/wav', - 'audio/ogg', - 'video/mp4', - 'video/webm', - 'video/ogg', - 'image/x-icon', -]); +import { getContentDisposition } from 'src/engine/core-modules/file/utils/get-content-disposition.utils'; export const setFileResponseHeaders = (res: Response, mimeType: string) => { const contentType = mimeType || 'application/octet-stream'; - const disposition = INLINE_SAFE_MIME_TYPES.has(contentType) - ? 'inline' - : 'attachment'; res.setHeader('Content-Type', contentType); res.setHeader('X-Content-Type-Options', 'nosniff'); - res.setHeader('Content-Disposition', disposition); + res.setHeader('Content-Disposition', getContentDisposition(contentType)); }; diff --git a/packages/twenty-server/src/engine/core-modules/twenty-config/config-variables.ts b/packages/twenty-server/src/engine/core-modules/twenty-config/config-variables.ts index aa4994ee39..4ce920fc72 100644 --- a/packages/twenty-server/src/engine/core-modules/twenty-config/config-variables.ts +++ b/packages/twenty-server/src/engine/core-modules/twenty-config/config-variables.ts @@ -420,7 +420,7 @@ export class ConfigVariables { @ConfigVariablesMetadata({ group: ConfigVariablesGroup.STORAGE_CONFIG, - description: 'S3 region for storage when using S3 storage type', + description: 'AWS region of the S3 bucket (e.g. eu-west-3). Required.', type: ConfigVariableType.STRING, }) @ValidateIf((env) => env.STORAGE_TYPE === StorageDriverType.S_3) @@ -429,7 +429,7 @@ export class ConfigVariables { @ConfigVariablesMetadata({ group: ConfigVariablesGroup.STORAGE_CONFIG, - description: 'S3 bucket name for storage when using S3 storage type', + description: 'Name of the S3 bucket used for file storage. Required.', type: ConfigVariableType.STRING, }) @ValidateIf((env) => env.STORAGE_TYPE === StorageDriverType.S_3) @@ -437,7 +437,8 @@ export class ConfigVariables { @ConfigVariablesMetadata({ group: ConfigVariablesGroup.STORAGE_CONFIG, - description: 'S3 endpoint for storage when using S3 storage type', + description: + 'Custom S3 endpoint URL. Optional — only needed for S3-compatible services like MinIO (e.g. http://minio:9000). Omit for native AWS S3, where the SDK resolves the endpoint from the region automatically.', type: ConfigVariableType.STRING, }) @ValidateIf((env) => env.STORAGE_TYPE === StorageDriverType.S_3) @@ -448,7 +449,7 @@ export class ConfigVariables { group: ConfigVariablesGroup.STORAGE_CONFIG, isSensitive: true, description: - 'S3 access key ID for authentication when using S3 storage type', + 'S3 access key ID. Optional — omit to use the default AWS credential chain (IAM role, instance profile, etc.).', type: ConfigVariableType.STRING, }) @ValidateIf((env) => env.STORAGE_TYPE === StorageDriverType.S_3) @@ -459,13 +460,44 @@ export class ConfigVariables { group: ConfigVariablesGroup.STORAGE_CONFIG, isSensitive: true, description: - 'S3 secret access key for authentication when using S3 storage type', + 'S3 secret access key. Required when STORAGE_S3_ACCESS_KEY_ID is set, ignored otherwise.', type: ConfigVariableType.STRING, }) @ValidateIf((env) => env.STORAGE_TYPE === StorageDriverType.S_3) @IsOptional() STORAGE_S3_SECRET_ACCESS_KEY: string; + @ConfigVariablesMetadata({ + group: ConfigVariablesGroup.STORAGE_CONFIG, + description: + 'When enabled, file downloads are 302-redirected to S3 presigned URLs instead of being proxied through the server. Reduces server load and bandwidth.', + type: ConfigVariableType.BOOLEAN, + }) + @ValidateIf((env) => env.STORAGE_TYPE === StorageDriverType.S_3) + @IsOptional() + // TODO: default to true once validated in production + STORAGE_S3_PRESIGNED_URL_ENABLED = false; + + @ConfigVariablesMetadata({ + group: ConfigVariablesGroup.STORAGE_CONFIG, + description: + 'Public S3 endpoint used for generating presigned URLs. Optional — only needed when STORAGE_S3_ENDPOINT is an internal address not reachable by browsers (e.g. http://minio:9000 in Docker). Set this to the publicly accessible equivalent (e.g. https://storage.example.com).', + type: ConfigVariableType.STRING, + }) + @ValidateIf((env) => env.STORAGE_TYPE === StorageDriverType.S_3) + @IsOptional() + STORAGE_S3_PRESIGNED_URL_BASE: string; + + @ConfigVariablesMetadata({ + group: ConfigVariablesGroup.STORAGE_CONFIG, + description: 'TTL in seconds for S3 presigned URLs.', + type: ConfigVariableType.NUMBER, + }) + @ValidateIf((env) => env.STORAGE_TYPE === StorageDriverType.S_3) + @CastToPositiveNumber() + @IsOptional() + STORAGE_S3_PRESIGNED_URL_EXPIRES_IN: number = 900; + @ConfigVariablesMetadata({ group: ConfigVariablesGroup.LOGIC_FUNCTION_CONFIG, description: 'Type of function execution (local or Lambda)',