Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

jwt.ParseRequest(...) seems to destroy error-wrapping stack #1175

Closed
mdjarv opened this issue Sep 5, 2024 · 5 comments · Fixed by #1176
Closed

jwt.ParseRequest(...) seems to destroy error-wrapping stack #1175

mdjarv opened this issue Sep 5, 2024 · 5 comments · Fixed by #1176
Assignees

Comments

@mdjarv
Copy link

mdjarv commented Sep 5, 2024

Describe the bug

When using jwt.ParseRequest(...) on a token that fails validation the resulting error can not be checked using errors.Is() because ParseRequest is not wrapping the validation errors.

This means that you can not check for specific validation failures in a reliable way using errors.Is.

To Reproduce / Expected behavior

token, err := jwt.ParseRequest(ctx.Request(), jwt.WithKeySet(set))
if errors.Is(err, jwt.ErrTokenExpired()) {
	return nil, echo.NewHTTPError(http.StatusUnauthorized, "token expired")
}
if err != nil {
	slog.Debug("failed to parse token", "operationID", operationID, "error", err)
	return nil, echo.NewHTTPError(http.StatusUnauthorized, fmt.Sprintf("invalid token"))
}

Parsing a request with an expired token above will not return "token expired".

The other Parse* functions I've tried work as expected so I can work around this by using jwt.ParseHeader(...) instead to make the example above work, however if keeping the error stack is possible for jwt.ParseRequest while also returning a well crafted error message I think that would be preferable as it will behave consistently with the other parse methods.

@lestrrat
Copy link
Collaborator

lestrrat commented Sep 5, 2024

Ah, this is one of those "when you can have multiple error returns from a single function, how do you present it to the caller" type of situations.

(talking aloud) I guess I could use one of those multiple-error modules, but I might want to just roll my own instead of adding too many dependencies -- after all it's just errors.Is that we want to fool... never mind, I forgot recent go's handle multiple errors wrapped into one just fine.

@lestrrat
Copy link
Collaborator

lestrrat commented Sep 5, 2024

@mdjarv Please try the branch in gh-1175 . Should be fixed.

@mdjarv
Copy link
Author

mdjarv commented Sep 5, 2024

Thank you, gh-1175 works as expected: errors.Is(err, jwt.ErrTokenExpired()) now properly matches my expired token

It took longer to wait for my token to expire so I could test it than it took for you to read my issue and provide a fix, incredible work, thank you!

@lestrrat
Copy link
Collaborator

lestrrat commented Sep 5, 2024

@mdjarv Cool ! :D

I'll merge, but please note that I usually wait for a few days before making releases. For the time being, please use the commit ID after I merge the PR

@mdjarv
Copy link
Author

mdjarv commented Sep 5, 2024

Yep, no rush on my part, and ParseHeader also works just fine until release, thanks again for the quick response :)

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 a pull request may close this issue.

2 participants