Fixed error message when empty string passed as expiresIn or notBefore option #531
Conversation
|
@MitMaro @ziluvatar may I ask you to review this change? Thanks! :) |
|
I wonder if the error message should be normalized or if this validation should be moved into the |
|
Good one @MitMaro , yes, probably it is better to update the schema, so if it is an string it has to have some value. |
andrewnester
force-pushed
the
update-tests
branch
from
f05328f to
8db0267
Compare
|
@MitMaro @ziluvatar thanks for comments! I moved validation to |
test/claim-exp.test.js
Outdated
| it(`should error with with value ''`, function (done) { | ||
| signWithExpiresIn('', {}, (err) => { | ||
| testUtils.asyncCheck(done, () => { | ||
| expect(err).to.be.instanceOf(Error); | ||
| expect(err).to.have.property('message', 'val is not a non-empty string or a valid number. val=""'); | ||
| expect(err).to.have.property('message') | ||
| .match(/"expiresIn" should be a number of seconds or string representing a timespan/); |
There was a problem hiding this comment.
You should be able to move this test above into the array in jwt.sign "expiresIn" option validation block.
test/claim-nbf.test.js
Outdated
| @@ -51,7 +51,8 @@ describe('not before', function() { | |||
| signWithNotBefore('', {}, (err) => { | |||
| testUtils.asyncCheck(done, () => { | |||
| expect(err).to.be.instanceOf(Error); | |||
| expect(err).to.have.property('message', 'val is not a non-empty string or a valid number. val=""'); | |||
| expect(err).to.have.property('message') | |||
| .match(/"notBefore" should be a number of seconds or string representing a timespan/); | |||
There was a problem hiding this comment.
Ditto to above, you should be able to move this test above into the array in jwt.sign "expiresIn" option validation block.
|
@MitMaro indeed, good point, thanks for the comments! Updated in latest commit. |
|
@ziluvatar sorry for bothering again, any other comments on this PR? :) |
|
@andrewnester nothing to sorry about, thanks for the interest! I will merge this one since we were returning an error anyway, however, in this case it will return a better known descriptive error. |
This PR fixes TODO items to change error message when empty string passed as
expiresInornotBeforeoptions