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

Fix validation issue in isJWT function #2217

Merged
merged 5 commits into from
Jun 26, 2023

Conversation

Prathamesh061
Copy link
Contributor

This PR fixes the validation issue in the isJWT function where it was returning true for a 2-part invalid JWT token. The fix is to check for len < 3 instead of len < 2 in the code.

Changes Made:

Updated the validation check for len < 3 in isJWT function
Testing:

Ran unit tests for the isJWT function and verified that it now returns false for 2-part invalid JWT tokens.

fix: Ensure isJWT returns false for 2 part invalid JWT tokens

Previously, the isJWT function would return true for 2 part invalid JWT tokens. This has been fixed by updating the isJWT function to return false for such tokens.
Copy link
Member

@WikiRik WikiRik left a comment

Choose a reason for hiding this comment

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

https://jwt.io/introduction does indeed mention that a JWT consists of 3 parts split by a dot so this does seem like a correct change to me. Could you add new tests that mark a two-part JWT as invalid?

@Prathamesh061
Copy link
Contributor Author

Thank you for your review and feedback on my pull request. I have made the necessary changes to address the issue and will be adding new tests to ensure that the code works as expected. Thank you again for your help!

Added test case for validating JSON web tokens (JWT)
Removed trailing spaces
@Prathamesh061
Copy link
Contributor Author

Hi WikiRiki,

I have added test cases for the isJWT validation. Could you please review the changes and let me know if there's anything else that needs to be done?

@WikiRik
Copy link
Member

WikiRik commented Apr 13, 2023

There are already some tests, could you change them here?

it('should validate JWT tokens', () => {
test({
validator: 'isJWT',
valid: [
'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJsb2dnZWRJbkFzIjoiYWRtaW4iLCJpYXQiOjE0MjI3Nzk2Mzh9.gzSraSYS8EXBxLN_oWnFSRgCzcmJmMjLiuyu5CSpyHI',
'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJsb3JlbSI6Imlwc3VtIn0.ymiJSsMJXR6tMSr8G9usjQ15_8hKPDv_CArLhxw28MI',
'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJkb2xvciI6InNpdCIsImFtZXQiOlsibG9yZW0iLCJpcHN1bSJdfQ.rRpe04zbWbbJjwM43VnHzAboDzszJtGrNsUxaqQ-GQ8',
'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJqb2huIjp7ImFnZSI6MjUsImhlaWdodCI6MTg1fSwiamFrZSI6eyJhZ2UiOjMwLCJoZWlnaHQiOjI3MH19.YRLPARDmhGMC3BBk_OhtwwK21PIkVCqQe8ncIRPKo-E',
'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ', // No signature
],
invalid: [
'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9',
'$Zs.ewu.su84',
'ks64$S/9.dy$§kz.3sd73b',
],
error: [
[],
{},
null,
undefined,
],
});
});

Refactor tests in isjwt and remove redundant test case
@codecov
Copy link

codecov bot commented Apr 14, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (9ba1735) 99.95% compared to head (8d050ed) 99.95%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2217   +/-   ##
=======================================
  Coverage   99.95%   99.95%           
=======================================
  Files         107      107           
  Lines        2405     2405           
  Branches      604      604           
=======================================
  Hits         2404     2404           
  Partials        1        1           
Impacted Files Coverage Δ
src/lib/isJWT.js 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Prathamesh061
Copy link
Contributor Author

Hello WikiRiki,
I have reviewed the code and made some modifications to the tests in isjwt as you suggested. Please take a look and let me know if there are any issues or if you have any additional feedback.

Copy link
Member

@WikiRik WikiRik left a comment

Choose a reason for hiding this comment

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

One final comment but then it looks good to me

'$Zs.ewu.su84',
'ks64$S/9.dy$§kz.3sd73b',
'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ',
Copy link
Member

Choose a reason for hiding this comment

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

I think this one is redundant, the first one test you added is already 2 parts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right

Removed redundant test in isJWT
@Prathamesh061
Copy link
Contributor Author

Now all seems to be good. Is there anything else that needs to be done? please let me know

@pano9000
Copy link
Contributor

@profnandaa could you kindly take a look here and merge this?

Thanks!

@@ -7,7 +7,7 @@ export default function isJWT(str) {
const dotSplit = str.split('.');
const len = dotSplit.length;

if (len > 3 || len < 2) {
if (len !== 3) {

Choose a reason for hiding this comment

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

consider adding reference link eg. https://jwt.io/introduction

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Chrislemus! for your valuable feedback.

Copy link

@chrislemus chrislemus left a comment

Choose a reason for hiding this comment

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

I suggest adding a reference link for posterity (eg. https://jwt.io/introduction). Otherwise LGTM!

@rubiin rubiin requested a review from profnandaa June 12, 2023 04:57
Copy link
Member

@profnandaa profnandaa left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fix.

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.

6 participants