Add verify option for nonce validation #540
Conversation
There was a problem hiding this comment.
Thanks @kazuki229 !! 👏
@MitMaro not sure if you have any other feedback.
| expect(err).to.be.null; | ||
| done(); | ||
| }) | ||
| }); |
There was a problem hiding this comment.
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.
@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!
| @@ -179,6 +179,12 @@ module.exports = function (jwtString, secretOrPublicKey, options, callback) { | |||
| } | |||
| } | |||
|
|
|||
| if (options.nonce) { | |||
There was a problem hiding this comment.
Could you return error if options.nonce is not a string?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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'},
]
|
Kudos to @kazuki229 for adding tests! I think there are a few additional tests that could be easily added. I will take a look this weekend and provide some direction to @kazuki229 . |
There was a problem hiding this comment.
@kazuki229 I left some notes that should assist you. Ping me if you need any other assistance and I will gladly help!
| @@ -179,6 +179,12 @@ module.exports = function (jwtString, secretOrPublicKey, options, callback) { | |||
| } | |||
| } | |||
|
|
|||
| if (options.nonce) { | |||
There was a problem hiding this comment.
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.
| @@ -179,6 +179,12 @@ module.exports = function (jwtString, secretOrPublicKey, options, callback) { | |||
| } | |||
| } | |||
|
|
|||
| if (options.nonce) { | |||
There was a problem hiding this comment.
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'},
]
test/option-nonce.test.js
Outdated
| }); | ||
| [ | ||
| { | ||
| description: 'should work with string', |
There was a problem hiding this comment.
Minor item: 'should work with a string' instead?
test/option-nonce.test.js
Outdated
| {}, | ||
| { foo: 'bar' }, | ||
| ].forEach((nonce) => { | ||
| let tokenhoge = jwt.sign({ foo: 'bar' }, undefined, { algorithm: 'none' }); |
There was a problem hiding this comment.
You should be able to remove this line and reuse the token generated in the beforeEach above.
| expect(err).to.be.null; | ||
| done(); | ||
| }) | ||
| }); |
There was a problem hiding this comment.
@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!
Co-Authored-By: kazuki229 <tsuzuku.k@gmail.com>
|
@ziluvatar @MitMaro |
|
Thanks @kazuki229 !! Good job 👏 I'll create a minor version with this and some other patch changes that are on master. |
|
Released on v8.4.0 🎉 |
|
@ziluvatar |
I want to verify nonce value in jwt.
Then I found the following pull request.
#344
But because tests and README have not been added, it has not yet been merged.
So I did add tests and README.