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

Define a "jwt base class" for exceptions #252

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

prince-chrismc
Copy link
Collaborator

closes #215

@prince-chrismc prince-chrismc marked this pull request as draft October 1, 2022 21:53
@prince-chrismc
Copy link
Collaborator Author

There's actually a lot of these left

image

using std::exception::exception;
};

struct signature_verification_exception : exception, public std::system_error {
Copy link
Owner

Choose a reason for hiding this comment

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

This feels really weird cause now signature_verification_exception inherits from std::exception twice.
Could we get away with inheriting exception from std::system_error (which inherits std::runtime_error, which in turn inherits std::exception) ?

@@ -2316,13 +2321,13 @@ namespace jwt {
/**
* Attempt to parse JSON was unsuccessful
*/
struct invalid_json_exception : public std::runtime_error {
struct invalid_json_exception : exception, public std::runtime_error {
Copy link
Owner

Choose a reason for hiding this comment

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

See above, given that we dont actually set a dynamic error message, we should probably add a "invalid_json" error code and stick that in.

@prince-chrismc
Copy link
Collaborator Author

I think we need a more clear goal for this one. I was not expecting it to be soo big 🙈

On the server side if you only do this

try { 
     process_jwt(token);
catch (jwt_exception e) {
     //... handling 
}

How can you know to return 401 vs 403 😕 there's a difference for the token being malformed and not granting permission 🤔

@Thalhammer
Copy link
Owner

there's a difference for the token being malformed and not granting permission

Wouldn't this normally be handled in two different places ?
First you check if the token is a valid token (i.e. not malformed, not expired, etc.) then you check if the token (which you can now trust) actually allows access to the resource at hand. At least in my code bases this doesn't even happen in the same place, the first check is inside a middleware whereas the second is in the individual handler.

Normally the difference between multiple error groups is handled by std::error_category, if the token is malformed you'd get signature_verification_error_category() as the category, whereas a expired token (or one missing claims,etc) would give you a token_verification_error_category() error. So your handling (assuming you check the resource access inside the verifier) would look something like this:

try { 
     process_jwt(token);
catch (jwt_exception e) {
    if(e.code().category() == signature_verification_error_category())
         return 401;
    if(e.code().category() == token_verification_error_category())
         return 403;
    return 500;
}

You could also dig down into the actual error code, e.g. to distinguish token_expired and audience_missmatch.

Regardless, the fact that all the exception types now inherit std::exception twice definitly feels wrong and will almost certainly cause issues. What we could do is have a jwt::exception inherit std::system_error (and thus std::runtime_error and std::exception) and then inherit something like invalid_json_exception from jwt::exception so you can still catch those individually instead of checking the error category if you want.

@prince-chrismc
Copy link
Collaborator Author

Tomorrow while I travel, I will look at this again... I agree this isn't up to par. Theres 130+ exceptions that are from the STL... some are for sure traits but others I have no clue...

Once we know what the exceptions are doing we can figure out a few user stories.

Come up with a plan to improve this.

@Thalhammer
Copy link
Owner

Theres 130+ exceptions that are from the STL

You're not really supposed to use any of them though except for the 9 in <stdexcept> and std::exception.

@prince-chrismc
Copy link
Collaborator Author

I mean we are using them a lot 🤷 despite the other ways we are also using

@prince-chrismc
Copy link
Collaborator Author

Excluding the traits... there's 5 left

image

  • 2 are from the evp key ref
  • 2 are invalid tokens
  • last is claim compare in verifier

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.

Use common base class for all exceptions thrown by jwt-cpp
2 participants