From d324cac74260c07f7d3fb8350a972831dec4c40b Mon Sep 17 00:00:00 2001 From: munch-lax <36762510+munch-lax@users.noreply.github.com> Date: Wed, 8 Jan 2025 23:39:33 +0530 Subject: [PATCH] Fix : #8825 If attachment token expires, it throws a 500 error instead of Unauthenticated (#9043) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #8825 FilePathGuard implements token verification via verifyWorkspaceToken function which throws AuthException error , since CanActivate expects a boolean value , we add a try catch while verifying the token if token is invalid/expired Screenshot 2024-12-12 at 9 44 58 PM else Screenshot 2024-12-12 at 9 47 10 PM --------- Co-authored-by: Félix Malfait Co-authored-by: Félix Malfait Co-authored-by: Charles Bochet --- .../auth/services/sign-in-up.service.ts | 2 +- .../file/controllers/file.controller.ts | 40 +++++++++++++---- .../core-modules/file/file.exception.ts | 14 ++++++ .../file/filters/file-api-exception.filter.ts | 43 ++++++++++++++++++ .../file/guards/file-path-guard.ts | 44 +++++-------------- .../file/services/file.service.ts | 7 +-- packages/twenty-ui/src/layout/index.ts | 8 ++++ 7 files changed, 108 insertions(+), 50 deletions(-) create mode 100644 packages/twenty-server/src/engine/core-modules/file/file.exception.ts create mode 100644 packages/twenty-server/src/engine/core-modules/file/filters/file-api-exception.filter.ts diff --git a/packages/twenty-server/src/engine/core-modules/auth/services/sign-in-up.service.ts b/packages/twenty-server/src/engine/core-modules/auth/services/sign-in-up.service.ts index c5ffa4603..900a3dec2 100644 --- a/packages/twenty-server/src/engine/core-modules/auth/services/sign-in-up.service.ts +++ b/packages/twenty-server/src/engine/core-modules/auth/services/sign-in-up.service.ts @@ -14,9 +14,9 @@ import { AuthExceptionCode, } from 'src/engine/core-modules/auth/auth.exception'; import { + PASSWORD_REGEX, compareHash, hashPassword, - PASSWORD_REGEX, } from 'src/engine/core-modules/auth/auth.util'; import { DomainManagerService } from 'src/engine/core-modules/domain-manager/service/domain-manager.service'; import { EnvironmentService } from 'src/engine/core-modules/environment/environment.service'; 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 cd95d43c7..4c9ed0744 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,4 +1,12 @@ -import { Controller, Get, Param, Req, Res, UseGuards } from '@nestjs/common'; +import { + Controller, + Get, + Param, + Req, + Res, + UseFilters, + UseGuards, +} from '@nestjs/common'; import { Response } from 'express'; @@ -7,15 +15,20 @@ import { FileStorageExceptionCode, } from 'src/engine/core-modules/file-storage/interfaces/file-storage-exception'; +import { + FileException, + FileExceptionCode, +} from 'src/engine/core-modules/file/file.exception'; import { checkFilePath, checkFilename, } from 'src/engine/core-modules/file/file.utils'; +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'; -// TODO: Add cookie authentication @Controller('files') +@UseFilters(FileApiExceptionFilter) @UseGuards(FilePathGuard) export class FileController { constructor(private readonly fileService: FileService) {} @@ -27,15 +40,15 @@ export class FileController { @Req() req: Request, ) { const folderPath = checkFilePath(params[0]); - const filename = checkFilename(params['filename']); const workspaceId = (req as any)?.workspaceId; if (!workspaceId) { - return res - .status(401) - .send({ error: 'Unauthorized, missing workspaceId' }); + throw new FileException( + 'Unauthorized: missing workspaceId', + FileExceptionCode.UNAUTHENTICATED, + ); } try { @@ -46,7 +59,10 @@ export class FileController { ); fileStream.on('error', () => { - res.status(500).send({ error: 'Internal server error' }); + throw new FileException( + 'Error streaming file from storage', + FileExceptionCode.INTERNAL_SERVER_ERROR, + ); }); fileStream.pipe(res); @@ -55,10 +71,16 @@ export class FileController { error instanceof FileStorageException && error.code === FileStorageExceptionCode.FILE_NOT_FOUND ) { - return res.status(404).send({ error: 'File not found' }); + throw new FileException( + 'File not found', + FileExceptionCode.FILE_NOT_FOUND, + ); } - return res.status(500).send({ error: 'Internal server error' }); + throw new FileException( + `Error retrieving file: ${error.message}`, + FileExceptionCode.INTERNAL_SERVER_ERROR, + ); } } } diff --git a/packages/twenty-server/src/engine/core-modules/file/file.exception.ts b/packages/twenty-server/src/engine/core-modules/file/file.exception.ts new file mode 100644 index 000000000..4a953f235 --- /dev/null +++ b/packages/twenty-server/src/engine/core-modules/file/file.exception.ts @@ -0,0 +1,14 @@ +import { CustomException } from 'src/utils/custom-exception'; + +export enum FileExceptionCode { + UNAUTHENTICATED = 'UNAUTHENTICATED', + INTERNAL_SERVER_ERROR = 'INTERNAL_SERVER_ERROR', + FILE_NOT_FOUND = 'FILE_NOT_FOUND', +} + +export class FileException extends CustomException { + code: FileExceptionCode; + constructor(message: string, code: FileExceptionCode) { + super(message, code); + } +} diff --git a/packages/twenty-server/src/engine/core-modules/file/filters/file-api-exception.filter.ts b/packages/twenty-server/src/engine/core-modules/file/filters/file-api-exception.filter.ts new file mode 100644 index 000000000..548c44155 --- /dev/null +++ b/packages/twenty-server/src/engine/core-modules/file/filters/file-api-exception.filter.ts @@ -0,0 +1,43 @@ +import { ArgumentsHost, Catch, ExceptionFilter } from '@nestjs/common'; + +import { Response } from 'express'; + +import { HttpExceptionHandlerService } from 'src/engine/core-modules/exception-handler/http-exception-handler.service'; +import { + FileException, + FileExceptionCode, +} from 'src/engine/core-modules/file/file.exception'; + +@Catch(FileException) +export class FileApiExceptionFilter implements ExceptionFilter { + constructor( + private readonly httpExceptionHandlerService: HttpExceptionHandlerService, + ) {} + + catch(exception: FileException, host: ArgumentsHost) { + const ctx = host.switchToHttp(); + const response = ctx.getResponse(); + + switch (exception.code) { + case FileExceptionCode.UNAUTHENTICATED: + return this.httpExceptionHandlerService.handleError( + exception, + response, + 403, + ); + case FileExceptionCode.FILE_NOT_FOUND: + return this.httpExceptionHandlerService.handleError( + exception, + response, + 404, + ); + case FileExceptionCode.INTERNAL_SERVER_ERROR: + default: + return this.httpExceptionHandlerService.handleError( + exception, + response, + 500, + ); + } + } +} diff --git a/packages/twenty-server/src/engine/core-modules/file/guards/file-path-guard.ts b/packages/twenty-server/src/engine/core-modules/file/guards/file-path-guard.ts index 3296a57cf..0cbe988a0 100644 --- a/packages/twenty-server/src/engine/core-modules/file/guards/file-path-guard.ts +++ b/packages/twenty-server/src/engine/core-modules/file/guards/file-path-guard.ts @@ -1,10 +1,4 @@ -import { - CanActivate, - ExecutionContext, - HttpException, - HttpStatus, - Injectable, -} from '@nestjs/common'; +import { CanActivate, ExecutionContext, Injectable } from '@nestjs/common'; import { JwtWrapperService } from 'src/engine/core-modules/jwt/services/jwt-wrapper.service'; @@ -20,12 +14,16 @@ export class FilePathGuard implements CanActivate { return false; } - const payload = await this.jwtWrapperService.verifyWorkspaceToken( - query['token'], - 'FILE', - ); + try { + const payload = await this.jwtWrapperService.verifyWorkspaceToken( + query['token'], + 'FILE', + ); - if (!payload.workspaceId) { + if (!payload.workspaceId) { + return false; + } + } catch (error) { return false; } @@ -33,32 +31,10 @@ export class FilePathGuard implements CanActivate { json: true, }); - const expirationDate = decodedPayload?.['expirationDate']; const workspaceId = decodedPayload?.['workspaceId']; - const isExpired = await this.isExpired(expirationDate); - - if (isExpired) { - return false; - } - request.workspaceId = workspaceId; return true; } - - private async isExpired(expirationDate: string): Promise { - if (!expirationDate) { - return true; - } - - if (new Date(expirationDate) < new Date()) { - throw new HttpException( - 'This url has expired. Please reload twenty page and open file again.', - HttpStatus.FORBIDDEN, - ); - } - - return false; - } } 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 c4c240f21..8bc2dc022 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 @@ -2,9 +2,6 @@ import { Injectable } from '@nestjs/common'; import { Stream } from 'stream'; -import { addMilliseconds } from 'date-fns'; -import ms from 'ms'; - import { EnvironmentService } from 'src/engine/core-modules/environment/environment.service'; import { FileStorageService } from 'src/engine/core-modules/file-storage/file-storage.service'; import { JwtWrapperService } from 'src/engine/core-modules/jwt/services/jwt-wrapper.service'; @@ -39,15 +36,13 @@ export class FileService { payloadToEncode.workspaceId, ); - const expirationDate = addMilliseconds(new Date(), ms(fileTokenExpiresIn)); - const signedPayload = this.jwtWrapperService.sign( { - expirationDate: expirationDate, ...payloadToEncode, }, { secret, + expiresIn: fileTokenExpiresIn, }, ); diff --git a/packages/twenty-ui/src/layout/index.ts b/packages/twenty-ui/src/layout/index.ts index 1f88712f8..8f33feb75 100644 --- a/packages/twenty-ui/src/layout/index.ts +++ b/packages/twenty-ui/src/layout/index.ts @@ -1,4 +1,12 @@ export * from './animated-expandable-container/components/AnimatedExpandableContainer'; +export * from './animated-expandable-container/types/AnimationDimension'; +export * from './animated-expandable-container/types/AnimationDurationObject'; +export * from './animated-expandable-container/types/AnimationDurations'; +export * from './animated-expandable-container/types/AnimationMode'; +export * from './animated-expandable-container/types/AnimationSize'; +export * from './animated-expandable-container/utils/getCommonStyles'; +export * from './animated-expandable-container/utils/getExpandableAnimationConfig'; +export * from './animated-expandable-container/utils/getTransitionValues'; export * from './animated-placeholder/components/AnimatedPlaceholder'; export * from './animated-placeholder/components/EmptyPlaceholderStyled'; export * from './animated-placeholder/components/ErrorPlaceholderStyled';