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.

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": "[email protected]",
+  "dependentChangeType": "patch"
+}

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);
+            }
         });
     }
 

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<string, string>
-    ): 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] !== "")
-        );
-    }
 }

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",
+            ]);
+        });
+    });
 });