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 for passing Buffer object as secretOrPublicKey #47

Merged
merged 5 commits into from
Mar 15, 2019
Merged

fix for passing Buffer object as secretOrPublicKey #47

merged 5 commits into from
Mar 15, 2019

Conversation

lwojcik
Copy link
Contributor

@lwojcik lwojcik commented Mar 12, 2019

Hi!

While moving one of my projects from Express to Fastify I noticed fastify-jwt doesn't handle Buffer objects passed as options.secret in a correct way, causing JWT verification attempts to fail. I'm working with a Twitch.tv extension and it expects me to verify tokens with Buffer objects. I was able to do it with jsonwebtoken and Buffer.from(...), but fastify-jwt throws Error('missing private key and/or public key').

I made a quick fix that causes the project to pass Buffer objects directly to secretOrPublicKey variable, making it possible to successfuly verify JWTs in cases like the one I described above.

Alternatively, !Buffer.isBuffer(secret) can also be used as a condition.

Copy link
Contributor

@cemremengu cemremengu left a comment

Choose a reason for hiding this comment

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

Thanks! Can you please also add a test for this?

@lwojcik
Copy link
Contributor Author

lwojcik commented Mar 13, 2019

Added support for signing tokens with a Buffer object and relevant tests. Please let me know if that works.

jwt.js Outdated
@@ -53,15 +53,17 @@ function fastifyJwt (fastify, options, next) {
signOptions &&
signOptions.algorithm &&
signOptions.algorithm.includes('RS') &&
typeof secret === 'string'
(typeof secret === 'string' ||
Buffer.isBuffer(secret))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Buffer.isBuffer(secret))
secret instanceof Buffer)

Copy link
Member

Choose a reason for hiding this comment

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

@mcollina why?

jwt.js Outdated Show resolved Hide resolved
Co-Authored-By: lwojcik <1711174+lwojcik@users.noreply.github.com>
Copy link
Contributor

@cemremengu cemremengu left a comment

Choose a reason for hiding this comment

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

LGTM

@leodutra
Copy link

leodutra commented Mar 15, 2019

Buffer enables byte array to be used as a secret, including base64 encoding.

On the readme of node-jsonwebtoken, search for "base64".
You will find a link for this comment about security

This is an essential feature of jwt.sign...

Please review

@cemremengu
Copy link
Contributor

@mcollina I will patch this in if you are good to go

@mcollina
Copy link
Member

mcollina commented Mar 15, 2019 via email

@cemremengu cemremengu merged commit aa126d4 into fastify:master Mar 15, 2019
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.

5 participants