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

[jakelacey2012]

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

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]

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]

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

[jakelacey2012]

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

[CharlesRea]

Intentional whitespace?

[CharlesRea]

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

[jakelacey2012]

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.

[CharlesRea]

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.

[jakelacey2012]

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]

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]

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

[jakelacey2012]

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

[CharlesRea]

Did we mean to remove this test?

[jakelacey2012]

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]

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

[jakelacey2012]

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

[CharlesRea]

Yep agreed :)