Fix a bug in handleRedirectPromise when invoked after logoutRedirect (#7680)

konstantin-msft · web-flow · commit 972b9938c6c5 · 2025-04-03T14:38:54.000-04:00
- Add interaction type to "interaction_in_progress" temporary key to
return from "handleRedirectPromise" early if invoked after signout. This
fixes a bug with the login flow being blocked after user logs out and
improves sign-out perf.
- Add try-catch block to the first parst of "handleRedirectPromise" to
make sure users can recover from "no_token_request_cache_error" if
session storage gets cleared during page navigation.
- Simplify redirect telemetry instrumentation.
diff --git a/change/@azure-msal-browser-89e5a8e4-9268-4c87-8e68-a8d783abd163.json b/change/@azure-msal-browser-89e5a8e4-9268-4c87-8e68-a8d783abd163.json
@@ -0,0 +1,7 @@
+{
+  "type": "minor",
+  "comment": "Fix a bug in handleRedirectPromise when invoked after logoutRedirect #7680",
+  "packageName": "@azure/msal-browser",
+  "email": "kshabelko@microsoft.com",
+  "dependentChangeType": "patch"
+}
diff --git a/lib/msal-browser/src/cache/BrowserCacheManager.ts b/lib/msal-browser/src/cache/BrowserCacheManager.ts
@@ -4,49 +4,50 @@
  */
 
 import {
-    Constants,
-    PersistentCacheKeys,
-    StringUtils,
-    ICrypto,
-    AccountEntity,
-    IdTokenEntity,
     AccessTokenEntity,
-    RefreshTokenEntity,
-    AppMetadataEntity,
-    CacheManager,
-    ServerTelemetryEntity,
-    ThrottlingEntity,
-    Logger,
-    AuthorityMetadataEntity,
-    DEFAULT_CRYPTO_IMPLEMENTATION,
+    AccountEntity,
     AccountInfo,
     ActiveAccountFilters,
-    TokenKeys,
-    CredentialType,
-    CacheRecord,
+    AppMetadataEntity,
     AuthenticationScheme,
-    createClientAuthError,
+    AuthorityMetadataEntity,
+    CacheError,
+    CacheHelpers,
+    CacheManager,
+    CacheRecord,
     ClientAuthErrorCodes,
-    PerformanceEvents,
+    CommonAuthorizationUrlRequest,
+    Constants,
+    createClientAuthError,
+    CredentialType,
+    DEFAULT_CRYPTO_IMPLEMENTATION,
+    ICrypto,
+    IdTokenEntity,
+    invokeAsync,
     IPerformanceClient,
+    Logger,
+    PerformanceEvents,
+    PersistentCacheKeys,
+    RefreshTokenEntity,
+    ServerTelemetryEntity,
     StaticAuthorityOptions,
-    CacheHelpers,
     StoreInCache,
-    CacheError,
-    invokeAsync,
+    StringUtils,
+    ThrottlingEntity,
     TimeUtils,
-    CommonAuthorizationUrlRequest,
+    TokenKeys,
 } from "@azure/msal-common/browser";
 import { CacheOptions } from "../config/Configuration.js";
 import {
-    createBrowserAuthError,
     BrowserAuthErrorCodes,
+    createBrowserAuthError,
 } from "../error/BrowserAuthError.js";
 import {
     BrowserCacheLocation,
-    TemporaryCacheKeys,
     InMemoryCacheKeys,
+    INTERACTION_TYPE,
     StaticCacheKeys,
+    TemporaryCacheKeys,
 } from "../utils/BrowserConstants.js";
 import { LocalStorage } from "./LocalStorage.js";
 import { SessionStorage } from "./SessionStorage.js";
@@ -1187,7 +1188,7 @@ export class BrowserCacheManager extends CacheManager {
     }
 
     isInteractionInProgress(matchClientId?: boolean): boolean {
-        const clientId = this.getInteractionInProgress();
+        const clientId = this.getInteractionInProgress()?.clientId;
 
         if (matchClientId) {
             return clientId === this.clientId;
@@ -1196,12 +1197,19 @@ export class BrowserCacheManager extends CacheManager {
         }
     }
 
-    getInteractionInProgress(): string | null {
+    getInteractionInProgress(): {
+        clientId: string;
+        type: INTERACTION_TYPE;
+    } | null {
         const key = `${Constants.CACHE_PREFIX}.${TemporaryCacheKeys.INTERACTION_STATUS_KEY}`;
-        return this.getTemporaryCache(key, false);
+        const value = this.getTemporaryCache(key, false);
+        return value ? JSON.parse(value) : null;
     }
 
-    setInteractionInProgress(inProgress: boolean): void {
+    setInteractionInProgress(
+        inProgress: boolean,
+        type: INTERACTION_TYPE = INTERACTION_TYPE.SIGNIN
+    ): void {
         // Ensure we don't overwrite interaction in progress for a different clientId
         const key = `${Constants.CACHE_PREFIX}.${TemporaryCacheKeys.INTERACTION_STATUS_KEY}`;
         if (inProgress) {
@@ -1211,11 +1219,15 @@ export class BrowserCacheManager extends CacheManager {
                 );
             } else {
                 // No interaction is in progress
-                this.setTemporaryCache(key, this.clientId, false);
+                this.setTemporaryCache(
+                    key,
+                    JSON.stringify({ clientId: this.clientId, type }),
+                    false
+                );
             }
         } else if (
             !inProgress &&
-            this.getInteractionInProgress() === this.clientId
+            this.getInteractionInProgress()?.clientId === this.clientId
         ) {
             this.removeTemporaryItem(key);
         }
diff --git a/lib/msal-browser/src/controllers/StandardController.ts b/lib/msal-browser/src/controllers/StandardController.ts
@@ -45,6 +45,7 @@ import {
     DEFAULT_REQUEST,
     BrowserConstants,
     iFrameRenewalPolicies,
+    INTERACTION_TYPE,
 } from "../utils/BrowserConstants.js";
 import * as BrowserUtils from "../utils/BrowserUtils.js";
 import { RedirectRequest } from "../request/RedirectRequest.js";
@@ -443,6 +444,16 @@ export class StandardController implements IController {
             return null;
         }
 
+        const interactionType =
+            this.browserStorage.getInteractionInProgress()?.type;
+        if (interactionType === INTERACTION_TYPE.SIGNOUT) {
+            this.logger.verbose(
+                "handleRedirectPromise removing interaction_in_progress flag and returning null after sign-out"
+            );
+            this.browserStorage.setInteractionInProgress(false);
+            return Promise.resolve(null);
+        }
+
         const loggedInAccounts = this.getAllAccounts();
         const platformBrokerRequest: NativeTokenRequest | null =
             this.browserStorage.getCachedNativeRequest();
@@ -455,63 +466,68 @@ export class StandardController implements IController {
             ) &&
             this.nativeExtensionProvider &&
             !hash;
-        let rootMeasurement = this.performanceClient.startMeasurement(
-            PerformanceEvents.AcquireTokenRedirect,
-            platformBrokerRequest?.correlationId || ""
-        );
+        let rootMeasurement: InProgressPerformanceEvent;
         this.eventHandler.emitEvent(
             EventType.HANDLE_REDIRECT_START,
             InteractionType.Redirect
         );
 
         let redirectResponse: Promise<AuthenticationResult | null>;
-        if (useNative && this.nativeExtensionProvider) {
-            this.logger.trace(
-                "handleRedirectPromise - acquiring token from native platform"
-            );
-            const nativeClient = new NativeInteractionClient(
-                this.config,
-                this.browserStorage,
-                this.browserCrypto,
-                this.logger,
-                this.eventHandler,
-                this.navigationClient,
-                ApiId.handleRedirectPromise,
-                this.performanceClient,
-                this.nativeExtensionProvider,
-                platformBrokerRequest.accountId,
-                this.nativeInternalStorage,
-                platformBrokerRequest.correlationId
-            );
+        try {
+            if (useNative && this.nativeExtensionProvider) {
+                rootMeasurement = this.performanceClient.startMeasurement(
+                    PerformanceEvents.AcquireTokenRedirect,
+                    platformBrokerRequest?.correlationId || ""
+                );
+                this.logger.trace(
+                    "handleRedirectPromise - acquiring token from native platform"
+                );
+                const nativeClient = new NativeInteractionClient(
+                    this.config,
+                    this.browserStorage,
+                    this.browserCrypto,
+                    this.logger,
+                    this.eventHandler,
+                    this.navigationClient,
+                    ApiId.handleRedirectPromise,
+                    this.performanceClient,
+                    this.nativeExtensionProvider,
+                    platformBrokerRequest.accountId,
+                    this.nativeInternalStorage,
+                    platformBrokerRequest.correlationId
+                );
 
-            redirectResponse = invokeAsync(
-                nativeClient.handleRedirectPromise.bind(nativeClient),
-                PerformanceEvents.HandleNativeRedirectPromiseMeasurement,
-                this.logger,
-                this.performanceClient,
-                rootMeasurement.event.correlationId
-            )(this.performanceClient, rootMeasurement.event.correlationId);
-        } else {
-            const [standardRequest, codeVerifier] =
-                this.browserStorage.getCachedRequest();
-            const correlationId = standardRequest.correlationId;
-            // Reset rootMeasurement now that we have correlationId
-            rootMeasurement.discard();
-            rootMeasurement = this.performanceClient.startMeasurement(
-                PerformanceEvents.AcquireTokenRedirect,
-                correlationId
-            );
-            this.logger.trace(
-                "handleRedirectPromise - acquiring token from web flow"
-            );
-            const redirectClient = this.createRedirectClient(correlationId);
-            redirectResponse = invokeAsync(
-                redirectClient.handleRedirectPromise.bind(redirectClient),
-                PerformanceEvents.HandleRedirectPromiseMeasurement,
-                this.logger,
-                this.performanceClient,
-                rootMeasurement.event.correlationId
-            )(hash, standardRequest, codeVerifier, rootMeasurement);
+                redirectResponse = invokeAsync(
+                    nativeClient.handleRedirectPromise.bind(nativeClient),
+                    PerformanceEvents.HandleNativeRedirectPromiseMeasurement,
+                    this.logger,
+                    this.performanceClient,
+                    rootMeasurement.event.correlationId
+                )(this.performanceClient, rootMeasurement.event.correlationId);
+            } else {
+                const [standardRequest, codeVerifier] =
+                    this.browserStorage.getCachedRequest();
+                const correlationId = standardRequest.correlationId;
+                // Reset rootMeasurement now that we have correlationId
+                rootMeasurement = this.performanceClient.startMeasurement(
+                    PerformanceEvents.AcquireTokenRedirect,
+                    correlationId
+                );
+                this.logger.trace(
+                    "handleRedirectPromise - acquiring token from web flow"
+                );
+                const redirectClient = this.createRedirectClient(correlationId);
+                redirectResponse = invokeAsync(
+                    redirectClient.handleRedirectPromise.bind(redirectClient),
+                    PerformanceEvents.HandleRedirectPromiseMeasurement,
+                    this.logger,
+                    this.performanceClient,
+                    rootMeasurement.event.correlationId
+                )(hash, standardRequest, codeVerifier, rootMeasurement);
+            }
+        } catch (e) {
+            this.browserStorage.resetRequestCache();
+            throw e;
         }
 
         return redirectResponse
@@ -657,7 +673,10 @@ export class StandardController implements IController {
         const isLoggedIn = this.getAllAccounts().length > 0;
         try {
             BrowserUtils.redirectPreflightCheck(this.initialized, this.config);
-            this.browserStorage.setInteractionInProgress(true);
+            this.browserStorage.setInteractionInProgress(
+                true,
+                INTERACTION_TYPE.SIGNIN
+            );
 
             if (isLoggedIn) {
                 this.eventHandler.emitEvent(
@@ -768,7 +787,10 @@ export class StandardController implements IController {
         try {
             this.logger.verbose("acquireTokenPopup called", correlationId);
             preflightCheck(this.initialized, atPopupMeasurement);
-            this.browserStorage.setInteractionInProgress(true);
+            this.browserStorage.setInteractionInProgress(
+                true,
+                INTERACTION_TYPE.SIGNIN
+            );
         } catch (e) {
             // Since this function is syncronous we need to reject
             return Promise.reject(e);
@@ -1349,7 +1371,10 @@ export class StandardController implements IController {
     async logoutRedirect(logoutRequest?: EndSessionRequest): Promise<void> {
         const correlationId = this.getRequestCorrelationId(logoutRequest);
         BrowserUtils.redirectPreflightCheck(this.initialized, this.config);
-        this.browserStorage.setInteractionInProgress(true);
+        this.browserStorage.setInteractionInProgress(
+            true,
+            INTERACTION_TYPE.SIGNOUT
+        );
 
         const redirectClient = this.createRedirectClient(correlationId);
         return redirectClient.logout(logoutRequest);
@@ -1363,7 +1388,10 @@ export class StandardController implements IController {
         try {
             const correlationId = this.getRequestCorrelationId(logoutRequest);
             BrowserUtils.preflightCheck(this.initialized);
-            this.browserStorage.setInteractionInProgress(true);
+            this.browserStorage.setInteractionInProgress(
+                true,
+                INTERACTION_TYPE.SIGNOUT
+            );
 
             const popupClient = this.createPopupClient(correlationId);
             return popupClient.logout(logoutRequest).finally(() => {
diff --git a/lib/msal-browser/src/interaction_client/RedirectClient.ts b/lib/msal-browser/src/interaction_client/RedirectClient.ts
@@ -24,6 +24,7 @@ import {
 import { StandardInteractionClient } from "./StandardInteractionClient.js";
 import {
     ApiId,
+    INTERACTION_TYPE,
     InteractionType,
     TemporaryCacheKeys,
 } from "../utils/BrowserConstants.js";
@@ -740,7 +741,10 @@ export class RedirectClient extends StandardInteractionClient {
                     );
                     // Ensure interaction is in progress
                     if (!this.browserStorage.getInteractionInProgress()) {
-                        this.browserStorage.setInteractionInProgress(true);
+                        this.browserStorage.setInteractionInProgress(
+                            true,
+                            INTERACTION_TYPE.SIGNOUT
+                        );
                     }
                     await this.navigationClient.navigateExternal(
                         logoutUri,
@@ -757,7 +761,10 @@ export class RedirectClient extends StandardInteractionClient {
             } else {
                 // Ensure interaction is in progress
                 if (!this.browserStorage.getInteractionInProgress()) {
-                    this.browserStorage.setInteractionInProgress(true);
+                    this.browserStorage.setInteractionInProgress(
+                        true,
+                        INTERACTION_TYPE.SIGNOUT
+                    );
                 }
                 await this.navigationClient.navigateExternal(
                     logoutUri,
diff --git a/lib/msal-browser/src/utils/BrowserConstants.ts b/lib/msal-browser/src/utils/BrowserConstants.ts
@@ -74,6 +74,13 @@ export const HTTP_REQUEST_TYPE = {
 export type HTTP_REQUEST_TYPE =
     (typeof HTTP_REQUEST_TYPE)[keyof typeof HTTP_REQUEST_TYPE];
 
+export const INTERACTION_TYPE = {
+    SIGNIN: "signin",
+    SIGNOUT: "signout",
+} as const;
+export type INTERACTION_TYPE =
+    (typeof INTERACTION_TYPE)[keyof typeof INTERACTION_TYPE];
+
 /**
  * Temporary cache keys for MSAL, deleted after any request.
  */
diff --git a/lib/msal-browser/test/app/PublicClientApplication.spec.ts b/lib/msal-browser/test/app/PublicClientApplication.spec.ts
