From bf638c3a4e2087c9189bb55b75a5bbeff3ac222d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Malfait?= Date: Wed, 19 Nov 2025 14:12:34 +0100 Subject: [PATCH] Fix file controller route parameter extraction (#15924) ## Problem When accessing file URLs like `/files/attachment/{token}/{filename}`, the server was returning 500 errors with no logs. The issue was that the route pattern `*path/:filename` was not properly extracting parameters, causing `request.params[0]` to be undefined. ## Solution - Changed route from `@Get('*path/:filename')` to `@Get(':folder/:token/:filename')` for explicit parameter extraction - Simplified `extractFileInfoFromRequest` to use named route parameters directly instead of complex path parsing - Removed unnecessary logging that was added during debugging - Added test to verify route parameters are correctly extracted and passed to FileService ## Testing - Added unit test that verifies folder, token, and filename parameters are correctly passed to the service - Test will catch any future regressions where route parameters are not properly extracted --- .../file/controllers/file.controller.spec.ts | 58 ++++++++++++++++++- .../file/controllers/file.controller.ts | 15 ++--- ...tract-file-info-from-request.utils.spec.ts | 42 ++++++++++---- .../extract-file-info-from-request.utils.ts | 12 ++-- .../rest-api-methods-should-be-guarded.ts | 12 ++-- tools/eslint-rules/utils/typedTokenHelpers.ts | 5 +- 6 files changed, 108 insertions(+), 36 deletions(-) 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 3291916633..c77236b261 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,6 +1,8 @@ import { type CanActivate } from '@nestjs/common'; import { Test, type TestingModule } from '@nestjs/testing'; +import { Readable } from 'stream'; + import { FileApiExceptionFilter } from 'src/engine/core-modules/file/filters/file-api-exception.filter'; import { FilePathGuard } from 'src/engine/core-modules/file/guards/file-path-guard'; import { FileService } from 'src/engine/core-modules/file/services/file.service'; @@ -9,6 +11,7 @@ import { FileController } from './file.controller'; describe('FileController', () => { let controller: FileController; + let fileService: FileService; const mock_FilePathGuard: CanActivate = { canActivate: jest.fn(() => true) }; beforeEach(async () => { @@ -17,7 +20,9 @@ describe('FileController', () => { providers: [ { provide: FileService, - useValue: {}, + useValue: { + getFileStream: jest.fn(), + }, }, ], }) @@ -28,9 +33,60 @@ describe('FileController', () => { .compile(); controller = module.get(FileController); + fileService = module.get(FileService); }); it('should be defined', () => { expect(controller).toBeDefined(); }); + + it('should extract folder, token and filename from 3-segment path', async () => { + const mockStream = new Readable(); + + mockStream.push('file content'); + mockStream.push(null); + mockStream.pipe = jest.fn(); + + jest.spyOn(fileService, 'getFileStream').mockResolvedValue(mockStream); + + const mockRequest = { + path: '/files/attachment/test-token/test-file.csv', + workspaceId: 'workspace-id', + } as any; + + const mockResponse = {} as any; + + await controller.getFile(mockResponse, mockRequest); + + expect(fileService.getFileStream).toHaveBeenCalledWith( + 'attachment', + 'test-file.csv', + 'workspace-id', + ); + }); + + it('should extract folder with size, token and filename from 4-segment path', async () => { + const mockStream = new Readable(); + + mockStream.push('file content'); + mockStream.push(null); + mockStream.pipe = jest.fn(); + + jest.spyOn(fileService, 'getFileStream').mockResolvedValue(mockStream); + + const mockRequest = { + path: '/files/profile-picture/original/test-token/avatar.jpg', + workspaceId: 'workspace-id', + } as any; + + const mockResponse = {} as any; + + await controller.getFile(mockResponse, mockRequest); + + expect(fileService.getFileStream).toHaveBeenCalledWith( + 'profile-picture/original', + 'avatar.jpg', + 'workspace-id', + ); + }); }); 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 34ba8c4277..703fe4e673 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,7 +1,6 @@ import { Controller, Get, - Param, Req, Res, UseFilters, @@ -24,25 +23,19 @@ import { FilePathGuard } from 'src/engine/core-modules/file/guards/file-path-gua import { FileService } from 'src/engine/core-modules/file/services/file.service'; import { extractFileInfoFromRequest } from 'src/engine/core-modules/file/utils/extract-file-info-from-request.utils'; import { NoPermissionGuard } from 'src/engine/guards/no-permission.guard'; -import { PublicEndpointGuard } from 'src/engine/guards/public-endpoint.guard'; @Controller('files') @UseFilters(FileApiExceptionFilter) -@UseGuards(FilePathGuard) export class FileController { constructor(private readonly fileService: FileService) {} - @Get('*path/:filename') - @UseGuards(PublicEndpointGuard, NoPermissionGuard) - async getFile( - @Param() _params: string[], - @Res() res: Response, - @Req() req: Request, - ) { + @Get('*') + @UseGuards(FilePathGuard, NoPermissionGuard) + async getFile(@Res() res: Response, @Req() req: Request) { // eslint-disable-next-line @typescript-eslint/no-explicit-any const workspaceId = (req as any)?.workspaceId; - const { filename, rawFolder } = extractFileInfoFromRequest(req); + const { rawFolder, filename } = extractFileInfoFromRequest(req); try { const fileStream = await this.fileService.getFileStream( diff --git a/packages/twenty-server/src/engine/core-modules/file/utils/__tests__/extract-file-info-from-request.utils.spec.ts b/packages/twenty-server/src/engine/core-modules/file/utils/__tests__/extract-file-info-from-request.utils.spec.ts index b73d36a239..cc4179f2f4 100644 --- a/packages/twenty-server/src/engine/core-modules/file/utils/__tests__/extract-file-info-from-request.utils.spec.ts +++ b/packages/twenty-server/src/engine/core-modules/file/utils/__tests__/extract-file-info-from-request.utils.spec.ts @@ -1,7 +1,7 @@ import { type Request } from 'express'; -import { checkFilename } from 'src/engine/core-modules/file/utils/check-file-name.utils'; import { checkFileFolder } from 'src/engine/core-modules/file/utils/check-file-folder.utils'; +import { checkFilename } from 'src/engine/core-modules/file/utils/check-file-name.utils'; import { extractFileInfoFromRequest } from 'src/engine/core-modules/file/utils/extract-file-info-from-request.utils'; jest.mock('src/engine/core-modules/file/utils/check-file-name.utils', () => ({ @@ -16,33 +16,53 @@ jest.mock( 'src/engine/core-modules/file/interfaces/file-folder.interface', () => ({ fileFolderConfigs: { - 'some-folder': { ignoreExpirationToken: true }, + attachment: { ignoreExpirationToken: false }, + 'profile-picture': { ignoreExpirationToken: true }, }, }), ); describe('extractFileInfoFromRequest', () => { - it('should extract all file info correctly from request', () => { + it('should extract all file info correctly from 3-segment path', () => { const mockRequest = { - params: { - filename: 'myfile.txt', - '0': 'some-folder/some-subfolder/filesig123', - }, + path: '/files/attachment/filesig123/myfile.txt', } as unknown as Request; (checkFilename as jest.Mock).mockReturnValue('validated-file.txt'); - (checkFileFolder as jest.Mock).mockReturnValue('some-folder'); + (checkFileFolder as jest.Mock).mockReturnValue('attachment'); const result = extractFileInfoFromRequest(mockRequest); expect(checkFilename).toHaveBeenCalledWith('myfile.txt'); - expect(checkFileFolder).toHaveBeenCalledWith('some-folder/some-subfolder'); + expect(checkFileFolder).toHaveBeenCalledWith('attachment'); expect(result).toEqual({ filename: 'validated-file.txt', fileSignature: 'filesig123', - rawFolder: 'some-folder/some-subfolder', - fileFolder: 'some-folder', + rawFolder: 'attachment', + fileFolder: 'attachment', + ignoreExpirationToken: false, + }); + }); + + it('should extract all file info correctly from 4-segment path with size', () => { + const mockRequest = { + path: '/files/profile-picture/original/filesig456/avatar.jpg', + } as unknown as Request; + + (checkFilename as jest.Mock).mockReturnValue('validated-avatar.jpg'); + (checkFileFolder as jest.Mock).mockReturnValue('profile-picture'); + + const result = extractFileInfoFromRequest(mockRequest); + + expect(checkFilename).toHaveBeenCalledWith('avatar.jpg'); + expect(checkFileFolder).toHaveBeenCalledWith('profile-picture/original'); + + expect(result).toEqual({ + filename: 'validated-avatar.jpg', + fileSignature: 'filesig456', + rawFolder: 'profile-picture/original', + fileFolder: 'profile-picture', ignoreExpirationToken: true, }); }); diff --git a/packages/twenty-server/src/engine/core-modules/file/utils/extract-file-info-from-request.utils.ts b/packages/twenty-server/src/engine/core-modules/file/utils/extract-file-info-from-request.utils.ts index 37a27592d4..15e481f005 100644 --- a/packages/twenty-server/src/engine/core-modules/file/utils/extract-file-info-from-request.utils.ts +++ b/packages/twenty-server/src/engine/core-modules/file/utils/extract-file-info-from-request.utils.ts @@ -6,13 +6,15 @@ import { checkFileFolder } from 'src/engine/core-modules/file/utils/check-file-f import { checkFilename } from 'src/engine/core-modules/file/utils/check-file-name.utils'; export const extractFileInfoFromRequest = (request: Request) => { - const filename = checkFilename(request.params.filename); + // Ex: /files/profile-picture/original/TOKEN/file.jpg + const pathSegments = request.path.split('/').filter((segment) => segment); - const parts = request.params[0].split('/'); + const segments = pathSegments.slice(1); - const fileSignature = parts.pop(); - - const rawFolder = parts.join('/'); + const filename = checkFilename(segments[segments.length - 1]); + const fileSignature = segments[segments.length - 2]; + const folderSegments = segments.slice(0, segments.length - 2); + const rawFolder = folderSegments.join('/'); const fileFolder = checkFileFolder(rawFolder); diff --git a/tools/eslint-rules/rules/rest-api-methods-should-be-guarded.ts b/tools/eslint-rules/rules/rest-api-methods-should-be-guarded.ts index 6f43b64451..84685e70ed 100644 --- a/tools/eslint-rules/rules/rest-api-methods-should-be-guarded.ts +++ b/tools/eslint-rules/rules/rest-api-methods-should-be-guarded.ts @@ -15,9 +15,9 @@ export const restApiMethodsShouldBeGuarded = (node: TSESTree.MethodDefinition) = const hasAuthGuards = typedTokenHelpers.nodeHasAuthGuards(node); const hasPermissionsGuard = typedTokenHelpers.nodeHasPermissionsGuard(node); - function findClassDeclaration( + const findClassDeclaration = ( node: TSESTree.Node - ): TSESTree.ClassDeclaration | null { + ): TSESTree.ClassDeclaration | null => { if (node.type === TSESTree.AST_NODE_TYPES.ClassDeclaration) { return node; } @@ -52,20 +52,20 @@ export const rule = createRule<[], 'restApiMethodsShouldBeGuarded'>({ meta: { docs: { description: - 'REST API endpoints should have authentication guards (UserAuthGuard or WorkspaceAuthGuard) or be explicitly marked as public (PublicEndpointGuard) and permission guards (SettingsPermissionsGuard or CustomPermissionGuard) to maintain our security model.', + 'REST API endpoints should have authentication guards (UserAuthGuard, WorkspaceAuthGuard, or FilePathGuard) or be explicitly marked as public (PublicEndpointGuard) and permission guards (SettingsPermissionsGuard or CustomPermissionGuard) to maintain our security model.', }, messages: { restApiMethodsShouldBeGuarded: - 'All REST API controller endpoints must have authentication guards (@UseGuards(UserAuthGuard/WorkspaceAuthGuard/PublicEndpointGuard)) and permission guards (@UseGuards(..., SettingsPermissionsGuard(PermissionFlagType.XXX)), CustomPermissionGuard for custom logic, or NoPermissionGuard for special cases).', + 'All REST API controller endpoints must have authentication guards (@UseGuards(UserAuthGuard/WorkspaceAuthGuard/FilePathGuard/PublicEndpointGuard)) and permission guards (@UseGuards(..., SettingsPermissionsGuard(PermissionFlagType.XXX)), CustomPermissionGuard for custom logic, or NoPermissionGuard for special cases).', }, schema: [], hasSuggestions: false, type: 'suggestion', }, defaultOptions: [], - create(context) { + create: (context) => { return { - MethodDefinition(node: TSESTree.MethodDefinition): void { + MethodDefinition: (node: TSESTree.MethodDefinition): void => { if (restApiMethodsShouldBeGuarded(node)) { context.report({ node: node, diff --git a/tools/eslint-rules/utils/typedTokenHelpers.ts b/tools/eslint-rules/utils/typedTokenHelpers.ts index 64a5d1c4cb..7f71237756 100644 --- a/tools/eslint-rules/utils/typedTokenHelpers.ts +++ b/tools/eslint-rules/utils/typedTokenHelpers.ts @@ -42,13 +42,14 @@ export const typedTokenHelpers = { TSESTree.AST_NODE_TYPES.Identifier && decorator.expression.callee.name === 'UseGuards' ) { - // Check the arguments for UserAuthGuard, WorkspaceAuthGuard, or PublicEndpoint + // Check the arguments for UserAuthGuard, WorkspaceAuthGuard, PublicEndpoint, or FilePathGuard return decorator.expression.arguments.some((arg) => { if (arg.type === TSESTree.AST_NODE_TYPES.Identifier) { return ( arg.name === 'UserAuthGuard' || arg.name === 'WorkspaceAuthGuard' || - arg.name === 'PublicEndpointGuard' + arg.name === 'PublicEndpointGuard' || + arg.name === 'FilePathGuard' ); } return false;