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

Version v5.2.0 is breaking our code #147

Closed
juliangp opened this issue Nov 8, 2023 · 11 comments
Closed

Version v5.2.0 is breaking our code #147

juliangp opened this issue Nov 8, 2023 · 11 comments
Labels
bug Something isn't working

Comments

@juliangp
Copy link

juliangp commented Nov 8, 2023

Hi,

We upgraded to v5.2.0 and our JWT tokens no longer parse as they do not contain the JWT header type (and I don't think that we can get this fixed any time soon).

This is the commit that broke us: 14ac6af

Is there any chance of making this test optional?

Thanks

@cristaloleg cristaloleg added the bug Something isn't working label Nov 8, 2023
@cristaloleg
Copy link
Member

cristaloleg commented Nov 8, 2023

Hey, sorry for the trouble. That's an interesting case.

Can you clarify why your tokens don't have 'typ:"JWT"' ? (or another value if any)

Also https://datatracker.ietf.org/doc/html/rfc7519#section-5.1

@frankdvd
Copy link

frankdvd commented Nov 8, 2023

Hi @cristaloleg The "typ" field in a JSON Web Token (JWT) is an optional field. According to the JWT specification (RFC 7519), it's used to specify the token type. However, it's not required, and many JWT implementations don't include it. So, maybe we should check the type only when it exists?

@cristaloleg
Copy link
Member

Yeah, that's another solution that came to my mind.

However, it looks very unintuitive 'cause sometimes it does a check and sometimes not.

@cristaloleg
Copy link
Member

Right now I see this as the simplest fix:

token, err := jwt.Parse(...) // or any other parse function from jwt
if err != nil {
    if !errors.Is(err, jwt.ErrNotJWTType) {
        // bad token data or bad signature or ...
    }
    // proper token BUT 'typ' field is not 'JWT'
}

@juliangp
Copy link
Author

juliangp commented Nov 8, 2023

Hi, I do not own the service that provides the token but as far as I can see the header only contains alg and kid.

@juliangp
Copy link
Author

juliangp commented Nov 8, 2023

I will try your suggestion above - thanks.

@cristaloleg
Copy link
Member

cristaloleg commented Nov 8, 2023

Again, sorry for the trouble. I hope it didn't end up as an emergency.

Please confirm if the solution above works for you. If so, I will document that. Thanks!

@juliangp
Copy link
Author

juliangp commented Nov 8, 2023

Hi, unfortunately your suggestion does not work because the token is coming back as nil?

@cristaloleg
Copy link
Member

cristaloleg commented Nov 8, 2023

Ah, indeed, can you check this PR #148 ?

@cristaloleg
Copy link
Member

@juliangp
Copy link
Author

juliangp commented Nov 8, 2023

@cristaloleg thanks for the fix, it is not very intuitive for an optional header but it does work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants