Skip to content

Commit e5fa16e

Browse files
authored
Fix uncaught exceptions in acquireTokenSilent (#7073)
A recent change to optimize parallel iframed calls resulted in a regression that logged an uncaught exception to the console in the event that a single iframed call was made and failed. This happened because the stored promise rejected and didn't have a catch handler registered because there is not a parallel call dependent on this promise. This PR resolves this issue by ensuring that the stored promise never rejects but rather resolves with a true/false indicating whether the call succeeded or failed. Fixes #7052
1 parent d556aed commit e5fa16e

File tree

3 files changed

+60
-21
lines changed

3 files changed

+60
-21
lines changed
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 uncaught exceptions in acquireTokenSilent #7073",
4+
"packageName": "@azure/msal-browser",
5+
"email": "thomas.norling@microsoft.com",
6+
"dependentChangeType": "patch"
7+
}

lib/msal-browser/src/controllers/StandardController.ts

Lines changed: 16 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,7 @@ export class StandardController implements IController {
158158
>;
159159

160160
// Active Iframe request
161-
private activeIframeRequest: [Promise<void>, string] | undefined;
161+
private activeIframeRequest: [Promise<boolean>, string] | undefined;
162162

163163
private ssoSilentMeasurement?: InProgressPerformanceEvent;
164164
private acquireTokenByCodeAsyncMeasurement?: InProgressPerformanceEvent;
@@ -2039,13 +2039,11 @@ export class StandardController implements IController {
20392039

20402040
if (shouldTryToResolveSilently) {
20412041
if (!this.activeIframeRequest) {
2042-
let _resolve: () => void,
2043-
_reject: (reason?: AuthError | Error) => void;
2042+
let _resolve: (result: boolean) => void;
20442043
// Always set the active request tracker immediately after checking it to prevent races
20452044
this.activeIframeRequest = [
2046-
new Promise((resolve, reject) => {
2045+
new Promise((resolve) => {
20472046
_resolve = resolve;
2048-
_reject = reject;
20492047
}),
20502048
silentRequest.correlationId,
20512049
];
@@ -2061,11 +2059,11 @@ export class StandardController implements IController {
20612059
silentRequest.correlationId
20622060
)(silentRequest)
20632061
.then((iframeResult) => {
2064-
_resolve();
2062+
_resolve(true);
20652063
return iframeResult;
20662064
})
20672065
.catch((e) => {
2068-
_reject(e);
2066+
_resolve(false);
20692067
throw e;
20702068
})
20712069
.finally(() => {
@@ -2087,20 +2085,11 @@ export class StandardController implements IController {
20872085
awaitIframeCorrelationId: activeCorrelationId,
20882086
});
20892087

2090-
// Await for errors first so we can distinguish errors thrown by activePromise versus errors thrown by .then below
2091-
await activePromise.catch(() => {
2092-
awaitConcurrentIframeMeasure.end({
2093-
success: false,
2094-
});
2095-
this.logger.info(
2096-
`Iframe request with correlationId: ${activeCorrelationId} failed. Interaction is required.`
2097-
);
2098-
// If previous iframe request failed, it's unlikely to succeed this time. Throw original error.
2099-
throw refreshTokenError;
2088+
const activePromiseResult = await activePromise;
2089+
awaitConcurrentIframeMeasure.end({
2090+
success: activePromiseResult,
21002091
});
2101-
2102-
return activePromise.then(() => {
2103-
awaitConcurrentIframeMeasure.end({ success: true });
2092+
if (activePromiseResult) {
21042093
this.logger.verbose(
21052094
`Parallel iframe request with correlationId: ${activeCorrelationId} succeeded. Retrying cache and/or RT redemption`,
21062095
silentRequest.correlationId
@@ -2110,7 +2099,13 @@ export class StandardController implements IController {
21102099
silentRequest,
21112100
cacheLookupPolicy
21122101
);
2113-
});
2102+
} else {
2103+
this.logger.info(
2104+
`Iframe request with correlationId: ${activeCorrelationId} failed. Interaction is required.`
2105+
);
2106+
// If previous iframe request failed, it's unlikely to succeed this time. Throw original error.
2107+
throw refreshTokenError;
2108+
}
21142109
} else {
21152110
// Cache policy set to skip and another iframe request is already in progress
21162111
this.logger.warning(

lib/msal-browser/test/app/PublicClientApplication.spec.ts

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4817,6 +4817,43 @@ describe("PublicClientApplication.ts Class Unit Tests", () => {
48174817
});
48184818
});
48194819

4820+
it("throws iframe error if iframe renewal throws", (done) => {
4821+
const testAccount: AccountInfo = {
4822+
homeAccountId: TEST_DATA_CLIENT_INFO.TEST_HOME_ACCOUNT_ID,
4823+
localAccountId: TEST_DATA_CLIENT_INFO.TEST_UID,
4824+
environment: "login.windows.net",
4825+
tenantId: "testTenantId",
4826+
username: "username@contoso.com",
4827+
};
4828+
4829+
jest.spyOn(
4830+
RefreshTokenClient.prototype,
4831+
"acquireTokenByRefreshToken"
4832+
).mockRejectedValue(
4833+
createInteractionRequiredAuthError(
4834+
InteractionRequiredAuthErrorCodes.refreshTokenExpired
4835+
)
4836+
);
4837+
4838+
const testIframeError = new InteractionRequiredAuthError(
4839+
"interaction_required",
4840+
"interaction is required"
4841+
);
4842+
4843+
jest.spyOn(
4844+
SilentIframeClient.prototype,
4845+
"acquireToken"
4846+
).mockRejectedValue(testIframeError);
4847+
4848+
pca.acquireTokenSilent({
4849+
scopes: ["Scope1"],
4850+
account: testAccount,
4851+
}).catch((e) => {
4852+
expect(e).toEqual(testIframeError);
4853+
done();
4854+
});
4855+
});
4856+
48204857
it("Falls back to silent handler if thrown error is a refresh token expired error", async () => {
48214858
const invalidGrantError: ServerError = new ServerError(
48224859
"invalid_grant",

0 commit comments

Comments
 (0)