maxAge: Fix logic with number + use seconds instead of ms #349
Conversation
| if (nowOrClockTimestamp - (payload.iat * 1000) > maxAge + (options.clockTolerance || 0) * 1000) { | ||
| return done(new TokenExpiredError('maxAge exceeded', new Date(payload.iat * 1000 + maxAge))); | ||
|
|
||
| var maxAgeTimestamp = timespan(options.maxAge, payload.iat); |
There was a problem hiding this comment.
Are you aware that your if options.maxAge was created by a new String(...) call, this will always return an error? Your timespan method does a typeof x === 'string' but typeof new String('foo') === object.
There was a problem hiding this comment.
I didn't think about that case, I reused one function we already have. I'll take a look to it.
There was a problem hiding this comment.
I just stumbled over it when looking at the implementation of that function. I guess different rules apply now since it has to deal with external input.
There was a problem hiding this comment.
I just took the same approach used in sign() https://github.com/auth0/node-jsonwebtoken/blob/master/sign.js#L114-L116 so that we can cover when the format of the string itself is not correct.
There was a problem hiding this comment.
I think it would be nicer for the user to generally test for string with something like
https://github.com/lodash/lodash/blob/master/isString.js, but that might better be done in a different PR.
There was a problem hiding this comment.
I wanted to merge a bunch of "breaking changes" and then increase major version. It seems creators of the others PRs forgot about them (we asked for changes).
So, I'll try to find time to fix them probably, but I don't have ETA for now.
why not simply ms(ms(options.maxAge))?
ms(ms(number)) => number
ms(ms(string)) => string
We want end up having the number.
There was a problem hiding this comment.
To add to the conversation above, both ms(String('1s')) and ms(String(1000)) work. No need to use new, as String object can be used as a global constructor for strings.
There was a problem hiding this comment.
Well, it is the consumer who is deciding what to send. But thanks to your comment I realized I didn't add a test for String('12s'), I'll add it later.
ziluvatar
force-pushed
the
fix-max-age-number-and-seconds
branch
from
07a8801 to
1912a14
Compare
test/verify.tests.js
Outdated
| it('should not error for claims issued before a certain timespan but still inside clockTolerance timespan', function (done) { | ||
| clock = sinon.useFakeTimers(1437018582500); | ||
| var options = {algorithms: ['HS256'], maxAge: '321ms', clockTolerance: 100}; | ||
| [String('6s'), '5s', 5].forEach(function (maxAge) { |
There was a problem hiding this comment.
All the other multi-tests have different representations of the same time; this is different. Seems like a mistake?
There was a problem hiding this comment.
Yep, copy&paste mistake. Fixing.
ziluvatar
force-pushed
the
fix-max-age-number-and-seconds
branch
from
1912a14 to
66a4f8b
Compare
Fixes #346
Fixes #273.
Since this will expect seconds to be used (even using string type), I mark it for
next-major.