Skip to content

Conversation

Piskoo
Copy link
Collaborator

@Piskoo Piskoo commented Sep 22, 2025

This PR grcp errors not being properly caught and replaced with more user friendly error in CLI.

Closes #2434

@Piskoo Piskoo marked this pull request as ready for review September 22, 2025 11:42
app/cli/main.go Outdated
func isWrappedErr(grpcStatus *status.Status, err *errors.Error) bool {
target := errors.FromError(grpcStatus.Err())
return target.Code == err.Code && err.Message == target.Message
return target.Code == err.Code && strings.Contains(target.Message, err.Message)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mm, this looks like a little bit of a hack that could cause false positives. Do you know when and why this started failing? Was it because we are now wrapping when we should not?

Copy link
Member

@jiparis jiparis Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error in backend might have changed. It was a hack already anyways, since it was comparing error strings in client side, instead of just the error code or a typed error object.

@migmartri
Copy link
Member

Example
Before it was checking for equality between JWT token has expired and rpc message JWT token has expired metadata = map[] cause =

So what's now?

Will be be reverted to what originally was which was handled locally as I mentioned in the issue?

@Piskoo
Copy link
Collaborator Author

Piskoo commented Sep 22, 2025

I'll work as it did before, seems like part of the output was missing in the description and I didn't notice. In general the problem is that grpc status returns message that looks like full error instead of only returning the message part:

rpc error: code = Unauthenticated desc = couldn't match JWT provider: error: code = 401 reason = UNAUTHORIZED message = JWT token has expired metadata = map[] cause = <nil>

Which then before was matched against JWT token has expired . It resulted in inequality so the error was not replaced in the end. Not sure when and why is started, but it's probably related to grpc-go being updated at some point. Went through Kratos and grpc and no changes were made to related files. Some backend change must have changed it

Signed-off-by: Sylwester Piskozub <sylwesterpiskozub@gmail.com>
@Piskoo Piskoo force-pushed the fix-error-overrides branch from 0a28c60 to 02ea58c Compare September 29, 2025 15:16
@Piskoo Piskoo requested a review from migmartri September 29, 2025 15:25
@Piskoo Piskoo changed the title fix(cli): update jwt error overrides fix(cli): error handling Sep 29, 2025
@migmartri
Copy link
Member

This is the actual fix #2450 so I'd suggest closing this one.

@Piskoo Piskoo closed this Sep 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

parsing authentication shows confusing error
3 participants