Conversation
Signed-off-by: Efren Lim <elim@linuxfoundation.org>
There was a problem hiding this comment.
Pull request overview
Updates the authentication callback API error handling so the catch block can throw an error with a more specific status code/message (instead of always returning a generic 500).
Changes:
- Adds logic in the callback handler catch block to derive
statusCode/statusMessagefrom anH3Error. - Changes the default error code/message used when the caught error is not an
H3Error.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (error instanceof H3Error) { | ||
| errorMessage = error.statusMessage || error.message || 'Authentication callback error'; | ||
| errorCode = error.statusCode; | ||
| } | ||
|
|
||
| throw createError({ | ||
| statusCode: 500, | ||
| statusMessage: 'Authentication callback error', | ||
| statusCode: errorCode, | ||
| statusMessage: errorMessage, |
There was a problem hiding this comment.
In the H3Error branch, error.statusCode may be undefined (it’s optional in some H3 error shapes). Passing that through as statusCode can lead to inconsistent responses. Consider defaulting with error.statusCode ?? 500 (or another explicit fallback) before calling createError.
| let errorMessage = 'Authentication callback error'; | ||
| let errorCode = 401; | ||
| if (error instanceof H3Error) { | ||
| errorMessage = error.statusMessage || error.message || 'Authentication callback error'; | ||
| errorCode = error.statusCode; | ||
| } | ||
|
|
||
| throw createError({ | ||
| statusCode: 500, | ||
| statusMessage: 'Authentication callback error', | ||
| statusCode: errorCode, | ||
| statusMessage: errorMessage, |
There was a problem hiding this comment.
The catch-all branch now forwards error.message/statusMessage to the HTTP response. Errors thrown by openid-client and other libraries can include internal details that shouldn’t be exposed to clients. Consider returning a generic statusMessage for unexpected errors and only surfacing curated/auth-specific messages (while keeping the detailed error logged server-side).
frontend/server/api/auth/callback.ts
Outdated
| deleteCookie(event, 'auth_refresh_token'); | ||
|
|
||
| let errorMessage = 'Authentication callback error'; | ||
| let errorCode = 401; |
There was a problem hiding this comment.
Defaulting to 401 for any non-H3Error exception will classify unexpected server-side failures (e.g., Auth0 discovery/network errors thrown as plain Error) as an auth failure, which can make debugging/monitoring harder and may be semantically incorrect. Consider defaulting to 500 and only using 401 for clearly identified authentication/authorization errors.
| let errorCode = 401; | |
| let errorCode = 500; |
There was a problem hiding this comment.
i agree with that suggestion
There was a problem hiding this comment.
Yeah I agree as well. The default should be 500. Then we should be able to throw specific error codes such as 401 when we know it's exactly for authentication/authorization errors.
frontend/server/api/auth/callback.ts
Outdated
| if (error instanceof H3Error) { | ||
| errorMessage = error.statusMessage || error.message || 'Authentication callback error'; | ||
| errorCode = error.statusCode; |
There was a problem hiding this comment.
H3Error is referenced as a runtime value in instanceof H3Error, but it is not imported in this file. This will fail TypeScript compilation (and at runtime H3Error will be undefined). Import H3Error from h3 (or use a different H3 error guard that doesn’t rely on an unimported symbol).
gaspergrom
left a comment
gaspergrom
left a comment
There was a problem hiding this comment.
i think copilot comments make sense
frontend/server/api/auth/callback.ts
Outdated
| deleteCookie(event, 'auth_refresh_token'); | ||
|
|
||
| let errorMessage = 'Authentication callback error'; | ||
| let errorCode = 401; |
There was a problem hiding this comment.
i agree with that suggestion
frontend/server/api/auth/callback.ts
Outdated
| deleteCookie(event, 'auth_refresh_token'); | ||
|
|
||
| let errorMessage = 'Authentication callback error'; | ||
| let errorCode = 401; |
There was a problem hiding this comment.
Yeah I agree as well. The default should be 500. Then we should be able to throw specific error codes such as 401 when we know it's exactly for authentication/authorization errors.
Signed-off-by: Efren Lim <elim@linuxfoundation.org>
3 participants
In this PR
Changed the catch block throw error message and code for the authentication callback.
Ticket
IN-957