diff --git a/change/@azure-msal-browser-30b7bab2-c13f-46ec-bc23-99211431d098.json b/change/@azure-msal-browser-30b7bab2-c13f-46ec-bc23-99211431d098.json new file mode 100644 index 0000000000..1b722569a5 --- /dev/null +++ b/change/@azure-msal-browser-30b7bab2-c13f-46ec-bc23-99211431d098.json @@ -0,0 +1,7 @@ +{ + "type": "minor", + "comment": "Optimize response parsing & address bugs related to query response type #6646", + "packageName": "@azure/msal-browser", + "email": "thomas.norling@microsoft.com", + "dependentChangeType": "patch" +} diff --git a/change/@azure-msal-common-d245eb0b-38fb-4f30-a5f6-aaafa5295fba.json b/change/@azure-msal-common-d245eb0b-38fb-4f30-a5f6-aaafa5295fba.json new file mode 100644 index 0000000000..64fefe87f6 --- /dev/null +++ b/change/@azure-msal-common-d245eb0b-38fb-4f30-a5f6-aaafa5295fba.json @@ -0,0 +1,7 @@ +{ + "type": "minor", + "comment": "Update hash/query parsing to use runtime provided APIs #6646", + "packageName": "@azure/msal-common", + "email": "thomas.norling@microsoft.com", + "dependentChangeType": "patch" +} diff --git a/change/@azure-msal-node-54978b56-aca4-4e1c-bc18-a79298d7231a.json b/change/@azure-msal-node-54978b56-aca4-4e1c-bc18-a79298d7231a.json new file mode 100644 index 0000000000..ddf61e2b88 --- /dev/null +++ b/change/@azure-msal-node-54978b56-aca4-4e1c-bc18-a79298d7231a.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "Use new query string parser in LoopbackClient #6646", + "packageName": "@azure/msal-node", + "email": "thomas.norling@microsoft.com", + "dependentChangeType": "patch" +} diff --git a/lib/msal-browser/src/controllers/StandardController.ts b/lib/msal-browser/src/controllers/StandardController.ts index 83871c0450..e293805c01 100644 --- a/lib/msal-browser/src/controllers/StandardController.ts +++ b/lib/msal-browser/src/controllers/StandardController.ts @@ -22,8 +22,6 @@ import { InProgressPerformanceEvent, RequestThumbprint, AccountEntity, - ServerResponseType, - UrlString, invokeAsync, createClientAuthError, ClientAuthErrorCodes, @@ -336,16 +334,6 @@ export class StandardController implements IController { // Block token acquisition before initialize has been called BrowserUtils.blockAPICallsBeforeInitialize(this.initialized); - let foundServerResponse = hash; - - if ( - this.config.auth.OIDCOptions?.serverResponseType === - ServerResponseType.QUERY - ) { - const url = window.location.href; - foundServerResponse = UrlString.parseQueryServerResponse(url); - } - const loggedInAccounts = this.getAllAccounts(); if (this.isBrowserEnvironment) { /** @@ -353,8 +341,7 @@ export class StandardController implements IController { * otherwise return the promise from the first invocation. Prevents race conditions when handleRedirectPromise is called * several times concurrently. */ - const redirectResponseKey = - foundServerResponse || Constants.EMPTY_STRING; + const redirectResponseKey = hash || ""; let response = this.redirectResponse.get(redirectResponseKey); if (typeof response === "undefined") { this.eventHandler.emitEvent( @@ -376,7 +363,7 @@ export class StandardController implements IController { this.nativeExtensionProvider ) && this.nativeExtensionProvider && - !foundServerResponse + !hash ) { this.logger.trace( "handleRedirectPromise - acquiring token from native platform" @@ -408,9 +395,7 @@ export class StandardController implements IController { const redirectClient = this.createRedirectClient(correlationId); redirectResponse = - redirectClient.handleRedirectPromise( - foundServerResponse - ); + redirectClient.handleRedirectPromise(hash); } response = redirectResponse diff --git a/lib/msal-browser/src/interaction_client/PopupClient.ts b/lib/msal-browser/src/interaction_client/PopupClient.ts index 26ca102dd2..a4e6d71a4c 100644 --- a/lib/msal-browser/src/interaction_client/PopupClient.ts +++ b/lib/msal-browser/src/interaction_client/PopupClient.ts @@ -11,9 +11,7 @@ import { UrlString, AuthError, OIDC_DEFAULT_SCOPES, - Constants, ProtocolUtils, - ServerAuthorizationCodeResponse, PerformanceEvents, IPerformanceClient, Logger, @@ -21,6 +19,7 @@ import { ProtocolMode, ServerResponseType, invokeAsync, + invoke, } from "@azure/msal-common"; import { StandardInteractionClient } from "./StandardInteractionClient"; import { EventType } from "../event/EventType"; @@ -47,6 +46,7 @@ import { InteractionHandler } from "../interaction_handler/InteractionHandler"; import { PopupWindowAttributes } from "../request/PopupWindowAttributes"; import { EventError } from "../event/EventMessage"; import { AuthenticationResult } from "../response/AuthenticationResult"; +import * as ResponseHandler from "../response/ResponseHandler"; export type PopupParams = { popup?: Window | null; @@ -284,10 +284,19 @@ export class PopupClient extends StandardInteractionClient { ); // Monitor the window for the hash. Return the string value and close the popup when the hash is received. Default timeout is 60 seconds. - const hash = await this.monitorPopupForHash(popupWindow); - // Deserialize hash fragment response parameters. - const serverParams: ServerAuthorizationCodeResponse = - UrlString.getDeserializedHash(hash); + const responseString = await this.monitorPopupForHash(popupWindow); + + const serverParams = invoke( + ResponseHandler.deserializeResponse, + PerformanceEvents.DeserializeResponse, + this.logger, + this.performanceClient, + this.correlationId + )( + responseString, + this.config.auth.OIDCOptions.serverResponseType, + this.logger + ); // Remove throttle if it exists ThrottlingUtils.removeThrottle( this.browserStorage, @@ -338,8 +347,8 @@ export class PopupClient extends StandardInteractionClient { } // Handle response from hash string. - const result = await interactionHandler.handleCodeResponseFromHash( - hash, + const result = await interactionHandler.handleCodeResponse( + serverParams, validRequest ); @@ -464,7 +473,9 @@ export class PopupClient extends StandardInteractionClient { null ); - await this.waitForLogoutPopup(popupWindow); + await this.monitorPopupForHash(popupWindow).catch(() => { + // Swallow any errors related to monitoring the window. Server logout is best effort + }); if (mainWindowRedirectUri) { const navigationOptions: NavigationOptions = { @@ -545,16 +556,7 @@ export class PopupClient extends StandardInteractionClient { * @param timeout - timeout for processing hash once popup is redirected back to application */ monitorPopupForHash(popupWindow: Window): Promise { - return new Promise((resolve, reject) => { - /* - * Polling for popups needs to be tick-based, - * since a non-trivial amount of time can be spent on interaction (which should not count against the timeout). - */ - const maxTicks = - this.config.system.windowHashTimeout / - this.config.system.pollIntervalMilliseconds; - let ticks = 0; - + return new Promise((resolve, reject) => { this.logger.verbose( "PopupHandler.monitorPopupForHash - polling started" ); @@ -565,7 +567,6 @@ export class PopupClient extends StandardInteractionClient { this.logger.error( "PopupHandler.monitorPopupForHash - window closed" ); - this.cleanPopup(); clearInterval(intervalId); reject( createBrowserAuthError( @@ -575,8 +576,7 @@ export class PopupClient extends StandardInteractionClient { return; } - let href = Constants.EMPTY_STRING; - let serverResponseString = Constants.EMPTY_STRING; + let href = ""; try { /* * Will throw if cross origin, @@ -584,117 +584,33 @@ export class PopupClient extends StandardInteractionClient { * since we need the interval to keep running while on STS UI. */ href = popupWindow.location.href; - serverResponseString = - this.extractServerResponseStringFromPopup( - popupWindow, - href - ); } catch (e) {} // Don't process blank pages or cross domain if (!href || href === "about:blank") { return; } + clearInterval(intervalId); - this.logger.verbose( - "PopupHandler.monitorPopupForHash - popup window is on same origin as caller" - ); - - /* - * Only run clock when we are on same domain for popups - * as popup operations can take a long time. - */ - ticks++; - - if (serverResponseString) { - this.logger.verbose( - "PopupHandler.monitorPopupForHash - found hash in url" - ); - clearInterval(intervalId); - this.cleanPopup(popupWindow); - - if ( - UrlString.hashContainsKnownProperties( - serverResponseString - ) - ) { - this.logger.verbose( - "PopupHandler.monitorPopupForHash - hash contains known properties, returning." - ); - resolve(serverResponseString); + let responseString = ""; + const responseType = + this.config.auth.OIDCOptions.serverResponseType; + if (popupWindow) { + if (responseType === ServerResponseType.QUERY) { + responseString = popupWindow.location.search; } else { - this.logger.error( - "PopupHandler.monitorPopupForHash - found hash in url but it does not contain known properties. Check that your router is not changing the hash prematurely." - ); - this.logger.errorPii( - `PopupHandler.monitorPopupForHash - hash found: ${serverResponseString}` - ); - reject( - createBrowserAuthError( - BrowserAuthErrorCodes.hashDoesNotContainKnownProperties - ) - ); + responseString = popupWindow.location.hash; } - } else if (ticks > maxTicks) { - this.logger.error( - "PopupHandler.monitorPopupForHash - unable to find hash in url, timing out" - ); - clearInterval(intervalId); - reject( - createBrowserAuthError( - BrowserAuthErrorCodes.monitorPopupTimeout - ) - ); - } - }, this.config.system.pollIntervalMilliseconds); - }); - } - - /** - * Waits for user interaction in logout popup window - * @param popupWindow - * @returns - */ - waitForLogoutPopup(popupWindow: Window): Promise { - return new Promise((resolve) => { - this.logger.verbose( - "PopupHandler.waitForLogoutPopup - polling started" - ); - - const intervalId = setInterval(() => { - // Window is closed - if (popupWindow.closed) { - this.logger.error( - "PopupHandler.waitForLogoutPopup - window closed" - ); - this.cleanPopup(); - clearInterval(intervalId); - resolve(); - } - - let href: string = Constants.EMPTY_STRING; - try { - /* - * Will throw if cross origin, - * which should be caught and ignored - * since we need the interval to keep running while on STS UI. - */ - href = popupWindow.location.href; - } catch (e) {} - - // Don't process blank pages or cross domain - if (!href || href === "about:blank") { - return; } this.logger.verbose( - "PopupHandler.waitForLogoutPopup - popup window is on same origin as caller, closing." + "PopupHandler.monitorPopupForHash - popup window is on same origin as caller" ); - clearInterval(intervalId); - this.cleanPopup(popupWindow); - resolve(); + resolve(responseString); }, this.config.system.pollIntervalMilliseconds); + }).finally(() => { + this.cleanPopup(popupWindow); }); } @@ -884,23 +800,4 @@ export class PopupClient extends StandardInteractionClient { const homeAccountId = request.account && request.account.homeAccountId; return `${BrowserConstants.POPUP_NAME_PREFIX}.${this.config.auth.clientId}.${homeAccountId}.${this.correlationId}`; } - - /** - * Extracts the server response from the popup window - */ - extractServerResponseStringFromPopup( - popupWindow: Window, - href: string - ): string { - let serverResponseString; - if ( - this.config.auth.OIDCOptions?.serverResponseType === - ServerResponseType.QUERY - ) { - serverResponseString = UrlString.parseQueryServerResponse(href); - } else { - serverResponseString = popupWindow.location.hash; - } - return serverResponseString; - } } diff --git a/lib/msal-browser/src/interaction_client/RedirectClient.ts b/lib/msal-browser/src/interaction_client/RedirectClient.ts index a8641b8a7b..34178e4678 100644 --- a/lib/msal-browser/src/interaction_client/RedirectClient.ts +++ b/lib/msal-browser/src/interaction_client/RedirectClient.ts @@ -19,6 +19,8 @@ import { PerformanceEvents, ProtocolMode, invokeAsync, + ServerResponseType, + UrlUtils, } from "@azure/msal-common"; import { StandardInteractionClient } from "./StandardInteractionClient"; import { @@ -201,13 +203,13 @@ export class RedirectClient extends StandardInteractionClient { ); return null; } - const responseHash = this.getRedirectResponseHash( - hash || window.location.hash + const [serverParams, responseString] = this.getRedirectResponse( + hash || "" ); - if (!responseHash) { + if (!serverParams) { // Not a recognized server response hash or hash not associated with a redirect request this.logger.info( - "handleRedirectPromise did not detect a response hash as a result of a redirect. Cleaning temporary cache." + "handleRedirectPromise did not detect a response as a result of a redirect. Cleaning temporary cache." ); this.browserStorage.cleanRequestByInteractionType( InteractionType.Redirect @@ -217,9 +219,6 @@ export class RedirectClient extends StandardInteractionClient { let state: string; try { - // Deserialize hash fragment response parameters. - const serverParams: ServerAuthorizationCodeResponse = - UrlString.getDeserializedHash(responseHash); state = this.validateAndExtractStateFromHash( serverParams, InteractionType.Redirect @@ -253,12 +252,7 @@ export class RedirectClient extends StandardInteractionClient { ) { // We are on the page we need to navigate to - handle hash this.logger.verbose( - "Current page is loginRequestUrl, handling hash" - ); - const handleHashResult = await this.handleHash( - responseHash, - state, - serverTelemetryManager + "Current page is loginRequestUrl, handling response" ); if (loginRequestUrl.indexOf("#") > -1) { @@ -266,13 +260,19 @@ export class RedirectClient extends StandardInteractionClient { BrowserUtils.replaceHash(loginRequestUrl); } + const handleHashResult = await this.handleResponse( + serverParams, + state, + serverTelemetryManager + ); + return handleHashResult; } else if (!this.config.auth.navigateToLoginRequestUrl) { this.logger.verbose( - "NavigateToLoginRequestUrl set to false, handling hash" + "NavigateToLoginRequestUrl set to false, handling response" ); - return this.handleHash( - responseHash, + return this.handleResponse( + serverParams, state, serverTelemetryManager ); @@ -286,7 +286,7 @@ export class RedirectClient extends StandardInteractionClient { */ this.browserStorage.setTemporaryCache( TemporaryCacheKeys.URL_HASH, - responseHash, + responseString, true ); const navigationOptions: NavigationOptions = { @@ -331,8 +331,8 @@ export class RedirectClient extends StandardInteractionClient { // If navigateInternal implementation returns false, handle the hash now if (!processHashOnRedirect) { - return this.handleHash( - responseHash, + return this.handleResponse( + serverParams, state, serverTelemetryManager ); @@ -357,18 +357,30 @@ export class RedirectClient extends StandardInteractionClient { * Returns null if interactionType in the state value is not "redirect" or the hash does not contain known properties * @param hash */ - protected getRedirectResponseHash(hash: string): string | null { + protected getRedirectResponse( + userProvidedResponse: string + ): [ServerAuthorizationCodeResponse | null, string] { this.logger.verbose("getRedirectResponseHash called"); // Get current location hash from window or cache. - const isResponseHash: boolean = - UrlString.hashContainsKnownProperties(hash); + let responseString = userProvidedResponse; + if (!responseString) { + if ( + this.config.auth.OIDCOptions.serverResponseType === + ServerResponseType.QUERY + ) { + responseString = window.location.search; + } else { + responseString = window.location.hash; + } + } + let response = UrlUtils.getDeserializedResponse(responseString); - if (isResponseHash) { + if (response) { BrowserUtils.clearHash(window); this.logger.verbose( "Hash contains known properties, returning response hash" ); - return hash; + return [response, responseString]; } const cachedHash = this.browserStorage.getTemporaryCache( @@ -379,10 +391,17 @@ export class RedirectClient extends StandardInteractionClient { this.browserStorage.generateCacheKey(TemporaryCacheKeys.URL_HASH) ); - this.logger.verbose( - "Hash does not contain known properties, returning cached hash" - ); - return cachedHash; + if (cachedHash) { + response = UrlUtils.getDeserializedResponse(cachedHash); + if (response) { + this.logger.verbose( + "Hash does not contain known properties, returning cached hash" + ); + return [response, cachedHash]; + } + } + + return [null, ""]; } /** @@ -390,16 +409,13 @@ export class RedirectClient extends StandardInteractionClient { * @param hash * @param state */ - protected async handleHash( - hash: string, + protected async handleResponse( + serverParams: ServerAuthorizationCodeResponse, state: string, serverTelemetryManager: ServerTelemetryManager ): Promise { const cachedRequest = this.browserStorage.getCachedRequest(state); - this.logger.verbose("handleHash called, retrieved cached request"); - - const serverParams: ServerAuthorizationCodeResponse = - UrlString.getDeserializedHash(hash); + this.logger.verbose("handleResponse called, retrieved cached request"); if (serverParams.accountId) { this.logger.verbose( @@ -467,7 +483,7 @@ export class RedirectClient extends StandardInteractionClient { this.logger, this.performanceClient ); - return await interactionHandler.handleCodeResponseFromHash(hash, state); + return await interactionHandler.handleCodeResponse(serverParams, state); } /** diff --git a/lib/msal-browser/src/interaction_client/SilentIframeClient.ts b/lib/msal-browser/src/interaction_client/SilentIframeClient.ts index 4a29ccfd03..e185a60ddb 100644 --- a/lib/msal-browser/src/interaction_client/SilentIframeClient.ts +++ b/lib/msal-browser/src/interaction_client/SilentIframeClient.ts @@ -10,12 +10,11 @@ import { CommonAuthorizationCodeRequest, AuthorizationCodeClient, AuthError, - UrlString, - ServerAuthorizationCodeResponse, ProtocolUtils, IPerformanceClient, PerformanceEvents, invokeAsync, + invoke, } from "@azure/msal-common"; import { StandardInteractionClient } from "./StandardInteractionClient"; import { AuthorizationUrlRequest } from "../request/AuthorizationUrlRequest"; @@ -38,6 +37,7 @@ import { NativeInteractionClient } from "./NativeInteractionClient"; import { AuthenticationResult } from "../response/AuthenticationResult"; import { InteractionHandler } from "../interaction_handler/InteractionHandler"; import * as BrowserUtils from "../utils/BrowserUtils"; +import * as ResponseHandler from "../response/ResponseHandler"; export class SilentIframeClient extends StandardInteractionClient { protected apiId: ApiId; @@ -231,8 +231,9 @@ export class SilentIframeClient extends StandardInteractionClient { correlationId, this.config.system.navigateFrameWait ); + const responseType = this.config.auth.OIDCOptions.serverResponseType; // Monitor the window for the hash. Return the string value and close the popup when the hash is received. Default timeout is 60 seconds. - const hash = await invokeAsync( + const responseString = await invokeAsync( monitorIframeForHash, PerformanceEvents.SilentHandlerMonitorIframeForHash, this.logger, @@ -244,26 +245,16 @@ export class SilentIframeClient extends StandardInteractionClient { this.config.system.pollIntervalMilliseconds, this.performanceClient, this.logger, - correlationId + correlationId, + responseType ); - if (!hash) { - // No hash is present - this.logger.error( - "The request has returned to the redirectUri but a hash is not present in the iframe. It's likely that the hash has been removed or the page has been redirected by code running on the redirectUri page." - ); - throw createBrowserAuthError(BrowserAuthErrorCodes.hashEmptyError); - } else if (!UrlString.hashContainsKnownProperties(hash)) { - this.logger.error( - "A hash is present in the iframe but it does not contain known properties. It's likely that the hash has been replaced by code running on the redirectUri page." - ); - this.logger.errorPii(`The hash detected in the iframe is: ${hash}`); - throw createBrowserAuthError( - BrowserAuthErrorCodes.hashDoesNotContainKnownProperties - ); - } - // Deserialize hash fragment response parameters. - const serverParams: ServerAuthorizationCodeResponse = - UrlString.getDeserializedHash(hash); + const serverParams = invoke( + ResponseHandler.deserializeResponse, + PerformanceEvents.DeserializeResponse, + this.logger, + this.performanceClient, + this.correlationId + )(responseString, responseType, this.logger); if (serverParams.accountId) { this.logger.verbose( @@ -309,13 +300,11 @@ export class SilentIframeClient extends StandardInteractionClient { // Handle response from hash string return invokeAsync( - interactionHandler.handleCodeResponseFromHash.bind( - interactionHandler - ), - PerformanceEvents.HandleCodeResponseFromHash, + interactionHandler.handleCodeResponse.bind(interactionHandler), + PerformanceEvents.HandleCodeResponse, this.logger, this.performanceClient, correlationId - )(hash, silentRequest); + )(serverParams, silentRequest); } } diff --git a/lib/msal-browser/src/interaction_handler/InteractionHandler.ts b/lib/msal-browser/src/interaction_handler/InteractionHandler.ts index 972d3ad3a2..f4c5a39476 100644 --- a/lib/msal-browser/src/interaction_handler/InteractionHandler.ts +++ b/lib/msal-browser/src/interaction_handler/InteractionHandler.ts @@ -14,6 +14,7 @@ import { PerformanceEvents, invokeAsync, CcsCredentialType, + ServerAuthorizationCodeResponse, } from "@azure/msal-common"; import { BrowserCacheManager } from "../cache/BrowserCacheManager"; @@ -52,23 +53,19 @@ export class InteractionHandler { * Function to handle response parameters from hash. * @param locationHash */ - async handleCodeResponseFromHash( - locationHash: string, + async handleCodeResponse( + response: ServerAuthorizationCodeResponse, request: AuthorizationUrlRequest ): Promise { this.performanceClient.addQueueMeasurement( - PerformanceEvents.HandleCodeResponseFromHash, + PerformanceEvents.HandleCodeResponse, request.correlationId ); - // Check that location hash isn't empty. - if (!locationHash) { - throw createBrowserAuthError(BrowserAuthErrorCodes.hashEmptyError); - } let authCodeResponse; try { authCodeResponse = this.authModule.handleFragmentResponse( - locationHash, + response, request.state ); } catch (e) { diff --git a/lib/msal-browser/src/interaction_handler/RedirectHandler.ts b/lib/msal-browser/src/interaction_handler/RedirectHandler.ts index f5c6563513..b16a3d7699 100644 --- a/lib/msal-browser/src/interaction_handler/RedirectHandler.ts +++ b/lib/msal-browser/src/interaction_handler/RedirectHandler.ts @@ -14,6 +14,7 @@ import { CcsCredential, invokeAsync, PerformanceEvents, + ServerAuthorizationCodeResponse, } from "@azure/msal-common"; import { createBrowserAuthError, @@ -141,17 +142,12 @@ export class RedirectHandler { * Handle authorization code response in the window. * @param hash */ - async handleCodeResponseFromHash( - locationHash: string, + async handleCodeResponse( + response: ServerAuthorizationCodeResponse, state: string ): Promise { this.logger.verbose("RedirectHandler.handleCodeResponse called"); - // Check that location hash isn't empty. - if (!locationHash) { - throw createBrowserAuthError(BrowserAuthErrorCodes.hashEmptyError); - } - // Interaction is completed - remove interaction status. this.browserStorage.setInteractionInProgress(false); @@ -168,7 +164,7 @@ export class RedirectHandler { let authCodeResponse; try { authCodeResponse = this.authModule.handleFragmentResponse( - locationHash, + response, requestState ); } catch (e) { diff --git a/lib/msal-browser/src/interaction_handler/SilentHandler.ts b/lib/msal-browser/src/interaction_handler/SilentHandler.ts index c6abd3940b..ce5dd7e95a 100644 --- a/lib/msal-browser/src/interaction_handler/SilentHandler.ts +++ b/lib/msal-browser/src/interaction_handler/SilentHandler.ts @@ -9,6 +9,7 @@ import { PerformanceEvents, invokeAsync, invoke, + ServerResponseType, } from "@azure/msal-common"; import { createBrowserAuthError, @@ -67,7 +68,8 @@ export async function monitorIframeForHash( pollIntervalMilliseconds: number, performanceClient: IPerformanceClient, logger: Logger, - correlationId: string + correlationId: string, + responseType: ServerResponseType ): Promise { performanceClient.addQueueMeasurement( PerformanceEvents.SilentHandlerMonitorIframeForHash, @@ -110,12 +112,17 @@ export async function monitorIframeForHash( return; } - const contentHash = contentWindow - ? contentWindow.location.hash - : ""; + let responseString = ""; + if (contentWindow) { + if (responseType === ServerResponseType.QUERY) { + responseString = contentWindow.location.search; + } else { + responseString = contentWindow.location.hash; + } + } window.clearTimeout(timeoutId); window.clearInterval(intervalId); - resolve(contentHash); + resolve(responseString); }, pollIntervalMilliseconds); }).finally(() => { invoke( diff --git a/lib/msal-browser/src/response/ResponseHandler.ts b/lib/msal-browser/src/response/ResponseHandler.ts new file mode 100644 index 0000000000..2ec1153bb6 --- /dev/null +++ b/lib/msal-browser/src/response/ResponseHandler.ts @@ -0,0 +1,43 @@ +/* + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. + */ + +import { + Logger, + ServerAuthorizationCodeResponse, + UrlUtils, +} from "@azure/msal-common"; +import { + BrowserAuthErrorCodes, + createBrowserAuthError, +} from "../error/BrowserAuthError"; + +export function deserializeResponse( + responseString: string, + responseLocation: string, + logger: Logger +): ServerAuthorizationCodeResponse { + // Deserialize hash fragment response parameters. + const serverParams = UrlUtils.getDeserializedResponse(responseString); + if (!serverParams) { + if (!UrlUtils.stripLeadingHashOrQuery(responseString)) { + // Hash or Query string is empty + logger.error( + `The request has returned to the redirectUri but a ${responseLocation} is not present. It's likely that the ${responseLocation} has been removed or the page has been redirected by code running on the redirectUri page.` + ); + throw createBrowserAuthError(BrowserAuthErrorCodes.hashEmptyError); + } else { + logger.error( + `A ${responseLocation} is present in the iframe but it does not contain known properties. It's likely that the ${responseLocation} has been replaced by code running on the redirectUri page.` + ); + logger.errorPii( + `The ${responseLocation} detected is: ${responseString}` + ); + throw createBrowserAuthError( + BrowserAuthErrorCodes.hashDoesNotContainKnownProperties + ); + } + } + return serverParams; +} diff --git a/lib/msal-browser/src/utils/BrowserProtocolUtils.ts b/lib/msal-browser/src/utils/BrowserProtocolUtils.ts index 2de276ab00..7fa725a26c 100644 --- a/lib/msal-browser/src/utils/BrowserProtocolUtils.ts +++ b/lib/msal-browser/src/utils/BrowserProtocolUtils.ts @@ -8,8 +8,6 @@ import { ICrypto, RequestStateObject, ProtocolUtils, - ServerAuthorizationCodeResponse, - UrlString, createClientAuthError, ClientAuthErrorCodes, } from "@azure/msal-common"; @@ -40,19 +38,4 @@ export class BrowserProtocolUtils { throw createClientAuthError(ClientAuthErrorCodes.invalidState); } } - - /** - * Parses properties of server response from url hash - * @param locationHash Hash from url - */ - static parseServerResponseFromHash( - locationHash: string - ): ServerAuthorizationCodeResponse { - if (!locationHash) { - return {}; - } - - const hashUrlString = new UrlString(locationHash); - return UrlString.getDeserializedHash(hashUrlString.getHash()); - } } diff --git a/lib/msal-browser/test/app/PublicClientApplication.spec.ts b/lib/msal-browser/test/app/PublicClientApplication.spec.ts index d9c1ca85c0..4727bb809b 100644 --- a/lib/msal-browser/test/app/PublicClientApplication.spec.ts +++ b/lib/msal-browser/test/app/PublicClientApplication.spec.ts @@ -194,6 +194,12 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { BrowserPerformanceMeasurement.flushMeasurements = jest .fn() .mockReturnValue(null); + + // Navigation not allowed in tests + jest.spyOn( + NavigationClient.prototype, + "navigateExternal" + ).mockImplementation(); }); afterEach(() => { @@ -901,21 +907,27 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { }); it("Looks for server code response in query param if OIDCOptions.serverResponseType is set to query", async () => { - /** - * The testing environment does not accept query params, so instead we see that it ignores a hash fragment. - * That means handleRedirectPromise is looking for a hash in a query param instead of a hash fragment. - */ - sinon - .stub(RedirectClient.prototype, "handleRedirectPromise") - .callsFake( - async (hash): Promise => { - expect(hash).toBe(""); - return null; - } - ); + const responseSpy = jest.spyOn( + RedirectClient.prototype, + "getRedirectResponse" + ); + jest.spyOn( + BrowserCacheManager.prototype, + "isInteractionInProgress" + ).mockReturnValue(true); + jest.spyOn(window, "location", "get").mockReturnValueOnce({ + ...window.location, + search: "?code=authCode", + }); + await pca.handleRedirectPromise().catch(() => { + // This will likely throw, but we're not testing the e2e here + }); - window.location.hash = "#code=hello"; - await pca.handleRedirectPromise(); + expect(responseSpy).toHaveBeenCalledTimes(1); + expect(responseSpy).lastReturnedWith([ + { code: "authCode" }, + "?code=authCode", + ]); }); }); diff --git a/lib/msal-browser/test/interaction_client/PopupClient.spec.ts b/lib/msal-browser/test/interaction_client/PopupClient.spec.ts index 3d367ee73f..3995d365cb 100644 --- a/lib/msal-browser/test/interaction_client/PopupClient.spec.ts +++ b/lib/msal-browser/test/interaction_client/PopupClient.spec.ts @@ -16,8 +16,6 @@ import { testNavUrl, TEST_STATE_VALUES, TEST_SSH_VALUES, - DEFAULT_OPENID_CONFIG_RESPONSE, - DEFAULT_TENANT_DISCOVERY_RESPONSE, TEST_TOKEN_RESPONSE, ID_TOKEN_CLAIMS, } from "../utils/StringConstants"; @@ -34,10 +32,8 @@ import { CommonEndSessionRequest, createClientConfigurationError, ClientConfigurationErrorCodes, - Authority, CommonAuthorizationCodeRequest, AuthError, - Logger, NetworkManager, ProtocolUtils, ProtocolMode, @@ -59,7 +55,6 @@ import { createBrowserAuthError, BrowserAuthErrorMessage, } from "../../src/error/BrowserAuthError"; -import { FetchClient } from "../../src/network/FetchClient"; import { InteractionHandler } from "../../src/interaction_handler/InteractionHandler"; import { getDefaultPerformanceClient } from "../utils/TelemetryUtils"; import { AuthenticationResult } from "../../src/response/AuthenticationResult"; @@ -129,35 +124,14 @@ describe("PopupClient", () => { const popupWindow = { ...window, close: () => {}, + focus: () => {}, + location: { + ...window.location, + assign: () => {}, + }, }; // @ts-ignore - sinon.stub(window, "open").returns(popupWindow); - sinon - .stub( - Authority.prototype, - "getEndpointMetadataFromNetwork" - ) - .returns(DEFAULT_OPENID_CONFIG_RESPONSE.body); - sinon - .stub(FetchClient.prototype, "sendGetRequestAsync") - .callsFake((url) => { - console.log("HERE"); - if ( - url.startsWith( - "https://login.microsoftonline.com/common/discovery/instance?" - ) - ) { - return Promise.resolve( - DEFAULT_TENANT_DISCOVERY_RESPONSE - ); - } else { - return Promise.reject({ - headers: {}, - status: 404, - body: {}, - }); - } - }); + jest.spyOn(window, "open").mockReturnValue(popupWindow); }); afterEach(() => { @@ -598,10 +572,7 @@ describe("PopupClient", () => { .stub(PopupClient.prototype, "monitorPopupForHash") .resolves(TEST_HASHES.TEST_SUCCESS_CODE_HASH_POPUP); sinon - .stub( - InteractionHandler.prototype, - "handleCodeResponseFromHash" - ) + .stub(InteractionHandler.prototype, "handleCodeResponse") .resolves(testTokenResponse); jest.spyOn(PkceGenerator, "generatePkceCodes").mockResolvedValue({ challenge: TEST_CONFIG.TEST_CHALLENGE, @@ -617,6 +588,50 @@ describe("PopupClient", () => { expect(tokenResp).toEqual(testTokenResponse); }); + it("throws hash_empty_error if popup returns to redirectUri without a hash", (done) => { + jest.spyOn( + PopupClient.prototype, + "monitorPopupForHash" + ).mockResolvedValue(""); + + popupClient + .acquireToken({ + redirectUri: TEST_URIS.TEST_REDIR_URI, + scopes: TEST_CONFIG.DEFAULT_SCOPES, + }) + .catch((e) => { + expect(e).toEqual( + createBrowserAuthError( + BrowserAuthErrorCodes.hashEmptyError + ) + ); + done(); + }); + }); + + it("throws hash_does_not_contain_known_properties error if popup returns to redirectUri with unrecognized params in the hash", (done) => { + jest.spyOn( + PopupClient.prototype, + "monitorPopupForHash" + ).mockResolvedValue( + "#fakeKey=fakeValue&anotherFakeKey=anotherFakeValue" + ); + + popupClient + .acquireToken({ + redirectUri: TEST_URIS.TEST_REDIR_URI, + scopes: TEST_CONFIG.DEFAULT_SCOPES, + }) + .catch((e) => { + expect(e).toEqual( + createBrowserAuthError( + BrowserAuthErrorCodes.hashDoesNotContainKnownProperties + ) + ); + done(); + }); + }); + describe("storeInCache tests", () => { beforeEach(() => { jest.spyOn(ProtocolUtils, "setRequestState").mockReturnValue( @@ -1626,25 +1641,6 @@ describe("PopupClient", () => { }, 50); }); - it("throws when popup has a hash but does not contain known properties", (done) => { - const popup: Window = { - //@ts-ignore - location: { - href: "http://localhost", - hash: "testHash", - }, - close: () => {}, - closed: false, - }; - popupClient.monitorPopupForHash(popup).catch((e) => { - expect(e.errorCode).toEqual( - BrowserAuthErrorMessage - .hashDoesNotContainKnownPropertiesError.code - ); - done(); - }); - }); - it("throws timeout if popup is same origin but no hash is present", async () => { const popup = { location: { @@ -1759,6 +1755,7 @@ describe("PopupClient", () => { const popup = { location: { href: TEST_URIS.TEST_QUERY_CODE_RESPONSE, + search: "?code=authCode", }, history: { replaceState: () => { @@ -1770,7 +1767,7 @@ describe("PopupClient", () => { // @ts-ignore const result = await popupClient.monitorPopupForHash(popup); - expect(result).toEqual("code=hello"); + expect(result).toEqual("?code=authCode"); }); it("closed", (done) => { diff --git a/lib/msal-browser/test/interaction_client/RedirectClient.spec.ts b/lib/msal-browser/test/interaction_client/RedirectClient.spec.ts index 6391ca1c04..d664cf806a 100644 --- a/lib/msal-browser/test/interaction_client/RedirectClient.spec.ts +++ b/lib/msal-browser/test/interaction_client/RedirectClient.spec.ts @@ -200,9 +200,10 @@ describe("RedirectClient", () => { `${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.REQUEST_STATE}.${stateId}`, TEST_STATE_VALUES.TEST_STATE_REDIRECT ); - sinon - .stub(RedirectClient.prototype, "getRedirectResponseHash") - .returns(TEST_HASHES.TEST_SUCCESS_HASH_NO_STATE); + jest.spyOn( + RedirectClient.prototype, + "getRedirectResponse" + ).mockReturnValue([{ code: "authCode" }, "#code=authCode"]); redirectClient.handleRedirectPromise().then((response) => { expect(response).toBe(null); expect(window.localStorage.length).toEqual(0); @@ -223,9 +224,16 @@ describe("RedirectClient", () => { `${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.REQUEST_STATE}.${stateId}`, TEST_STATE_VALUES.TEST_STATE_REDIRECT ); - sinon - .stub(RedirectClient.prototype, "getRedirectResponseHash") - .returns(TEST_HASHES.TEST_SUCCESS_CODE_HASH_POPUP); + jest.spyOn( + RedirectClient.prototype, + "getRedirectResponse" + ).mockReturnValue([ + { + code: "authCode", + state: TEST_STATE_VALUES.TEST_STATE_POPUP, + }, + TEST_HASHES.TEST_SUCCESS_CODE_HASH_POPUP, + ]); redirectClient.handleRedirectPromise().then((response) => { expect(response).toBe(null); expect(window.localStorage.length).toEqual(0); @@ -250,9 +258,12 @@ describe("RedirectClient", () => { "Unexpected error!", "Unexpected error" ); - sinon - .stub(RedirectClient.prototype, "getRedirectResponseHash") - .throws(testError); + jest.spyOn( + RedirectClient.prototype, + "getRedirectResponse" + ).mockImplementation(() => { + throw testError; + }); redirectClient.handleRedirectPromise().catch((e) => { expect(e).toMatchObject(testError); expect(window.localStorage.length).toEqual(0); @@ -273,10 +284,16 @@ describe("RedirectClient", () => { `${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.REQUEST_STATE}.${stateId}`, TEST_STATE_VALUES.TEST_STATE_REDIRECT ); - //sinon.stub(BrowserProtocolUtils, "extractBrowserRequestState").returns(null); - sinon - .stub(RedirectClient.prototype, "getRedirectResponseHash") - .returns(TEST_HASHES.TEST_SUCCESS_HASH_STATE_NO_META); + jest.spyOn( + RedirectClient.prototype, + "getRedirectResponse" + ).mockReturnValue([ + { + code: "authCode", + state: TEST_STATE_VALUES.ENCODED_LIB_STATE, + }, + TEST_HASHES.TEST_SUCCESS_HASH_STATE_NO_META, + ]); redirectClient.handleRedirectPromise().then((response) => { expect(response).toBe(null); expect(window.localStorage.length).toEqual(0); @@ -1545,13 +1562,16 @@ describe("RedirectClient", () => { `${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.ORIGIN_URI}`, loginRequestUrl ); - sinon - .stub(RedirectClient.prototype, "handleHash") - .callsFake((responseHash) => { - expect(responseHash).toEqual( - TEST_HASHES.TEST_SUCCESS_CODE_HASH_REDIRECT - ); + jest.spyOn( + RedirectClient.prototype, + "handleResponse" + ).mockImplementation((response) => { + expect(response).toEqual({ + code: "thisIsATestCode", + state: TEST_STATE_VALUES.TEST_STATE_REDIRECT, + client_info: TEST_DATA_CLIENT_INFO.TEST_RAW_CLIENT_INFO, }); + }); redirectClient.handleRedirectPromise().then(() => { expect(window.location.href).toEqual(loginRequestUrl); }); @@ -1564,13 +1584,16 @@ describe("RedirectClient", () => { `${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.ORIGIN_URI}`, loginRequestUrl ); - sinon - .stub(RedirectClient.prototype, "handleHash") - .callsFake((responseHash) => { - expect(responseHash).toEqual( - TEST_HASHES.TEST_SUCCESS_CODE_HASH_REDIRECT - ); + jest.spyOn( + RedirectClient.prototype, + "handleResponse" + ).mockImplementation((response) => { + expect(response).toEqual({ + code: "thisIsATestCode", + state: TEST_STATE_VALUES.TEST_STATE_REDIRECT, + client_info: TEST_DATA_CLIENT_INFO.TEST_RAW_CLIENT_INFO, }); + }); redirectClient .handleRedirectPromise( TEST_HASHES.TEST_SUCCESS_CODE_HASH_REDIRECT @@ -1594,13 +1617,16 @@ describe("RedirectClient", () => { window.location.hash = "testHash"; const clearHashSpy = sinon.spy(BrowserUtils, "clearHash"); - sinon - .stub(RedirectClient.prototype, "handleHash") - .callsFake((responseHash) => { - expect(responseHash).toEqual( - TEST_HASHES.TEST_SUCCESS_CODE_HASH_REDIRECT - ); + jest.spyOn( + RedirectClient.prototype, + "handleResponse" + ).mockImplementation((response) => { + expect(response).toEqual({ + code: "thisIsATestCode", + state: TEST_STATE_VALUES.TEST_STATE_REDIRECT, + client_info: TEST_DATA_CLIENT_INFO.TEST_RAW_CLIENT_INFO, }); + }); redirectClient.handleRedirectPromise().then(() => { expect(clearHashSpy.notCalled).toBe(true); @@ -1618,14 +1644,17 @@ describe("RedirectClient", () => { `${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.ORIGIN_URI}`, loginRequestUrl ); - sinon - .stub(RedirectClient.prototype, "handleHash") - .callsFake((responseHash) => { - expect(responseHash).toEqual( - TEST_HASHES.TEST_SUCCESS_CODE_HASH_REDIRECT - ); - done(); + jest.spyOn( + RedirectClient.prototype, + "handleResponse" + ).mockImplementation((response) => { + expect(response).toEqual({ + code: "thisIsATestCode", + client_info: TEST_DATA_CLIENT_INFO.TEST_RAW_CLIENT_INFO, + state: TEST_STATE_VALUES.TEST_STATE_REDIRECT, }); + done(); + }); redirectClient.handleRedirectPromise(); }); @@ -1683,15 +1712,18 @@ describe("RedirectClient", () => { `${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.ORIGIN_URI}`, loginRequestUrl ); - sinon - .stub(RedirectClient.prototype, "handleHash") - .callsFake((responseHash) => { - expect(window.location.href).not.toContain("#testHash"); - expect(responseHash).toEqual( - TEST_HASHES.TEST_SUCCESS_CODE_HASH_REDIRECT - ); - done(); + jest.spyOn( + RedirectClient.prototype, + "handleResponse" + ).mockImplementation((response) => { + expect(window.location.href).not.toContain("#testHash"); + expect(response).toEqual({ + code: "thisIsATestCode", + client_info: TEST_DATA_CLIENT_INFO.TEST_RAW_CLIENT_INFO, + state: TEST_STATE_VALUES.TEST_STATE_REDIRECT, }); + done(); + }); redirectClient.handleRedirectPromise(); }); }); diff --git a/lib/msal-browser/test/interaction_client/SilentIframeClient.spec.ts b/lib/msal-browser/test/interaction_client/SilentIframeClient.spec.ts index 224be55600..9cae9dadd9 100644 --- a/lib/msal-browser/test/interaction_client/SilentIframeClient.spec.ts +++ b/lib/msal-browser/test/interaction_client/SilentIframeClient.spec.ts @@ -240,10 +240,7 @@ describe("SilentIframeClient", () => { .stub(SilentHandler, "monitorIframeForHash") .resolves(TEST_HASHES.TEST_SUCCESS_CODE_HASH_SILENT); sinon - .stub( - InteractionHandler.prototype, - "handleCodeResponseFromHash" - ) + .stub(InteractionHandler.prototype, "handleCodeResponse") .resolves(testTokenResponse); jest.spyOn(PkceGenerator, "generatePkceCodes").mockResolvedValue({ challenge: TEST_CONFIG.TEST_CHALLENGE, @@ -310,10 +307,7 @@ describe("SilentIframeClient", () => { .stub(SilentHandler, "monitorIframeForHash") .resolves(TEST_HASHES.TEST_SUCCESS_CODE_HASH_SILENT); sinon - .stub( - InteractionHandler.prototype, - "handleCodeResponseFromHash" - ) + .stub(InteractionHandler.prototype, "handleCodeResponse") .resolves(testTokenResponse); jest.spyOn(PkceGenerator, "generatePkceCodes").mockResolvedValue({ challenge: TEST_CONFIG.TEST_CHALLENGE, diff --git a/lib/msal-browser/test/interaction_handler/InteractionHandler.spec.ts b/lib/msal-browser/test/interaction_handler/InteractionHandler.spec.ts index 6a425ba623..e9db85304b 100644 --- a/lib/msal-browser/test/interaction_handler/InteractionHandler.spec.ts +++ b/lib/msal-browser/test/interaction_handler/InteractionHandler.spec.ts @@ -382,37 +382,7 @@ describe("InteractionHandler.ts Unit Tests", () => { }); }); - describe("handleCodeResponseFromHash()", () => { - it("throws error if given location hash is empty", async () => { - const interactionHandler = new TestInteractionHandler( - authCodeModule, - browserStorage - ); - expect( - interactionHandler.handleCodeResponseFromHash("", { - authority: "https://www.contoso.com/common/", - scopes: ["User.Read"], - correlationId: TEST_CONFIG.CORRELATION_ID, - redirectUri: "/", - responseMode: "fragment", - nonce: TEST_CONFIG.CORRELATION_ID, - state: TEST_STATE_VALUES.TEST_STATE_REDIRECT, - }) - ).rejects.toMatchObject( - createBrowserAuthError(BrowserAuthErrorCodes.hashEmptyError) - ); - //@ts-ignore - expect( - interactionHandler.handleCodeResponseFromHash( - //@ts-ignore - null, - "" - ) - ).rejects.toMatchObject( - createBrowserAuthError(BrowserAuthErrorCodes.hashEmptyError) - ); - }); - + describe("handleCodeResponse()", () => { // TODO: Need to improve these tests it("successfully uses a new authority if cloud_instance_host_name is different", async () => { const idTokenClaims = { @@ -504,19 +474,21 @@ describe("InteractionHandler.ts Unit Tests", () => { browserStorage ); await interactionHandler.initiateAuthRequest("testNavUrl"); - const tokenResponse = - await interactionHandler.handleCodeResponseFromHash( - TEST_HASHES.TEST_SUCCESS_CODE_HASH_REDIRECT, - { - authority: "https://www.contoso.com/common/", - scopes: ["User.Read"], - correlationId: TEST_CONFIG.CORRELATION_ID, - redirectUri: "/", - responseMode: "fragment", - nonce: TEST_CONFIG.CORRELATION_ID, - state: TEST_STATE_VALUES.TEST_STATE_REDIRECT, - } - ); + const tokenResponse = await interactionHandler.handleCodeResponse( + { + code: "authCode", + state: TEST_STATE_VALUES.TEST_STATE_REDIRECT, + }, + { + authority: "https://www.contoso.com/common/", + scopes: ["User.Read"], + correlationId: TEST_CONFIG.CORRELATION_ID, + redirectUri: "/", + responseMode: "fragment", + nonce: TEST_CONFIG.CORRELATION_ID, + state: TEST_STATE_VALUES.TEST_STATE_REDIRECT, + } + ); expect( updateAuthoritySpy.calledWith( "contoso.com", @@ -632,19 +604,21 @@ describe("InteractionHandler.ts Unit Tests", () => { browserStorage ); await interactionHandler.initiateAuthRequest("testNavUrl"); - const tokenResponse = - await interactionHandler.handleCodeResponseFromHash( - TEST_HASHES.TEST_SUCCESS_CODE_HASH_REDIRECT, - { - authority: "https://www.contoso.com/common/", - scopes: ["User.Read"], - correlationId: TEST_CONFIG.CORRELATION_ID, - redirectUri: "/", - responseMode: "fragment", - nonce: TEST_CONFIG.CORRELATION_ID, - state: TEST_STATE_VALUES.TEST_STATE_REDIRECT, - } - ); + const tokenResponse = await interactionHandler.handleCodeResponse( + { + code: "authCode", + state: TEST_STATE_VALUES.TEST_STATE_REDIRECT, + }, + { + authority: "https://www.contoso.com/common/", + scopes: ["User.Read"], + correlationId: TEST_CONFIG.CORRELATION_ID, + redirectUri: "/", + responseMode: "fragment", + nonce: TEST_CONFIG.CORRELATION_ID, + state: TEST_STATE_VALUES.TEST_STATE_REDIRECT, + } + ); expect( updateAuthoritySpy.calledWith( "contoso.com", diff --git a/lib/msal-browser/test/interaction_handler/RedirectHandler.spec.ts b/lib/msal-browser/test/interaction_handler/RedirectHandler.spec.ts index c2d1674534..4de21e72b0 100644 --- a/lib/msal-browser/test/interaction_handler/RedirectHandler.spec.ts +++ b/lib/msal-browser/test/interaction_handler/RedirectHandler.spec.ts @@ -358,32 +358,7 @@ describe("RedirectHandler.ts Unit Tests", () => { }); }); - describe("handleCodeResponseFromHash()", () => { - it("throws error if given hash is empty", () => { - const redirectHandler = new RedirectHandler( - authCodeModule, - browserStorage, - defaultTokenRequest, - browserRequestLogger, - performanceClient - ); - expect( - redirectHandler.handleCodeResponseFromHash("", "") - ).rejects.toMatchObject( - createBrowserAuthError(BrowserAuthErrorCodes.hashEmptyError) - ); - //@ts-ignore - expect( - redirectHandler.handleCodeResponseFromHash( - //@ts-ignore - null, - "" - ) - ).rejects.toMatchObject( - createBrowserAuthError(BrowserAuthErrorCodes.hashEmptyError) - ); - }); - + describe("handleCodeResponse()", () => { it("successfully handles response", async () => { const idTokenClaims = { ver: "2.0", @@ -476,11 +451,13 @@ describe("RedirectHandler.ts Unit Tests", () => { browserRequestLogger, performanceClient ); - const tokenResponse = - await redirectHandler.handleCodeResponseFromHash( - TEST_HASHES.TEST_SUCCESS_CODE_HASH_REDIRECT, - TEST_STATE_VALUES.TEST_STATE_REDIRECT - ); + const tokenResponse = await redirectHandler.handleCodeResponse( + { + code: "thisIsATestCode", + state: TEST_STATE_VALUES.TEST_STATE_REDIRECT, + }, + TEST_STATE_VALUES.TEST_STATE_REDIRECT + ); expect(tokenResponse).toEqual(testTokenResponse); expect( browserStorage.getTemporaryCache( @@ -597,11 +574,13 @@ describe("RedirectHandler.ts Unit Tests", () => { browserRequestLogger, performanceClient ); - const tokenResponse = - await redirectHandler.handleCodeResponseFromHash( - TEST_HASHES.TEST_SUCCESS_CODE_HASH_REDIRECT, - TEST_STATE_VALUES.TEST_STATE_REDIRECT - ); + const tokenResponse = await redirectHandler.handleCodeResponse( + { + code: "thisIsATestCode", + state: TEST_STATE_VALUES.TEST_STATE_REDIRECT, + }, + TEST_STATE_VALUES.TEST_STATE_REDIRECT + ); expect(tokenResponse).toEqual(testTokenResponse); expect( browserStorage.getTemporaryCache( diff --git a/lib/msal-browser/test/interaction_handler/SilentHandler.spec.ts b/lib/msal-browser/test/interaction_handler/SilentHandler.spec.ts index d225055c69..b8f2f4eb5e 100644 --- a/lib/msal-browser/test/interaction_handler/SilentHandler.spec.ts +++ b/lib/msal-browser/test/interaction_handler/SilentHandler.spec.ts @@ -3,7 +3,12 @@ * Licensed under the MIT License. */ -import { Logger, LoggerOptions, IPerformanceClient } from "@azure/msal-common"; +import { + Logger, + LoggerOptions, + IPerformanceClient, + ServerResponseType, +} from "@azure/msal-common"; import * as SilentHandler from "../../src/interaction_handler/SilentHandler"; import { testNavUrl, RANDOM_TEST_GUID } from "../utils/StringConstants"; import { @@ -111,7 +116,8 @@ describe("SilentHandler.ts Unit Tests", () => { DEFAULT_POLL_INTERVAL_MS, performanceClient, browserRequestLogger, - RANDOM_TEST_GUID + RANDOM_TEST_GUID, + ServerResponseType.FRAGMENT ).catch((e) => { expect(e).toBeInstanceOf(BrowserAuthError); expect(e).toMatchObject( @@ -142,7 +148,8 @@ describe("SilentHandler.ts Unit Tests", () => { DEFAULT_POLL_INTERVAL_MS, performanceClient, browserRequestLogger, - RANDOM_TEST_GUID + RANDOM_TEST_GUID, + ServerResponseType.FRAGMENT ).catch((e) => { expect(e).toBeInstanceOf(BrowserAuthError); expect(e).toMatchObject( @@ -194,7 +201,8 @@ describe("SilentHandler.ts Unit Tests", () => { DEFAULT_POLL_INTERVAL_MS, performanceClient, browserRequestLogger, - RANDOM_TEST_GUID + RANDOM_TEST_GUID, + ServerResponseType.FRAGMENT ).then((hash: string) => { expect(hash).toEqual("#code=hello"); done(); diff --git a/lib/msal-browser/test/response/ResponseHandler.spec.ts b/lib/msal-browser/test/response/ResponseHandler.spec.ts new file mode 100644 index 0000000000..bfbacd2f7e --- /dev/null +++ b/lib/msal-browser/test/response/ResponseHandler.spec.ts @@ -0,0 +1,151 @@ +import { Logger } from "@azure/msal-common"; +import * as ResponseHandler from "../../src/response/ResponseHandler"; +import { BrowserAuthErrorCodes } from "../../src"; +import { createBrowserAuthError } from "../../src/error/BrowserAuthError"; + +describe("ResponseHandler tests", () => { + describe("deserializeResponse", () => { + it("Returns deserialized response from hash", () => { + // Test response with leading hash + expect( + ResponseHandler.deserializeResponse( + "#code=hello&state=state", + "hash", + new Logger({}) + ) + ).toEqual({ code: "hello", state: "state" }); + + // Test response without leading hash + expect( + ResponseHandler.deserializeResponse( + "code=hello&state=state", + "hash", + new Logger({}) + ) + ).toEqual({ code: "hello", state: "state" }); + + // Test error response + expect( + ResponseHandler.deserializeResponse( + "#error=errorCode&error_description=errorDescription", + "hash", + new Logger({}) + ) + ).toEqual({ + error: "errorCode", + error_description: "errorDescription", + }); + }); + + it("Returns deserialized response from query", () => { + // Test response with leading ? + expect( + ResponseHandler.deserializeResponse( + "?code=hello&state=state", + "query", + new Logger({}) + ) + ).toEqual({ code: "hello", state: "state" }); + + // Test response without leading ? + expect( + ResponseHandler.deserializeResponse( + "code=hello&state=state", + "query", + new Logger({}) + ) + ).toEqual({ code: "hello", state: "state" }); + + // Test error response + expect( + ResponseHandler.deserializeResponse( + "?error=errorCode&error_description=errorDescription", + "query", + new Logger({}) + ) + ).toEqual({ + error: "errorCode", + error_description: "errorDescription", + }); + }); + + it("Throws error if response is empty", () => { + expect(() => + ResponseHandler.deserializeResponse("#", "hash", new Logger({})) + ).toThrowError( + createBrowserAuthError(BrowserAuthErrorCodes.hashEmptyError) + ); + + expect(() => + ResponseHandler.deserializeResponse("", "hash", new Logger({})) + ).toThrowError( + createBrowserAuthError(BrowserAuthErrorCodes.hashEmptyError) + ); + + expect(() => + ResponseHandler.deserializeResponse( + "?", + "query", + new Logger({}) + ) + ).toThrowError( + createBrowserAuthError(BrowserAuthErrorCodes.hashEmptyError) + ); + + expect(() => + ResponseHandler.deserializeResponse("", "query", new Logger({})) + ).toThrowError( + createBrowserAuthError(BrowserAuthErrorCodes.hashEmptyError) + ); + }); + + it("Throws error if response does not contain known properties", () => { + expect(() => + ResponseHandler.deserializeResponse( + "#hello", + "hash", + new Logger({}) + ) + ).toThrowError( + createBrowserAuthError( + BrowserAuthErrorCodes.hashDoesNotContainKnownProperties + ) + ); + expect(() => + ResponseHandler.deserializeResponse( + "#key=value", + "hash", + new Logger({}) + ) + ).toThrowError( + createBrowserAuthError( + BrowserAuthErrorCodes.hashDoesNotContainKnownProperties + ) + ); + + expect(() => + ResponseHandler.deserializeResponse( + "?hello", + "query", + new Logger({}) + ) + ).toThrowError( + createBrowserAuthError( + BrowserAuthErrorCodes.hashDoesNotContainKnownProperties + ) + ); + + expect(() => + ResponseHandler.deserializeResponse( + "?key=value", + "query", + new Logger({}) + ) + ).toThrowError( + createBrowserAuthError( + BrowserAuthErrorCodes.hashDoesNotContainKnownProperties + ) + ); + }); + }); +}); diff --git a/lib/msal-browser/test/utils/BrowserProtocolUtils.spec.ts b/lib/msal-browser/test/utils/BrowserProtocolUtils.spec.ts index 8ddb35ba6e..3fada421d8 100644 --- a/lib/msal-browser/test/utils/BrowserProtocolUtils.spec.ts +++ b/lib/msal-browser/test/utils/BrowserProtocolUtils.spec.ts @@ -74,63 +74,4 @@ describe("BrowserProtocolUtils.ts Unit Tests", () => { ); expect(popupPlatformState!.interactionType).toBe(InteractionType.Popup); }); - - describe("parseServerResponseFromHash", () => { - it("parses code from hash", () => { - const serverParams = - BrowserProtocolUtils.parseServerResponseFromHash( - TEST_HASHES.TEST_SUCCESS_CODE_HASH_REDIRECT - ); - - expect(serverParams).toEqual({ - client_info: - "eyJ1aWQiOiIxMjMtdGVzdC11aWQiLCJ1dGlkIjoiNDU2LXRlc3QtdXRpZCJ9", - code: "thisIsATestCode", - state: "eyJpZCI6IjExNTUzYTliLTcxMTYtNDhiMS05ZDQ4LWY2ZDRhOGZmODM3MSIsIm1ldGEiOnsiaW50ZXJhY3Rpb25UeXBlIjoicmVkaXJlY3QifX0=|userState", - }); - }); - - it("parses error from hash", () => { - const serverParams = - BrowserProtocolUtils.parseServerResponseFromHash( - TEST_HASHES.TEST_ERROR_HASH - ); - - expect(serverParams).toEqual({ - error: "error_code", - error_description: "msal error description", - state: "eyJpZCI6IjExNTUzYTliLTcxMTYtNDhiMS05ZDQ4LWY2ZDRhOGZmODM3MSIsIm1ldGEiOnsiaW50ZXJhY3Rpb25UeXBlIjoicmVkaXJlY3QifX0=|userState", - }); - }); - - it("parses tokens from hash", () => { - const serverParams = - BrowserProtocolUtils.parseServerResponseFromHash( - TEST_HASHES.TEST_SUCCESS_ACCESS_TOKEN_HASH - ); - - expect(serverParams).toEqual({ - access_token: "this.isan.accesstoken", - client_info: - "eyJ1aWQiOiIxMjMtdGVzdC11aWQiLCJ1dGlkIjoiNDU2LXRlc3QtdXRpZCJ9", - expiresIn: "3599", - id_token: - "eyJ0eXAiOiJKV1QiLCJhbGciOiJSUzI1NiIsImtpZCI6IjFMVE16YWtpaGlSbGFfOHoyQkVKVlhlV01xbyJ9.eyJ2ZXIiOiIyLjAiLCJpc3MiOiJodHRwczovL2xvZ2luLm1pY3Jvc29mdG9ubGluZS5jb20vOTE4ODA0MGQtNmM2Ny00YzViLWIxMTItMzZhMzA0YjY2ZGFkL3YyLjAiLCJzdWIiOiJBQUFBQUFBQUFBQUFBQUFBQUFBQUFJa3pxRlZyU2FTYUZIeTc4MmJidGFRIiwiYXVkIjoiNmNiMDQwMTgtYTNmNS00NmE3LWI5OTUtOTQwYzc4ZjVhZWYzIiwiZXhwIjoxNTM2MzYxNDExLCJpYXQiOjE1MzYyNzQ3MTEsIm5iZiI6MTUzNjI3NDcxMSwibmFtZSI6IkFiZSBMaW5jb2xuIiwicHJlZmVycmVkX3VzZXJuYW1lIjoiQWJlTGlAbWljcm9zb2Z0LmNvbSIsIm9pZCI6IjAwMDAwMDAwLTAwMDAtMDAwMC02NmYzLTMzMzJlY2E3ZWE4MSIsInRpZCI6IjMzMzgwNDBkLTZjNjctNGM1Yi1iMTEyLTM2YTMwNGI2NmRhZCIsIm5vbmNlIjoiMTIzNTIzIiwiYWlvIjoiRGYyVVZYTDFpeCFsTUNXTVNPSkJjRmF0emNHZnZGR2hqS3Y4cTVnMHg3MzJkUjVNQjVCaXN2R1FPN1lXQnlqZDhpUURMcSFlR2JJRGFreXA1bW5PcmNkcUhlWVNubHRlcFFtUnA2QUlaOGpZIn0=.1AFWW-Ck5nROwSlltm7GzZvDwUkqvhSQpm55TQsmVo9Y59cLhRXpvB8n-55HCr9Z6G_31_UbeUkoz612I2j_Sm9FFShSDDjoaLQr54CreGIJvjtmS3EkK9a7SJBbcpL1MpUtlfygow39tFjY7EVNW9plWUvRrTgVk7lYLprvfzw-CIqw3gHC-T7IK_m_xkr08INERBtaecwhTeN4chPC4W3jdmw_lIxzC48YoQ0dB1L9-ImX98Egypfrlbm0IBL5spFzL6JDZIRRJOu8vecJvj1mq-IUhGt0MacxX8jdxYLP-KUu2d9MbNKpCKJuZ7p8gwTL5B7NlUdh_dmSviPWrw", - scope: "test", - state: "eyJpZCI6IjExNTUzYTliLTcxMTYtNDhiMS05ZDQ4LWY2ZDRhOGZmODM3MSIsIm1ldGEiOnsiaW50ZXJhY3Rpb25UeXBlIjoicmVkaXJlY3QifX0=|userState", - }); - }); - - it("handles empty hash", () => { - const serverParams = - BrowserProtocolUtils.parseServerResponseFromHash("#"); - expect(serverParams).toEqual({}); - }); - - it("handles empty string", () => { - const serverParams = - BrowserProtocolUtils.parseServerResponseFromHash(""); - expect(serverParams).toEqual({}); - }); - }); }); diff --git a/lib/msal-common/src/client/AuthorizationCodeClient.ts b/lib/msal-common/src/client/AuthorizationCodeClient.ts index 80e4302a6d..14777d3580 100644 --- a/lib/msal-common/src/client/AuthorizationCodeClient.ts +++ b/lib/msal-common/src/client/AuthorizationCodeClient.ts @@ -171,7 +171,7 @@ export class AuthorizationCodeClient extends BaseClient { * @param hashFragment */ handleFragmentResponse( - hashFragment: string, + serverParams: ServerAuthorizationCodeResponse, cachedState: string ): AuthorizationCodePayload { // Handle responses. @@ -184,13 +184,6 @@ export class AuthorizationCodeClient extends BaseClient { null ); - const serverParams: ServerAuthorizationCodeResponse = - UrlString.getDeserializedCodeResponse( - this.config.authOptions.authority.options.OIDCOptions - ?.serverResponseType, - hashFragment - ); - // Get code response responseHandler.validateServerAuthorizationCodeResponse( serverParams, @@ -203,11 +196,8 @@ export class AuthorizationCodeClient extends BaseClient { ClientAuthErrorCodes.authorizationCodeMissingFromServerResponse ); } - return { - ...serverParams, - // Code param is optional in ServerAuthorizationCodeResponse but required in AuthorizationCodePaylod - code: serverParams.code, - }; + + return serverParams as AuthorizationCodePayload; } /** diff --git a/lib/msal-common/src/index.ts b/lib/msal-common/src/index.ts index 0c3e8341ae..1c81403f99 100644 --- a/lib/msal-common/src/index.ts +++ b/lib/msal-common/src/index.ts @@ -203,6 +203,7 @@ export { LibraryStateObject, } from "./utils/ProtocolUtils"; export { TimeUtils } from "./utils/TimeUtils"; +export * as UrlUtils from "./utils/UrlUtils"; export * from "./utils/FunctionWrappers"; // Server Telemetry export { ServerTelemetryManager } from "./telemetry/server/ServerTelemetryManager"; diff --git a/lib/msal-common/src/telemetry/performance/PerformanceEvent.ts b/lib/msal-common/src/telemetry/performance/PerformanceEvent.ts index 50ec0442a0..c7fde5ce7e 100644 --- a/lib/msal-common/src/telemetry/performance/PerformanceEvent.ts +++ b/lib/msal-common/src/telemetry/performance/PerformanceEvent.ts @@ -208,7 +208,7 @@ export const PerformanceEvents = { * Functions from InteractionHandler (msal-browser) */ HandleCodeResponseFromServer: "handleCodeResponseFromServer", - HandleCodeResponseFromHash: "handleCodeResponseFromHash", + HandleCodeResponse: "handleCodeResponse", UpdateTokenEndpointAuthority: "updateTokenEndpointAuthority", /** @@ -229,6 +229,7 @@ export const PerformanceEvents = { * handleServerTokenResponse API in ResponseHandler (msal-common) */ HandleServerTokenResponse: "handleServerTokenResponse", + DeserializeResponse: "deserializeResponse", /** * Authority functions diff --git a/lib/msal-common/src/url/UrlString.ts b/lib/msal-common/src/url/UrlString.ts index 2228db44e0..c93726f81e 100644 --- a/lib/msal-common/src/url/UrlString.ts +++ b/lib/msal-common/src/url/UrlString.ts @@ -3,22 +3,14 @@ * Licensed under the MIT License. */ -import { ServerAuthorizationCodeResponse } from "../response/ServerAuthorizationCodeResponse"; import { createClientConfigurationError, ClientConfigurationErrorCodes, } from "../error/ClientConfigurationError"; -import { - ClientAuthErrorCodes, - createClientAuthError, -} from "../error/ClientAuthError"; import { StringUtils } from "../utils/StringUtils"; import { IUri } from "./IUri"; -import { - AADAuthorityConstants, - Constants, - ServerResponseType, -} from "../utils/Constants"; +import { AADAuthorityConstants, Constants } from "../utils/Constants"; +import * as UrlUtils from "../utils/UrlUtils"; /** * Url object class which can perform various transformations on url strings. @@ -39,7 +31,7 @@ export class UrlString { ); } - if (!this.getHash()) { + if (!url.includes("#")) { this._urlString = UrlString.canonicalizeUri(url); } } @@ -142,13 +134,6 @@ export class UrlString { return UrlString.constructAuthorityUriFromObject(urlObject); } - /** - * Returns the anchor part(#) of the URL - */ - getHash(): string { - return UrlString.parseHash(this.urlString); - } - /** * Parses out the components from a url string. * @returns An object with the various components. Please cache this value insted of calling this multiple times on the same url. @@ -221,58 +206,6 @@ export class UrlString { return relativeUrl; } - /** - * Parses hash string from given string. Returns empty string if no hash symbol is found. - * @param hashString - */ - static parseHash(hashString: string): string { - const hashIndex1 = hashString.indexOf("#"); - const hashIndex2 = hashString.indexOf("#/"); - if (hashIndex2 > -1) { - return hashString.substring(hashIndex2 + 2); - } else if (hashIndex1 > -1) { - return hashString.substring(hashIndex1 + 1); - } - return Constants.EMPTY_STRING; - } - - /** - * Parses query string from given string. Returns empty string if no query symbol is found. - * @param queryString - */ - static parseQueryString(queryString: string): string { - const queryIndex1 = queryString.indexOf("?"); - const queryIndex2 = queryString.indexOf("/?"); - if (queryIndex2 > -1) { - return queryString.substring(queryIndex2 + 2); - } else if (queryIndex1 > -1) { - return queryString.substring(queryIndex1 + 1); - } - return Constants.EMPTY_STRING; - } - - /** - * Parses query server response string from given string. - * Extract hash between '?code=' and '#' if trailing '# is present. - * Returns empty string if no query symbol is found. - * @param queryString - */ - static parseQueryServerResponse(queryString: string): string { - const queryIndex1 = queryString.indexOf("?code"); - const queryIndex2 = queryString.indexOf("/?code"); - const hashIndex = queryString.indexOf("#"); - if (queryIndex2 > -1 && hashIndex > -1) { - return queryString.substring(queryIndex2 + 2, hashIndex); - } else if (queryIndex2 > -1) { - return queryString.substring(queryIndex2 + 2); - } else if (queryIndex1 > -1 && hashIndex > -1) { - return queryString.substring(queryIndex1 + 1, hashIndex); - } else if (queryIndex1 > -1) { - return queryString.substring(queryIndex1 + 1); - } - return Constants.EMPTY_STRING; - } - static constructAuthorityUriFromObject(urlObject: IUri): UrlString { return new UrlString( urlObject.Protocol + @@ -283,91 +216,11 @@ export class UrlString { ); } - /** - * Returns URL hash as server auth code response object. - */ - static getDeserializedHash(hash: string): ServerAuthorizationCodeResponse { - // Check if given hash is empty - if (!hash) { - return {}; - } - // Strip the # symbol if present - const parsedHash = UrlString.parseHash(hash); - // If # symbol was not present, above will return empty string, so give original hash value - const deserializedHash: ServerAuthorizationCodeResponse = - StringUtils.queryStringToObject( - parsedHash || hash - ); - // Check if deserialization didn't work - if (!deserializedHash) { - throw createClientAuthError( - ClientAuthErrorCodes.hashNotDeserialized - ); - } - return deserializedHash; - } - - /** - * Returns URL query string as server auth code response object. - */ - static getDeserializedQueryString( - query: string - ): ServerAuthorizationCodeResponse { - // Check if given query is empty - if (!query) { - return {}; - } - // Strip the ? symbol if present - const parsedQueryString = UrlString.parseQueryString(query); - // If ? symbol was not present, above will return empty string, so give original query value - const deserializedQueryString: ServerAuthorizationCodeResponse = - StringUtils.queryStringToObject( - parsedQueryString || query - ); - // Check if deserialization didn't work - if (!deserializedQueryString) { - throw createClientAuthError( - ClientAuthErrorCodes.hashNotDeserialized - ); - } - return deserializedQueryString; - } - /** - * Returns either deserialized query string or deserialized hash, depending on the serverResponseType - * as a server auth code response object. - */ - static getDeserializedCodeResponse( - serverResponseType: ServerResponseType | undefined, - hashFragment: string - ): ServerAuthorizationCodeResponse { - const hashUrlString = new UrlString(hashFragment); - let serverParams: ServerAuthorizationCodeResponse; - if (serverResponseType === ServerResponseType.QUERY) { - serverParams = UrlString.getDeserializedQueryString(hashFragment); - } else { - serverParams = UrlString.getDeserializedHash( - hashUrlString.getHash() - ); - } - return serverParams; - } - /** * Check if the hash of the URL string contains known properties + * @deprecated This API will be removed in a future version */ - static hashContainsKnownProperties(hash: string): boolean { - if (!hash || hash.indexOf("=") < 0) { - // Hash doesn't contain key/value pairs - return false; - } - - const parameters: ServerAuthorizationCodeResponse = - UrlString.getDeserializedHash(hash); - return !!( - parameters.code || - parameters.error_description || - parameters.error || - parameters.state - ); + static hashContainsKnownProperties(response: string): boolean { + return !!UrlUtils.getDeserializedResponse(response); } } diff --git a/lib/msal-common/src/utils/UrlUtils.ts b/lib/msal-common/src/utils/UrlUtils.ts new file mode 100644 index 0000000000..f80b966ec6 --- /dev/null +++ b/lib/msal-common/src/utils/UrlUtils.ts @@ -0,0 +1,60 @@ +/* + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. + */ + +import { ServerAuthorizationCodeResponse } from "../response/ServerAuthorizationCodeResponse"; +import { + ClientAuthErrorCodes, + createClientAuthError, +} from "../error/ClientAuthError"; + +/** + * Parses hash string from given string. Returns empty string if no hash symbol is found. + * @param hashString + */ +export function stripLeadingHashOrQuery(responseString: string): string { + if (responseString.startsWith("#/")) { + return responseString.substring(2); + } else if ( + responseString.startsWith("#") || + responseString.startsWith("?") + ) { + return responseString.substring(1); + } + + return responseString; +} + +/** + * Returns URL hash as server auth code response object. + */ +export function getDeserializedResponse( + responseString: string +): ServerAuthorizationCodeResponse | null { + // Check if given hash is empty + if (!responseString || responseString.indexOf("=") < 0) { + return null; + } + try { + // Strip the # or ? symbol if present + const normalizedResponse = stripLeadingHashOrQuery(responseString); + // If # symbol was not present, above will return empty string, so give original hash value + const deserializedHash: ServerAuthorizationCodeResponse = + Object.fromEntries(new URLSearchParams(normalizedResponse)); + + // Check for known response properties + if ( + deserializedHash.code || + deserializedHash.error || + deserializedHash.error_description || + deserializedHash.state + ) { + return deserializedHash; + } + } catch (e) { + throw createClientAuthError(ClientAuthErrorCodes.hashNotDeserialized); + } + + return null; +} diff --git a/lib/msal-common/test/client/AuthorizationCodeClient.spec.ts b/lib/msal-common/test/client/AuthorizationCodeClient.spec.ts index d6074516cc..6e53527a63 100644 --- a/lib/msal-common/test/client/AuthorizationCodeClient.spec.ts +++ b/lib/msal-common/test/client/AuthorizationCodeClient.spec.ts @@ -44,6 +44,7 @@ import { createClientAuthError, } from "../../src/error/ClientAuthError"; import { + AuthError, CcsCredentialType, ClientConfigurationErrorCodes, createClientConfigurationError, @@ -1112,55 +1113,18 @@ describe("AuthorizationCodeClient unit tests", () => { describe("handleFragmentResponse()", () => { it("returns valid server code response", async () => { - sinon - .stub( - Authority.prototype, - "getEndpointMetadataFromNetwork" - ) - .resolves(DEFAULT_OPENID_CONFIG_RESPONSE.body); const config: ClientConfiguration = await ClientTestUtils.createTestClientConfiguration(); - if (!config.cryptoInterface) { - throw TestError.createTestSetupError( - "configuration crypto interface not initialized correctly." - ); - } - const testSuccessHash = `#code=thisIsATestCode&client_info=${ - TEST_DATA_CLIENT_INFO.TEST_RAW_CLIENT_INFO - }&state=${encodeURIComponent(TEST_STATE_VALUES.ENCODED_LIB_STATE)}`; - sinon - .stub(config.cryptoInterface, "base64Decode") - .callsFake((input) => { - switch (input) { - case TEST_DATA_CLIENT_INFO.TEST_RAW_CLIENT_INFO: - return TEST_DATA_CLIENT_INFO.TEST_DECODED_CLIENT_INFO; - case TEST_POP_VALUES.ENCODED_REQ_CNF: - return TEST_POP_VALUES.DECODED_REQ_CNF; - default: - return input; - } - }); - - sinon - .stub(config.cryptoInterface, "base64Encode") - .callsFake((input) => { - switch (input) { - case "123-test-uid": - return "MTIzLXRlc3QtdWlk"; - case "456-test-utid": - return "NDU2LXRlc3QtdXRpZA=="; - case TEST_POP_VALUES.DECODED_REQ_CNF: - return TEST_POP_VALUES.ENCODED_REQ_CNF; - default: - return input; - } - }); const client: AuthorizationCodeClient = new AuthorizationCodeClient( config ); const authCodePayload = client.handleFragmentResponse( - testSuccessHash, + { + code: "thisIsATestCode", + state: TEST_STATE_VALUES.ENCODED_LIB_STATE, + client_info: TEST_DATA_CLIENT_INFO.TEST_RAW_CLIENT_INFO, + }, TEST_STATE_VALUES.ENCODED_LIB_STATE ); expect(authCodePayload.code).toBe("thisIsATestCode"); @@ -1170,9 +1134,6 @@ describe("AuthorizationCodeClient unit tests", () => { }); it("throws server error when error is in hash", async () => { - const testErrorHash = `#error=error_code&error_description=msal+error+description&state=${encodeURIComponent( - TEST_STATE_VALUES.ENCODED_LIB_STATE - )}`; sinon .stub( Authority.prototype, @@ -1186,21 +1147,23 @@ describe("AuthorizationCodeClient unit tests", () => { ); const cacheStorageMock = config.storageInterface as MockStorageClass; - expect(() => - client.handleFragmentResponse( - testErrorHash, - TEST_STATE_VALUES.ENCODED_LIB_STATE - ) - ).toThrowError("msal error description"); - expect(cacheStorageMock.getKeys().length).toBe(1); - expect(cacheStorageMock.getAuthorityMetadataKeys().length).toBe(1); - expect(() => + let error: AuthError | null = null; + try { client.handleFragmentResponse( - testErrorHash, + { + error: "error_code", + error_description: "msal error description", + state: TEST_STATE_VALUES.ENCODED_LIB_STATE, + }, TEST_STATE_VALUES.ENCODED_LIB_STATE - ) - ).toThrowError(ServerError); + ); + } catch (e) { + error = e as AuthError; + } + expect(error).toBeInstanceOf(ServerError); + expect(error?.errorCode).toEqual("error_code"); + expect(error?.errorMessage).toEqual("msal error description"); expect(cacheStorageMock.getKeys().length).toBe(1); expect(cacheStorageMock.getAuthorityMetadataKeys().length).toBe(1); }); diff --git a/lib/msal-common/test/url/UrlString.spec.ts b/lib/msal-common/test/url/UrlString.spec.ts index 47da47c4c7..7a840f2b6d 100644 --- a/lib/msal-common/test/url/UrlString.spec.ts +++ b/lib/msal-common/test/url/UrlString.spec.ts @@ -129,54 +129,6 @@ describe("UrlString.ts Class Unit Tests", () => { expect(newUrlObj.urlString).not.toContain(sampleTenantId2); }); - it("getHash returns the anchor part of the URL correctly, or nothing if there is no anchor", () => { - const urlWithHash = - TEST_URIS.TEST_AUTH_ENDPT + TEST_HASHES.TEST_SUCCESS_ID_TOKEN_HASH; - const urlWithHashAndSlash = - TEST_URIS.TEST_AUTH_ENDPT + - "#/" + - TEST_HASHES.TEST_SUCCESS_ID_TOKEN_HASH.substring(1); - const urlWithoutHash = TEST_URIS.TEST_AUTH_ENDPT; - - const urlObjWithHash = new UrlString(urlWithHash); - const urlObjWithHashAndSlash = new UrlString(urlWithHashAndSlash); - const urlObjWithoutHash = new UrlString(urlWithoutHash); - - expect(urlObjWithHash.getHash()).toBe( - TEST_HASHES.TEST_SUCCESS_ID_TOKEN_HASH.substring(1) - ); - expect(urlObjWithHashAndSlash.getHash()).toBe( - TEST_HASHES.TEST_SUCCESS_ID_TOKEN_HASH.substring(1) - ); - expect(urlObjWithoutHash.getHash()).toHaveLength(0); - }); - - it("getDeserializedHash returns the hash as a deserialized object", () => { - const serializedHash = "#param1=value1¶m2=value2¶m3=value3"; - const deserializedHash = { - param1: "value1", - param2: "value2", - param3: "value3", - }; - - expect(UrlString.getDeserializedHash(serializedHash)).toEqual( - deserializedHash - ); - }); - - it("getDeserializedHash returns empty object if key/value is undefined", () => { - let serializedHash = "#=value1"; - const deserializedHash = {}; - expect(UrlString.getDeserializedHash(serializedHash)).toEqual( - deserializedHash - ); - - serializedHash = "#key1="; - expect(UrlString.getDeserializedHash(serializedHash)).toEqual( - deserializedHash - ); - }); - it("getUrlComponents returns all path components", () => { const urlObj = new UrlString(TEST_URIS.TEST_AUTH_ENDPT_WITH_PARAMS2); expect(urlObj.getUrlComponents()).toEqual({ diff --git a/lib/msal-common/test/utils/UrlUtils.spec.ts b/lib/msal-common/test/utils/UrlUtils.spec.ts new file mode 100644 index 0000000000..2e100a6feb --- /dev/null +++ b/lib/msal-common/test/utils/UrlUtils.spec.ts @@ -0,0 +1,92 @@ +import * as UrlUtils from "../../src/utils/UrlUtils"; + +describe("UrlUtils.ts Class Unit Tests", () => { + describe("stripLeadingHashOrQuery Tests", () => { + it("strips leading # if present", () => { + expect(UrlUtils.stripLeadingHashOrQuery("#value")).toEqual("value"); + }); + + it("strips leading ? if present", () => { + expect(UrlUtils.stripLeadingHashOrQuery("?value")).toEqual("value"); + }); + + it("strips leading #/ if present", () => { + expect(UrlUtils.stripLeadingHashOrQuery("#/value")).toEqual( + "value" + ); + }); + + it("returns input as-is if # or ? are not present", () => { + expect(UrlUtils.stripLeadingHashOrQuery("value")).toEqual("value"); + }); + }); + + describe("getDeserializedResponse Tests", () => { + it("getDeserializedResponse returns object if hash contains known properties", () => { + expect(UrlUtils.getDeserializedResponse("#code=value")).toEqual({ + code: "value", + }); + expect(UrlUtils.getDeserializedResponse("#state=value")).toEqual({ + state: "value", + }); + expect(UrlUtils.getDeserializedResponse("#error=value")).toEqual({ + error: "value", + }); + expect( + UrlUtils.getDeserializedResponse("#error_description=value") + ).toEqual({ error_description: "value" }); + }); + + it("getDeserializedResponse returns object if query string contains known properties", () => { + expect(UrlUtils.getDeserializedResponse("?code=value")).toEqual({ + code: "value", + }); + expect(UrlUtils.getDeserializedResponse("?state=value")).toEqual({ + state: "value", + }); + expect(UrlUtils.getDeserializedResponse("?error=value")).toEqual({ + error: "value", + }); + expect( + UrlUtils.getDeserializedResponse("?error_description=value") + ).toEqual({ error_description: "value" }); + }); + + it("getDeserializedResponse returns the hash as a deserialized object", () => { + const serializedHash = + "#code=value1&state=value2&client_info=value3"; + const deserializedHash = { + code: "value1", + state: "value2", + client_info: "value3", + }; + + expect(UrlUtils.getDeserializedResponse(serializedHash)).toEqual( + deserializedHash + ); + }); + + it("getDeserializedResponse returns the queryString as a deserialized object", () => { + const serializedHash = + "?code=value1&state=value2&client_info=value3"; + const deserializedHash = { + code: "value1", + state: "value2", + client_info: "value3", + }; + + expect(UrlUtils.getDeserializedResponse(serializedHash)).toEqual( + deserializedHash + ); + }); + + it("getDeserializedResponse returns null if key/value is undefined", () => { + expect(UrlUtils.getDeserializedResponse("#")).toBe(null); + expect(UrlUtils.getDeserializedResponse("?")).toBe(null); + expect(UrlUtils.getDeserializedResponse("#=value1")).toBe(null); + expect(UrlUtils.getDeserializedResponse("?=value1")).toBe(null); + expect(UrlUtils.getDeserializedResponse("#key1=")).toBe(null); + expect(UrlUtils.getDeserializedResponse("?key1=")).toBe(null); + }); + }); +}); diff --git a/lib/msal-node/src/network/LoopbackClient.ts b/lib/msal-node/src/network/LoopbackClient.ts index a6aba36b6a..1d79bfdd2f 100644 --- a/lib/msal-node/src/network/LoopbackClient.ts +++ b/lib/msal-node/src/network/LoopbackClient.ts @@ -6,8 +6,8 @@ import { Constants as CommonConstants, ServerAuthorizationCodeResponse, - UrlString, HttpStatus, + UrlUtils, } from "@azure/msal-common"; import http from "http"; import { NodeAuthError } from "../error/NodeAuthError.js"; @@ -53,10 +53,13 @@ export class LoopbackClient implements ILoopbackClient { return; } + const redirectUri = this.getRedirectUri(); + const parsedUrl = new URL(url, redirectUri); const authCodeResponse = - UrlString.getDeserializedQueryString(url); + UrlUtils.getDeserializedResponse( + parsedUrl.search + ) || {}; if (authCodeResponse.code) { - const redirectUri = this.getRedirectUri(); res.writeHead(HttpStatus.REDIRECT, { location: redirectUri, }); // Prevent auth code from being saved in the browser history