Fix : #8825 If attachment token expires, it throws a 500 error instead of Unauthenticated #9043
Conversation
Welcome!
Hello there, congrats on your first PR! We're excited to have you contributing to this project. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR adds error handling in FilePathGuard to properly handle expired or invalid attachment tokens, preventing 500 errors by returning appropriate authentication responses.
- Added try-catch block in
packages/twenty-server/src/engine/core-modules/file/guards/file-path-guard.tsto handle AuthException during token verification - Returns
falseinstead of throwing 500 error when token is unauthenticated - Throws AuthException with INTERNAL_SERVER_ERROR code for non-authentication token verification failures
- Maintains workspaceId validation and expiration date checks after successful verification
💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!
1 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
| if (!query || !query['token']) { | ||
| return false; | ||
| } | ||
|
|
||
| const payload = await this.jwtWrapperService.verifyWorkspaceToken( | ||
| let payload:any ={} |
There was a problem hiding this comment.
syntax: Missing space after colon in type declaration. Should be 'payload: any = {}'
There was a problem hiding this comment.
Please check run your linter: npx nx run twenty-server:lint
| if(error instanceof AuthException && error.code === AuthExceptionCode.UNAUTHENTICATED ){ | ||
| return false |
There was a problem hiding this comment.
syntax: Missing semicolons at line endings
| if(error instanceof AuthException && error.code === AuthExceptionCode.UNAUTHENTICATED ){ | ||
| return false | ||
| } | ||
| else{ |
There was a problem hiding this comment.
no need for else as you are already returning
if (error instance of...) {
return
}
throw ...
There was a problem hiding this comment.
actually forget about this comment as I think we should have another strategy :)
There was a problem hiding this comment.
Hi @munch-lax thanks for the PR. We would like to follow another strategy here:
- Create a FileAPIExceptionFilter (similar to AuthRestApiExceptionFilter) this filter should be able to handle the AuthException (with AuthExceptionCode.UNAUTHENTICATED)
- use it on top of the file.controller.ts (see GoogleAPIsAuthController as an example)
Now the exception that is thrown in the file-path-guard will be properly handled.
|
Hey @charlesBochet do we need to return the error in the same format that is by using handleExceptionService.handleError ? |
|
It should be very similar to AuthRestApiExceptionFilter :) so same output format |
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
else