-
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
Add verify option for nonce validation #540
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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'); | ||
}); | ||
}); | ||
}); | ||
}); | ||
[ | ||
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') | ||
}); | ||
}); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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){ | ||
|
@@ -179,6 +183,12 @@ module.exports = function (jwtString, secretOrPublicKey, options, callback) { | |
} | ||
} | ||
|
||
if (options.nonce) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you return error if There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Perhaps something like adding some validation in the same area as the Something like (note: untested):
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
|
||
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')); | ||
|
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.
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)
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.
@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
:To check for an error you would do something along the lines of:
Ping me if you need any other assistance and I will gladly help!