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

Add verify option for nonce validation #540

Merged
merged 4 commits into from
Nov 14, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ As mentioned in [this comment](https://github.com/auth0/node-jsonwebtoken/issues
* `clockTolerance`: number of seconds to tolerate when checking the `nbf` and `exp` claims, to deal with small clock differences among different servers
* `maxAge`: the maximum allowed age for tokens to still be valid. It is expressed in seconds or a string describing a time span [zeit/ms](https://github.com/zeit/ms). Eg: `1000`, `"2 days"`, `"10h"`, `"7d"`. A numeric value is interpreted as a seconds count. If you use a string be sure you provide the time units (days, hours, etc), otherwise milliseconds unit is used by default (`"120"` is equal to `"120ms"`).
* `clockTimestamp`: the time in seconds that should be used as the current time for all necessary comparisons.
* `nonce`: if you want to check `nonce` claim, provide a string value here. It is used on Open ID for the ID Tokens. ([Open ID implementation notes](https://openid.net/specs/openid-connect-core-1_0.html#NonceNotes))


```js
Expand Down
57 changes: 57 additions & 0 deletions test/option-nonce.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
'use strict';

const jwt = require('../');
const expect = require('chai').expect;
const util = require('util');
const testUtils = require('./test-utils')

describe('nonce option', function () {
let token;

beforeEach(function () {
token = jwt.sign({ nonce: 'abcde' }, undefined, { algorithm: 'none' });
});
[
{
description: 'should work with a string',
nonce: 'abcde',
},
].forEach((testCase) => {
it(testCase.description, function (done) {
testUtils.verifyJWTHelper(token, undefined, { nonce: testCase.nonce }, (err, decoded) => {
testUtils.asyncCheck(done, () => {
expect(err).to.be.null;
expect(decoded).to.have.property('nonce', 'abcde');
});
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you do the tests using the helpers that @MitMaro prepared?
It will internally assert sync and async behavior (like you did well here, just for consistency across tests)

Copy link
Contributor

Choose a reason for hiding this comment

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

@kazuki229 Some direction that hopefully will make things a little easier.

Add const testUtils = require('./test-utils'); to get the test utilities.

To use the test helpers for verify:

testUtils.verifyJWTHelper(token, undefined, {nonce: testCase.nonce}, (err, decoded) => {
  testUtils.asyncCheck(done, () => {
    expect(err).to.be.null;
    expect(decoded).to.have.property('nonce', 'abcde');
  });
});

To check for an error you would do something along the lines of:

testUtils.verifyJWTHelper(token, undefined, {nonce}, (err) => {
  testUtils.asyncCheck(done, () => {
    expect(err).to.be.instanceOf(jwt.JsonWebTokenError);
    expect(err).to.have.property('message', 'nonce must be a non-empty string')
  });
});

Ping me if you need any other assistance and I will gladly help!

});
[
true,
false,
null,
-1,
0,
1,
-1.1,
1.1,
-Infinity,
Infinity,
NaN,
'',
' ',
[],
['foo'],
{},
{ foo: 'bar' },
].forEach((nonce) => {
it(`should error with value ${util.inspect(nonce)}`, function (done) {
testUtils.verifyJWTHelper(token, undefined, { nonce }, (err) => {
testUtils.asyncCheck(done, () => {
expect(err).to.be.instanceOf(jwt.JsonWebTokenError);
expect(err).to.have.property('message', 'nonce must be a non-empty string')
});
});
});
});
});
10 changes: 10 additions & 0 deletions verify.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ module.exports = function (jwtString, secretOrPublicKey, options, callback) {
return done(new JsonWebTokenError('clockTimestamp must be a number'));
}

if (options.nonce !== undefined && (typeof options.nonce !== 'string' || options.nonce.trim() === '')) {
return done(new JsonWebTokenError('nonce must be a non-empty string'));
}

var clockTimestamp = options.clockTimestamp || Math.floor(Date.now() / 1000);

if (!jwtString){
Expand Down Expand Up @@ -179,6 +183,12 @@ module.exports = function (jwtString, secretOrPublicKey, options, callback) {
}
}

if (options.nonce) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you return error if options.nonce is not a string?

Copy link
Contributor

Choose a reason for hiding this comment

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

The verify option really could use some validation of the options, but that is currently not the case for any option besides for clockTimestamp.

Perhaps something like adding some validation in the same area as the clockTimestamp validation?

Something like (note: untested):

  if (options.nonce !== undefined && (typeof options.nonce !== 'string' || options.nonce.trim() === '')) {
    return done(new JsonWebTokenError('nonce must be a non-empty string'));
  }

Ideally, we would handle the validation in a similar matter to the validation in verify, but that would be adding a lot of additional weight to this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

With the above validation is added, then we should test that the following values result in the error being returned:

[
  true,
  false,
  null,
  -1,
  0,
  1,
  -1.1,
  1.1,
  -Infinity,
  Infinity,
  NaN,
  '',
  ' ',
  [],
  ['foo'],
  {},
  {foo: 'bar'},
]

if (payload.nonce !== options.nonce) {
return done(new JsonWebTokenError('jwt nonce invalid. expected: ' + options.nonce));
}
}

if (options.maxAge) {
if (typeof payload.iat !== 'number') {
return done(new JsonWebTokenError('iat required when maxAge is specified'));
Expand Down