From 0a905a98bb3926ae8d0e34446ba740177c99a359 Mon Sep 17 00:00:00 2001 From: Thomas Norling Date: Tue, 18 Jun 2024 13:26:23 -0700 Subject: [PATCH] Fix extraQueryParameters being dropped from request (#7166) In cases where a field included in extraQueryParameters already exists on the query string it was being deleted from the object which prevented it from being used on the next request i.e. removed on /authorize, no longer exists on /token. This caused a bug in Pairwise broker and NAA flows ultimately resulting in the server throwing an "unauthorized_client" error. This PR fixes this bug. Note: Objects in JavaScript are passed by reference which is why the original implementation caused this bug. There are other places in the code where we similarly edit an object that has been passed in. A separate work item has been created to address those other instances in a separate PR & turn on a lint rule to prevent this pattern in the future. --- ...-1e1b6efa-031c-4454-8c25-e62ef4ef523e.json | 7 ++ .../src/request/RequestParameterBuilder.ts | 10 +-- .../src/request/RequestValidator.ts | 26 ------- .../request/RequestParameterBuilder.spec.ts | 75 +++++++++++++++++++ 4 files changed, 86 insertions(+), 32 deletions(-) create mode 100644 change/@azure-msal-common-1e1b6efa-031c-4454-8c25-e62ef4ef523e.json diff --git a/change/@azure-msal-common-1e1b6efa-031c-4454-8c25-e62ef4ef523e.json b/change/@azure-msal-common-1e1b6efa-031c-4454-8c25-e62ef4ef523e.json new file mode 100644 index 0000000000..d28f6b45cf --- /dev/null +++ b/change/@azure-msal-common-1e1b6efa-031c-4454-8c25-e62ef4ef523e.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "Fix extraQueryParameters being dropped from request", + "packageName": "@azure/msal-common", + "email": "thomas.norling@microsoft.com", + "dependentChangeType": "patch" +} diff --git a/lib/msal-common/src/request/RequestParameterBuilder.ts b/lib/msal-common/src/request/RequestParameterBuilder.ts index 9770fb53ce..26d146ef03 100644 --- a/lib/msal-common/src/request/RequestParameterBuilder.ts +++ b/lib/msal-common/src/request/RequestParameterBuilder.ts @@ -465,12 +465,10 @@ export class RequestParameterBuilder { * @param eQParams */ addExtraQueryParameters(eQParams: StringDict): void { - const sanitizedEQParams = RequestValidator.sanitizeEQParams( - eQParams, - this.parameters - ); - Object.keys(sanitizedEQParams).forEach((key) => { - this.parameters.set(key, eQParams[key]); + Object.entries(eQParams).forEach(([key, value]) => { + if (!this.parameters.has(key) && value) { + this.parameters.set(key, value); + } }); } diff --git a/lib/msal-common/src/request/RequestValidator.ts b/lib/msal-common/src/request/RequestValidator.ts index 1a8d86dcdd..011eece4bd 100644 --- a/lib/msal-common/src/request/RequestValidator.ts +++ b/lib/msal-common/src/request/RequestValidator.ts @@ -8,7 +8,6 @@ import { ClientConfigurationErrorCodes, } from "../error/ClientConfigurationError"; import { PromptValue, CodeChallengeMethodValues } from "../utils/Constants"; -import { StringDict } from "../utils/MsalTypes"; /** * Validates server consumable params from the "request" objects @@ -88,29 +87,4 @@ export class RequestValidator { ); } } - - /** - * Removes unnecessary, duplicate, and empty string query parameters from extraQueryParameters - * @param request - */ - static sanitizeEQParams( - eQParams: StringDict, - queryParams: Map - ): StringDict { - if (!eQParams) { - return {}; - } - - // Remove any query parameters already included in SSO params - queryParams.forEach((_value, key) => { - if (eQParams[key]) { - delete eQParams[key]; - } - }); - - // remove empty string parameters - return Object.fromEntries( - Object.entries(eQParams).filter((kv) => kv[1] !== "") - ); - } } diff --git a/lib/msal-common/test/request/RequestParameterBuilder.spec.ts b/lib/msal-common/test/request/RequestParameterBuilder.spec.ts index 56a3134891..c37bd15482 100644 --- a/lib/msal-common/test/request/RequestParameterBuilder.spec.ts +++ b/lib/msal-common/test/request/RequestParameterBuilder.spec.ts @@ -620,4 +620,79 @@ describe("RequestParameterBuilder unit tests", () => { ); }); }); + + describe("addExtraQueryParameters tests", () => { + it("adds extra query parameters to the request", () => { + const requestParameterBuilder = new RequestParameterBuilder(); + requestParameterBuilder.addClientId(TEST_CONFIG.MSAL_CLIENT_ID); + const eqp = { + testKey1: "testVal1", + testKey2: "testVal2", + }; + + requestParameterBuilder.addExtraQueryParameters(eqp); + const expectedString = `client_id=${TEST_CONFIG.MSAL_CLIENT_ID}&testKey1=testVal1&testKey2=testVal2`; + + expect(requestParameterBuilder.createQueryString()).toBe( + expectedString + ); + }); + + it("Does not add extra query parameters if they are empty", () => { + const requestParameterBuilder = new RequestParameterBuilder(); + requestParameterBuilder.addClientId(TEST_CONFIG.MSAL_CLIENT_ID); + const eqp = { + testKey1: "testVal1", + testKey2: "testVal2", + testKey3: "", + }; + + requestParameterBuilder.addExtraQueryParameters(eqp); + const expectedString = `client_id=${TEST_CONFIG.MSAL_CLIENT_ID}&testKey1=testVal1&testKey2=testVal2`; + + expect(requestParameterBuilder.createQueryString()).toBe( + expectedString + ); + }); + + it("Does not add extra query parameters if they already exist in the request", () => { + const requestParameterBuilder = new RequestParameterBuilder(); + requestParameterBuilder.addClientId(TEST_CONFIG.MSAL_CLIENT_ID); + const eqp = { + testKey1: "testVal1", + testKey2: "testVal2", + client_id: "some-other-client-id", + }; + + requestParameterBuilder.addExtraQueryParameters(eqp); + const expectedString = `client_id=${TEST_CONFIG.MSAL_CLIENT_ID}&testKey1=testVal1&testKey2=testVal2`; + + expect(requestParameterBuilder.createQueryString()).toBe( + expectedString + ); + }); + + it("Does not mutate the original extraQueryParameters object", () => { + const requestParameterBuilder = new RequestParameterBuilder(); + requestParameterBuilder.addClientId(TEST_CONFIG.MSAL_CLIENT_ID); + const eqp = { + testKey1: "testVal1", + testKey2: "testVal2", + client_id: "some-other-client-id", + }; + + requestParameterBuilder.addExtraQueryParameters(eqp); + + expect(Object.keys(eqp)).toEqual([ + "testKey1", + "testKey2", + "client_id", + ]); + expect(Object.values(eqp)).toEqual([ + "testVal1", + "testVal2", + "some-other-client-id", + ]); + }); + }); });