Repository: auth0/auth0-spa-js
Pull request #422 (2020-04-15 23:30:58)

Fix leaking windows message event listener
In both "loginWithPopup" and "loginWithRedirect", window.addEventListener('message',...  is called.

There are issues with the cleanup of this to properly remove it from the window. In the case of timeouts, it is not being removed properly.  For loginWithPopup, it wasn't being removed at all causing leaking resources the more times its called.

stevehobbsdev: Thanks for this @yinzara. The test is failing thanks to some whitespace removal but is being address in #420. Once that is merged we can bring it in here too.
stevehobbsdev: @yinzara Please let me know if you'd like to continue with this PR. There are a few unit tests that look to be broken - could you evaluate and fix? Thanks.
stevehobbsdev: @adamjmcgrath I have fixed up this PR, could you please take a pass over?

Patch:
diff --git a/src/utils.ts b/src/utils.ts
index 9594485..6910b5f 100644
--- a/src/utils.ts
+++ b/src/utils.ts
@@ -43,6 +43,7 @@ export const runIframe = (
 ) => {
   return new Promise<AuthenticationResult>((res, rej) => {
     const iframe = window.document.createElement('iframe');
+
     iframe.setAttribute('width', '0');
     iframe.setAttribute('height', '0');
     iframe.style.display = 'none';
@@ -50,30 +51,39 @@ export const runIframe = (
     const removeIframe = () => {
       if (window.document.body.contains(iframe)) {
         window.document.body.removeChild(iframe);
+        window.removeEventListener('message', iframeEventHandler, false);
       }
     };
 
+    let iframeEventHandler: (e: MessageEvent) => void;
+
     const timeoutSetTimeoutId = setTimeout(() => {
       rej(new TimeoutError());
       removeIframe();
     }, timeoutInSeconds * 1000);
 
-    const iframeEventHandler = function (e: MessageEvent) {
+    iframeEventHandler = function (e: MessageEvent) {
       if (e.origin != eventOrigin) return;
       if (!e.data || e.data.type !== 'authorization_response') return;
+
       const eventSource = e.source;
+
       if (eventSource) {
         (eventSource as any).close();
       }
+
       e.data.response.error
         ? rej(GenericError.fromPayload(e.data.response))
         : res(e.data.response);
+
       clearTimeout(timeoutSetTimeoutId);
       window.removeEventListener('message', iframeEventHandler, false);
+
       // Delay the removal of the iframe to prevent hanging loading status
       // in Chrome: https://github.com/auth0/auth0-spa-js/issues/240
       setTimeout(removeIframe, CLEANUP_IFRAME_TIMEOUT_IN_SECONDS * 1000);
     };
+
     window.addEventListener('message', iframeEventHandler, false);
     window.document.body.appendChild(iframe);
     iframe.setAttribute('src', authorizeUrl);
@@ -107,20 +117,30 @@ export const runPopup = (authorizeUrl: string, config: PopupConfigOptions) => {
   }
 
   return new Promise<AuthenticationResult>((resolve, reject) => {
+    let popupEventListener;
+
     const timeoutId = setTimeout(() => {
       reject(new PopupTimeoutError(popup));
+      window.removeEventListener('message', popupEventListener, false);
     }, (config.timeoutInSeconds || DEFAULT_AUTHORIZE_TIMEOUT_IN_SECONDS) * 1000);
-    window.addEventListener('message', e => {
+
+    popupEventListener = function (e: MessageEvent) {
       if (!e.data || e.data.type !== 'authorization_response') {
         return;
       }
+
       clearTimeout(timeoutId);
+      window.removeEventListener('message', popupEventListener, false);
       popup.close();
+
       if (e.data.response.error) {
         return reject(GenericError.fromPayload(e.data.response));
       }
+
       resolve(e.data.response);
-    });
+    };
+
+    window.addEventListener('message', e => popupEventListener(e));
   });
 };
 
@@ -367,9 +387,7 @@ export const validateCrypto = () => {
   }
   if (typeof getCryptoSubtle() === 'undefined') {
     throw new Error(`
-      auth0-spa-js must run on a secure origin.
-      See https://github.com/auth0/auth0-spa-js/blob/master/FAQ.md#why-do-i-get-auth0-spa-js-must-run-on-a-secure-origin 
-      for more information.
+      auth0-spa-js must run on a secure origin. See https://github.com/auth0/auth0-spa-js/blob/master/FAQ.md#why-do-i-get-auth0-spa-js-must-run-on-a-secure-origin for more information.
     `);
   }
 };


Test patch:
diff --git a/__tests__/utils.test.ts b/__tests__/utils.test.ts
index 7d0347d..3730ac1 100644
--- a/__tests__/utils.test.ts
+++ b/__tests__/utils.test.ts
@@ -793,6 +793,7 @@ describe('utils', () => {
       jest.runAllTimers();
       expect(message.source.close).toHaveBeenCalled();
       expect(window.document.body.removeChild).toHaveBeenCalledWith(iframe);
+      expect(window.removeEventListener).toBeCalled();
     });
     it('times out after timeoutInSeconds', async () => {
       const { iframe, url, origin } = setup('');
@@ -803,6 +804,15 @@ describe('utils', () => {
       await expect(promise).rejects.toMatchObject(TIMEOUT_ERROR);
       expect(window.document.body.removeChild).toHaveBeenCalledWith(iframe);
     });
+    it('removes the message event listener in the event of a timeout', async () => {
+      const { url, origin } = setup('');
+      const seconds = 10;
+      jest.useFakeTimers();
+      const promise = runIframe(url, origin, seconds);
+      jest.runTimersToTime(seconds * 1000);
+      await expect(promise).rejects.toMatchObject(TIMEOUT_ERROR);
+      expect(window.removeEventListener).toBeCalled();
+    });
   });
   describe('getCrypto', () => {
     it('should use msCrypto when window.crypto is unavailable', () => {
@@ -854,9 +864,7 @@ describe('utils', () => {
       (<any>global).crypto = {};
 
       expect(validateCrypto).toThrowError(`
-      auth0-spa-js must run on a secure origin.
-      See https://github.com/auth0/auth0-spa-js/blob/master/FAQ.md#why-do-i-get-auth0-spa-js-must-run-on-a-secure-origin 
-      for more information.
+      auth0-spa-js must run on a secure origin. See https://github.com/auth0/auth0-spa-js/blob/master/FAQ.md#why-do-i-get-auth0-spa-js-must-run-on-a-secure-origin for more information.
     `);
     });
   });