Skip to content

Commit 77c8e32

Browse files
mjsalingermjsalinger
authored
Merge pull request #391 from fabianfett/feature/invalid-accessTokenExpiresAt
Potential Security Risk: accessTokenExpiresAt should be mandatory.
2 parents 4f607a5 + 789db64 commit 77c8e32

File tree

5 files changed

+111
-20
lines changed

5 files changed

+111
-20
lines changed

‎lib/handlers/authenticate-handler.js‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -216,11 +216,11 @@ AuthenticateHandler.prototype.getAccessToken = function(token) {
216216
*/
217217

218218
AuthenticateHandler.prototype.validateAccessToken = function(accessToken) {
219-
if (accessToken.accessTokenExpiresAt && !(accessToken.accessTokenExpiresAt instanceof Date)) {
220-
throw new ServerError('Server error: `expires` must be a Date instance');
219+
if (!(accessToken.accessTokenExpiresAt instanceof Date)) {
220+
throw new ServerError('Server error: `accessTokenExpiresAt` must be a Date instance');
221221
}
222222

223-
if (accessToken.accessTokenExpiresAt && accessToken.accessTokenExpiresAt < new Date()) {
223+
if (accessToken.accessTokenExpiresAt < new Date()) {
224224
throw new InvalidTokenError('Invalid token: access token has expired');
225225
}
226226

‎test/integration/handlers/authenticate-handler_test.js‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,10 @@ describe('AuthenticateHandler integration', function() {
169169
});
170170

171171
it('should return an access token', function() {
172-
var accessToken = { user: {} };
172+
var accessToken = {
173+
user: {},
174+
accessTokenExpiresAt: new Date(new Date().getTime() + 10000)
175+
};
173176
var model = {
174177
getAccessToken: function() {
175178
return accessToken;
@@ -439,7 +442,10 @@ describe('AuthenticateHandler integration', function() {
439442
});
440443

441444
it('should return an access token', function() {
442-
var accessToken = { user: {} };
445+
var accessToken = {
446+
user: {},
447+
accessTokenExpiresAt: new Date(new Date().getTime() + 10000)
448+
};
443449
var handler = new AuthenticateHandler({ model: { getAccessToken: function() {} } });
444450

445451
handler.validateAccessToken(accessToken).should.equal(accessToken);

‎test/integration/handlers/authorize-handler_test.js‎

Lines changed: 38 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -180,7 +180,10 @@ describe('AuthorizeHandler integration', function() {
180180
it('should redirect to an error response if a non-oauth error is thrown', function() {
181181
var model = {
182182
getAccessToken: function() {
183-
return { user: {} };
183+
return {
184+
user: {},
185+
accessTokenExpiresAt: new Date(new Date().getTime() + 10000)
186+
};
184187
},
185188
getClient: function() {
186189
return { grants: ['authorization_code'], redirectUris: ['http://example.com/cb'] };
@@ -215,7 +218,10 @@ describe('AuthorizeHandler integration', function() {
215218
it('should redirect to an error response if an oauth error is thrown', function() {
216219
var model = {
217220
getAccessToken: function() {
218-
return { user: {} };
221+
return {
222+
user: {},
223+
accessTokenExpiresAt: new Date(new Date().getTime() + 10000)
224+
};
219225
},
220226
getClient: function() {
221227
return { grants: ['authorization_code'], redirectUris: ['http://example.com/cb'] };
@@ -251,7 +257,11 @@ describe('AuthorizeHandler integration', function() {
251257
var client = { grants: ['authorization_code'], redirectUris: ['http://example.com/cb'] };
252258
var model = {
253259
getAccessToken: function() {
254-
return { client: client, user: {} };
260+
return {
261+
client: client,
262+
user: {},
263+
accessTokenExpiresAt: new Date(new Date().getTime() + 10000)
264+
};
255265
},
256266
getClient: function() {
257267
return client;
@@ -286,7 +296,10 @@ describe('AuthorizeHandler integration', function() {
286296
it('should redirect to an error response if `scope` is invalid', function() {
287297
var model = {
288298
getAccessToken: function() {
289-
return { user: {} };
299+
return {
300+
user: {},
301+
accessTokenExpiresAt: new Date(new Date().getTime() + 10000)
302+
};
290303
},
291304
getClient: function() {
292305
return { grants: ['authorization_code'], redirectUris: ['http://example.com/cb'] };
@@ -322,7 +335,10 @@ describe('AuthorizeHandler integration', function() {
322335
it('should redirect to an error response if `state` is missing', function() {
323336
var model = {
324337
getAccessToken: function() {
325-
return { user: {} };
338+
return {
339+
user: {},
340+
accessTokenExpiresAt: new Date(new Date().getTime() + 10000)
341+
};
326342
},
327343
getClient: function() {
328344
return { grants: ['authorization_code'], redirectUris: ['http://example.com/cb'] };
@@ -355,7 +371,10 @@ describe('AuthorizeHandler integration', function() {
355371
it('should redirect to an error response if `response_type` is invalid', function() {
356372
var model = {
357373
getAccessToken: function() {
358-
return { user: {} };
374+
return {
375+
user: {},
376+
accessTokenExpiresAt: new Date(new Date().getTime() + 10000)
377+
};
359378
},
360379
getClient: function() {
361380
return { grants: ['authorization_code'], redirectUris: ['http://example.com/cb'] };
@@ -390,7 +409,10 @@ describe('AuthorizeHandler integration', function() {
390409
it('should fail on invalid `response_type` before calling model.saveAuthorizationCode()', function() {
391410
var model = {
392411
getAccessToken: function() {
393-
return { user: {} };
412+
return {
413+
user: {},
414+
accessTokenExpiresAt: new Date(new Date().getTime() + 10000)
415+
};
394416
},
395417
getClient: function() {
396418
return { grants: ['authorization_code'], redirectUris: ['http://example.com/cb'] };
@@ -426,7 +448,11 @@ describe('AuthorizeHandler integration', function() {
426448
var client = { grants: ['authorization_code'], redirectUris: ['http://example.com/cb'] };
427449
var model = {
428450
getAccessToken: function() {
429-
return { client: client, user: {} };
451+
return {
452+
client: client,
453+
user: {},
454+
accessTokenExpiresAt: new Date(new Date().getTime() + 10000)
455+
};
430456
},
431457
getClient: function() {
432458
return client;
@@ -889,7 +915,10 @@ describe('AuthorizeHandler integration', function() {
889915
var user = {};
890916
var model = {
891917
getAccessToken: function() {
892-
return { user: user };
918+
return {
919+
user: user,
920+
accessTokenExpiresAt: new Date(new Date().getTime() + 10000)
921+
};
893922
},
894923
getClient: function() {},
895924
saveAuthorizationCode: function() {}

‎test/integration/server_test.js‎

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,10 @@ describe('Server integration', function() {
4040
it('should set the default `options`', function() {
4141
var model = {
4242
getAccessToken: function() {
43-
return { user: {} };
43+
return {
44+
user: {},
45+
accessTokenExpiresAt: new Date(new Date().getTime() + 10000)
46+
};
4447
}
4548
};
4649
var server = new Server({ model: model });
@@ -59,7 +62,10 @@ describe('Server integration', function() {
5962
it('should return a promise', function() {
6063
var model = {
6164
getAccessToken: function(token, callback) {
62-
callback(null, { user: {} });
65+
callback(null, {
66+
user: {},
67+
accessTokenExpiresAt: new Date(new Date().getTime() + 10000)
68+
});
6369
}
6470
};
6571
var server = new Server({ model: model });
@@ -73,7 +79,10 @@ describe('Server integration', function() {
7379
it('should support callbacks', function(next) {
7480
var model = {
7581
getAccessToken: function() {
76-
return { user: {} };
82+
return {
83+
user: {},
84+
accessTokenExpiresAt: new Date(new Date().getTime() + 10000)
85+
};
7786
}
7887
};
7988
var server = new Server({ model: model });
@@ -88,7 +97,10 @@ describe('Server integration', function() {
8897
it('should set the default `options`', function() {
8998
var model = {
9099
getAccessToken: function() {
91-
return { user: {} };
100+
return {
101+
user: {},
102+
accessTokenExpiresAt: new Date(new Date().getTime() + 10000)
103+
};
92104
},
93105
getClient: function() {
94106
return { grants: ['authorization_code'], redirectUris: ['http://example.com/cb'] };
@@ -112,7 +124,10 @@ describe('Server integration', function() {
112124
it('should return a promise', function() {
113125
var model = {
114126
getAccessToken: function() {
115-
return { user: {} };
127+
return {
128+
user: {},
129+
accessTokenExpiresAt: new Date(new Date().getTime() + 10000)
130+
};
116131
},
117132
getClient: function() {
118133
return { grants: ['authorization_code'], redirectUris: ['http://example.com/cb'] };
@@ -132,7 +147,10 @@ describe('Server integration', function() {
132147
it('should support callbacks', function(next) {
133148
var model = {
134149
getAccessToken: function() {
135-
return { user: {} };
150+
return {
151+
user: {},
152+
accessTokenExpiresAt: new Date(new Date().getTime() + 10000)
153+
};
136154
},
137155
getClient: function() {
138156
return { grants: ['authorization_code'], redirectUris: ['http://example.com/cb'] };

‎test/unit/handlers/authenticate-handler_test.js‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ var AuthenticateHandler = require('../../../lib/handlers/authenticate-handler');
88
var Request = require('../../../lib/request');
99
var sinon = require('sinon');
1010
var should = require('should');
11+
var ServerError = require('../../../lib/errors/server-error');
1112

1213
/**
1314
* Test `AuthenticateHandler`.
@@ -93,6 +94,43 @@ describe('AuthenticateHandler', function() {
9394
});
9495
});
9596

97+
describe('validateAccessToken()', function() {
98+
it('should fail if token has no valid `accessTokenExpiresAt` date', function() {
99+
var model = {
100+
getAccessToken: function() {}
101+
};
102+
var handler = new AuthenticateHandler({ model: model });
103+
104+
var failed = false;
105+
try {
106+
handler.validateAccessToken({
107+
user: {}
108+
});
109+
}
110+
catch (err) {
111+
err.should.be.an.instanceOf(ServerError);
112+
failed = true;
113+
}
114+
failed.should.equal(true);
115+
});
116+
117+
it('should succeed if token has valid `accessTokenExpiresAt` date', function() {
118+
var model = {
119+
getAccessToken: function() {}
120+
};
121+
var handler = new AuthenticateHandler({ model: model });
122+
try {
123+
handler.validateAccessToken({
124+
user: {},
125+
accessTokenExpiresAt: new Date(new Date().getTime() + 10000)
126+
});
127+
}
128+
catch (err) {
129+
should.fail();
130+
}
131+
});
132+
});
133+
96134
describe('verifyScope()', function() {
97135
it('should call `model.getAccessToken()` if scope is defined', function() {
98136
var model = {

0 commit comments

Comments
 (0)