Fix : #8825 If attachment token expires, it throws a 500 error instead of Unauthenticated #9043
Conversation
![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 |
packages/twenty-server/src/engine/core-modules/file/controllers/file.controller.ts
Show resolved
Hide resolved
|
Thanks! Let's keep unauthenticated as a 403 (as you did) for now to stay consistent with the rest of the codebase. But I created a followup issue here #9347 |
|
There's another issue with our current code which I'll fix (not linked to your PR). We've implemented a second notion of "expirationDate" just for the file service. The expiry is something JWT handle natively well and we should rely on the JWT expiry instead of introducing a second expirationDate. With the current code the file expiration date would be the lowest of the default token expiration date and the file token expiration date which doesn't make sense. |
There was a problem hiding this comment.
Good for me! But I'll let you check again before @charlesBochet since it can affect security
|
I'm back from holidays, I'm taking a look |
|
Thank you @munch-lax :) I've moved your code to a different location (file.exception.ts instead of auth.exception.ts and same for the filter) |
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
else