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 e3a488d1288..60f0394349a 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 @@ -1,10 +1,15 @@ -import { type CanActivate } from '@nestjs/common'; +import { type CanActivate, Logger } from '@nestjs/common'; import { Test, type TestingModule } from '@nestjs/testing'; import { Readable } from 'stream'; +import { pipeline } from 'node:stream/promises'; import { FileFolder } from 'twenty-shared/types'; +jest.mock('node:stream/promises', () => ({ + pipeline: jest.fn(), +})); + import { FileStorageException, FileStorageExceptionCode, @@ -17,8 +22,8 @@ import { import { FileApiExceptionFilter } from 'src/engine/core-modules/file/filters/file-api-exception.filter'; import { FileByIdGuard } from 'src/engine/core-modules/file/guards/file-by-id.guard'; import { FileService } from 'src/engine/core-modules/file/services/file.service'; -import { PublicEndpointGuard } from 'src/engine/guards/public-endpoint.guard'; import { NoPermissionGuard } from 'src/engine/guards/no-permission.guard'; +import { PublicEndpointGuard } from 'src/engine/guards/public-endpoint.guard'; import { FileController } from './file.controller'; @@ -32,11 +37,17 @@ const createMockStream = (): Readable => { return stream; }; -const createMockResponse = () => ({ +const createMockResponse = ({ + headersSent = false, +}: { headersSent?: boolean } = {}) => ({ setHeader: jest.fn(), redirect: jest.fn(), + headersSent, + destroy: jest.fn(), }); +const mockPipeline = jest.mocked(pipeline); + describe('FileController', () => { let controller: FileController; let fileService: FileService; @@ -74,6 +85,9 @@ describe('FileController', () => { controller = module.get(FileController); fileService = module.get(FileService); + + // Default to a resolved pipeline so happy-path tests don't have to wire it up. + mockPipeline.mockResolvedValue(undefined); }); it('should be defined', () => { @@ -260,7 +274,7 @@ describe('FileController', () => { 'Content-Disposition', 'inline', ); - expect(mockStream.pipe).toHaveBeenCalledWith(mockResponse); + expect(mockPipeline).toHaveBeenCalledWith(mockStream, mockResponse); }); it('should handle single-segment path', async () => { @@ -292,15 +306,8 @@ describe('FileController', () => { }); }); - it('should throw FileException with FILE_NOT_FOUND when asset is not found', async () => { - jest - .spyOn(fileService, 'getFileStreamByPath') - .mockRejectedValue( - new FileStorageException( - 'File not found', - FileStorageExceptionCode.FILE_NOT_FOUND, - ), - ); + it('should throw FILE_NOT_FOUND when the service yields null', async () => { + jest.spyOn(fileService, 'getFileStreamByPath').mockResolvedValue(null); const mockRequest = { params: { path: ['missing-asset.png'] }, @@ -320,10 +327,17 @@ describe('FileController', () => { ); }); - it('should throw FileException with INTERNAL_SERVER_ERROR for unexpected errors', async () => { + it('should throw INTERNAL_SERVER_ERROR without leaking the underlying message, and log the original error', async () => { + const loggerSpy = jest + .spyOn(Logger.prototype, 'error') + .mockImplementation(() => undefined); + const underlyingError = new Error( + 'Connection refused: postgres://secret-host:5432', + ); + jest .spyOn(fileService, 'getFileStreamByPath') - .mockRejectedValue(new Error('Connection refused')); + .mockRejectedValue(underlyingError); const mockRequest = { params: { path: ['broken-asset.png'] }, @@ -331,6 +345,43 @@ describe('FileController', () => { const mockResponse = createMockResponse() as any; + const promise = controller.getPublicAssets( + mockResponse, + mockRequest, + 'workspace-id', + 'app-id', + ); + + await expect(promise).rejects.toThrow( + new FileException( + 'Error retrieving file', + FileExceptionCode.INTERNAL_SERVER_ERROR, + ), + ); + await expect(promise).rejects.not.toThrow(/secret-host/); + + expect(loggerSpy).toHaveBeenCalledWith( + 'getFileStreamByPath failed unexpectedly', + { error: underlyingError }, + ); + }); + + it('should throw INTERNAL_SERVER_ERROR when the stream errors before headers are sent', async () => { + const mockStream = createMockStream(); + + jest.spyOn(fileService, 'getFileStreamByPath').mockResolvedValue({ + stream: mockStream, + mimeType: 'image/png', + }); + + mockPipeline.mockRejectedValue(new Error('source backend exploded')); + + const mockRequest = { + params: { path: ['mid-stream-error.png'] }, + } as any; + + const mockResponse = createMockResponse({ headersSent: false }) as any; + await expect( controller.getPublicAssets( mockResponse, @@ -340,10 +391,40 @@ describe('FileController', () => { ), ).rejects.toThrow( new FileException( - 'Error retrieving file: Connection refused', + 'Error streaming file from storage', FileExceptionCode.INTERNAL_SERVER_ERROR, ), ); + + expect(mockResponse.destroy).not.toHaveBeenCalled(); + }); + + it('should destroy the response without throwing when the stream errors after headers are sent', async () => { + const mockStream = createMockStream(); + + jest.spyOn(fileService, 'getFileStreamByPath').mockResolvedValue({ + stream: mockStream, + mimeType: 'image/png', + }); + + mockPipeline.mockRejectedValue(new Error('socket reset mid-flight')); + + const mockRequest = { + params: { path: ['mid-stream-after-headers.png'] }, + } as any; + + const mockResponse = createMockResponse({ headersSent: true }) as any; + + // No throw expected — once headers are out, the controller cannot honestly + // switch to a 500 response, so it tears the socket down instead. + await controller.getPublicAssets( + mockResponse, + mockRequest, + 'workspace-id', + 'app-id', + ); + + expect(mockResponse.destroy).toHaveBeenCalledTimes(1); }); }); }); 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 6811b21c712..ff3193fad71 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 @@ -1,6 +1,7 @@ import { Controller, Get, + Logger, Param, Req, Res, @@ -8,6 +9,7 @@ import { UseGuards, } from '@nestjs/common'; +import { pipeline } from 'node:stream/promises'; import { join } from 'path'; import { Request, Response } from 'express'; @@ -17,7 +19,7 @@ import { FileStorageException, FileStorageExceptionCode, } from 'src/engine/core-modules/file-storage/interfaces/file-storage-exception'; - +import { validateFilePath } from 'src/engine/core-modules/file-storage/utils/validate-file-path.util'; import { FileException, FileExceptionCode, @@ -35,6 +37,8 @@ import { PublicEndpointGuard } from 'src/engine/guards/public-endpoint.guard'; @Controller() @UseFilters(FileApiExceptionFilter) export class FileController { + private readonly logger = new Logger(FileController.name); + constructor(private readonly fileService: FileService) {} @Get('public-assets/:workspaceId/:applicationId/*path') @@ -48,39 +52,60 @@ export class FileController { ) { const filepath = join(...req.params.path); - try { - const { stream, mimeType } = await this.fileService.getFileStreamByPath({ + const filePathValidationResult = validateFilePath({ + resourcePath: filepath, + fileFolder: FileFolder.PublicAsset, + }); + + if (!filePathValidationResult.isValid) { + throw new FileException( + 'File not found', + FileExceptionCode.FILE_NOT_FOUND, + ); + } + + const fileStream = await this.fileService + .getFileStreamByPath({ workspaceId, applicationId, fileFolder: FileFolder.PublicAsset, filepath, - }); + }) + .catch((error) => { + this.logger.error('getFileStreamByPath failed unexpectedly', { + error, + }); - setFileResponseHeaders(res, mimeType); - - stream.on('error', () => { throw new FileException( - 'Error streaming file from storage', + 'Error retrieving file', FileExceptionCode.INTERNAL_SERVER_ERROR, ); }); - stream.pipe(res); + if (fileStream === null) { + throw new FileException( + 'File not found', + FileExceptionCode.FILE_NOT_FOUND, + ); + } + + const { stream, mimeType } = fileStream; + + setFileResponseHeaders(res, mimeType); + + try { + await pipeline(stream, res); } catch (error) { - if ( - error instanceof FileStorageException && - error.code === FileStorageExceptionCode.FILE_NOT_FOUND - ) { + this.logger.error('Public asset stream failed mid-transfer', { error }); + + if (!res.headersSent) { throw new FileException( - 'File not found', - FileExceptionCode.FILE_NOT_FOUND, + 'Error streaming file from storage', + FileExceptionCode.INTERNAL_SERVER_ERROR, ); } - throw new FileException( - `Error retrieving file: ${error.message}`, - FileExceptionCode.INTERNAL_SERVER_ERROR, - ); + res.destroy(); } } 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 adee8c3c660..ffb82b7f11b 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 @@ -8,6 +8,10 @@ import { Like, Repository } from 'typeorm'; import { ApplicationEntity } from 'src/engine/core-modules/application/application.entity'; import { FileStorageService } from 'src/engine/core-modules/file-storage/file-storage.service'; +import { + FileStorageException, + FileStorageExceptionCode, +} from 'src/engine/core-modules/file-storage/interfaces/file-storage-exception'; import { FileEntity } from 'src/engine/core-modules/file/entities/file.entity'; 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'; @@ -38,8 +42,19 @@ export class FileService { applicationId: string; filepath: string; fileFolder: FileFolder; - }): Promise<{ stream: Readable; mimeType: string }> { - const file = await this.fileRepository.findOneOrFail({ + }): Promise<{ stream: Readable; mimeType: string } | null> { + const application = await this.applicationRepository.findOne({ + where: { + id: applicationId, + workspaceId, + }, + }); + + if (application === null) { + return null; + } + + const file = await this.fileRepository.findOne({ where: { path: `${fileFolder}/${filepath}`, workspaceId, @@ -47,24 +62,32 @@ export class FileService { }, }); - const application = await this.applicationRepository.findOneOrFail({ - where: { - id: applicationId, + if (file === null) { + return null; + } + + try { + const stream = await this.fileStorageService.readFile({ + resourcePath: filepath, + fileFolder, + applicationUniversalIdentifier: application.universalIdentifier, workspaceId, - }, - }); + }); - const stream = await this.fileStorageService.readFile({ - resourcePath: filepath, - fileFolder, - applicationUniversalIdentifier: application.universalIdentifier, - workspaceId, - }); + return { + stream, + mimeType: file.mimeType, + }; + } catch (error) { + if ( + error instanceof FileStorageException && + error.code === FileStorageExceptionCode.FILE_NOT_FOUND + ) { + return null; + } - return { - stream, - mimeType: file.mimeType, - }; + throw error; + } } async getFileStreamById({ diff --git a/packages/twenty-server/test/integration/metadata/suites/application/__snapshots__/failing-public-assets-controller-download.integration-spec.ts.snap b/packages/twenty-server/test/integration/metadata/suites/application/__snapshots__/failing-public-assets-controller-download.integration-spec.ts.snap new file mode 100644 index 00000000000..a987e95665c --- /dev/null +++ b/packages/twenty-server/test/integration/metadata/suites/application/__snapshots__/failing-public-assets-controller-download.integration-spec.ts.snap @@ -0,0 +1,99 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`Public assets controller download should fail when path attempts to escape into another file folder 1`] = ` +{ + "body": { + "code": "FILE_NOT_FOUND", + "error": "Error", + "messages": [ + "File not found", + ], + "statusCode": 404, + }, + "status": 404, +} +`; + +exports[`Public assets controller download should fail when path contains URL-encoded backslash traversal 1`] = ` +{ + "body": { + "code": "FILE_NOT_FOUND", + "error": "Error", + "messages": [ + "File not found", + ], + "statusCode": 404, + }, + "status": 404, +} +`; + +exports[`Public assets controller download should fail when path contains a leading parent-directory segment (..) 1`] = ` +{ + "body": { + "code": "FILE_NOT_FOUND", + "error": "Error", + "messages": [ + "File not found", + ], + "statusCode": 404, + }, + "status": 404, +} +`; + +exports[`Public assets controller download should fail when path contains multiple upward traversal segments (../../) 1`] = ` +{ + "body": { + "code": "FILE_NOT_FOUND", + "error": "Error", + "messages": [ + "File not found", + ], + "statusCode": 404, + }, + "status": 404, +} +`; + +exports[`Public assets controller download should fail when the applicationId does not match any application 1`] = ` +{ + "body": { + "code": "FILE_NOT_FOUND", + "error": "Error", + "messages": [ + "File not found", + ], + "statusCode": 404, + }, + "status": 404, +} +`; + +exports[`Public assets controller download should fail when the requested asset does not exist 1`] = ` +{ + "body": { + "code": "FILE_NOT_FOUND", + "error": "Error", + "messages": [ + "File not found", + ], + "statusCode": 404, + }, + "status": 404, +} +`; + +exports[`Public assets controller download should fail when the workspaceId does not match any workspace 1`] = ` +{ + "body": { + "code": "FILE_NOT_FOUND", + "error": "Error", + "messages": [ + "File not found", + ], + "statusCode": 404, + }, + "status": 404, +} +`; diff --git a/packages/twenty-server/test/integration/metadata/suites/application/failing-public-assets-controller-download.integration-spec.ts b/packages/twenty-server/test/integration/metadata/suites/application/failing-public-assets-controller-download.integration-spec.ts new file mode 100644 index 00000000000..89f5bf389b0 --- /dev/null +++ b/packages/twenty-server/test/integration/metadata/suites/application/failing-public-assets-controller-download.integration-spec.ts @@ -0,0 +1,141 @@ +import request from 'supertest'; +import { cleanupApplicationAndAppRegistration } from 'test/integration/metadata/suites/application/utils/cleanup-application-and-app-registration.util'; +import { setupApplicationForSync } from 'test/integration/metadata/suites/application/utils/setup-application-for-sync.util'; +import { uploadApplicationFile } from 'test/integration/metadata/suites/application/utils/upload-application-file.util'; +import { expectOneNotInternalServerErrorHttpResponseSnapshot } from 'test/integration/utils/expect-one-not-internal-server-error-http-response-snapshot.util'; +import { + type EachTestingContext, + eachTestingContextFilter, +} from 'twenty-shared/testing'; +import { v4 as uuidv4 } from 'uuid'; + +const TEST_APP_UID = uuidv4(); +const TEST_WORKSPACE_ID = '20202020-1c25-4d02-bf25-6aeccf7ea419'; +const UNKNOWN_WORKSPACE_ID = uuidv4(); +const UNKNOWN_APPLICATION_ID = uuidv4(); + +const PUBLIC_ASSET_PATH = 'assets/logo.svg'; +const PUBLIC_ASSET_CONTENT = ''; +const PUBLIC_ASSET_CONTENT_TYPE = 'image/svg+xml'; + +type FailingCase = { + buildUrl: (validApplicationId: string) => string; +}; + +const FAILING_CASES: EachTestingContext[] = [ + { + title: 'when path contains a leading parent-directory segment (..)', + context: { + buildUrl: (applicationId) => + `/public-assets/${TEST_WORKSPACE_ID}/${applicationId}/..`, + }, + }, + { + title: 'when path contains multiple upward traversal segments (../../)', + context: { + buildUrl: (applicationId) => + `/public-assets/${TEST_WORKSPACE_ID}/${applicationId}/../../sensitive-file`, + }, + }, + { + title: 'when path attempts to escape into another file folder', + context: { + buildUrl: (applicationId) => + `/public-assets/${TEST_WORKSPACE_ID}/${applicationId}/../workflow/secret.json`, + }, + }, + { + title: 'when path contains URL-encoded backslash traversal', + context: { + buildUrl: (applicationId) => + `/public-assets/${TEST_WORKSPACE_ID}/${applicationId}/..%5C..%5Cetc%5Cpasswd`, + }, + }, + { + title: 'when the requested asset does not exist', + context: { + buildUrl: (applicationId) => + `/public-assets/${TEST_WORKSPACE_ID}/${applicationId}/assets/does-not-exist.svg`, + }, + }, + { + title: 'when the workspaceId does not match any workspace', + context: { + buildUrl: (applicationId) => + `/public-assets/${UNKNOWN_WORKSPACE_ID}/${applicationId}/${PUBLIC_ASSET_PATH}`, + }, + }, + { + title: 'when the applicationId does not match any application', + context: { + buildUrl: () => + `/public-assets/${TEST_WORKSPACE_ID}/${UNKNOWN_APPLICATION_ID}/${PUBLIC_ASSET_PATH}`, + }, + }, +]; + +describe('Public assets controller download should fail', () => { + let applicationId: string; + + beforeAll(async () => { + await setupApplicationForSync({ + applicationUniversalIdentifier: TEST_APP_UID, + name: 'Test Public Assets Download Failure App', + description: + 'App for testing failing public-assets controller downloads', + sourcePath: 'test-public-assets-download-failure', + }); + + const [{ id }] = await globalThis.testDataSource.query( + `SELECT id FROM core."application" WHERE "universalIdentifier" = $1`, + [TEST_APP_UID], + ); + + applicationId = id; + + // A real public asset must exist so the "no content leak" guard below is + // meaningful — without it, `not.toContain(PUBLIC_ASSET_CONTENT)` would + // pass trivially regardless of the controller's behavior. + jest.useRealTimers(); + + await uploadApplicationFile({ + applicationUniversalIdentifier: TEST_APP_UID, + fileFolder: 'PublicAsset', + filePath: PUBLIC_ASSET_PATH, + fileBuffer: Buffer.from(PUBLIC_ASSET_CONTENT), + filename: 'logo.svg', + contentType: PUBLIC_ASSET_CONTENT_TYPE, + expectToFail: false, + }); + + jest.useFakeTimers(); + }, 60000); + + afterAll(async () => { + await cleanupApplicationAndAppRegistration({ + applicationUniversalIdentifier: TEST_APP_UID, + }); + }); + + it.each(eachTestingContextFilter(FAILING_CASES))( + '$title', + async ({ context }) => { + jest.useRealTimers(); + + const response = await request(global.app.getHttpServer()).get( + context.buildUrl(applicationId), + ); + + jest.useFakeTimers(); + + // The legitimate asset content must never leak through a failure path. + expect(response.text).not.toContain(PUBLIC_ASSET_CONTENT); + + expectOneNotInternalServerErrorHttpResponseSnapshot({ + status: response.status, + body: response.body, + }); + }, + 30000, + ); +}); diff --git a/packages/twenty-server/test/integration/metadata/suites/application/successful-public-assets-controller-download.integration-spec.ts b/packages/twenty-server/test/integration/metadata/suites/application/successful-public-assets-controller-download.integration-spec.ts new file mode 100644 index 00000000000..53a56e88267 --- /dev/null +++ b/packages/twenty-server/test/integration/metadata/suites/application/successful-public-assets-controller-download.integration-spec.ts @@ -0,0 +1,82 @@ +import request from 'supertest'; +import { cleanupApplicationAndAppRegistration } from 'test/integration/metadata/suites/application/utils/cleanup-application-and-app-registration.util'; +import { setupApplicationForSync } from 'test/integration/metadata/suites/application/utils/setup-application-for-sync.util'; +import { uploadApplicationFile } from 'test/integration/metadata/suites/application/utils/upload-application-file.util'; +import { v4 as uuidv4 } from 'uuid'; + +const TEST_APP_UID = uuidv4(); +const TEST_WORKSPACE_ID = '20202020-1c25-4d02-bf25-6aeccf7ea419'; + +const PUBLIC_ASSET_PATH = 'assets/logo.svg'; +const PUBLIC_ASSET_CONTENT = ''; +const PUBLIC_ASSET_CONTENT_TYPE = 'image/svg+xml'; + +describe('Public assets controller download should succeed', () => { + let applicationId: string; + + beforeAll(async () => { + await setupApplicationForSync({ + applicationUniversalIdentifier: TEST_APP_UID, + name: 'Test Public Assets Download Success App', + description: + 'App for testing successful public-assets controller downloads', + sourcePath: 'test-public-assets-download-success', + }); + + const [{ id }] = await globalThis.testDataSource.query( + `SELECT id FROM core."application" WHERE "universalIdentifier" = $1`, + [TEST_APP_UID], + ); + + applicationId = id; + + jest.useRealTimers(); + + await uploadApplicationFile({ + applicationUniversalIdentifier: TEST_APP_UID, + fileFolder: 'PublicAsset', + filePath: PUBLIC_ASSET_PATH, + fileBuffer: Buffer.from(PUBLIC_ASSET_CONTENT), + filename: 'logo.svg', + contentType: PUBLIC_ASSET_CONTENT_TYPE, + expectToFail: false, + }); + + jest.useFakeTimers(); + }, 60000); + + afterAll(async () => { + await cleanupApplicationAndAppRegistration({ + applicationUniversalIdentifier: TEST_APP_UID, + }); + }); + + it('should stream a public asset with the correct headers and body', async () => { + jest.useRealTimers(); + + // `image/svg+xml` is treated as binary by superagent: the bytes land in + // `response.body` as a Buffer, and `response.text` stays undefined. + const response = await request(global.app.getHttpServer()) + .get( + `/public-assets/${TEST_WORKSPACE_ID}/${applicationId}/${PUBLIC_ASSET_PATH}`, + ) + .buffer(true) + .parse((res, callback) => { + const chunks: Buffer[] = []; + + res.on('data', (chunk) => chunks.push(chunk)); + res.on('end', () => callback(null, Buffer.concat(chunks))); + }); + + jest.useFakeTimers(); + + expect(response.status).toBe(200); + expect(response.headers['content-type']).toContain( + PUBLIC_ASSET_CONTENT_TYPE, + ); + expect(response.headers['x-content-type-options']).toBe('nosniff'); + expect((response.body as Buffer).toString('utf-8')).toBe( + PUBLIC_ASSET_CONTENT, + ); + }, 30000); +}); diff --git a/packages/twenty-server/test/integration/utils/expect-one-not-internal-server-error-http-response-snapshot.util.ts b/packages/twenty-server/test/integration/utils/expect-one-not-internal-server-error-http-response-snapshot.util.ts new file mode 100644 index 00000000000..7be33b940bd --- /dev/null +++ b/packages/twenty-server/test/integration/utils/expect-one-not-internal-server-error-http-response-snapshot.util.ts @@ -0,0 +1,13 @@ +// REST counterpart of `expectOneNotInternalServerErrorSnapshot`. +export const expectOneNotInternalServerErrorHttpResponseSnapshot = ({ + status, + body, +}: { + status: number; + body: Record; +}) => { + expect(status).not.toBe(500); + expect(body.code).not.toBe('INTERNAL_SERVER_ERROR'); + + expect({ status, body }).toMatchSnapshot(); +};