Skip to content

Commit bd0b1e1

Browse files
Fix JSON object conversion in PlatformDOMRequest v5 (#8348)
Update the getDOMExtraParams method to stringify JS object properly and ignore empty, null and undefined values from the extraParameters. --------- Co-authored-by: Thomas Norling <thomas.norling@microsoft.com>
1 parent ec68aae commit bd0b1e1

3 files changed

Lines changed: 119 additions & 20 deletions

File tree

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
{
2+
"type": "patch",
3+
"comment": "Fix JSON object conversion in PlatformDOMRequest [#8348](https://github.com/AzureAD/microsoft-authentication-library-for-js/pull/8348)",
4+
"packageName": "@azure/msal-browser",
5+
"email": "lalimasharda@microsoft.com",
6+
"dependentChangeType": "patch"
7+
}

lib/msal-browser/src/broker/nativeBroker/PlatformAuthDOMHandler.ts

Lines changed: 34 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -148,16 +148,21 @@ export class PlatformAuthDOMHandler implements IPlatformAuthHandler {
148148
...remainingProperties
149149
} = request;
150150

151-
const validExtraParameters: DOMExtraParameters =
152-
this.getDOMExtraParams(remainingProperties);
151+
const validExtraParameters: DOMExtraParameters = this.getDOMExtraParams(
152+
remainingProperties,
153+
correlationId
154+
);
153155

154156
const platformDOMRequest: PlatformDOMTokenRequest = {
155157
accountId: accountId,
156158
brokerId: this.getExtensionId(),
157159
authority: authority,
158160
clientId: clientId,
159161
correlationId: correlationId || this.correlationId,
160-
extraParameters: { ...extraParameters, ...validExtraParameters },
162+
extraParameters: {
163+
...extraParameters,
164+
...validExtraParameters,
165+
},
161166
isSecurityTokenService: false,
162167
redirectUri: redirectUri,
163168
scope: scope,
@@ -245,20 +250,32 @@ export class PlatformAuthDOMHandler implements IPlatformAuthHandler {
245250
}
246251

247252
private getDOMExtraParams(
248-
extraParameters: Record<string, unknown>
253+
extraParameters: Record<string, unknown>,
254+
correlationId: string
249255
): DOMExtraParameters {
250-
const stringifiedParams = Object.entries(extraParameters).reduce(
251-
(record, [key, value]) => {
252-
record[key] = String(value);
253-
return record;
254-
},
255-
{} as StringDict
256-
);
257-
258-
const validExtraParams: DOMExtraParameters = {
259-
...stringifiedParams,
260-
};
261-
262-
return validExtraParams;
256+
try {
257+
const stringifiedProperties: StringDict = {};
258+
for (const [key, value] of Object.entries(extraParameters)) {
259+
if (!value) {
260+
continue;
261+
}
262+
if (typeof value === "object") {
263+
stringifiedProperties[key] = JSON.stringify(value);
264+
} else {
265+
stringifiedProperties[key] = String(value);
266+
}
267+
}
268+
return stringifiedProperties;
269+
} catch (e) {
270+
this.logger.error(
271+
`'${this.platformAuthType}' - Error stringifying extra parameters`,
272+
correlationId
273+
);
274+
this.logger.errorPii(
275+
`'${this.platformAuthType}' - Error stringifying extra parameters: '${e}'`,
276+
correlationId
277+
);
278+
return {};
279+
}
263280
}
264281
}

lib/msal-browser/test/broker/PlatformAuthDOMHandler.spec.ts

Lines changed: 78 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ import {
1616
} from "../utils/StringConstants.js";
1717
import { PlatformAuthRequest } from "../../src/broker/nativeBroker/PlatformAuthRequest.js";
1818
import { NativeAuthError } from "../../src/error/NativeAuthError.js";
19-
import { sign } from "crypto";
2019

2120
describe("PlatformAuthDOMHandler tests", () => {
2221
let performanceClient: IPerformanceClient;
@@ -533,9 +532,16 @@ describe("PlatformAuthDOMHandler tests", () => {
533532
nonce: "test-nonce",
534533
claims: "test-claims",
535534
instanceAware: true,
536-
windowTitleSubstring: "test-window-substring",
535+
windowTitleSubstring: null,
537536
extendedExpiryToken: true,
538537
signPopToken: true,
538+
account: {
539+
nativeAccountId: "native-test-id",
540+
userName: "test-user",
541+
name: "Test User",
542+
username: "testest@test.com",
543+
},
544+
someArrayParam: ["value1", "value2"],
539545
};
540546
const domExtraParams =
541547
//@ts-ignore
@@ -545,10 +551,79 @@ describe("PlatformAuthDOMHandler tests", () => {
545551
nonce: "test-nonce",
546552
claims: "test-claims",
547553
instanceAware: "true",
548-
windowTitleSubstring: "test-window-substring",
549554
extendedExpiryToken: "true",
550555
signPopToken: "true",
556+
account: JSON.stringify(testExtraParameters.account),
557+
someArrayParam: '["value1","value2"]',
558+
});
559+
});
560+
561+
it("should omit undefined values", async () => {
562+
getSupportedContractsMock.mockResolvedValue([
563+
PlatformAuthConstants.PLATFORM_DOM_APIS,
564+
]);
565+
const platformAuthDOMHandler =
566+
await PlatformAuthDOMHandler.createProvider(
567+
logger,
568+
performanceClient,
569+
"test-correlation-id"
570+
);
571+
572+
const testExtraParameters = {
573+
prompt: Constants.PromptValue.NONE,
574+
nonce: "test-nonce",
575+
claims: "test-claims",
576+
instanceAware: undefined,
577+
};
578+
579+
const domExtraParams =
580+
//@ts-ignore
581+
platformAuthDOMHandler.getDOMExtraParams(testExtraParameters);
582+
expect(domExtraParams).toEqual({
583+
prompt: "none",
584+
nonce: "test-nonce",
585+
claims: "test-claims",
551586
});
587+
expect(domExtraParams).not.toHaveProperty("instanceAware");
588+
});
589+
590+
it("should catch JSON.stringify error and return empty object", async () => {
591+
getSupportedContractsMock.mockResolvedValue([
592+
PlatformAuthConstants.PLATFORM_DOM_APIS,
593+
]);
594+
const platformAuthDOMHandler =
595+
await PlatformAuthDOMHandler.createProvider(
596+
logger,
597+
performanceClient,
598+
"test-correlation-id"
599+
);
600+
601+
// Create a circular reference that will cause JSON.stringify to throw
602+
const circularObj: any = { name: "test" };
603+
circularObj.self = circularObj;
604+
605+
const testExtraParameters = {
606+
prompt: Constants.PromptValue.NONE,
607+
problemParam: circularObj,
608+
};
609+
610+
console.log("Testing circular object:", testExtraParameters);
611+
612+
const loggerErrorSpy = jest.spyOn(logger, "error");
613+
const loggerErrorPiiSpy = jest.spyOn(logger, "errorPii");
614+
615+
const domExtraParams =
616+
//@ts-ignore
617+
platformAuthDOMHandler.getDOMExtraParams(testExtraParameters);
618+
619+
expect(domExtraParams).toEqual({});
620+
expect(loggerErrorSpy.mock.calls[0][0]).toContain(
621+
"'PlatformAuthDOMHandler' - Error stringifying extra parameters"
622+
);
623+
expect(loggerErrorPiiSpy).toHaveBeenCalled();
624+
expect(loggerErrorPiiSpy.mock.calls[0][0]).toContain(
625+
"'PlatformAuthDOMHandler' - Error stringifying extra parameters"
626+
);
552627
});
553628
});
554629
});

0 commit comments

Comments
 (0)