Early return in public assets (#20881)

# Introduction
Related https://github.com/twentyhq/twenty/issues/20879

More abstracted response error and cleaner integrity check before
performing any in database search
Nothing critical patched here

Also added integration coverage to the related endpoint

Fixed the stream on error throw that would have been bubbling up into
node process

## Next
Once this has been approved will re-apply to all the existing prone
file.getBy* methods and controllers endpoints

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>(FileController);
     fileService = module.get<FileService>(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);
     });
   });
 });

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

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({

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,
+}
+`;

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 = '<svg><circle r="10" /></svg>';
+const PUBLIC_ASSET_CONTENT_TYPE = 'image/svg+xml';
+
+type FailingCase = {
+  buildUrl: (validApplicationId: string) => string;
+};
+
+const FAILING_CASES: EachTestingContext<FailingCase>[] = [
+  {
+    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,
+  );
+});

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 = '<svg><circle r="10" /></svg>';
+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);
+});

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<string, unknown>;
+}) => {
+  expect(status).not.toBe(500);
+  expect(body.code).not.toBe('INTERNAL_SERVER_ERROR');
+
+  expect({ status, body }).toMatchSnapshot();
+};