-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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(verify)!: Remove default none
support verify
methods, and require it to be explicitly configured
#851
Conversation
Is this test still necessary? I think we should enable (with appropriate modifications) or remove this test as part of this change. |
test/async_sign.tests.js
Outdated
@@ -34,7 +34,7 @@ describe('signing a token asynchronously', function() { | |||
}); | |||
|
|||
it('should work with none algorithm where secret is set', function(done) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test description does not match anymore. Perhaps we should we flip this and assert an error when the none
algorithm is specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh good catch actually I think we can remove this test because we already test this as part of the schema here.
https://github.com/auth0/node-jsonwebtoken/blob/d3b8c1d1538800f5fa9536a6419e0ec8761d193b/test/schema.tests.js#L22-L24
none
support from sign
and verify
methodsnone
support from sign
and verify
methods, and require is explicitly.
none
support from sign
and verify
methods, and require is explicitly.none
support from sign
and verify
methods, and require it to be explicitly configured
none
support from sign
and verify
methods, and require it to be explicitly configurednone
support from sign
and verify
methods, and require it to be explicitly configured
…ethods, and require it to be explicitly configured BREAKING CHANGE: Removes fallback for none algorithm for the verify method.
7d3ec02
to
1a95784
Compare
verify.js
Outdated
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intentional whitespace?
README.md
Outdated
@@ -38,6 +38,7 @@ encoded private key for RSA and ECDSA. In case of a private key with passphrase | |||
`options`: | |||
|
|||
* `algorithm` (default: `HS256`) | |||
* * `none` MUST be configured in order to create unsigned tokens. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this comment necessary? Since algorithm
must be supplied, there's no danger of someone unintentionally creating an unsigned token
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats true, this sign method has always been explicit with the algorithm - my reasoning was mainly to make this as easily as possible for new people adopting the library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we want to make it easy to create none
tokens :P I'd remove this personally, I think it adds confusion to someone wanting to use the library who isn't hugely experienced with best practices here - let's not call out none
specifically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah maybe if this documentation was to exist it would need to be included in a best practises section, which describes that none
tokens should only be used for testing and not for production. Removing this to avoid confusing users sounds fine to me.
encoding: 'utf8' | ||
}); | ||
|
||
jwt.verify(signed, null, {typ: 'JWT'}, function(err, p) { | ||
expect(function () { | ||
jwt.verify(signed, 'secret', {typ: 'JWT'}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This case would already return an error before your changes, as you're passing in a secret and we do this check:
if (!hasSignature && secretOrPublicKey){
return done(new JsonWebTokenError('jwt signature is required'));
}
We should have a test for both cases where secret is provided, or is null.
verify.js
Outdated
if (!hasSignature && !options.algorithms) { | ||
options.algorithms = ['none']; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If secretOrPublicKey
and options.algorithms
are both null / undefined, line 116 will error. This was previously not possible as if secretOrPublicKey
is undefined then hasSignature
is guaranteed to be false (per check inline 111), which means options.algorithms
was always set to [none]
.
If alg === none
then we'll have already failed, but if alg !== none
then we'll get an can't read toString() of undefined
error. It is correct that the token isn't valid in that case, but we should return an actual invalid token error (from passing this through to jws.verify
).
IMO, we should also avoid changing the error message unnecessarily if we were previously going to be caught by the two checks above.
So what do you think about removing the check you've added on line 83, and putting the check here instead with the old condition. This will still prevent none
algs being verified without being specified, as we no longer default to none
in this case:
if (!hasSignature && !options.algorithms) {
return done(new JsonWebTokenError('specify "none" in "algorithms" to verify unsigned tokens'));
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, yeah what you've said makes sense let's move the check there.
test/async_sign.tests.js
Outdated
@@ -41,16 +41,6 @@ describe('signing a token asynchronously', function() { | |||
}); | |||
}); | |||
|
|||
//Known bug: https://github.com/brianloveswords/node-jws/issues/62 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we mean to remove this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something I changed as part of David's comment and was changed when we were removing the none
functionality all together, since we're not doing that anymore I think we can just add this back in.
@@ -6,7 +6,7 @@ const util = require('util'); | |||
const testUtils = require('./test-utils'); | |||
|
|||
function signWithAudience(audience, payload, callback) { | |||
const options = {algorithm: 'none'}; | |||
const options = {algorithm: 'HS256'}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may be able to revert all these test changes with the latest change to sign
. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would be able to revert but the places that we use none and verify we would need to configure that algorithm explicitly, do you see an advantage in using none
in these tests or was comment more about making the PR as small as possible?
I think I prefer using HS256
for example rather than none
, since its something we don't want to encourage and should be used in exceptional cases. Also means in the tests we just configure the algorithm, the secret and no options for the verify - the alternative seems verbose and repetitive IMO.
Let me know what you think :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep agreed :)
none
support from sign
and verify
methods, and require it to be explicitly configurednone
support verify
methods, and require it to be explicitly configured
Description
This PR removes default support for the
none
algorithmverify
methods, now you have to specifynone
before you cansign
andverify
. This is to fix the issue raised in #711, but still allow users to usenone
if they wish to do so for development/testing.BREAKING CHANGE: Removes fallback for
none
algorithm for theverify
method.References
Testing
Please download this version of the package and then run the following code examples, which you can do by running
git@github.com:jakelacey2012/node-jsonwebtoken.git#IPS-2481
.Simple test to get some background, should not throw... should not be effected by this change.
Test sign method with algorithm
none
option, should not error and return unsigned token.Test sign method with algorithm
none
, and not specifynone
in options.Testing verifying without
none
specified, should throw errorplease specify "none" in "algorithms" to verify unsigned tokens
Testing verifying with
none
specified, should not throw and return a decoded token.Checklist