[PM-4012] Vault Timing out on Chrome and Edge breaks passkeys until p…

…age is reloaded (#6845) * changed content script injection strategy * added persistent connection and reinjection of the content script * cleanup resources on disconnect * cleanup resources on disconnect * concluded messanger event listeners cleanup and added unit tests * Switched to use browser api add listener instead of navtive apis * renamed cleanup to destroy and added reconnect and disconnect command functions * refactored to use foreach and check for only https urls * refactored the content script to only load the page script if it currently doesn't extist of the page, and if it does sends a reconnect command to the page-script to replace the native webauthn methods * updated unit test * removed memoized logic * moved the send disconect command to the messenger * updated unit test * test messenger handler * [PM-4012] fix: add `senderId` to messenger * destroy pending requets * cleaned up page script and terminated pending request * fixed cannot read properties of undefined * rearranged functions, renamed misspelled words, and created test * mocked EventTarget as there are issues on jest for listeners getting the events * Return fall back error instead * Update apps/browser/src/vault/fido2/content/content-script.ts Co-authored-by: Cesar Gonzalez <[email protected]> * Update apps/browser/src/vault/fido2/content/messaging/messenger.ts Co-authored-by: Cesar Gonzalez <[email protected]> * removed whitespace --------- Co-authored-by: Andreas Coroiu <[email protected]> Co-authored-by: Cesar Gonzalez <[email protected]>
bitwarden · Dec 12, 2023 · f0cdccc · f0cdccc
1 parent 3e174fe
commit f0cdccc
Show file tree

Hide file tree

Showing 14 changed files with 378 additions and 68 deletions.
diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts
@@ -156,7 +156,9 @@ import { BrowserSendService } from "../services/browser-send.service";
 import { BrowserSettingsService } from "../services/browser-settings.service";
 import VaultTimeoutService from "../services/vault-timeout/vault-timeout.service";
 import { BrowserFido2UserInterfaceService } from "../vault/fido2/browser-fido2-user-interface.service";
+import { Fido2Service as Fido2ServiceAbstraction } from "../vault/services/abstractions/fido2.service";
 import { BrowserFolderService } from "../vault/services/browser-folder.service";
+import Fido2Service from "../vault/services/fido2.service";
 import { VaultFilterService } from "../vault/services/vault-filter.service";
 
 import CommandsBackground from "./commands.background";
@@ -232,6 +234,7 @@ export default class MainBackground {
   authRequestCryptoService: AuthRequestCryptoServiceAbstraction;
   accountService: AccountServiceAbstraction;
   globalStateProvider: GlobalStateProvider;
+  fido2Service: Fido2ServiceAbstraction;
 
   // Passed to the popup for Safari to workaround issues with theming, downloading, etc.
   backgroundWindow = window;
@@ -597,6 +600,7 @@ export default class MainBackground {
       this.messagingService,
     );
 
+    this.fido2Service = new Fido2Service();
     this.fido2UserInterfaceService = new BrowserFido2UserInterfaceService(this.authService);
     this.fido2AuthenticatorService = new Fido2AuthenticatorService(
       this.cipherService,
@@ -645,6 +649,7 @@ export default class MainBackground {
       this.messagingService,
       this.logService,
       this.configService,
+      this.fido2Service,
     );
     this.nativeMessagingBackground = new NativeMessagingBackground(
       this.cryptoService,
@@ -778,6 +783,8 @@ export default class MainBackground {
     await this.idleBackground.init();
     await this.webRequestBackground.init();
 
+    await this.fido2Service.init();
+
     if (this.platformUtilsService.isFirefox() && !this.isPrivateMode) {
       // Set Private Mode windows to the default icon - they do not share state with the background page
       const privateWindows = await BrowserApi.getPrivateModeWindows();

diff --git a/apps/browser/src/background/runtime.background.ts b/apps/browser/src/background/runtime.background.ts
@@ -20,6 +20,7 @@ import { BrowserStateService } from "../platform/services/abstractions/browser-s
 import { BrowserEnvironmentService } from "../platform/services/browser-environment.service";
 import BrowserPlatformUtilsService from "../platform/services/browser-platform-utils.service";
 import { AbortManager } from "../vault/background/abort-manager";
+import { Fido2Service } from "../vault/services/abstractions/fido2.service";
 
 import MainBackground from "./main.background";
 
@@ -42,6 +43,7 @@ export default class RuntimeBackground {
     private messagingService: MessagingService,
     private logService: LogService,
     private configService: ConfigServiceAbstraction,
+    private fido2Service: Fido2Service,
   ) {
     // onInstalled listener must be wired up before anything else, so we do it in the ctor
     chrome.runtime.onInstalled.addListener((details: any) => {
@@ -257,6 +259,9 @@ export default class RuntimeBackground {
       case "getClickedElementResponse":
         this.platformUtilsService.copyToClipboard(msg.identifier, { window: window });
         break;
+      case "triggerFido2ContentScriptInjection":
+        await this.fido2Service.injectFido2ContentScripts(sender);
+        break;
       case "fido2AbortRequest":
         this.abortManager.abort(msg.abortedRequestId);
         break;

diff --git a/apps/browser/src/manifest.json b/apps/browser/src/manifest.json
@@ -17,7 +17,10 @@
   "content_scripts": [
     {
       "all_frames": true,
-      "js": ["content/trigger-autofill-script-injection.js", "content/fido2/content-script.js"],
+      "js": [
+        "content/trigger-autofill-script-injection.js",
+        "content/fido2/trigger-fido2-content-script-injection.js"
+      ],
       "matches": ["http://*/*", "https://*/*", "file:///*"],
       "run_at": "document_start"
     },

diff --git a/apps/browser/src/vault/fido2/content/content-script.ts b/apps/browser/src/vault/fido2/content/content-script.ts
@@ -61,12 +61,28 @@ async function isLocationBitwardenVault(activeUserSettings: Record<string, any>)
   return window.location.origin === activeUserSettings.serverConfig.environment.vault;
 }
 
-function initializeFido2ContentScript() {
-  const s = document.createElement("script");
-  s.src = chrome.runtime.getURL("content/fido2/page-script.js");
-  (document.head || document.documentElement).appendChild(s);
+const messenger = Messenger.forDOMCommunication(window);
+
+function injectPageScript() {
+  // Locate an existing page-script on the page
+  const existingPageScript = document.getElementById("bw-fido2-page-script");
+
+  // Inject the page-script if it doesn't exist
+  if (!existingPageScript) {
+    const s = document.createElement("script");
+    s.src = chrome.runtime.getURL("content/fido2/page-script.js");
+    s.id = "bw-fido2-page-script";
+    (document.head || document.documentElement).appendChild(s);
 
-  const messenger = Messenger.forDOMCommunication(window);
+    return;
+  }
+
+  // If the page-script already exists, send a reconnect message to the page-script
+  messenger.sendReconnectCommand();
+}
+
+function initializeFido2ContentScript() {
+  injectPageScript();
 
   messenger.handler = async (message, abortController) => {
     const requestId = Date.now().toString();
@@ -78,7 +94,7 @@ function initializeFido2ContentScript() {
     abortController.signal.addEventListener("abort", abortHandler);
 
     if (message.type === MessageType.CredentialCreationRequest) {
-      return new Promise((resolve, reject) => {
+      return new Promise<Message | undefined>((resolve, reject) => {
         const data: CreateCredentialParams = {
           ...message.data,
           origin: window.location.origin,
@@ -92,7 +108,7 @@ function initializeFido2ContentScript() {
             requestId: requestId,
           },
           (response) => {
-            if (response.error !== undefined) {
+            if (response && response.error !== undefined) {
               return reject(response.error);
             }
 
@@ -106,7 +122,7 @@ function initializeFido2ContentScript() {
     }
 
     if (message.type === MessageType.CredentialGetRequest) {
-      return new Promise((resolve, reject) => {
+      return new Promise<Message | undefined>((resolve, reject) => {
         const data: AssertCredentialParams = {
           ...message.data,
           origin: window.location.origin,
@@ -120,7 +136,7 @@ function initializeFido2ContentScript() {
             requestId: requestId,
           },
           (response) => {
-            if (response.error !== undefined) {
+            if (response && response.error !== undefined) {
               return reject(response.error);
             }
 
@@ -155,6 +171,12 @@ async function run() {
   }
 
   initializeFido2ContentScript();
+
+  const port = chrome.runtime.connect({ name: "fido2ContentScriptReady" });
+  port.onDisconnect.addListener(() => {
+    // Cleanup the messenger and remove the event listener
+    messenger.destroy();
+  });
 }
 
 run();
diff --git a/apps/browser/src/vault/fido2/content/messaging/message.ts b/apps/browser/src/vault/fido2/content/messaging/message.ts
@@ -11,6 +11,8 @@ export enum MessageType {
   CredentialGetRequest,
   CredentialGetResponse,
   AbortRequest,
+  DisconnectRequest,
+  ReconnectRequest,
   AbortResponse,
   ErrorResponse,
 }
@@ -60,6 +62,14 @@ export type AbortRequest = {
   abortedRequestId: string;
 };
 
+export type DisconnectRequest = {
+  type: MessageType.DisconnectRequest;
+};
+
+export type ReconnectRequest = {
+  type: MessageType.ReconnectRequest;
+};
+
 export type ErrorResponse = {
   type: MessageType.ErrorResponse;
   error: string;
@@ -76,5 +86,7 @@ export type Message =
   | CredentialGetRequest
   | CredentialGetResponse
   | AbortRequest
+  | DisconnectRequest
+  | ReconnectRequest
   | AbortResponse
   | ErrorResponse;
diff --git a/apps/browser/src/vault/fido2/content/messaging/messenger.spec.ts b/apps/browser/src/vault/fido2/content/messaging/messenger.spec.ts
@@ -12,6 +12,12 @@ describe("Messenger", () => {
   beforeEach(() => {
     // jest does not support MessageChannel
     window.MessageChannel = MockMessageChannel as any;
+    Object.defineProperty(window, "location", {
+      value: {
+        origin: "https://bitwarden.com",
+      },
+      writable: true,
+    });
 
     const channelPair = new TestChannelPair();
     messengerA = new Messenger(channelPair.channelA);
@@ -27,7 +33,7 @@ describe("Messenger", () => {
     const request = createRequest();
     messengerA.request(request);
 
-    const received = handlerB.recieve();
+    const received = handlerB.receive();
 
     expect(received.length).toBe(1);
     expect(received[0].message).toMatchObject(request);
@@ -37,7 +43,7 @@ describe("Messenger", () => {
     const request = createRequest();
     const response = createResponse();
     const requestPromise = messengerA.request(request);
-    const received = handlerB.recieve();
+    const received = handlerB.receive();
     received[0].respond(response);
 
     const returned = await requestPromise;
@@ -49,7 +55,7 @@ describe("Messenger", () => {
     const request = createRequest();
     const error = new Error("Test error");
     const requestPromise = messengerA.request(request);
-    const received = handlerB.recieve();
+    const received = handlerB.receive();
 
     received[0].reject(error);
 
@@ -61,10 +67,60 @@ describe("Messenger", () => {
     messengerA.request(createRequest(), abortController);
     abortController.abort();
 
-    const received = handlerB.recieve();
+    const received = handlerB.receive();
 
     expect(received[0].abortController.signal.aborted).toBe(true);
   });
+
+  describe("destroy", () => {
+    beforeEach(() => {
+      /**
+       * In Jest's jsdom environment, there is an issue where event listeners are not
+       * triggered upon dispatching an event. This is a workaround to mock the EventTarget
+       */
+      window.EventTarget = MockEventTarget as any;
+    });
+
+    it("should remove the message event listener", async () => {
+      const channelPair = new TestChannelPair();
+      const addEventListenerSpy = jest.spyOn(channelPair.channelA, "addEventListener");
+      const removeEventListenerSpy = jest.spyOn(channelPair.channelA, "removeEventListener");
+      messengerA = new Messenger(channelPair.channelA);
+      jest
+        .spyOn(messengerA as any, "sendDisconnectCommand")
+        .mockImplementation(() => Promise.resolve());
+
+      expect(addEventListenerSpy).toHaveBeenCalled();
+
+      await messengerA.destroy();
+
+      expect(removeEventListenerSpy).toHaveBeenCalled();
+    });
+
+    it("should dispatch the destroy event on messenger destruction", async () => {
+      const request = createRequest();
+      messengerA.request(request);
+
+      const dispatchEventSpy = jest.spyOn((messengerA as any).onDestroy, "dispatchEvent");
+      messengerA.destroy();
+
+      expect(dispatchEventSpy).toHaveBeenCalledWith(expect.any(Event));
+    });
+
+    it("should trigger onDestroyListener when the destroy event is dispatched", async () => {
+      const request = createRequest();
+      messengerA.request(request);
+
+      const onDestroyListener = jest.fn();
+      (messengerA as any).onDestroy.addEventListener("destroy", onDestroyListener);
+      messengerA.destroy();
+
+      expect(onDestroyListener).toHaveBeenCalled();
+      const eventArg = onDestroyListener.mock.calls[0][0];
+      expect(eventArg).toBeInstanceOf(Event);
+      expect(eventArg.type).toBe("destroy");
+    });
+  });
 });
 
 type TestMessage = MessageWithMetadata & { testId: string };
@@ -86,11 +142,13 @@ class TestChannelPair {
 
     this.channelA = {
       addEventListener: (listener) => (broadcastChannel.port1.onmessage = listener),
+      removeEventListener: () => (broadcastChannel.port1.onmessage = null),
       postMessage: (message, port) => broadcastChannel.port1.postMessage(message, port),
     };
 
     this.channelB = {
       addEventListener: (listener) => (broadcastChannel.port2.onmessage = listener),
+      removeEventListener: () => (broadcastChannel.port1.onmessage = null),
       postMessage: (message, port) => broadcastChannel.port2.postMessage(message, port),
     };
   }
@@ -102,7 +160,7 @@ class TestMessageHandler {
     abortController?: AbortController,
   ) => Promise<Message | undefined>;
 
-  private recievedMessages: {
+  private receivedMessages: {
     message: TestMessage;
     respond: (response: TestMessage) => void;
     reject: (error: Error) => void;
@@ -112,7 +170,7 @@ class TestMessageHandler {
   constructor() {
     this.handler = (message, abortController) =>
       new Promise((resolve, reject) => {
-        this.recievedMessages.push({
+        this.receivedMessages.push({
           message,
           abortController,
           respond: (response) => resolve(response),
@@ -121,9 +179,9 @@ class TestMessageHandler {
       });
   }
 
-  recieve() {
-    const received = this.recievedMessages;
-    this.recievedMessages = [];
+  receive() {
+    const received = this.receivedMessages;
+    this.receivedMessages = [];
     return received;
   }
 }
@@ -144,11 +202,32 @@ class MockMessagePort<T> {
 
   postMessage(message: T, port?: MessagePort) {
     this.remotePort.onmessage(
-      new MessageEvent("message", { data: message, ports: port ? [port] : [] }),
+      new MessageEvent("message", {
+        data: message,
+        ports: port ? [port] : [],
+        origin: "https://bitwarden.com",
+      }),
     );
   }
 
   close() {
     // Do nothing
   }
 }
+
+class MockEventTarget {
+  listeners: Record<string, EventListener[]> = {};
+
+  addEventListener(type: string, callback: EventListener) {
+    this.listeners[type] = this.listeners[type] || [];
+    this.listeners[type].push(callback);
+  }
+
+  dispatchEvent(event: Event) {
+    (this.listeners[event.type] || []).forEach((callback) => callback(event));
+  }
+
+  removeEventListener(type: string, callback: EventListener) {
+    this.listeners[type] = (this.listeners[type] || []).filter((listener) => listener !== callback);
+  }
+}