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.

Sign up for GitHub

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

maxAge: Fix logic with number + use seconds instead of ms #349

Merged
merged 2 commits into from
Sep 6, 2017
Merged

maxAge: Fix logic with number + use seconds instead of ms #349

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b61cc34
maxAge: Fix logic with number + use seconds instead of ms
ziluvatar May 13, 2017
66a4f8b
maxAge: Add validation to timespan result
ziluvatar May 16, 2017
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
4 changes: 2 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,8 @@ encoded public key for RSA and ECDSA.
* `ignoreNotBefore`...
* `subject`: if you want to check subject (`sub`), provide a value here
* `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. Currently it is expressed in milliseconds or a string describing a time span [zeit/ms](https://github.com/zeit/ms). Eg: `1000`, `"2 days"`, `"10h"`, `"7d"`. **We advise against using milliseconds precision, though, since JWTs can only contain seconds. The maximum precision might be reduced to seconds in the future.**
* `clockTimestamp`: the time in seconds that should be used as the current time for all necessary comparisons (also against `maxAge`, so our advise is to avoid using `clockTimestamp` and a `maxAge` in milliseconds together)
* `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"`.
* `clockTimestamp`: the time in seconds that should be used as the current time for all necessary comparisons.


```js
Expand Down
137 changes: 75 additions & 62 deletions test/verify.tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,8 @@ describe('verify', function() {
});

describe('expiration', function () {
// { foo: 'bar', iat: 1437018582, exp: 1437018583 }
var token = 'eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJmb28iOiJiYXIiLCJpYXQiOjE0MzcwMTg1ODIsImV4cCI6MTQzNzAxODU4M30.NmMv7sXjM1dW0eALNXud8LoXknZ0mH14GtnFclwJv0s';
// { foo: 'bar', iat: 1437018582, exp: 1437018592 }
var token = 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJmb28iOiJiYXIiLCJpYXQiOjE0MzcwMTg1ODIsImV4cCI6MTQzNzAxODU5Mn0.3aR3vocmgRpG05rsI9MpR6z2T_BGtMQaPq2YR6QaroU';
var key = 'key';

var clock;
Expand All @@ -78,22 +78,22 @@ describe('verify', function() {
});

it('should error on expired token', function (done) {
clock = sinon.useFakeTimers(1437018650000);
clock = sinon.useFakeTimers(1437018650000); // iat + 58s, exp + 48s
var options = {algorithms: ['HS256']};

jwt.verify(token, key, options, function (err, p) {
assert.equal(err.name, 'TokenExpiredError');
assert.equal(err.message, 'jwt expired');
assert.equal(err.expiredAt.constructor.name, 'Date');
assert.equal(Number(err.expiredAt), 1437018583000);
assert.equal(Number(err.expiredAt), 1437018592000);
assert.isUndefined(p);
done();
});
});

it('should not error on expired token within clockTolerance interval', function (done) {
clock = sinon.useFakeTimers(1437018584000);
var options = {algorithms: ['HS256'], clockTolerance: 100}
clock = sinon.useFakeTimers(1437018594000); // iat + 12s, exp + 2s
var options = {algorithms: ['HS256'], clockTolerance: 5 }

jwt.verify(token, key, options, function (err, p) {
assert.isNull(err);
Expand All @@ -103,8 +103,8 @@ describe('verify', function() {
});

it('should not error if within maxAge timespan', function (done) {
clock = sinon.useFakeTimers(1437018582500);
var options = {algorithms: ['HS256'], maxAge: '600ms'};
clock = sinon.useFakeTimers(1437018587500); // iat + 5.5s, exp - 4.5s
var options = {algorithms: ['HS256'], maxAge: '6s'};

jwt.verify(token, key, options, function (err, p) {
assert.isNull(err);
Expand All @@ -114,70 +114,83 @@ describe('verify', function() {
});

describe('option: maxAge', function () {
it('should error for claims issued before a certain timespan', function (done) {
clock = sinon.useFakeTimers(1437018582500);
var options = {algorithms: ['HS256'], maxAge: '321ms'};

jwt.verify(token, key, options, function (err, p) {
assert.equal(err.name, 'TokenExpiredError');
assert.equal(err.message, 'maxAge exceeded');
assert.equal(err.expiredAt.constructor.name, 'Date');
assert.equal(Number(err.expiredAt), 1437018582321);
assert.isUndefined(p);
done();
['3s', 3].forEach(function(maxAge) {
it(`should error for claims issued before a certain timespan (${typeof maxAge} type)`, function (done) {
clock = sinon.useFakeTimers(1437018587000); // iat + 5s, exp - 5s
var options = {algorithms: ['HS256'], maxAge: maxAge};

jwt.verify(token, key, options, function (err, p) {
assert.equal(err.name, 'TokenExpiredError');
assert.equal(err.message, 'maxAge exceeded');
assert.equal(err.expiredAt.constructor.name, 'Date');
assert.equal(Number(err.expiredAt), 1437018585000);
assert.isUndefined(p);
done();
});
});
});

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};
['5s', 5].forEach(function (maxAge) {
it(`should not error for claims issued before a certain timespan but still inside clockTolerance timespan (${typeof maxAge} type)`, function (done) {
clock = sinon.useFakeTimers(1437018587500); // iat + 5.5s, exp - 4.5s
var options = {algorithms: ['HS256'], maxAge: maxAge, clockTolerance: 1 };

jwt.verify(token, key, options, function (err, p) {
assert.isNull(err);
assert.equal(p.foo, 'bar');
done();
jwt.verify(token, key, options, function (err, p) {
assert.isNull(err);
assert.equal(p.foo, 'bar');
done();
});
});
});

it('should not error if within maxAge timespan', function (done) {
clock = sinon.useFakeTimers(1437018582500);
var options = {algorithms: ['HS256'], maxAge: '600ms'};
['6s', 6].forEach(function (maxAge) {
it(`should not error if within maxAge timespan (${typeof maxAge} type)`, function (done) {
clock = sinon.useFakeTimers(1437018587500);// iat + 5.5s, exp - 4.5s
var options = {algorithms: ['HS256'], maxAge: maxAge};

jwt.verify(token, key, options, function (err, p) {
assert.isNull(err);
assert.equal(p.foo, 'bar');
done();
jwt.verify(token, key, options, function (err, p) {
assert.isNull(err);
assert.equal(p.foo, 'bar');
done();
});
});
});
it('can be more restrictive than expiration', function (done) {
clock = sinon.useFakeTimers(1437018582900);
var options = {algorithms: ['HS256'], maxAge: '800ms'};

jwt.verify(token, key, options, function (err, p) {
assert.equal(err.name, 'TokenExpiredError');
assert.equal(err.message, 'maxAge exceeded');
assert.equal(err.expiredAt.constructor.name, 'Date');
assert.equal(Number(err.expiredAt), 1437018582800);
assert.isUndefined(p);
done();
['8s', 8].forEach(function (maxAge) {
it(`can be more restrictive than expiration (${typeof maxAge} type)`, function (done) {
clock = sinon.useFakeTimers(1437018591900); // iat + 9.9s, exp - 0.1s
var options = {algorithms: ['HS256'], maxAge: maxAge };

jwt.verify(token, key, options, function (err, p) {
assert.equal(err.name, 'TokenExpiredError');
assert.equal(err.message, 'maxAge exceeded');
assert.equal(err.expiredAt.constructor.name, 'Date');
assert.equal(Number(err.expiredAt), 1437018590000);
assert.isUndefined(p);
done();
});
});
});
it('cannot be more permissive than expiration', function (done) {
clock = sinon.useFakeTimers(1437018583100);
var options = {algorithms: ['HS256'], maxAge: '1200ms'};

jwt.verify(token, key, options, function (err, p) {
// maxAge not exceded, but still expired
assert.equal(err.name, 'TokenExpiredError');
assert.equal(err.message, 'jwt expired');
assert.equal(err.expiredAt.constructor.name, 'Date');
assert.equal(Number(err.expiredAt), 1437018583000);
assert.isUndefined(p);
done();
['12s', 12].forEach(function (maxAge) {
it(`cannot be more permissive than expiration (${typeof maxAge} type)`, function (done) {
clock = sinon.useFakeTimers(1437018593000); // iat + 11s, exp + 1s
var options = {algorithms: ['HS256'], maxAge: '12s'};

jwt.verify(token, key, options, function (err, p) {
// maxAge not exceded, but still expired
assert.equal(err.name, 'TokenExpiredError');
assert.equal(err.message, 'jwt expired');
assert.equal(err.expiredAt.constructor.name, 'Date');
assert.equal(Number(err.expiredAt), 1437018592000);
assert.isUndefined(p);
done();
});
});
});

it('should error if maxAge is specified but there is no iat claim', function (done) {
clock = sinon.useFakeTimers(1437018582900);
var token = 'eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJmb28iOiJiYXIifQ.0MBPd4Bru9-fK_HY3xmuDAc6N_embknmNuhdb9bKL_U';
var options = {algorithms: ['HS256'], maxAge: '1s'};

Expand Down Expand Up @@ -249,7 +262,7 @@ describe('verify', function() {
});

describe('option: maxAge and clockTimestamp', function () {
// { foo: 'bar', iat: 1437018582, exp: 1437018800 }
// { foo: 'bar', iat: 1437018582, exp: 1437018800 } exp = iat + 218s
var token = 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJmb28iOiJiYXIiLCJpYXQiOjE0MzcwMTg1ODIsImV4cCI6MTQzNzAxODgwMH0.AVOsNC7TiT-XVSpCpkwB1240izzCIJ33Lp07gjnXVpA';
it('should error for claims issued before a certain timespan', function (done) {
var clockTimestamp = 1437018682;
Expand All @@ -265,12 +278,12 @@ describe('verify', function() {
});
});
it('should not error for claims issued before a certain timespan but still inside clockTolerance timespan', function (done) {
var clockTimestamp = 1437018582;
var clockTimestamp = 1437018592; // iat + 10s
var options = {
algorithms: ['HS256'],
clockTimestamp: clockTimestamp,
maxAge: '321ms',
clockTolerance: 100
maxAge: '3s',
clockTolerance: 10
};

jwt.verify(token, key, options, function (err, p) {
Expand All @@ -280,8 +293,8 @@ describe('verify', function() {
});
});
it('should not error if within maxAge timespan', function (done) {
var clockTimestamp = 1437018582;
var options = {algorithms: ['HS256'], clockTimestamp: clockTimestamp, maxAge: '600ms'};
var clockTimestamp = 1437018587; // iat + 5s
var options = {algorithms: ['HS256'], clockTimestamp: clockTimestamp, maxAge: '6s'};

jwt.verify(token, key, options, function (err, p) {
assert.isNull(err);
Expand All @@ -290,7 +303,7 @@ describe('verify', function() {
});
});
it('can be more restrictive than expiration', function (done) {
var clockTimestamp = 1437018588;
var clockTimestamp = 1437018588; // iat + 6s
var options = {algorithms: ['HS256'], clockTimestamp: clockTimestamp, maxAge: '5s'};

jwt.verify(token, key, options, function (err, p) {
Expand All @@ -303,7 +316,7 @@ describe('verify', function() {
});
});
it('cannot be more permissive than expiration', function (done) {
var clockTimestamp = 1437018900;
var clockTimestamp = 1437018900; // iat + 318s (exp: iat + 218s)
var options = {algorithms: ['HS256'], clockTimestamp: clockTimestamp, maxAge: '1000y'};

jwt.verify(token, key, options, function (err, p) {
Expand Down
12 changes: 5 additions & 7 deletions verify.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ var JsonWebTokenError = require('./lib/JsonWebTokenError');
var NotBeforeError = require('./lib/NotBeforeError');
var TokenExpiredError = require('./lib/TokenExpiredError');
var decode = require('./decode');
var timespan = require('./lib/timespan');
var jws = require('jws');
var ms = require('ms');
var xtend = require('xtend');

module.exports = function (jwtString, secretOrPublicKey, options, callback) {
Expand Down Expand Up @@ -165,15 +165,13 @@ module.exports = function (jwtString, secretOrPublicKey, options, callback) {
}

if (options.maxAge) {
var maxAge = ms(options.maxAge);
if (typeof payload.iat !== 'number') {
return done(new JsonWebTokenError('iat required when maxAge is specified'));
}
// We have to compare against either options.clockTimestamp or the currentDate _with_ millis
// to not change behaviour (version 7.2.1). Should be resolve somehow for next major.
var nowOrClockTimestamp = ((options.clockTimestamp || 0) * 1000) || Date.now();
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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't think about that case, I reused one function we already have. I'll take a look to it.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

Ok, thanks, got it.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

Makes sense - thanks for noting that!

if (clockTimestamp >= maxAgeTimestamp + (options.clockTolerance || 0)) {
return done(new TokenExpiredError('maxAge exceeded', new Date(maxAgeTimestamp * 1000)));
}
}

Expand Down