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

isJWT bug #964

Open
jsnoble opened this issue Jan 17, 2019 · 11 comments · Fixed by #1316
Open

isJWT bug #964

jsnoble opened this issue Jan 17, 2019 · 11 comments · Fixed by #1316

Comments

@jsnoble
Copy link

jsnoble commented Jan 17, 2019

There is a problem with the method isJWT in that it returns true for incorrect values

const validator = require('validator');
const jwt = 'eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJ1c2VySWQiOiJiMDhmODZhZi0zNWRhLTQ4ZjItOGZhYi1jZWYzOTA0NjYwYmQifQ.-xN_h82PHVTCMA9vdoHrcZxH-x5mb11y1537t3rGzcM';

validator.isJWT(jwt);  => true as expected

// The problem is here
validator.isJWT("56.234"); => true, should return false
@Vengarioth
Copy link

the problem here is that the example 56.234 contains only valid JWT characters, which i suspect is the scope of this library. To further validate (and use) JWTs i suggest using one of the libraries around, like jsonwebtoken.

@profnandaa
Copy link
Member

Sure, I see, our test for isJWT is quite basic... 🤔

@profnandaa
Copy link
Member

@jsnoble - PR is welcome! 👍

@ezkemboi
Copy link
Member

I am doing some research on this issue. Though based on issue #609, the user requested for functionality not to decode JWT but just verify string received.
#885 added the functionality, which was also modified on #906 as it was said signature may not be necessary for this case.
I have done research to get the minimum length for header, payload, and signature, but there is no set minimum or maximum set up.
But, when I will have some points to consider, we can surely make a discussion.
Or, if I see where I can make a better validator for the same, i.e improve it, I will surely do that.

Cc. @jsnoble, @Vengarioth and @profnandaa.

@parasg1999
Copy link
Contributor

parasg1999 commented May 25, 2020

The test cases for isJWT are not correct. JWT is either 2 (in case of no signature) or 3 [dot] separated base64 strings. The test cases include symbols like _ and - which are not valid base64 characters.

The OP has given an example for which the result should be opposite in this case.

const jwt = 'eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJ1c2VySWQiOiJiMDhmODZhZi0zNWRhLTQ4ZjItOGZhYi1jZWYzOTA0NjYwYmQifQ.-xN_h82PHVTCMA9vdoHrcZxH-x5mb11y1537t3rGzcM';

validator.isJWT(jwt);  => true as expected  // **should be false**

@profnandaa

@parasg1999
Copy link
Contributor

@profnandaa
Okay, so after reading a bit. I found out that JWT is url safe base64. RFC7519, section 3
So, the symbols + and / in normal implementation are replaced by - and _

If the PR #1277 is merged, we can solve this issue. The PR has merge conflicts so I'll be happy to make a new PR if the author isn't active.

@profnandaa
Copy link
Member

@parasg1999 -- let's wait on @mum-never-proud or if okays you to pick up the PR.

@profnandaa
Copy link
Member

@parasg1999 -- merged, could you please check if this is solved now?

@leosuncin
Copy link

isJWT pass with string with valid base64 characters, by example, 'A.A.A'. See my fix http://runkit.com/suncin/isjwt-fail-to-detect-valid-jwt-string

@profnandaa profnandaa reopened this Jun 14, 2020
@profnandaa
Copy link
Member

@parasg1999 -- can check this one?

@ItalyPaleAle
Copy link
Contributor

I implemented #906.

Right now, isJWT just checks if the string is in the correct format, with 2 or 3 sequences of (URL-encoded) base64 sequences, separated by a dot. To further validate if the string is an actual JWT, we would need to decode the base64 sequences and then decode the JSON.

I want to point out that while implementing that is possible, it's a slippery slope, as then someone else will ask why the library doesn't also check the signature of the JWT, etc. At that point, we may as well just re-implement the entire JWT standard :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants