From 506227452748bbd9d6e33fb9c3f73f50180c3f79 Mon Sep 17 00:00:00 2001 From: Vijay Budhram Date: Wed, 14 Dec 2022 14:24:32 -0500 Subject: [PATCH] feat(oauth): Support multiple client redirect urls --- .../fxa_oauth/patches/patch-031-32.sql | 5 +++ .../fxa_oauth/patches/patch-032-031.sql | 3 ++ .../databases/fxa_oauth/target-patch.json | 2 +- .../lib/routes/oauth/authorization.js | 33 ++++++++++++++----- .../app/scripts/lib/validate.js | 15 +++++++++ .../fxa-content-server/app/scripts/lib/vat.js | 4 +++ .../app/scripts/models/reliers/oauth.js | 30 +++++++++++++---- .../app/tests/spec/lib/validate.js | 22 +++++++++++++ .../app/tests/spec/models/reliers/oauth.js | 30 ++++++++++++++--- 9 files changed, 125 insertions(+), 19 deletions(-) create mode 100644 packages/db-migrations/databases/fxa_oauth/patches/patch-031-32.sql create mode 100644 packages/db-migrations/databases/fxa_oauth/patches/patch-032-031.sql diff --git a/packages/db-migrations/databases/fxa_oauth/patches/patch-031-32.sql b/packages/db-migrations/databases/fxa_oauth/patches/patch-031-32.sql new file mode 100644 index 0000000000..f095c88384 --- /dev/null +++ b/packages/db-migrations/databases/fxa_oauth/patches/patch-031-32.sql @@ -0,0 +1,5 @@ +-- To support multiple redirect uris, we are changing the column form varchar 256 to 2048 +-- should enough to store 20 redirect uris. +ALTER TABLE `clients` CHANGE COLUMN `redirectUri` `redirectUri` VARCHAR(2048) NULL DEFAULT NULL; + +UPDATE dbMetadata SET value = '32' WHERE name = 'schema-patch-level'; diff --git a/packages/db-migrations/databases/fxa_oauth/patches/patch-032-031.sql b/packages/db-migrations/databases/fxa_oauth/patches/patch-032-031.sql new file mode 100644 index 0000000000..a16ad9f8a4 --- /dev/null +++ b/packages/db-migrations/databases/fxa_oauth/patches/patch-032-031.sql @@ -0,0 +1,3 @@ +-- ALTER TABLE `clients` CHANGE COLUMN `redirectUri` `redirectUri` VARCHAR(256) NULL DEFAULT NULL; + +-- UPDATE dbMetadata SET value = '31' WHERE name = 'schema-patch-level'; diff --git a/packages/db-migrations/databases/fxa_oauth/target-patch.json b/packages/db-migrations/databases/fxa_oauth/target-patch.json index d3db69fad7..ac7b76bb82 100644 --- a/packages/db-migrations/databases/fxa_oauth/target-patch.json +++ b/packages/db-migrations/databases/fxa_oauth/target-patch.json @@ -1,3 +1,3 @@ { - "level": 31 + "level": 32 } diff --git a/packages/fxa-auth-server/lib/routes/oauth/authorization.js b/packages/fxa-auth-server/lib/routes/oauth/authorization.js index 77e3bcffd6..dc477f05fb 100644 --- a/packages/fxa-auth-server/lib/routes/oauth/authorization.js +++ b/packages/fxa-auth-server/lib/routes/oauth/authorization.js @@ -126,14 +126,26 @@ module.exports = ({ log, oauthDB, config }) => { } function validateClientDetails(client, payload) { - // Clients must use a single specific redirect_uri, - // but they're allowed to not provide one and have us fill it in automatically. - payload.redirect_uri = payload.redirect_uri || client.redirectUri; - if (payload.redirect_uri !== client.redirectUri) { - log.debug('redirect.mismatch', { - param: payload.redirect_uri, - registered: client.redirectUri, - }); + if (!payload.redirect_uri && !client.redirectUri) { + throw OauthError.incorrectRedirect(); + } + + // Starting in train-248, FxA added the ability for an OAuth client to support + // multiple redirect uris (comma separated list). The authorization flow redirect uri + // must match one of these exactly. Pattern matching is not supported. + const redirectUris = client.redirectUri.split(','); + + // Authorization flow must use a single specific redirect_uri, + // but allowed to not provide one and have us fill it in automatically. + payload.redirect_uri = payload.redirect_uri || redirectUris[0]; + + const validUri = redirectUris.some((uri) => { + if (uri === payload.redirect_uri) { + return true; + } + }); + + if (!validUri) { if ( config.oauthServer.localRedirects && isLocalHost(payload.redirect_uri) @@ -142,6 +154,11 @@ module.exports = ({ log, oauthDB, config }) => { } else { throw OauthError.incorrectRedirect(payload.redirect_uri); } + } else { + log.debug('redirect.mismatch', { + param: payload.redirect_uri, + registered: client.redirectUri, + }); } } diff --git a/packages/fxa-content-server/app/scripts/lib/validate.js b/packages/fxa-content-server/app/scripts/lib/validate.js index d4ab241728..a691d0ae92 100644 --- a/packages/fxa-content-server/app/scripts/lib/validate.js +++ b/packages/fxa-content-server/app/scripts/lib/validate.js @@ -234,6 +234,21 @@ var Validate = { return JWT_STRING.test(value); }, + /** + * Check whether `redirectUri` string contains only valid uris. + * Clients can specify a comma separated list and this checks to see + * if each is a valid uri. + * + * @param {String} redirectUrisStr + * @returns {Boolean} + */ + isRedirectUriValid(redirectUrisStr) { + const redirectUris = redirectUrisStr.split(','); + return redirectUris.every((value) => { + return urlRegEx.test(value); + }); + }, + /** * Check whether `newsletters` contains only valid newsletter slugs. * diff --git a/packages/fxa-content-server/app/scripts/lib/vat.js b/packages/fxa-content-server/app/scripts/lib/vat.js index 01347ec3f8..61af124580 100644 --- a/packages/fxa-content-server/app/scripts/lib/vat.js +++ b/packages/fxa-content-server/app/scripts/lib/vat.js @@ -44,6 +44,10 @@ Vat.register( 'verificationRedirect', Vat.string().test(Validate.isVerificationRedirectValid) ); +Vat.register( + 'redirectUri', + Vat.string().required().test(Validate.isRedirectUriValid) +); // depends on hex, must come afterwards Vat.register('clientId', Vat.hex()); diff --git a/packages/fxa-content-server/app/scripts/models/reliers/oauth.js b/packages/fxa-content-server/app/scripts/models/reliers/oauth.js index f0925382fc..5684432e1a 100644 --- a/packages/fxa-content-server/app/scripts/models/reliers/oauth.js +++ b/packages/fxa-content-server/app/scripts/models/reliers/oauth.js @@ -21,7 +21,9 @@ const CLIENT_INFO_SCHEMA = { id: Vat.hex().required().renameTo('clientId'), image_uri: Vat.url().allow('').renameTo('imageUri'), name: Vat.string().required().min(1).renameTo('serviceName'), - redirect_uri: Vat.url().required().renameTo('redirectUri'), + + // This can be a single uri or comma separated list + redirect_uri: Vat.redirectUri().renameTo('redirectUri'), trusted: Vat.boolean().required(), }; @@ -246,8 +248,8 @@ var OAuthRelier = Relier.extend({ * * Verification (email) flows do not have a redirect uri, nothing to validate */ - if (!isCorrectRedirect(this.get('redirectUri'), result.redirectUri)) { - // if provided redirect uri doesn't match with client info then throw + if (!isCorrectRedirect(this.get('redirectUri'), result)) { + // if provided redirect uri doesn't match with any client redirectUri then throw throw OAuthErrors.toError('INCORRECT_REDIRECT'); } @@ -270,12 +272,28 @@ var OAuthRelier = Relier.extend({ } ); - function isCorrectRedirect(queryRedirectUri, resultRedirectUri) { + function isCorrectRedirect(queryRedirectUri, client) { + // If RP doesn't specify redirectUri, we default to the first redirectUri + // for the client + const redirectUris = client.redirectUri.split(','); if (!queryRedirectUri) { + client.redirectUri = redirectUris[0]; return true; - } else if (queryRedirectUri === resultRedirectUri) { + } + + const hasRedirectUri = redirectUris.some((uri) => { + if (queryRedirectUri === uri) { + return true; + } + }); + if (hasRedirectUri) { + client.redirectUri = queryRedirectUri; return true; - } else if ( + } + + // Pairing has a special redirectUri that deep links into the specific + // mobile app + if ( queryRedirectUri === Constants.DEVICE_PAIRING_AUTHORITY_REDIRECT_URI ) { return true; diff --git a/packages/fxa-content-server/app/tests/spec/lib/validate.js b/packages/fxa-content-server/app/tests/spec/lib/validate.js index 1f7f35e366..ef8c8a795c 100644 --- a/packages/fxa-content-server/app/tests/spec/lib/validate.js +++ b/packages/fxa-content-server/app/tests/spec/lib/validate.js @@ -297,4 +297,26 @@ describe('lib/validate', function () { assert.isTrue(Validate.isUtmValid('marketing-snippet')); }); }); + + describe('isRedirectUriValid', () => { + it('returns false if any redirect uri is not valid', () => { + const testString = 'https://localhost,http://'; + assert.isFalse(Validate.isRedirectUriValid(testString)); + }); + + it('returns false for invalid uri', () => { + const testString = 'c'; + assert.isFalse(Validate.isRedirectUriValid(testString)); + }); + + it('returns true for valid uris', () => { + const testString = 'https://localhost,http://mozilla.org'; + assert.isTrue(Validate.isRedirectUriValid(testString)); + }); + + it('returns true for single valid uri', () => { + const testString = 'https://localhost'; + assert.isTrue(Validate.isRedirectUriValid(testString)); + }); + }); }); diff --git a/packages/fxa-content-server/app/tests/spec/models/reliers/oauth.js b/packages/fxa-content-server/app/tests/spec/models/reliers/oauth.js index 846b5574d9..0c11bb9021 100644 --- a/packages/fxa-content-server/app/tests/spec/models/reliers/oauth.js +++ b/packages/fxa-content-server/app/tests/spec/models/reliers/oauth.js @@ -44,6 +44,8 @@ describe('models/reliers/oauth', () => { var SCOPE_WITH_EXTRAS = 'profile:email profile:uid profile:non_whitelisted'; var SCOPE_WITH_OPENID = 'profile:email profile:uid openid'; var SERVER_REDIRECT_URI = 'http://localhost:8080/api/oauth'; + var SERVER_REDIRECT_URIS = + 'http://localhost:8080/api/oauth,https://www.mozilla.org'; var SERVICE = 'service'; var SERVICE_NAME = '123Done'; var STATE = 'fakestatetoken'; @@ -181,6 +183,22 @@ describe('models/reliers/oauth', () => { assert.equal(err.param, 'code_challenge_method'); }); }); + + it('throws if incorrect `redirectUri` is specified', () => { + windowMock.location.search = toSearchString({ + access_type: ACCESS_TYPE, + action: ACTION, + client_id: CLIENT_ID, + prompt: PROMPT, + redirect_uri: 'http://www.notvalidclienturl.com', + scope: SCOPE, + state: STATE, + }); + + return relier.fetch().then(assert.fail, (err) => { + assert.isTrue(OAuthErrors.is(err, 'INCORRECT_REDIRECT')); + }); + }); }); describe('verification flow', () => { @@ -403,7 +421,12 @@ describe('models/reliers/oauth', () => { const invalidValues = ['', ' ', 'invalid']; testInvalidQueryParams('prompt', invalidValues); - const validValues = [undefined, OAuthPrompt.CONSENT, OAuthPrompt.NONE]; + const validValues = [ + undefined, + OAuthPrompt.CONSENT, + OAuthPrompt.NONE, + OAuthPrompt.LOGIN, + ]; testValidQueryParams('prompt', validValues, 'prompt', validValues); }); @@ -575,7 +598,7 @@ describe('models/reliers/oauth', () => { testMissingClientInfoValue('redirect_uri'); }); - var invalidClientInfoValues = ['', ' ']; + var invalidClientInfoValues = ['', ' ', ',', 'http://moz.org,']; testInvalidClientInfoValues('redirect_uri', invalidClientInfoValues); }); @@ -1242,7 +1265,7 @@ describe('models/reliers/oauth', () => { var clientInfo = { id: CLIENT_ID, name: SERVICE_NAME, - redirect_uri: SERVER_REDIRECT_URI, + redirect_uri: SERVER_REDIRECT_URIS, trusted: isTrusted, }; @@ -1260,7 +1283,6 @@ describe('models/reliers/oauth', () => { function fetchExpectError(params) { windowMock.location.search = toSearchString(params); - return relier.fetch().then(assert.fail, function (_err) { err = _err; });