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

No check needed. Parsing will throw an error if base64 is not valid #1

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

levinunnink
Copy link

We don't actually need to run the regex check on the base64 string.

  1. It was failing on a valid base64 string for me.
  2. The window.atob or the Buffer.from will both throw errors if the string is invalid and the parseJwtToken will return false.

By removing this, we don't get any invalid results on valid jwt tokens.

@michaelzoidl
Copy link
Member

Hey @levinunnink thanks for the PR, your right, thanks for removing the function (i was anyway not happy with this big and complex regex)

Could you also remove the isValidBase64 tests from the test.js file? :) After that i will update some dependencies and deploy a new version 🎉

@levinunnink
Copy link
Author

@michaelzoidl Whoops! I totally missed the test coverage. It should be fixed now.

@coveralls
Copy link

coveralls commented May 3, 2018

Coverage Status

Coverage decreased (-0.2%) to 89.286% when pulling 2b67833 on levinunnink:master into d951c19 on entwicklerstube:master.

@michaelzoidl
Copy link
Member

Wow, totally forgot this PR.

I've seen you published your own version with the name is-jwt-valid right? Do you want still to merge your version into this one?

Alternative i could mention your repository in the README of this one.

@levinunnink
Copy link
Author

@michaelzoidl No worries! I just need to ship this fix into production so I forked it. I'm fine with whatever you want to do. :-)

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.

3 participants