From 3df52fdf2c324933cfa8b6c00782d3de8d70c31b Mon Sep 17 00:00:00 2001 From: Francesco Stefanni Date: Sun, 5 Dec 2021 14:11:34 +0100 Subject: [PATCH 1/5] supported custom validateRedirectUri --- lib/handlers/authorize-handler.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/handlers/authorize-handler.js b/lib/handlers/authorize-handler.js index e825012..4919490 100644 --- a/lib/handlers/authorize-handler.js +++ b/lib/handlers/authorize-handler.js @@ -160,6 +160,7 @@ AuthorizeHandler.prototype.getAuthorizationCodeLifetime = function() { */ AuthorizeHandler.prototype.getClient = function(request) { + const self = this; const clientId = request.body.client_id || request.query.client_id; if (!clientId) { @@ -193,7 +194,11 @@ AuthorizeHandler.prototype.getClient = function(request) { throw new InvalidClientError('Invalid client: missing client `redirectUri`'); } - if (redirectUri && !client.redirectUris.includes(redirectUri)) { + if (redirectUri && typeof self.model.validateRedirectUri === 'function') { + if (self.model.validateRedirectUri(redirectUri, client.redirectUris)) { + throw new InvalidClientError('Invalid client: `redirect_uri` does not match client value'); + } + } else if (redirectUri && !client.redirectUris.includes(redirectUri)) { throw new InvalidClientError('Invalid client: `redirect_uri` does not match client value'); } return client; From 88ef515d932b05d477f95c27387c4fe895a5d361 Mon Sep 17 00:00:00 2001 From: Joren Vandeweyer Date: Sat, 11 Dec 2021 14:47:07 +0100 Subject: [PATCH 2/5] added test for `validateRedirectUri` --- lib/handlers/authorize-handler.js | 2 +- test/unit/handlers/authorize-handler_test.js | 30 ++++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/lib/handlers/authorize-handler.js b/lib/handlers/authorize-handler.js index 4919490..c88c5a8 100644 --- a/lib/handlers/authorize-handler.js +++ b/lib/handlers/authorize-handler.js @@ -195,7 +195,7 @@ AuthorizeHandler.prototype.getClient = function(request) { } if (redirectUri && typeof self.model.validateRedirectUri === 'function') { - if (self.model.validateRedirectUri(redirectUri, client.redirectUris)) { + if (!self.model.validateRedirectUri(redirectUri, client.redirectUris)) { throw new InvalidClientError('Invalid client: `redirect_uri` does not match client value'); } } else if (redirectUri && !client.redirectUris.includes(redirectUri)) { diff --git a/test/unit/handlers/authorize-handler_test.js b/test/unit/handlers/authorize-handler_test.js index 86ce336..c27a9ee 100644 --- a/test/unit/handlers/authorize-handler_test.js +++ b/test/unit/handlers/authorize-handler_test.js @@ -99,4 +99,34 @@ describe('AuthorizeHandler', function() { .catch(should.fail); }); }); + + describe('validateRedirectUri()', function() { + it('should call `model.validateRedirectUri()`', function() { + const client = { grants: ['authorization_code'], redirectUris: ['http://example.com/cb'] }; + const redirect_uri = 'http://example.com/cb/2'; + const model = { + getAccessToken: function() {}, + getClient: sinon.stub().returns(client), + saveAuthorizationCode: function() {}, + validateRedirectUri: sinon.stub().returns(true) + }; + const handler = new AuthorizeHandler({ authorizationCodeLifetime: 120, model: model }); + const request = new Request({ body: { client_id: 12345, client_secret: 'secret', redirect_uri }, headers: {}, method: {}, query: {} }); + + return handler.getClient(request) + .then(function() { + model.getClient.callCount.should.equal(1); + model.getClient.firstCall.args.should.have.length(2); + model.getClient.firstCall.args[0].should.equal(12345); + model.getClient.firstCall.thisValue.should.equal(model); + + model.validateRedirectUri.callCount.should.equal(1); + model.validateRedirectUri.firstCall.args.should.have.length(2); + model.validateRedirectUri.firstCall.args[0].should.equal(redirect_uri); + model.validateRedirectUri.firstCall.args[1].should.equal(client.redirectUris); + model.validateRedirectUri.firstCall.thisValue.should.equal(model); + }) + .catch(should.fail); + }); + }); }); From 0a86e694ac77a1ee272aac9c54f544a838c18b56 Mon Sep 17 00:00:00 2001 From: Joren Vandeweyer Date: Sat, 11 Dec 2021 15:19:42 +0100 Subject: [PATCH 3/5] updated documentation --- docs/model/overview.rst | 1 + docs/model/spec.rst | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/docs/model/overview.rst b/docs/model/overview.rst index 5e345ab..62c9ffa 100644 --- a/docs/model/overview.rst +++ b/docs/model/overview.rst @@ -37,6 +37,7 @@ Model functions used by the authorization code grant: - :ref:`Model#saveAuthorizationCode` - :ref:`Model#revokeAuthorizationCode` - :ref:`Model#validateScope` +- :ref:`Model#validateRedirectUri` -------- diff --git a/docs/model/spec.rst b/docs/model/spec.rst index 4cdd1fd..b13f438 100644 --- a/docs/model/spec.rst +++ b/docs/model/spec.rst @@ -985,3 +985,40 @@ Returns ``true`` if the access token passes, ``false`` otherwise. return requestedScopes.every(s => authorizedScopes.indexOf(s) >= 0); } +-------- + +.. _Model#validateRedirectUri: + +``validateRedirectUri(redirect_uri, redirect_uris, [callback])`` +================================================================ + +Invoked to check if the provided ``redirect_uri`` is valid for a particular ``client``. + +This model function is **optional**. If not implemented, the redirect_uri should be included in the provided redirect_uris of the client. + +**Invoked during:** + +- ``authorization_code`` grant + +**Arguments:** + ++-----------------+----------+---------------------------------------------------------------------+ +| Name | Type | Description | ++=================+==========+=====================================================================+ +| redirect_uri | String | The redirect URI to validate. | ++-----------------+----------+---------------------------------------------------------------------+ +| redirect_uris | Array | The list of redirect URIs configured for the client. | ++-----------------+----------+---------------------------------------------------------------------+ + +**Return value:** + +Returns ``true`` if the ``redirect_uri`` is valid, ``false`` otherwise. + +**Remarks:** + +:: + + function validateRedirectUri(redirect_uri, redirect_uris) { + return redirect_uris.includes(redirect_uri); + } + From 98a9d197ef7c0d3f20cca65ffec59973f2089d66 Mon Sep 17 00:00:00 2001 From: Joren Vandeweyer Date: Sat, 11 Dec 2021 15:59:08 +0100 Subject: [PATCH 4/5] better implementation of validateRedirectUri --- docs/model/spec.rst | 14 ++--- lib/handlers/authorize-handler.js | 25 +++++--- .../handlers/authorize-handler_test.js | 59 +++++++++++++++++++ test/unit/handlers/authorize-handler_test.js | 2 +- 4 files changed, 85 insertions(+), 15 deletions(-) diff --git a/docs/model/spec.rst b/docs/model/spec.rst index b13f438..46b6494 100644 --- a/docs/model/spec.rst +++ b/docs/model/spec.rst @@ -989,12 +989,12 @@ Returns ``true`` if the access token passes, ``false`` otherwise. .. _Model#validateRedirectUri: -``validateRedirectUri(redirect_uri, redirect_uris, [callback])`` +``validateRedirectUri(redirectUri, client, [callback])`` ================================================================ -Invoked to check if the provided ``redirect_uri`` is valid for a particular ``client``. +Invoked to check if the provided ``redirectUri`` is valid for a particular ``client``. -This model function is **optional**. If not implemented, the redirect_uri should be included in the provided redirect_uris of the client. +This model function is **optional**. If not implemented, the ``redirectUri`` should be included in the provided ``redirectUris`` of the client. **Invoked during:** @@ -1007,18 +1007,18 @@ This model function is **optional**. If not implemented, the redirect_uri should +=================+==========+=====================================================================+ | redirect_uri | String | The redirect URI to validate. | +-----------------+----------+---------------------------------------------------------------------+ -| redirect_uris | Array | The list of redirect URIs configured for the client. | +| client | Object | The associated client. | +-----------------+----------+---------------------------------------------------------------------+ **Return value:** -Returns ``true`` if the ``redirect_uri`` is valid, ``false`` otherwise. +Returns ``true`` if the ``redirectUri`` is valid, ``false`` otherwise. **Remarks:** :: - function validateRedirectUri(redirect_uri, redirect_uris) { - return redirect_uris.includes(redirect_uri); + function validateRedirectUri(redirectUri, client) { + return client.redirectUris.includes(redirectUri); } diff --git a/lib/handlers/authorize-handler.js b/lib/handlers/authorize-handler.js index c88c5a8..96a47e3 100644 --- a/lib/handlers/authorize-handler.js +++ b/lib/handlers/authorize-handler.js @@ -194,14 +194,17 @@ AuthorizeHandler.prototype.getClient = function(request) { throw new InvalidClientError('Invalid client: missing client `redirectUri`'); } - if (redirectUri && typeof self.model.validateRedirectUri === 'function') { - if (!self.model.validateRedirectUri(redirectUri, client.redirectUris)) { - throw new InvalidClientError('Invalid client: `redirect_uri` does not match client value'); - } - } else if (redirectUri && !client.redirectUris.includes(redirectUri)) { - throw new InvalidClientError('Invalid client: `redirect_uri` does not match client value'); + if (redirectUri) { + return self.validateRedirectUri(redirectUri, client) + .then(function(valid) { + if (!valid) { + throw new InvalidClientError('Invalid client: `redirect_uri` does not match client value'); + } + return client; + }); + } else { + return client; } - return client; }); }; @@ -294,6 +297,14 @@ AuthorizeHandler.prototype.saveAuthorizationCode = function(authorizationCode, e return promisify(this.model.saveAuthorizationCode, 3).call(this.model, code, client, user); }; + +AuthorizeHandler.prototype.validateRedirectUri = function(redirectUri, client) { + if (this.model.validateRedirectUri) { + return promisify(this.model.validateRedirectUri, 2).call(this.model, redirectUri, client); + } + + return Promise.resolve(client.redirectUris.includes(redirectUri)); +}; /** * Get response type. */ diff --git a/test/integration/handlers/authorize-handler_test.js b/test/integration/handlers/authorize-handler_test.js index 49d2c0d..6ff8621 100644 --- a/test/integration/handlers/authorize-handler_test.js +++ b/test/integration/handlers/authorize-handler_test.js @@ -634,6 +634,65 @@ describe('AuthorizeHandler integration', function() { }); }); + describe('validateRedirectUri()', function() { + it('should support empty model', function() { + const model = { + getAccessToken: function() {}, + getClient: function() {}, + saveAuthorizationCode: function() {} + }; + + const handler = new AuthorizeHandler({ authorizationCodeLifetime: 120, model: model }); + + handler.validateRedirectUri('http://example.com/a', { redirectUris: ['http://example.com/a'] }).should.be.an.instanceOf(Promise); + }); + + it('should support promises', function() { + const model = { + getAccessToken: function() {}, + getClient: function() {}, + saveAuthorizationCode: function() {}, + validateRedirectUri: function() { + return Promise.resolve(true); + } + }; + + const handler = new AuthorizeHandler({ authorizationCodeLifetime: 120, model: model }); + + handler.validateRedirectUri('http://example.com/a', { }).should.be.an.instanceOf(Promise); + }); + + it('should support non-promises', function() { + const model = { + getAccessToken: function() {}, + getClient: function() {}, + saveAuthorizationCode: function() {}, + validateRedirectUri: function() { + return true; + } + }; + + const handler = new AuthorizeHandler({ authorizationCodeLifetime: 120, model: model }); + + handler.validateRedirectUri('http://example.com/a', { }).should.be.an.instanceOf(Promise); + }); + + it('should support callbacks', function() { + const model = { + getAccessToken: function() {}, + getClient: function() {}, + saveAuthorizationCode: function() {}, + validateRedirectUri: function(redirectUri, client, callback) { + callback(null, false); + } + }; + + const handler = new AuthorizeHandler({ authorizationCodeLifetime: 120, model: model }); + + handler.validateRedirectUri('http://example.com/a', { }).should.be.an.instanceOf(Promise); + }); + }); + describe('getClient()', function() { it('should throw an error if `client_id` is missing', function() { const model = { diff --git a/test/unit/handlers/authorize-handler_test.js b/test/unit/handlers/authorize-handler_test.js index c27a9ee..2a44bc6 100644 --- a/test/unit/handlers/authorize-handler_test.js +++ b/test/unit/handlers/authorize-handler_test.js @@ -123,7 +123,7 @@ describe('AuthorizeHandler', function() { model.validateRedirectUri.callCount.should.equal(1); model.validateRedirectUri.firstCall.args.should.have.length(2); model.validateRedirectUri.firstCall.args[0].should.equal(redirect_uri); - model.validateRedirectUri.firstCall.args[1].should.equal(client.redirectUris); + model.validateRedirectUri.firstCall.args[1].should.equal(client); model.validateRedirectUri.firstCall.thisValue.should.equal(model); }) .catch(should.fail); From 3a1e4330a174ede3eae9f8bd79e7a2c6dd073ac6 Mon Sep 17 00:00:00 2001 From: Joren Vandeweyer Date: Sun, 19 Dec 2021 14:31:31 +0100 Subject: [PATCH 5/5] added warning in docs, added extra tests that actually use parameters --- docs/model/spec.rst | 5 ++ .../handlers/authorize-handler_test.js | 2 +- test/unit/handlers/authorize-handler_test.js | 46 +++++++++++++++++++ 3 files changed, 52 insertions(+), 1 deletion(-) diff --git a/docs/model/spec.rst b/docs/model/spec.rst index 46b6494..301ab95 100644 --- a/docs/model/spec.rst +++ b/docs/model/spec.rst @@ -1015,6 +1015,11 @@ This model function is **optional**. If not implemented, the ``redirectUri`` sho Returns ``true`` if the ``redirectUri`` is valid, ``false`` otherwise. **Remarks:** +When implementing this method you should take care of possible security risks related to ``redirectUri``. +.. _rfc6819: https://datatracker.ietf.org/doc/html/rfc6819 + +Section-5.2.3.5 is implemented by default. +.. _Section-5.2.3.5: https://datatracker.ietf.org/doc/html/rfc6819#section-5.2.3.5 :: diff --git a/test/integration/handlers/authorize-handler_test.js b/test/integration/handlers/authorize-handler_test.js index 6ff8621..1ff7721 100644 --- a/test/integration/handlers/authorize-handler_test.js +++ b/test/integration/handlers/authorize-handler_test.js @@ -635,7 +635,7 @@ describe('AuthorizeHandler integration', function() { }); describe('validateRedirectUri()', function() { - it('should support empty model', function() { + it('should support empty method', function() { const model = { getAccessToken: function() {}, getClient: function() {}, diff --git a/test/unit/handlers/authorize-handler_test.js b/test/unit/handlers/authorize-handler_test.js index 2a44bc6..376bc1e 100644 --- a/test/unit/handlers/authorize-handler_test.js +++ b/test/unit/handlers/authorize-handler_test.js @@ -128,5 +128,51 @@ describe('AuthorizeHandler', function() { }) .catch(should.fail); }); + + it('should be successful validation', function () { + const client = { grants: ['authorization_code'], redirectUris: ['http://example.com/cb'] }; + const redirect_uri = 'http://example.com/cb'; + const model = { + getAccessToken: function() {}, + getClient: sinon.stub().returns(client), + saveAuthorizationCode: function() {}, + validateRedirectUri: function (redirectUri, client) { + return client.redirectUris.includes(redirectUri); + } + }; + + const handler = new AuthorizeHandler({ authorizationCodeLifetime: 120, model: model }); + const request = new Request({ body: { client_id: 12345, client_secret: 'secret', redirect_uri }, headers: {}, method: {}, query: {} }); + + return handler.getClient(request) + .then((client) => { + client.should.equal(client); + }); + }); + + it('should be unsuccessful validation', function () { + const client = { grants: ['authorization_code'], redirectUris: ['http://example.com/cb'] }; + const redirect_uri = 'http://example.com/callback'; + const model = { + getAccessToken: function() {}, + getClient: sinon.stub().returns(client), + saveAuthorizationCode: function() {}, + validateRedirectUri: function (redirectUri, client) { + return client.redirectUris.includes(redirectUri); + } + }; + + const handler = new AuthorizeHandler({ authorizationCodeLifetime: 120, model: model }); + const request = new Request({ body: { client_id: 12345, client_secret: 'secret', redirect_uri }, headers: {}, method: {}, query: {} }); + + return handler.getClient(request) + .then(() => { + throw Error('should not resolve'); + }) + .catch((err) => { + err.name.should.equal('invalid_client'); + err.message.should.equal('Invalid client: `redirect_uri` does not match client value'); + }); + }); }); });