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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Nov 29, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1a95784
fix(sign&verify)!: Remove default none support from sign and verify m…
jakelacey2012 Nov 10, 2022
4021b3c
fix: add back old test
jakelacey2012 Nov 18, 2022
d46a1f6
chore: white space
jakelacey2012 Nov 18, 2022
5705a41
fix: secretOrPublicKey toString() of undefined
jakelacey2012 Nov 18, 2022
32ca143
fix: add coverage should not be able to verify unsigned token when se…
jakelacey2012 Nov 18, 2022
6ab90a5
fix: add back comment
jakelacey2012 Nov 18, 2022
c673e4f
chore: remove none documentation
jakelacey2012 Nov 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions test/claim-aud.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'};
Copy link

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?

Copy link
Contributor Author

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

Copy link

Choose a reason for hiding this comment

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

Yep agreed :)

if (audience !== undefined) {
options.audience = audience;
}
Expand All @@ -15,7 +15,7 @@ function signWithAudience(audience, payload, callback) {
}

function verifyWithAudience(token, audience, callback) {
testUtils.verifyJWTHelper(token, undefined, {audience}, callback);
testUtils.verifyJWTHelper(token, 'secret', {audience}, callback);
}

describe('audience', function() {
Expand Down Expand Up @@ -47,7 +47,7 @@ describe('audience', function() {

// undefined needs special treatment because {} is not the same as {aud: undefined}
it('should error with with value undefined', function (done) {
testUtils.signJWTHelper({}, 'secret', {audience: undefined, algorithm: 'none'}, (err) => {
testUtils.signJWTHelper({}, 'secret', {audience: undefined, algorithm: 'HS256'}, (err) => {
testUtils.asyncCheck(done, () => {
expect(err).to.be.instanceOf(Error);
expect(err).to.have.property('message', '"audience" must be a string or array');
Expand Down
39 changes: 19 additions & 20 deletions test/claim-exp.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,10 @@ const expect = require('chai').expect;
const sinon = require('sinon');
const util = require('util');
const testUtils = require('./test-utils');

const base64UrlEncode = testUtils.base64UrlEncode;
const noneAlgorithmHeader = 'eyJhbGciOiJub25lIiwidHlwIjoiSldUIn0';
const jws = require('jws');

function signWithExpiresIn(expiresIn, payload, callback) {
const options = {algorithm: 'none'};
const options = {algorithm: 'HS256'};
if (expiresIn !== undefined) {
options.expiresIn = expiresIn;
}
Expand Down Expand Up @@ -49,7 +47,7 @@ describe('expires', function() {

// undefined needs special treatment because {} is not the same as {expiresIn: undefined}
it('should error with with value undefined', function (done) {
testUtils.signJWTHelper({}, undefined, {expiresIn: undefined, algorithm: 'none'}, (err) => {
testUtils.signJWTHelper({}, 'secret', {expiresIn: undefined, algorithm: 'HS256'}, (err) => {
testUtils.asyncCheck(done, () => {
expect(err).to.be.instanceOf(Error);
expect(err).to.have.property(
Expand Down Expand Up @@ -133,9 +131,10 @@ describe('expires', function() {
{foo: 'bar'},
].forEach((exp) => {
it(`should error with with value ${util.inspect(exp)}`, function (done) {
const encodedPayload = base64UrlEncode(JSON.stringify({exp}));
const token = `${noneAlgorithmHeader}.${encodedPayload}.`;
testUtils.verifyJWTHelper(token, undefined, {exp}, (err) => {
const header = { alg: 'HS256' };
const payload = { exp };
const token = jws.sign({ header, payload, secret: 'secret', encoding: 'utf8' });
testUtils.verifyJWTHelper(token, 'secret', { exp }, (err) => {
testUtils.asyncCheck(done, () => {
expect(err).to.be.instanceOf(jwt.JsonWebTokenError);
expect(err).to.have.property('message', 'invalid exp value');
Expand All @@ -158,7 +157,7 @@ describe('expires', function() {
it('should set correct "exp" with negative number of seconds', function(done) {
signWithExpiresIn(-10, {}, (e1, token) => {
fakeClock.tick(-10001);
testUtils.verifyJWTHelper(token, undefined, {}, (e2, decoded) => {
testUtils.verifyJWTHelper(token, 'secret', {}, (e2, decoded) => {
testUtils.asyncCheck(done, () => {
expect(e1).to.be.null;
expect(e2).to.be.null;
Expand All @@ -170,7 +169,7 @@ describe('expires', function() {

it('should set correct "exp" with positive number of seconds', function(done) {
signWithExpiresIn(10, {}, (e1, token) => {
testUtils.verifyJWTHelper(token, undefined, {}, (e2, decoded) => {
testUtils.verifyJWTHelper(token, 'secret', {}, (e2, decoded) => {
testUtils.asyncCheck(done, () => {
expect(e1).to.be.null;
expect(e2).to.be.null;
Expand All @@ -183,7 +182,7 @@ describe('expires', function() {
it('should set correct "exp" with zero seconds', function(done) {
signWithExpiresIn(0, {}, (e1, token) => {
fakeClock.tick(-1);
testUtils.verifyJWTHelper(token, undefined, {}, (e2, decoded) => {
testUtils.verifyJWTHelper(token, 'secret', {}, (e2, decoded) => {
testUtils.asyncCheck(done, () => {
expect(e1).to.be.null;
expect(e2).to.be.null;
Expand All @@ -196,7 +195,7 @@ describe('expires', function() {
it('should set correct "exp" with negative string timespan', function(done) {
signWithExpiresIn('-10 s', {}, (e1, token) => {
fakeClock.tick(-10001);
testUtils.verifyJWTHelper(token, undefined, {}, (e2, decoded) => {
testUtils.verifyJWTHelper(token, 'secret', {}, (e2, decoded) => {
testUtils.asyncCheck(done, () => {
expect(e1).to.be.null;
expect(e2).to.be.null;
Expand All @@ -209,7 +208,7 @@ describe('expires', function() {
it('should set correct "exp" with positive string timespan', function(done) {
signWithExpiresIn('10 s', {}, (e1, token) => {
fakeClock.tick(-10001);
testUtils.verifyJWTHelper(token, undefined, {}, (e2, decoded) => {
testUtils.verifyJWTHelper(token, 'secret', {}, (e2, decoded) => {
testUtils.asyncCheck(done, () => {
expect(e1).to.be.null;
expect(e2).to.be.null;
Expand All @@ -222,7 +221,7 @@ describe('expires', function() {
it('should set correct "exp" with zero string timespan', function(done) {
signWithExpiresIn('0 s', {}, (e1, token) => {
fakeClock.tick(-1);
testUtils.verifyJWTHelper(token, undefined, {}, (e2, decoded) => {
testUtils.verifyJWTHelper(token, 'secret', {}, (e2, decoded) => {
testUtils.asyncCheck(done, () => {
expect(e1).to.be.null;
expect(e2).to.be.null;
Expand Down Expand Up @@ -267,7 +266,7 @@ describe('expires', function() {

it('should set correct "exp" when "iat" is passed', function (done) {
signWithExpiresIn(-10, {iat: 80}, (e1, token) => {
testUtils.verifyJWTHelper(token, undefined, {}, (e2, decoded) => {
testUtils.verifyJWTHelper(token, 'secret', {}, (e2, decoded) => {
testUtils.asyncCheck(done, () => {
expect(e1).to.be.null;
expect(e2).to.be.null;
Expand All @@ -279,7 +278,7 @@ describe('expires', function() {

it('should verify "exp" using "clockTimestamp"', function (done) {
signWithExpiresIn(10, {}, (e1, token) => {
testUtils.verifyJWTHelper(token, undefined, {clockTimestamp: 69}, (e2, decoded) => {
testUtils.verifyJWTHelper(token, 'secret', {clockTimestamp: 69}, (e2, decoded) => {
testUtils.asyncCheck(done, () => {
expect(e1).to.be.null;
expect(e2).to.be.null;
Expand All @@ -293,7 +292,7 @@ describe('expires', function() {
it('should verify "exp" using "clockTolerance"', function (done) {
signWithExpiresIn(5, {}, (e1, token) => {
fakeClock.tick(10000);
testUtils.verifyJWTHelper(token, undefined, {clockTimestamp: 6}, (e2, decoded) => {
testUtils.verifyJWTHelper(token, 'secret', {clockTimestamp: 6}, (e2, decoded) => {
testUtils.asyncCheck(done, () => {
expect(e1).to.be.null;
expect(e2).to.be.null;
Expand All @@ -306,7 +305,7 @@ describe('expires', function() {

it('should ignore a expired token when "ignoreExpiration" is true', function (done) {
signWithExpiresIn('-10 s', {}, (e1, token) => {
testUtils.verifyJWTHelper(token, undefined, {ignoreExpiration: true}, (e2, decoded) => {
testUtils.verifyJWTHelper(token, 'secret', {ignoreExpiration: true}, (e2, decoded) => {
testUtils.asyncCheck(done, () => {
expect(e1).to.be.null;
expect(e2).to.be.null;
Expand All @@ -319,7 +318,7 @@ describe('expires', function() {

it('should error on verify if "exp" is at current time', function(done) {
signWithExpiresIn(undefined, {exp: 60}, (e1, token) => {
testUtils.verifyJWTHelper(token, undefined, {}, (e2) => {
testUtils.verifyJWTHelper(token, 'secret', {}, (e2) => {
testUtils.asyncCheck(done, () => {
expect(e1).to.be.null;
expect(e2).to.be.instanceOf(jwt.TokenExpiredError);
Expand All @@ -331,7 +330,7 @@ describe('expires', function() {

it('should error on verify if "exp" is before current time using clockTolerance', function (done) {
signWithExpiresIn(-5, {}, (e1, token) => {
testUtils.verifyJWTHelper(token, undefined, {clockTolerance: 5}, (e2) => {
testUtils.verifyJWTHelper(token, 'secret', {clockTolerance: 5}, (e2) => {
testUtils.asyncCheck(done, () => {
expect(e1).to.be.null;
expect(e2).to.be.instanceOf(jwt.TokenExpiredError);
Expand Down
31 changes: 15 additions & 16 deletions test/claim-iat.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,24 +5,22 @@ const expect = require('chai').expect;
const sinon = require('sinon');
const util = require('util');
const testUtils = require('./test-utils');

const base64UrlEncode = testUtils.base64UrlEncode;
const noneAlgorithmHeader = 'eyJhbGciOiJub25lIiwidHlwIjoiSldUIn0';
const jws = require('jws');

function signWithIssueAt(issueAt, options, callback) {
const payload = {};
if (issueAt !== undefined) {
payload.iat = issueAt;
}
const opts = Object.assign({algorithm: 'none'}, options);
const opts = Object.assign({algorithm: 'HS256'}, options);
// async calls require a truthy secret
// see: https://github.com/brianloveswords/node-jws/issues/62
testUtils.signJWTHelper(payload, 'secret', opts, callback);
}

function verifyWithIssueAt(token, maxAge, options, callback) {
function verifyWithIssueAt(token, maxAge, options, secret, callback) {
const opts = Object.assign({maxAge}, options);
testUtils.verifyJWTHelper(token, undefined, opts, callback);
testUtils.verifyJWTHelper(token, secret, opts, callback);
}

describe('issue at', function() {
Expand Down Expand Up @@ -50,7 +48,7 @@ describe('issue at', function() {

// undefined needs special treatment because {} is not the same as {iat: undefined}
it('should error with iat of undefined', function (done) {
testUtils.signJWTHelper({iat: undefined}, 'secret', {algorithm: 'none'}, (err) => {
testUtils.signJWTHelper({iat: undefined}, 'secret', {algorithm: 'HS256'}, (err) => {
testUtils.asyncCheck(done, () => {
expect(err).to.be.instanceOf(Error);
expect(err.message).to.equal('"iat" should be a number of seconds');
Expand All @@ -76,9 +74,10 @@ describe('issue at', function() {
{foo: 'bar'},
].forEach((iat) => {
it(`should error with iat of ${util.inspect(iat)}`, function (done) {
const encodedPayload = base64UrlEncode(JSON.stringify({iat}));
const token = `${noneAlgorithmHeader}.${encodedPayload}.`;
verifyWithIssueAt(token, '1 min', {}, (err) => {
const header = { alg: 'HS256' };
const payload = { iat };
const token = jws.sign({ header, payload, secret: 'secret', encoding: 'utf8' });
verifyWithIssueAt(token, '1 min', {}, 'secret', (err) => {
testUtils.asyncCheck(done, () => {
expect(err).to.be.instanceOf(jwt.JsonWebTokenError);
expect(err.message).to.equal('iat required when maxAge is specified');
Expand Down Expand Up @@ -188,9 +187,9 @@ describe('issue at', function() {
},
].forEach((testCase) => {
it(testCase.description, function (done) {
const token = jwt.sign({}, 'secret', {algorithm: 'none'});
const token = jwt.sign({}, 'secret', {algorithm: 'HS256'});
fakeClock.tick(testCase.clockAdvance);
verifyWithIssueAt(token, testCase.maxAge, testCase.options, (err, token) => {
verifyWithIssueAt(token, testCase.maxAge, testCase.options, 'secret', (err, token) => {
testUtils.asyncCheck(done, () => {
expect(err).to.be.null;
expect(token).to.be.a('object');
Expand Down Expand Up @@ -235,10 +234,10 @@ describe('issue at', function() {
].forEach((testCase) => {
it(testCase.description, function(done) {
const expectedExpiresAtDate = new Date(testCase.expectedExpiresAt);
const token = jwt.sign({}, 'secret', {algorithm: 'none'});
const token = jwt.sign({}, 'secret', {algorithm: 'HS256'});
fakeClock.tick(testCase.clockAdvance);

verifyWithIssueAt(token, testCase.maxAge, testCase.options, (err) => {
verifyWithIssueAt(token, testCase.maxAge, testCase.options, 'secret', (err) => {
testUtils.asyncCheck(done, () => {
expect(err).to.be.instanceOf(jwt.JsonWebTokenError);
expect(err.message).to.equal(testCase.expectedError);
Expand All @@ -252,7 +251,7 @@ describe('issue at', function() {
describe('with string payload', function () {
it('should not add iat to string', function (done) {
const payload = 'string payload';
const options = {algorithm: 'none'};
const options = {algorithm: 'HS256'};
testUtils.signJWTHelper(payload, 'secret', options, (err, token) => {
const decoded = jwt.decode(token);
testUtils.asyncCheck(done, () => {
Expand All @@ -264,7 +263,7 @@ describe('issue at', function() {

it('should not add iat to stringified object', function (done) {
const payload = '{}';
const options = {algorithm: 'none', header: {typ: 'JWT'}};
const options = {algorithm: 'HS256', header: {typ: 'JWT'}};
testUtils.signJWTHelper(payload, 'secret', options, (err, token) => {
const decoded = jwt.decode(token);
testUtils.asyncCheck(done, () => {
Expand Down
22 changes: 11 additions & 11 deletions test/claim-iss.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ const util = require('util');
const testUtils = require('./test-utils');

function signWithIssuer(issuer, payload, callback) {
const options = {algorithm: 'none'};
const options = {algorithm: 'HS256'};
if (issuer !== undefined) {
options.issuer = issuer;
}
Expand Down Expand Up @@ -44,7 +44,7 @@ describe('issuer', function() {

// undefined needs special treatment because {} is not the same as {issuer: undefined}
it('should error with with value undefined', function (done) {
testUtils.signJWTHelper({}, undefined, {issuer: undefined, algorithm: 'none'}, (err) => {
testUtils.signJWTHelper({}, 'secret', {issuer: undefined, algorithm: 'HS256'}, (err) => {
testUtils.asyncCheck(done, () => {
expect(err).to.be.instanceOf(Error);
expect(err).to.have.property('message', '"issuer" must be a string');
Expand Down Expand Up @@ -92,7 +92,7 @@ describe('issuer', function() {
describe('when signing and verifying a token', function () {
it('should not verify "iss" if verify "issuer" option not provided', function(done) {
signWithIssuer(undefined, {iss: 'foo'}, (e1, token) => {
testUtils.verifyJWTHelper(token, undefined, {}, (e2, decoded) => {
testUtils.verifyJWTHelper(token, 'secret', {}, (e2, decoded) => {
testUtils.asyncCheck(done, () => {
expect(e1).to.be.null;
expect(e2).to.be.null;
Expand All @@ -105,7 +105,7 @@ describe('issuer', function() {
describe('with string "issuer" option', function () {
it('should verify with a string "issuer"', function (done) {
signWithIssuer('foo', {}, (e1, token) => {
testUtils.verifyJWTHelper(token, undefined, {issuer: 'foo'}, (e2, decoded) => {
testUtils.verifyJWTHelper(token, 'secret', {issuer: 'foo'}, (e2, decoded) => {
testUtils.asyncCheck(done, () => {
expect(e1).to.be.null;
expect(e2).to.be.null;
Expand All @@ -117,7 +117,7 @@ describe('issuer', function() {

it('should verify with a string "iss"', function (done) {
signWithIssuer(undefined, {iss: 'foo'}, (e1, token) => {
testUtils.verifyJWTHelper(token, undefined, {issuer: 'foo'}, (e2, decoded) => {
testUtils.verifyJWTHelper(token, 'secret', {issuer: 'foo'}, (e2, decoded) => {
testUtils.asyncCheck(done, () => {
expect(e1).to.be.null;
expect(e2).to.be.null;
Expand All @@ -129,7 +129,7 @@ describe('issuer', function() {

it('should error if "iss" does not match verify "issuer" option', function(done) {
signWithIssuer(undefined, {iss: 'foobar'}, (e1, token) => {
testUtils.verifyJWTHelper(token, undefined, {issuer: 'foo'}, (e2) => {
testUtils.verifyJWTHelper(token, 'secret', {issuer: 'foo'}, (e2) => {
testUtils.asyncCheck(done, () => {
expect(e1).to.be.null;
expect(e2).to.be.instanceOf(jwt.JsonWebTokenError);
Expand All @@ -141,7 +141,7 @@ describe('issuer', function() {

it('should error without "iss" and with verify "issuer" option', function(done) {
signWithIssuer(undefined, {}, (e1, token) => {
testUtils.verifyJWTHelper(token, undefined, {issuer: 'foo'}, (e2) => {
testUtils.verifyJWTHelper(token, 'secret', {issuer: 'foo'}, (e2) => {
testUtils.asyncCheck(done, () => {
expect(e1).to.be.null;
expect(e2).to.be.instanceOf(jwt.JsonWebTokenError);
Expand All @@ -155,7 +155,7 @@ describe('issuer', function() {
describe('with array "issuer" option', function () {
it('should verify with a string "issuer"', function (done) {
signWithIssuer('bar', {}, (e1, token) => {
testUtils.verifyJWTHelper(token, undefined, {issuer: ['foo', 'bar']}, (e2, decoded) => {
testUtils.verifyJWTHelper(token, 'secret', {issuer: ['foo', 'bar']}, (e2, decoded) => {
testUtils.asyncCheck(done, () => {
expect(e1).to.be.null;
expect(e2).to.be.null;
Expand All @@ -167,7 +167,7 @@ describe('issuer', function() {

it('should verify with a string "iss"', function (done) {
signWithIssuer(undefined, {iss: 'foo'}, (e1, token) => {
testUtils.verifyJWTHelper(token, undefined, {issuer: ['foo', 'bar']}, (e2, decoded) => {
testUtils.verifyJWTHelper(token, 'secret', {issuer: ['foo', 'bar']}, (e2, decoded) => {
testUtils.asyncCheck(done, () => {
expect(e1).to.be.null;
expect(e2).to.be.null;
Expand All @@ -179,7 +179,7 @@ describe('issuer', function() {

it('should error if "iss" does not match verify "issuer" option', function(done) {
signWithIssuer(undefined, {iss: 'foobar'}, (e1, token) => {
testUtils.verifyJWTHelper(token, undefined, {issuer: ['foo', 'bar']}, (e2) => {
testUtils.verifyJWTHelper(token, 'secret', {issuer: ['foo', 'bar']}, (e2) => {
testUtils.asyncCheck(done, () => {
expect(e1).to.be.null;
expect(e2).to.be.instanceOf(jwt.JsonWebTokenError);
Expand All @@ -191,7 +191,7 @@ describe('issuer', function() {

it('should error without "iss" and with verify "issuer" option', function(done) {
signWithIssuer(undefined, {}, (e1, token) => {
testUtils.verifyJWTHelper(token, undefined, {issuer: ['foo', 'bar']}, (e2) => {
testUtils.verifyJWTHelper(token, 'secret', {issuer: ['foo', 'bar']}, (e2) => {
testUtils.asyncCheck(done, () => {
expect(e1).to.be.null;
expect(e2).to.be.instanceOf(jwt.JsonWebTokenError);
Expand Down
Loading