fix(verify)!: Remove default `none` support `verify` methods, and require it to be explicitly configured #851

jakelacey2012 · 2022-11-10T16:51:56Z

Description

This PR removes default support for the none algorithm verify methods, now you have to specify none before you can sign and verify. This is to fix the issue raised in #711, but still allow users to use none if they wish to do so for development/testing.

BREAKING CHANGE: Removes fallback for none algorithm for the verify method.

References

RFC confrontation on "none" algo #711

Testing

Please download this version of the package and then run the following code examples, which you can do by running [email protected]:jakelacey2012/node-jsonwebtoken.git#IPS-2481.

Simple test to get some background, should not throw... should not be effected by this change.

var jwt = require('jsonwebtoken');
const decoded = jwt.verify(jwt.sign({ foo: 'bar' }, 'sshhhhhh'), 'sshhhhhh');
// should decode and not error.

Test sign method with algorithm `none` option, should not error and return unsigned token.

var jwt = require('jsonwebtoken');
const sign_none = jwt.sign({ foo: 'bar' }, undefined, { algorithm: 'none' });
// return unsigned token.

Test sign method with algorithm `none`, and not specify `none` in options.

var jwt = require('jsonwebtoken');

jwt.sign({ foo: 'bar' }, undefined, { }); 
// this returns an error `secretOrPrivateKey must have a value`

jwt.sign({ foo: 'bar' }, 'secret', { }); 
// no error returned but token returned is signed with HS256

Testing verifying without `none` specified, should throw error `please specify "none" in "algorithms" to verify unsigned tokens`

const unsigned = jwt.sign({ foo: 'bar' }, 'secret', { algorithm: 'none' });
const decoded = jwt.verify(unsigned, undefined, { algorithms: [] });
// should throw error 'please specify "none" in "algorithms" to verify unsigned tokens'

Testing verifying with `none` specified, should not throw and return a decoded token.

const unsigned = jwt.sign({ foo: 'bar' }, 'secret', { algorithm: 'none' });
const decoded = jwt.verify(unsigned, undefined, { algorithms: ['none'] });
// should not throw and return a decoded token.

Describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. If this library has unit and/or integration testing, tests should be added for new functionality and existing tests should complete without errors.

Please include any manual steps for testing end-to-end or functionality not covered by unit/integration tests.

Also include details of the environment this PR was developed in (language/platform/browser version).

This change adds test coverage for new/changed/fixed functionality

Checklist

I have added documentation for new/changed functionality in this PR or in auth0.com/docs
All active GitHub checks for tests, formatting, and security are passing
The correct base branch is being used, if not the default branch

david-renaud-okta · 2022-11-14T20:14:37Z

Is this test still necessary? I think we should enable (with appropriate modifications) or remove this test as part of this change.

https://github.com/jakelacey2012/node-jsonwebtoken/blob/IPS-2481/test/async_sign.tests.js#L44-L52

david-renaud-okta · 2022-11-14T19:55:50Z

test/async_sign.tests.js

@@ -34,7 +34,7 @@ describe('signing a token asynchronously', function() {
    });

    it('should work with none algorithm where secret is set', function(done) {


Test description does not match anymore. Perhaps we should we flip this and assert an error when the none algorithm is specified.

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

…ethods, and require it to be explicitly configured BREAKING CHANGE: Removes fallback for none algorithm for the verify method.

CharlesRea · 2022-11-16T15:44:12Z

verify.js

+
+

Intentional whitespace?

CharlesRea · 2022-11-18T10:39:14Z

README.md

@@ -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.


Is this comment necessary? Since algorithm must be supplied, there's no danger of someone unintentionally creating an unsigned token

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.

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.

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.

CharlesRea · 2022-11-18T11:19:44Z

test/verify.tests.js

      encoding: 'utf8'
    });

-    jwt.verify(signed, null, {typ: 'JWT'}, function(err, p) {
+    expect(function () {
+      jwt.verify(signed, 'secret', {typ: 'JWT'});


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.

CharlesRea · 2022-11-18T11:28:04Z

verify.js

-    if (!hasSignature && !options.algorithms) {
-      options.algorithms = ['none'];
-    }


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')); }

Good catch, yeah what you've said makes sense let's move the check there.

CharlesRea · 2022-11-18T11:50:16Z

test/async_sign.tests.js

@@ -41,16 +41,6 @@ describe('signing a token asynchronously', function() {
      });
    });

-    //Known bug: https://github.com/brianloveswords/node-jws/issues/62


Did we mean to remove this test?

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.

CharlesRea · 2022-11-18T11:50:40Z

test/claim-aud.test.js

@@ -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'};


We may be able to revert all these test changes with the latest change to sign. What do you think?

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 :)

Yep agreed :)

…cret is undefined

david-renaud-okta suggested changes Nov 14, 2022

View reviewed changes

jakelacey2012 marked this pull request as ready for review November 15, 2022 09:22

david-renaud-okta approved these changes Nov 15, 2022

View reviewed changes

jakelacey2012 changed the title ~~fix(sign&verify): Remove none support from sign and verify methods~~ fix(sign&verify): Remove default none support from sign and verify methods, and require is explicitly. Nov 18, 2022

jakelacey2012 changed the title ~~fix(sign&verify): Remove default none support from sign and verify methods, and require is explicitly.~~ fix(sign&verify): Remove default none support from sign and verify methods, and require it to be explicitly configured Nov 18, 2022

jakelacey2012 changed the title ~~fix(sign&verify): Remove default none support from sign and verify methods, and require it to be explicitly configured~~ fix(sign&verify)!: Remove default none support from sign and verify methods, and require it to be explicitly configured Nov 18, 2022

fix(sign&verify)!: Remove default none support from sign and verify m…

1a95784

…ethods, and require it to be explicitly configured BREAKING CHANGE: Removes fallback for none algorithm for the verify method.

jakelacey2012 force-pushed the IPS-2481 branch from 7d3ec02 to 1a95784 Compare November 18, 2022 09:48

CharlesRea reviewed Nov 18, 2022

View reviewed changes

jakelacey2012 added 6 commits November 18, 2022 14:37

fix: add back old test

4021b3c

chore: white space

d46a1f6

fix: secretOrPublicKey toString() of undefined

5705a41

fix: add coverage should not be able to verify unsigned token when se…

32ca143

…cret is undefined

fix: add back comment

6ab90a5

chore: remove none documentation

c673e4f

CharlesRea approved these changes Nov 28, 2022

View reviewed changes

jakelacey2012 mentioned this pull request Nov 29, 2022

chore: add migration guide link to readme for v9 #855

Open

3 tasks

jakelacey2012 merged commit 8345030 into auth0:master Nov 29, 2022

jakelacey2012 deleted the IPS-2481 branch November 29, 2022 14:32

jakelacey2012 mentioned this pull request Nov 29, 2022

RFC confrontation on "none" algo #711

Closed

jakelacey2012 changed the title ~~fix(sign&verify)!: Remove default none support from sign and verify methods, and require it to be explicitly configured~~ fix(verify)!: Remove default none support verify methods, and require it to be explicitly configured Dec 1, 2022

bsoufflet mentioned this pull request Jan 25, 2023

chore(deps): bump jsonwebtoken from 8.5.1 to 9.0.0 GrottoCenter/grottocenter-api#1091

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(verify)!: Remove default `none` support `verify` methods, and require it to be explicitly configured #851

fix(verify)!: Remove default `none` support `verify` methods, and require it to be explicitly configured #851

jakelacey2012 commented Nov 10, 2022 •

edited

Loading

david-renaud-okta commented Nov 14, 2022

david-renaud-okta Nov 14, 2022

jakelacey2012 Nov 15, 2022

CharlesRea Nov 16, 2022

CharlesRea Nov 18, 2022

jakelacey2012 Nov 18, 2022

CharlesRea Nov 28, 2022

jakelacey2012 Nov 28, 2022

CharlesRea Nov 18, 2022

CharlesRea Nov 18, 2022

jakelacey2012 Nov 18, 2022

CharlesRea Nov 18, 2022

jakelacey2012 Nov 18, 2022

CharlesRea Nov 18, 2022

jakelacey2012 Nov 18, 2022

CharlesRea Nov 28, 2022

		@@ -34,7 +34,7 @@ describe('signing a token asynchronously', function() {
		});

		it('should work with none algorithm where secret is set', function(done) {

fix(verify)!: Remove default none support verify methods, and require it to be explicitly configured #851

fix(verify)!: Remove default none support verify methods, and require it to be explicitly configured #851

Conversation

jakelacey2012 commented Nov 10, 2022 • edited Loading

Description

References

Testing

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 specify none in options.

Testing verifying without none specified, should throw error please specify "none" in "algorithms" to verify unsigned tokens

Testing verifying with none specified, should not throw and return a decoded token.

Checklist

david-renaud-okta commented Nov 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fix(verify)!: Remove default `none` support `verify` methods, and require it to be explicitly configured #851

fix(verify)!: Remove default `none` support `verify` methods, and require it to be explicitly configured #851

jakelacey2012 commented Nov 10, 2022 •

edited

Loading

Test sign method with algorithm `none` option, should not error and return unsigned token.

Test sign method with algorithm `none`, and not specify `none` in options.

Testing verifying without `none` specified, should throw error `please specify "none" in "algorithms" to verify unsigned tokens`

Testing verifying with `none` specified, should not throw and return a decoded token.