Skip to content

Commit 9c94bba

Browse files
visvkvisvk
committed
prevent empty code challenge and undo refactor
1 parent 0bf1061 commit 9c94bba

8 files changed

+21
-165
lines changed

‎lib/grant-types/abstract-grant-type.js

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@ function AbstractGrantType(options) {
3030
this.model = options.model;
3131
this.refreshTokenLifetime = options.refreshTokenLifetime;
3232
this.alwaysIssueNewRefreshToken = options.alwaysIssueNewRefreshToken;
33-
this.PKCEEnabled = options.PKCEEnabled;
3433
}
3534

3635
/**

‎lib/grant-types/authorization-code-grant-type.js

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,6 @@ AuthorizationCodeGrantType.prototype.getAuthorizationCode = function(request, cl
9292
}
9393

9494
return promisify(this.model.getAuthorizationCode, 1).call(this.model, request.body.code)
95-
.bind(this)
9695
.then(function(code) {
9796
if (!code) {
9897
throw new InvalidGrantError('Invalid grant: authorization code is invalid');
@@ -122,20 +121,15 @@ AuthorizationCodeGrantType.prototype.getAuthorizationCode = function(request, cl
122121
throw new InvalidGrantError('Invalid grant: `redirect_uri` is not a valid URI');
123122
}
124123

125-
if (this.PKCEEnabled && client.isPublic) {
126-
if (!code.codeChallenge) {
127-
throw new InvalidGrantError('Invalid grant: code challenge is invalid');
128-
}
129-
130-
/**
131-
* The "code_challenge_method" is bound to the Authorization Code when
132-
* the Authorization Code is issued. That is the method that the token
133-
* endpoint MUST use to verify the "code_verifier".
134-
*/
124+
if (code.codeChallenge) {
135125
if (!code.codeChallengeMethod) {
136126
throw new ServerError('Server error: `getAuthorizationCode()` did not return a `codeChallengeMethod` property');
137127
}
138128

129+
if (!request.body.code_verifier) {
130+
throw new InvalidGrantError('Missing parameter: `code_verifier`');
131+
}
132+
139133
if (code.codeChallengeMethod === 'plain' && code.codeChallenge !== request.body.code_verifier) {
140134
throw new InvalidGrantError('Invalid grant: code verifier is invalid');
141135
}

‎lib/handlers/authorize-handler.js

Lines changed: 9 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,6 @@ function AuthorizeHandler(options) {
6262
this.allowEmptyState = options.allowEmptyState;
6363
this.authenticateHandler = options.authenticateHandler || new AuthenticateHandler(options);
6464
this.authorizationCodeLifetime = options.authorizationCodeLifetime;
65-
this.PKCEEnabled = options.PKCEEnabled;
6665
this.model = options.model;
6766
}
6867

@@ -99,17 +98,11 @@ AuthorizeHandler.prototype.handle = function(request, response) {
9998
var scope;
10099
var state;
101100
var ResponseType;
102-
var codeChallenge;
103-
var codeChallengeMethod;
101+
var codeChallenge = this.getCodeChallenge(request);
102+
var codeChallengeMethod = codeChallenge && this.getCodeChallengeMethod(request);
104103

105104
return Promise.bind(this)
106105
.then(function() {
107-
codeChallenge = this.getCodeChallenge(request, client);
108-
109-
if (codeChallenge) {
110-
codeChallengeMethod = this.getCodeChallengeMethod(request);
111-
}
112-
113106
var requestedScope = this.getScope(request);
114107

115108
return this.validateScope(user, client, requestedScope);
@@ -157,19 +150,15 @@ AuthorizeHandler.prototype.generateAuthorizationCode = function(client, user, sc
157150
return tokenUtil.generateRandomToken();
158151
};
159152

160-
AuthorizeHandler.prototype.getCodeChallenge = function(request, client) {
153+
AuthorizeHandler.prototype.getCodeChallenge = function(request) {
161154
var codeChallenge = request.body.code_challenge || request.query.code_challenge;
162155

163-
/**
164-
* If the server requires Proof Key for Code Exchange (PKCE) by OAuth
165-
* public clients and the client does not send the "code_challenge" in
166-
* the request, the authorization endpoint MUST return the authorization
167-
* error response with the "error" value set to "invalid_request". The
168-
* "error_description" or the response of "error_uri" SHOULD explain the
169-
* nature of error, e.g., code challenge required.
170-
*/
171-
if (this.PKCEEnabled && client.isPublic && _.isEmpty(codeChallenge)) {
172-
throw new InvalidRequestError('Missing parameter: `code_challenge`. Public clients must include a code_challenge');
156+
if (!codeChallenge || codeChallenge === '') {
157+
return;
158+
}
159+
160+
if (!is.vschar(codeChallenge)) {
161+
throw new InvalidRequestError('Invalid parameter: `code_challenge`');
173162
}
174163

175164
return codeChallenge;
@@ -217,7 +206,6 @@ AuthorizeHandler.prototype.getClient = function(request) {
217206
throw new InvalidRequestError('Invalid request: `redirect_uri` is not a valid URI');
218207
}
219208
return promisify(this.model.getClient, 2).call(this.model, clientId, null)
220-
.bind(this)
221209
.then(function(client) {
222210
if (!client) {
223211
throw new InvalidClientError('Invalid client: client credentials are invalid');
@@ -239,15 +227,6 @@ AuthorizeHandler.prototype.getClient = function(request) {
239227
throw new InvalidClientError('Invalid client: `redirect_uri` does not match client value');
240228
}
241229

242-
if (this.PKCEEnabled) {
243-
if (client.isPublic === undefined) {
244-
throw new InvalidClientError('Invalid client: missing client `isPublic`');
245-
}
246-
247-
if (typeof client.isPublic !== 'boolean') {
248-
throw new InvalidClientError('Invalid client: `isPublic` must be a boolean');
249-
}
250-
}
251230
return client;
252231
});
253232
};

‎lib/handlers/token-handler.js

Lines changed: 1 addition & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,6 @@ function TokenHandler(options) {
6262
this.allowExtendedTokenAttributes = options.allowExtendedTokenAttributes;
6363
this.requireClientAuthentication = options.requireClientAuthentication || {};
6464
this.alwaysIssueNewRefreshToken = options.alwaysIssueNewRefreshToken !== false;
65-
this.PKCEEnabled = options.PKCEEnabled;
6665
}
6766

6867
/**
@@ -137,7 +136,6 @@ TokenHandler.prototype.getClient = function(request, response) {
137136
}
138137

139138
return promisify(this.model.getClient, 2).call(this.model, credentials.clientId, credentials.clientSecret)
140-
.bind(this)
141139
.then(function(client) {
142140
if (!client) {
143141
throw new InvalidClientError('Invalid client: client is invalid');
@@ -151,16 +149,6 @@ TokenHandler.prototype.getClient = function(request, response) {
151149
throw new ServerError('Server error: `grants` must be an array');
152150
}
153151

154-
if (this.PKCEEnabled) {
155-
if (client.isPublic === undefined) {
156-
throw new ServerError('Server error: missing client `isPublic`');
157-
}
158-
159-
if (typeof client.isPublic !== 'boolean') {
160-
throw new ServerError('Server error: invalid client, `isPublic` must be a boolean');
161-
}
162-
}
163-
164152
return client;
165153
})
166154
.catch(function(e) {
@@ -239,8 +227,7 @@ TokenHandler.prototype.handleGrantType = function(request, client) {
239227
accessTokenLifetime: accessTokenLifetime,
240228
model: this.model,
241229
refreshTokenLifetime: refreshTokenLifetime,
242-
alwaysIssueNewRefreshToken: this.alwaysIssueNewRefreshToken,
243-
PKCEenabled: this.PKCEenabled
230+
alwaysIssueNewRefreshToken: this.alwaysIssueNewRefreshToken
244231
};
245232

246233
return new Type(options)

‎lib/server.js

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -51,8 +51,7 @@ OAuth2Server.prototype.authenticate = function(request, response, options, callb
5151
OAuth2Server.prototype.authorize = function(request, response, options, callback) {
5252
options = _.assign({
5353
allowEmptyState: false,
54-
authorizationCodeLifetime: 5 * 60, // 5 minutes.
55-
PKCEEnabled: false
54+
authorizationCodeLifetime: 5 * 60 // 5 minutes.
5655
}, this.options, options);
5756

5857
return new AuthorizeHandler(options)
@@ -69,8 +68,7 @@ OAuth2Server.prototype.token = function(request, response, options, callback) {
6968
accessTokenLifetime: 60 * 60, // 1 hour.
7069
refreshTokenLifetime: 60 * 60 * 24 * 14, // 2 weeks.
7170
allowExtendedTokenAttributes: false,
72-
requireClientAuthentication: {}, // defaults to true for all grant types
73-
PKCEEnabled: false
71+
requireClientAuthentication: {} // defaults to true for all grant types
7472
}, this.options, options);
7573

7674
return new TokenHandler(options)

‎test/integration/grant-types/authorization-code-grant-type_test.js

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -379,7 +379,7 @@ describe('AuthorizationCodeGrantType integration', function() {
379379
revokeAuthorizationCode: function() {},
380380
saveToken: function() {}
381381
};
382-
var grantType = new AuthorizationCodeGrantType({ accessTokenLifetime: 123, model: model, PKCEEnabled: true });
382+
var grantType = new AuthorizationCodeGrantType({ accessTokenLifetime: 123, model: model });
383383
var request = new Request({ body: { code: 12345, code_verifier: 'foo' }, headers: {}, method: {}, query: {} });
384384

385385
return grantType.getAuthorizationCode(request, client)
@@ -405,7 +405,7 @@ describe('AuthorizationCodeGrantType integration', function() {
405405
revokeAuthorizationCode: function() {},
406406
saveToken: function() {}
407407
};
408-
var grantType = new AuthorizationCodeGrantType({ accessTokenLifetime: 123, model: model, PKCEEnabled: true });
408+
var grantType = new AuthorizationCodeGrantType({ accessTokenLifetime: 123, model: model });
409409
var request = new Request({ body: { code: 12345, code_verifier: 'foo' }, headers: {}, method: {}, query: {} });
410410

411411
return grantType.getAuthorizationCode(request, client)
@@ -432,7 +432,7 @@ describe('AuthorizationCodeGrantType integration', function() {
432432
revokeAuthorizationCode: function() {},
433433
saveToken: function() {}
434434
};
435-
var grantType = new AuthorizationCodeGrantType({ accessTokenLifetime: 123, model: model, PKCEEnabled: true });
435+
var grantType = new AuthorizationCodeGrantType({ accessTokenLifetime: 123, model: model });
436436
var request = new Request({ body: { code: 12345, code_verifier: codeVerifier }, headers: {}, method: {}, query: {} });
437437

438438
return grantType.getAuthorizationCode(request, client)
@@ -457,7 +457,7 @@ describe('AuthorizationCodeGrantType integration', function() {
457457
revokeAuthorizationCode: function() {},
458458
saveToken: function() {}
459459
};
460-
var grantType = new AuthorizationCodeGrantType({ accessTokenLifetime: 123, model: model, PKCEEnabled: true });
460+
var grantType = new AuthorizationCodeGrantType({ accessTokenLifetime: 123, model: model });
461461
var request = new Request({ body: { code: 12345, code_verifier: 'baz' }, headers: {}, method: {}, query: {} });
462462

463463
return grantType.getAuthorizationCode(request, client)

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

Lines changed: 0 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -783,44 +783,6 @@ describe('AuthorizeHandler integration', function() {
783783
e.message.should.equal('Invalid client: `redirect_uri` does not match client value');
784784
});
785785
});
786-
describe('with PKCEEnabled', function() {
787-
it('should throw an error if `client.isPublic` is missing`', function() {
788-
var model = {
789-
getAccessToken: function() {},
790-
getClient: function() {
791-
return { grants: ['authorization_code'], redirectUris: ['https://example.com'] };
792-
},
793-
saveAuthorizationCode: function() {}
794-
};
795-
var handler = new AuthorizeHandler({ authorizationCodeLifetime: 120, model: model, PKCEEnabled: true });
796-
var request = new Request({ body: { client_id: 12345, response_type: 'code', redirect_uri: 'https://example.com' }, headers: {}, method: {}, query: {} });
797-
798-
return handler.getClient(request)
799-
.then(should.fail)
800-
.catch(function(e) {
801-
e.should.be.an.instanceOf(InvalidClientError);
802-
e.message.should.equal('Invalid client: missing client `isPublic`');
803-
});
804-
});
805-
it('should throw an error if `client.isPublic` is invalid`', function() {
806-
var model = {
807-
getAccessToken: function() {},
808-
getClient: function() {
809-
return { grants: ['authorization_code'], redirectUris: ['https://example.com'], isPublic: 'foo' };
810-
},
811-
saveAuthorizationCode: function() {}
812-
};
813-
var handler = new AuthorizeHandler({ authorizationCodeLifetime: 120, model: model, PKCEEnabled: true });
814-
var request = new Request({ body: { client_id: 12345, response_type: 'code', redirect_uri: 'https://example.com' }, headers: {}, method: {}, query: {} });
815-
816-
return handler.getClient(request)
817-
.then(should.fail)
818-
.catch(function(e) {
819-
e.should.be.an.instanceOf(InvalidClientError);
820-
e.message.should.equal('Invalid client: `isPublic` must be a boolean');
821-
});
822-
});
823-
});
824786

825787
it('should support promises', function() {
826788
var model = {

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

Lines changed: 0 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -114,16 +114,6 @@ describe('TokenHandler integration', function() {
114114
handler.alwaysIssueNewRefreshToken.should.equal(true);
115115
});
116116

117-
it('should set the `PKCEEnabled` to true', function() {
118-
var model = {
119-
getClient: function() {},
120-
saveToken: function() {}
121-
};
122-
var handler = new TokenHandler({ accessTokenLifetime: 123, model: model, refreshTokenLifetime: 120, PKCEEnabled: true });
123-
124-
handler.PKCEEnabled.should.equal(true);
125-
});
126-
127117
it('should set the `extendedGrantTypes`', function() {
128118
var extendedGrantTypes = { foo: 'bar' };
129119
var model = {
@@ -595,59 +585,6 @@ describe('TokenHandler integration', function() {
595585
.catch(should.fail);
596586
});
597587
});
598-
describe('with PKCE enabled', function() {
599-
it('should return a client with `isPublic` parameter', function() {
600-
var client = { id: 12345, grants: [], isPublic: true };
601-
var model = {
602-
getClient: function() { return client; },
603-
saveToken: function() {}
604-
};
605-
606-
var handler = new TokenHandler({
607-
accessTokenLifetime: 120,
608-
model: model,
609-
refreshTokenLifetime: 120,
610-
PKCEEnabled: true
611-
});
612-
var request = new Request({ body: { client_id: 'foo', client_secret: 'baz', grant_type: 'authorization_code' }, headers: {}, method: {}, query: {} });
613-
614-
return handler.getClient(request)
615-
.then(function(data) {
616-
data.should.equal(client);
617-
})
618-
.catch(should.fail);
619-
});
620-
it('should throw an error if `client.isPublic` is invalid', function() {
621-
var model = {
622-
getClient: function() { return { id: 123, grants: [], isPublic: 'foo' }; },
623-
saveToken: function() {}
624-
};
625-
var handler = new TokenHandler({ accessTokenLifetime: 120, model: model, refreshTokenLifetime: 120, PKCEEnabled: true });
626-
var request = new Request({ body: { client_id: 12345, client_secret: 'secret' }, headers: {}, method: {}, query: {} });
627-
628-
return handler.getClient(request)
629-
.then(should.fail)
630-
.catch(function(e) {
631-
e.should.be.an.instanceOf(ServerError);
632-
e.message.should.equal('Server error: invalid client, `isPublic` must be a boolean');
633-
});
634-
});
635-
it('should throw an error if `client.isPublic` is missing', function() {
636-
var model = {
637-
getClient: function() { return { id: 123, grants: [] }; },
638-
saveToken: function() {}
639-
};
640-
var handler = new TokenHandler({ accessTokenLifetime: 120, model: model, refreshTokenLifetime: 120, PKCEEnabled: true });
641-
var request = new Request({ body: { client_id: 12345, client_secret: 'secret' }, headers: {}, method: {}, query: {} });
642-
643-
return handler.getClient(request)
644-
.then(should.fail)
645-
.catch(function(e) {
646-
e.should.be.an.instanceOf(ServerError);
647-
e.message.should.equal('Server error: missing client `isPublic`');
648-
});
649-
});
650-
});
651588

652589
it('should support promises', function() {
653590
var model = {

0 commit comments

Comments
 (0)