Remove child proxying for web authentication workflow (#2710)

* Refactor all authentication web code. * Remove all type errors in code to make debugging easier. * Added change file. * Make changefile more explicit. * Remove leftover import. * Remove all null assertions and always code expected number of assertions.
OfficeDev · Feb 7, 2025 · d09b02e · d09b02e
1 parent 732d627
commit d09b02e
Show file tree

Hide file tree

Showing 5 changed files with 495 additions and 965 deletions.
diff --git a/change/@microsoft-teams-js-ce65abd1-c746-4271-990d-553f17260449.json b/change/@microsoft-teams-js-ce65abd1-c746-4271-990d-553f17260449.json
@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "Fixed security issue in `authenticate` function for web hosts to avoid using child proxying.",
+  "packageName": "@microsoft/teams-js",
+  "email": "jcardenasr123@gmail.com",
+  "dependentChangeType": "patch"
+}
diff --git a/packages/teams-js/src/internal/appHelpers.ts b/packages/teams-js/src/internal/appHelpers.ts
@@ -16,7 +16,6 @@ import { getLogger } from '../internal/telemetry';
 import { isNullOrUndefined } from '../internal/typeCheckUtilities';
 import { compareSDKVersions, inServerSideRenderingEnvironment, runWithTimeout } from '../internal/utils';
 import * as app from '../public/app/app';
-import * as authentication from '../public/authentication';
 import { FrameContexts } from '../public/constants';
 import * as dialog from '../public/dialog/dialog';
 import * as menus from '../public/menus';
@@ -201,7 +200,6 @@ function initializeHelper(apiVersionTag: string, validMessageOrigins?: string[])
         },
       );
 
-      authentication.initialize();
       menus.initialize();
       pages.config.initialize();
       dialog.initialize();

diff --git a/packages/teams-js/src/public/authentication.ts b/packages/teams-js/src/public/authentication.ts
@@ -7,17 +7,14 @@
 
 import {
   Communication,
-  sendMessageEventToChild,
   sendMessageToParent,
   sendMessageToParentAsync,
   waitForMessageQueue,
 } from '../internal/communication';
-import { GlobalVars } from '../internal/globalVars';
-import { registerHandler, removeHandler } from '../internal/handlers';
 import { ensureInitializeCalled, ensureInitialized } from '../internal/internalAPIs';
 import { ApiName, ApiVersionNumber, getApiVersionTag } from '../internal/telemetry';
 import { fullyQualifyUrlString, validateUrl } from '../internal/utils';
-import { FrameContexts, HostClientType } from './constants';
+import { FrameContexts } from './constants';
 import { SdkError } from './interfaces';
 import { runtime } from './runtime';
 
@@ -27,35 +24,11 @@ import { runtime } from './runtime';
 const authenticationTelemetryVersionNumber_v1: ApiVersionNumber = ApiVersionNumber.V_1;
 const authenticationTelemetryVersionNumber_v2: ApiVersionNumber = ApiVersionNumber.V_2;
 
-let authHandlers: { success: (string) => void; fail: (string) => void } | undefined;
-let authWindowMonitor: number | undefined;
-
 /**
  * @hidden
  * @internal
  * Limited to Microsoft-internal use; automatically called when library is initialized
  */
-export function initialize(): void {
-  registerHandler(
-    getApiVersionTag(
-      authenticationTelemetryVersionNumber_v1,
-      ApiName.Authentication_RegisterAuthenticateSuccessHandler,
-    ),
-    'authentication.authenticate.success',
-    handleSuccess,
-    false,
-  );
-  registerHandler(
-    getApiVersionTag(
-      authenticationTelemetryVersionNumber_v1,
-      ApiName.Authentication_RegisterAuthenticateFailureHandler,
-    ),
-    'authentication.authenticate.failure',
-    handleFailure,
-    false,
-  );
-}
-
 let authParams: AuthenticateParameters | undefined;
 /**
  * @deprecated
@@ -160,35 +133,25 @@ export function authenticate(authenticateParameters?: AuthenticateParameters): P
     });
 }
 
-function authenticateHelper(apiVersionTag: string, authenticateParameters: AuthenticateParameters): Promise<string> {
-  return new Promise<string>((resolve, reject) => {
-    if (GlobalVars.hostClientType !== HostClientType.web) {
-      // Convert any relative URLs into absolute URLs before sending them over to the parent window.
-      const fullyQualifiedURL: URL = fullyQualifyUrlString(authenticateParameters.url);
-      validateUrl(fullyQualifiedURL);
+async function authenticateHelper(
+  apiVersionTag: string,
+  authenticateParameters: AuthenticateParameters,
+): Promise<string> {
+  // Convert any relative URLs into absolute URLs before sending them over to the parent window.
+  const fullyQualifiedURL: URL = fullyQualifyUrlString(authenticateParameters.url);
+  validateUrl(fullyQualifiedURL);
 
-      // Ask the parent window to open an authentication window with the parameters provided by the caller.
-      resolve(
-        sendMessageToParentAsync<[boolean, string]>(apiVersionTag, 'authentication.authenticate', [
-          fullyQualifiedURL.href,
-          authenticateParameters.width,
-          authenticateParameters.height,
-          authenticateParameters.isExternal,
-        ]).then(([success, response]: [boolean, string]) => {
-          if (success) {
-            return response;
-          } else {
-            throw new Error(response);
-          }
-        }),
-      );
+  // Ask the parent window to open an authentication window with the parameters provided by the caller.
+  return sendMessageToParentAsync<[boolean, string]>(apiVersionTag, 'authentication.authenticate', [
+    fullyQualifiedURL.href,
+    authenticateParameters.width,
+    authenticateParameters.height,
+    authenticateParameters.isExternal,
+  ]).then(([success, response]: [boolean, string]) => {
+    if (success) {
+      return response;
     } else {
-      // Open an authentication window with the parameters provided by the caller.
-      authHandlers = {
-        success: resolve,
-        fail: reject,
-      };
-      openAuthenticationWindow(authenticateParameters);
+      throw new Error(response);
     }
   });
 }
@@ -326,132 +289,6 @@ function getUserHelper(apiVersionTag: string): Promise<UserProfile> {
   });
 }
 
-function closeAuthenticationWindow(): void {
-  // Stop monitoring the authentication window
-  stopAuthenticationWindowMonitor();
-  // Try to close the authentication window and clear all properties associated with it
-  try {
-    if (Communication.childWindow) {
-      Communication.childWindow.close();
-    }
-  } finally {
-    Communication.childWindow = null;
-    Communication.childOrigin = null;
-  }
-}
-
-/**
- * Different browsers handle authentication flows in pop-up windows differently.
- * Firefox and Safari, which use Quantum and WebKit browser engines respectively, block the use of 'window.open' for pop-up windows.
- * Any chrome-based browser (Chrome, Edge, Brave, etc.) opens a new browser window without any user-prompts.
- * To ensure consistent behavior across all browsers, consider using the following function to create a new authentication window.
- *
- * @param authenticateParameters - Parameters describing the authentication window used for executing the authentication flow.
- */
-function openAuthenticationWindow(authenticateParameters: AuthenticateParameters): void {
-  // Close the previously opened window if we have one
-  closeAuthenticationWindow();
-  // Start with a sensible default size
-  let width = authenticateParameters.width || 600;
-  let height = authenticateParameters.height || 400;
-  // Ensure that the new window is always smaller than our app's window so that it never fully covers up our app
-  width = Math.min(width, Communication.currentWindow.outerWidth - 400);
-  height = Math.min(height, Communication.currentWindow.outerHeight - 200);
-
-  // Convert any relative URLs into absolute URLs before sending them over to the parent window
-  const fullyQualifiedURL = fullyQualifyUrlString(authenticateParameters.url.replace('{oauthRedirectMethod}', 'web'));
-  validateUrl(fullyQualifiedURL);
-
-  // We are running in the browser, so we need to center the new window ourselves
-  let left: number =
-    typeof Communication.currentWindow.screenLeft !== 'undefined'
-      ? Communication.currentWindow.screenLeft
-      : Communication.currentWindow.screenX;
-  let top: number =
-    typeof Communication.currentWindow.screenTop !== 'undefined'
-      ? Communication.currentWindow.screenTop
-      : Communication.currentWindow.screenY;
-  left += Communication.currentWindow.outerWidth / 2 - width / 2;
-  top += Communication.currentWindow.outerHeight / 2 - height / 2;
-  // Open a child window with a desired set of standard browser features
-  Communication.childWindow = Communication.currentWindow.open(
-    fullyQualifiedURL.href,
-    '_blank',
-    'toolbar=no, location=yes, status=no, menubar=no, scrollbars=yes, top=' +
-      top +
-      ', left=' +
-      left +
-      ', width=' +
-      width +
-      ', height=' +
-      height,
-  );
-  if (Communication.childWindow) {
-    // Start monitoring the authentication window so that we can detect if it gets closed before the flow completes
-    startAuthenticationWindowMonitor();
-  } else {
-    // If we failed to open the window, fail the authentication flow
-    handleFailure('FailedToOpenWindow');
-  }
-}
-
-function stopAuthenticationWindowMonitor(): void {
-  if (authWindowMonitor) {
-    clearInterval(authWindowMonitor);
-    authWindowMonitor = 0;
-  }
-  removeHandler('initialize');
-  removeHandler('navigateCrossDomain');
-}
-
-function startAuthenticationWindowMonitor(): void {
-  // Stop the previous window monitor if one is running
-  stopAuthenticationWindowMonitor();
-  // Create an interval loop that
-  // - Notifies the caller of failure if it detects that the authentication window is closed
-  // - Keeps pinging the authentication window while it is open to re-establish
-  //   contact with any pages along the authentication flow that need to communicate
-  //   with us
-  authWindowMonitor = Communication.currentWindow.setInterval(() => {
-    if (!Communication.childWindow || Communication.childWindow.closed) {
-      handleFailure('CancelledByUser');
-    } else {
-      const savedChildOrigin = Communication.childOrigin;
-      try {
-        Communication.childOrigin = '*';
-        sendMessageEventToChild('ping');
-      } finally {
-        Communication.childOrigin = savedChildOrigin;
-      }
-    }
-  }, 100);
-  // Set up an initialize-message handler that gives the authentication window its frame context
-  registerHandler(
-    getApiVersionTag(
-      authenticationTelemetryVersionNumber_v1,
-      ApiName.Authentication_AuthenticationWindow_RegisterInitializeHandler,
-    ),
-    'initialize',
-    () => {
-      return [FrameContexts.authentication, GlobalVars.hostClientType];
-    },
-  );
-  // Set up a navigateCrossDomain message handler that blocks cross-domain re-navigation attempts
-  // in the authentication window. We could at some point choose to implement this method via a call to
-  // authenticationWindow.location.href = url; however, we would first need to figure out how to
-  // validate the URL against the tab's list of valid domains.
-  registerHandler(
-    getApiVersionTag(
-      authenticationTelemetryVersionNumber_v1,
-      ApiName.Authentication_AuthenticationWindow_RegisterNavigateCrossDomainHandler,
-    ),
-    'navigateCrossDomain',
-    () => {
-      return false;
-    },
-  );
-}
-
 /**
  * When using {@link authentication.authenticate authentication.authenticate(authenticateParameters: AuthenticatePopUpParameters): Promise\<string\>}, the
  * window that was opened to execute the authentication flow should call this method after authentication to notify the caller of
@@ -520,28 +357,6 @@ export function notifyFailure(reason?: string, _callbackUrl?: string): void {
   waitForMessageQueue(Communication.parentWindow, () => setTimeout(() => Communication.currentWindow.close(), 200));
 }
 
-function handleSuccess(result?: string): void {
-  try {
-    if (authHandlers) {
-      authHandlers.success(result);
-    }
-  } finally {
-    authHandlers = undefined;
-    closeAuthenticationWindow();
-  }
-}
-
-function handleFailure(reason?: string): void {
-  try {
-    if (authHandlers) {
-      authHandlers.fail(new Error(reason));
-    }
-  } finally {
-    authHandlers = undefined;
-    closeAuthenticationWindow();
-  }
-}
-
 /**
  * @deprecated
  * As of TeamsJS v2.0.0, this interface has been deprecated in favor of leveraging the `Promise` returned from {@link authentication.authenticate authentication.authenticate(authenticateParameters: AuthenticatePopUpParameters): Promise\<string\>}

diff --git a/packages/teams-js/test/public/app.spec.ts b/packages/teams-js/test/public/app.spec.ts
@@ -2,7 +2,7 @@ import { errorLibraryNotInitialized } from '../../src/internal/constants';
 import { GlobalVars } from '../../src/internal/globalVars';
 import { DOMMessageEvent } from '../../src/internal/interfaces';
 import { ensureInitialized } from '../../src/internal/internalAPIs';
-import { AppId, authentication, dialog, menus, pages } from '../../src/public';
+import { AppId, dialog, menus, pages } from '../../src/public';
 import * as app from '../../src/public/app/app';
 import {
   ChannelType,
@@ -343,17 +343,6 @@ describe('Testing app capability', () => {
         });
       });
 
-      it('app.initialize should call authentication.initialize', async () => {
-        const spy = jest.spyOn(authentication, 'initialize');
-
-        const initPromise = app.initialize();
-        const initMessage = utils.findMessageByFunc('initialize');
-        await utils.respondToMessage(initMessage, FrameContexts.content);
-        await initPromise;
-
-        expect(spy).toHaveBeenCalled();
-      });
-
       it('app.initialize should call menus.initialize', async () => {
         const spy = jest.spyOn(menus, 'initialize');
 
@@ -1357,22 +1346,6 @@ describe('Testing app capability', () => {
         });
       });
 
-      it('app.initialize should call authentication.initialize', async () => {
-        const spy = jest.spyOn(authentication, 'initialize');
-
-        const initPromise = app.initialize();
-        const initMessage = utils.findMessageByFunc('initialize');
-        await utils.respondToFramelessMessage({
-          data: {
-            id: initMessage.id,
-            args: [],
-          },
-        } as DOMMessageEvent);
-        await initPromise;
-
-        expect(spy).toHaveBeenCalled();
-      });
-
       it('app.initialize should call menus.initialize', async () => {
         const spy = jest.spyOn(menus, 'initialize');