Fix redirect clearing popup hash (#6652)

The handleRedirectPromise logic contains a bug which can result in popup windows not closing. This PR fixes that bug. Today handleRedirectPromise does the following things: 1. Parses the hash and looks for known properties 2. Clears the hash from the window 3. Parses state to determine if the response belongs to a Redirect 4. Returns null if not Because we clear the hash before we've determined if the response is even a redirect response this can result in race conditions preventing popup windows from closing if the popup uses a redirectUri which invokes handleRedirectPromise (most apps using React or Angular do this out of the box) This PR addresses this bug by parsing state for the interactionType _before_ clearing the hash.
AzureAD · Nov 6, 2023 · 460e17d · 460e17d
1 parent 619e10f
commit 460e17d
Show file tree

Hide file tree

Showing 9 changed files with 137 additions and 164 deletions.
diff --git a/change/@azure-msal-browser-db1a2b10-8002-4f2a-9b54-81d7632afa03.json b/change/@azure-msal-browser-db1a2b10-8002-4f2a-9b54-81d7632afa03.json
@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "Fix race condition which may cause popups not to close #6652",
+  "packageName": "@azure/msal-browser",
+  "email": "[email protected]",
+  "dependentChangeType": "patch"
+}
diff --git a/lib/msal-browser/src/cache/BrowserCacheManager.ts b/lib/msal-browser/src/cache/BrowserCacheManager.ts
@@ -53,7 +53,7 @@ import {
 import { BrowserStorage } from "./BrowserStorage";
 import { MemoryStorage } from "./MemoryStorage";
 import { IWindowStorage } from "./IWindowStorage";
-import { BrowserProtocolUtils } from "../utils/BrowserProtocolUtils";
+import { extractBrowserRequestState } from "../utils/BrowserProtocolUtils";
 import { NativeTokenRequest } from "../broker/nativeBroker/NativeRequest";
 import { AuthenticationResult } from "../response/AuthenticationResult";
 import { SilentRequest } from "../request/SilentRequest";
@@ -1584,7 +1584,7 @@ export class BrowserCacheManager extends CacheManager {
                 return;
             }
             // Extract state and ensure it matches given InteractionType, then clean request cache
-            const parsedState = BrowserProtocolUtils.extractBrowserRequestState(
+            const parsedState = extractBrowserRequestState(
                 this.cryptoImpl,
                 stateValue
             );

diff --git a/lib/msal-browser/src/interaction_client/RedirectClient.ts b/lib/msal-browser/src/interaction_client/RedirectClient.ts
@@ -46,6 +46,7 @@ import { EventHandler } from "../event/EventHandler";
 import { INavigationClient } from "../navigation/INavigationClient";
 import { EventError } from "../event/EventMessage";
 import { AuthenticationResult } from "../response/AuthenticationResult";
+import * as ResponseHandler from "../response/ResponseHandler";
 
 export class RedirectClient extends StandardInteractionClient {
     protected nativeStorage: BrowserCacheManager;
@@ -217,23 +218,6 @@ export class RedirectClient extends StandardInteractionClient {
                 return null;
             }
 
-            let state: string;
-            try {
-                state = this.validateAndExtractStateFromHash(
-                    serverParams,
-                    InteractionType.Redirect
-                );
-                this.logger.verbose("State extracted from hash");
-            } catch (e) {
-                this.logger.info(
-                    `handleRedirectPromise was unable to extract state due to: ${e}`
-                );
-                this.browserStorage.cleanRequestByInteractionType(
-                    InteractionType.Redirect
-                );
-                return null;
-            }
-
             // If navigateToLoginRequestUrl is true, get the url where the redirect request was initiated
             const loginRequestUrl =
                 this.browserStorage.getTemporaryCache(
@@ -262,7 +246,6 @@ export class RedirectClient extends StandardInteractionClient {
 
                 const handleHashResult = await this.handleResponse(
                     serverParams,
-                    state,
                     serverTelemetryManager
                 );
 
@@ -273,7 +256,6 @@ export class RedirectClient extends StandardInteractionClient {
                 );
                 return this.handleResponse(
                     serverParams,
-                    state,
                     serverTelemetryManager
                 );
             } else if (
@@ -333,7 +315,6 @@ export class RedirectClient extends StandardInteractionClient {
                 if (!processHashOnRedirect) {
                     return this.handleResponse(
                         serverParams,
-                        state,
                         serverTelemetryManager
                     );
                 }
@@ -376,6 +357,21 @@ export class RedirectClient extends StandardInteractionClient {
         let response = UrlUtils.getDeserializedResponse(responseString);
 
         if (response) {
+            try {
+                ResponseHandler.validateInteractionType(
+                    response,
+                    this.browserCrypto,
+                    InteractionType.Redirect
+                );
+            } catch (e) {
+                if (e instanceof AuthError) {
+                    this.logger.error(
+                        `Interaction type validation failed due to ${e.errorCode}: ${e.errorMessage}`
+                    );
+                }
+                return [null, ""];
+            }
+
             BrowserUtils.clearHash(window);
             this.logger.verbose(
                 "Hash contains known properties, returning response hash"
@@ -411,9 +407,13 @@ export class RedirectClient extends StandardInteractionClient {
      */
     protected async handleResponse(
         serverParams: ServerAuthorizationCodeResponse,
-        state: string,
         serverTelemetryManager: ServerTelemetryManager
     ): Promise<AuthenticationResult> {
+        const state = serverParams.state;
+        if (!state) {
+            throw createBrowserAuthError(BrowserAuthErrorCodes.noStateInHash);
+        }
+
         const cachedRequest = this.browserStorage.getCachedRequest(state);
         this.logger.verbose("handleResponse called, retrieved cached request");
 

diff --git a/lib/msal-browser/src/interaction_client/StandardInteractionClient.ts b/lib/msal-browser/src/interaction_client/StandardInteractionClient.ts
@@ -12,7 +12,6 @@ import {
     AuthorityOptions,
     Authority,
     AuthorityFactory,
-    ServerAuthorizationCodeResponse,
     UrlString,
     CommonEndSessionRequest,
     ProtocolUtils,
@@ -28,14 +27,7 @@ import { BaseInteractionClient } from "./BaseInteractionClient";
 import { AuthorizationUrlRequest } from "../request/AuthorizationUrlRequest";
 import { BrowserConstants, InteractionType } from "../utils/BrowserConstants";
 import { version } from "../packageMetadata";
-import {
-    createBrowserAuthError,
-    BrowserAuthErrorCodes,
-} from "../error/BrowserAuthError";
-import {
-    BrowserProtocolUtils,
-    BrowserStateObject,
-} from "../utils/BrowserProtocolUtils";
+import { BrowserStateObject } from "../utils/BrowserProtocolUtils";
 import { EndSessionRequest } from "../request/EndSessionRequest";
 import * as BrowserUtils from "../utils/BrowserUtils";
 import { RedirectRequest } from "../request/RedirectRequest";
@@ -294,44 +286,6 @@ export abstract class StandardInteractionClient extends BaseInteractionClient {
         };
     }
 
-    /**
-     * @param hash
-     * @param interactionType
-     */
-    protected validateAndExtractStateFromHash(
-        serverParams: ServerAuthorizationCodeResponse,
-        interactionType: InteractionType,
-        requestCorrelationId?: string
-    ): string {
-        this.logger.verbose(
-            "validateAndExtractStateFromHash called",
-            requestCorrelationId
-        );
-        if (!serverParams.state) {
-            throw createBrowserAuthError(BrowserAuthErrorCodes.noStateInHash);
-        }
-
-        const platformStateObj =
-            BrowserProtocolUtils.extractBrowserRequestState(
-                this.browserCrypto,
-                serverParams.state
-            );
-        if (!platformStateObj) {
-            throw createBrowserAuthError(
-                BrowserAuthErrorCodes.unableToParseState
-            );
-        }
-
-        if (platformStateObj.interactionType !== interactionType) {
-            throw createBrowserAuthError(
-                BrowserAuthErrorCodes.stateInteractionTypeMismatch
-            );
-        }
-
-        this.logger.verbose("Returning state from hash", requestCorrelationId);
-        return serverParams.state;
-    }
-
     /**
      * Used to get a discovered version of the default authority.
      * @param requestAuthority

diff --git a/lib/msal-browser/src/response/ResponseHandler.ts b/lib/msal-browser/src/response/ResponseHandler.ts
@@ -4,6 +4,7 @@
  */
 
 import {
+    ICrypto,
     Logger,
     ServerAuthorizationCodeResponse,
     UrlUtils,
@@ -12,6 +13,8 @@ import {
     BrowserAuthErrorCodes,
     createBrowserAuthError,
 } from "../error/BrowserAuthError";
+import { extractBrowserRequestState } from "../utils/BrowserProtocolUtils";
+import { InteractionType } from "../utils/BrowserConstants";
 
 export function deserializeResponse(
     responseString: string,
@@ -41,3 +44,30 @@ export function deserializeResponse(
     }
     return serverParams;
 }
+
+/**
+ * Returns the interaction type that the response object belongs to
+ */
+export function validateInteractionType(
+    response: ServerAuthorizationCodeResponse,
+    browserCrypto: ICrypto,
+    interactionType: InteractionType
+): void {
+    if (!response.state) {
+        throw createBrowserAuthError(BrowserAuthErrorCodes.noStateInHash);
+    }
+
+    const platformStateObj = extractBrowserRequestState(
+        browserCrypto,
+        response.state
+    );
+    if (!platformStateObj) {
+        throw createBrowserAuthError(BrowserAuthErrorCodes.unableToParseState);
+    }
+
+    if (platformStateObj.interactionType !== interactionType) {
+        throw createBrowserAuthError(
+            BrowserAuthErrorCodes.stateInteractionTypeMismatch
+        );
+    }
+}
diff --git a/lib/msal-browser/src/utils/BrowserProtocolUtils.ts b/lib/msal-browser/src/utils/BrowserProtocolUtils.ts
@@ -16,26 +16,24 @@ export type BrowserStateObject = {
     interactionType: InteractionType;
 };
 
-export class BrowserProtocolUtils {
-    /**
-     * Extracts the BrowserStateObject from the state string.
-     * @param browserCrypto
-     * @param state
-     */
-    static extractBrowserRequestState(
-        browserCrypto: ICrypto,
-        state: string
-    ): BrowserStateObject | null {
-        if (!state) {
-            return null;
-        }
+/**
+ * Extracts the BrowserStateObject from the state string.
+ * @param browserCrypto
+ * @param state
+ */
+export function extractBrowserRequestState(
+    browserCrypto: ICrypto,
+    state: string
+): BrowserStateObject | null {
+    if (!state) {
+        return null;
+    }
 
-        try {
-            const requestStateObj: RequestStateObject =
-                ProtocolUtils.parseRequestState(browserCrypto, state);
-            return requestStateObj.libraryState.meta as BrowserStateObject;
-        } catch (e) {
-            throw createClientAuthError(ClientAuthErrorCodes.invalidState);
-        }
+    try {
+        const requestStateObj: RequestStateObject =
+            ProtocolUtils.parseRequestState(browserCrypto, state);
+        return requestStateObj.libraryState.meta as BrowserStateObject;
+    } catch (e) {
+        throw createClientAuthError(ClientAuthErrorCodes.invalidState);
     }
 }