Skip to content

Commit

Permalink
[PM-5976] Safari Browser SSO Initialization Race Condition Attempted …
Browse files Browse the repository at this point in the history
…Fix 3 (#7800)

* [PM-5976] Safari Browser SSO Initialization Race Condition Attempted Fix 3

* [PM-5976] Safari Browser SSO Initialization Race Condition Attempted Fix 3

* [PM-5976] Removing usage of pinging system and keeping reworked top-level registration of window message listener events

* [PM-5976] Pulling the implementation of the static content script delcaration for the content-message-handler file to the top of the list of content_scripts

* [PM-5976] Pulling the implementation of the static content script delcaration for the content-message-handler file to the top of the list of content_scripts

* [PM-5976] Removing the useCapture value within the window message event listener
  • Loading branch information
cagonzalezcs authored and Cesar Gonzalez committed Feb 5, 2024
1 parent c8009ba commit 57ccfd8
Show file tree
Hide file tree
Showing 11 changed files with 168 additions and 159 deletions.
Original file line number Diff line number Diff line change
@@ -1,6 +1,24 @@
interface ContentMessageHandler {
init(): void;
destroy(): void;
}
type ContentMessageWindowData = {
command: string;
lastpass?: boolean;
code?: string;
state?: string;
data?: string;
remember?: boolean;
};
type ContentMessageWindowEventParams = {
data: ContentMessageWindowData;
referrer: string;
};

export { ContentMessageHandler };
type ContentMessageWindowEventHandlers = {
[key: string]: ({ data, referrer }: ContentMessageWindowEventParams) => void;
authResult: ({ data, referrer }: ContentMessageWindowEventParams) => void;
webAuthnResult: ({ data, referrer }: ContentMessageWindowEventParams) => void;
};

export {
ContentMessageWindowData,
ContentMessageWindowEventParams,
ContentMessageWindowEventHandlers,
};

This file was deleted.

59 changes: 30 additions & 29 deletions apps/browser/src/autofill/content/content-message-handler.spec.ts
Original file line number Diff line number Diff line change
@@ -1,37 +1,31 @@
import { postWindowMessage, sendExtensionRuntimeMessage } from "../jest/testing-utils";
import { mock } from "jest-mock-extended";

import ContentMessageHandler from "./content-message-handler";
import { postWindowMessage, sendExtensionRuntimeMessage } from "../jest/testing-utils";

describe("ContentMessageHandler", () => {
let contentMessageHandler: ContentMessageHandler;
const sendMessageSpy = jest.spyOn(chrome.runtime, "sendMessage");
let portOnDisconnectAddListenerCallback: CallableFunction;
chrome.runtime.connect = jest.fn(() =>
mock<chrome.runtime.Port>({
onDisconnect: {
addListener: jest.fn((callback) => {
portOnDisconnectAddListenerCallback = callback;
}),
removeListener: jest.fn(),
},
}),
);

beforeEach(() => {
contentMessageHandler = new ContentMessageHandler();
require("./content-message-handler");
});

afterEach(() => {
jest.resetModules();
jest.clearAllMocks();
contentMessageHandler.destroy();
});

describe("init", () => {
it("should add event listeners", () => {
const addEventListenerSpy = jest.spyOn(window, "addEventListener");
const addListenerSpy = jest.spyOn(chrome.runtime.onMessage, "addListener");

contentMessageHandler.init();

expect(addEventListenerSpy).toHaveBeenCalledTimes(1);
expect(addListenerSpy).toHaveBeenCalledTimes(1);
});
});

describe("handleWindowMessage", () => {
beforeEach(() => {
contentMessageHandler.init();
});

describe("handled window messages", () => {
it("ignores messages from other sources", () => {
postWindowMessage({ command: "authResult" }, "https://localhost/", null);

Expand All @@ -47,7 +41,6 @@ describe("ContentMessageHandler", () => {
it("sends an authResult message", () => {
postWindowMessage({ command: "authResult", lastpass: true, code: "code", state: "state" });

expect(sendMessageSpy).toHaveBeenCalledTimes(1);
expect(sendMessageSpy).toHaveBeenCalledWith({
command: "authResult",
code: "code",
Expand All @@ -60,7 +53,6 @@ describe("ContentMessageHandler", () => {
it("sends a webAuthnResult message", () => {
postWindowMessage({ command: "webAuthnResult", data: "data", remember: true });

expect(sendMessageSpy).toHaveBeenCalledTimes(1);
expect(sendMessageSpy).toHaveBeenCalledWith({
command: "webAuthnResult",
data: "data",
Expand All @@ -70,11 +62,7 @@ describe("ContentMessageHandler", () => {
});
});

describe("handleExtensionMessage", () => {
beforeEach(() => {
contentMessageHandler.init();
});

describe("handled extension messages", () => {
it("ignores the message to the extension background if it is not present in the forwardCommands list", () => {
sendExtensionRuntimeMessage({ command: "someOtherCommand" });

Expand All @@ -88,4 +76,17 @@ describe("ContentMessageHandler", () => {
expect(sendMessageSpy).toHaveBeenCalledWith({ command: "bgUnlockPopoutOpened" });
});
});

describe("extension disconnect action", () => {
it("removes the window message listener and the extension message listener", () => {
const removeEventListenerSpy = jest.spyOn(window, "removeEventListener");

portOnDisconnectAddListenerCallback(mock<chrome.runtime.Port>());

expect(removeEventListenerSpy).toHaveBeenCalledTimes(1);
expect(removeEventListenerSpy).toHaveBeenCalledWith("message", expect.any(Function));
expect(chrome.runtime.onMessage.removeListener).toHaveBeenCalledTimes(1);
expect(chrome.runtime.onMessage.removeListener).toHaveBeenCalledWith(expect.any(Function));
});
});
});
166 changes: 99 additions & 67 deletions apps/browser/src/autofill/content/content-message-handler.ts
Original file line number Diff line number Diff line change
@@ -1,78 +1,110 @@
import { ContentMessageHandler as ContentMessageHandlerInterface } from "./abstractions/content-message-handler";
import {
ContentMessageWindowData,
ContentMessageWindowEventHandlers,
} from "./abstractions/content-message-handler";

class ContentMessageHandler implements ContentMessageHandlerInterface {
private forwardCommands = [
"bgUnlockPopoutOpened",
"addToLockedVaultPendingNotifications",
"unlockCompleted",
"addedCipher",
];
/**
* IMPORTANT: Safari seems to have a bug where it doesn't properly handle
* window message events from content scripts when the listener these events
* is registered within a class. This is why these listeners are registered
* at the top level of this file.
*/
window.addEventListener("message", handleWindowMessageEvent, false);
chrome.runtime.onMessage.addListener(handleExtensionMessage);
setupExtensionDisconnectAction(() => {
window.removeEventListener("message", handleWindowMessageEvent);
chrome.runtime.onMessage.removeListener(handleExtensionMessage);
});

/**
* Initialize the content message handler. Sets up
* a window message listener and a chrome runtime
* message listener.
*/
init() {
window.addEventListener("message", this.handleWindowMessage, false);
chrome.runtime.onMessage.addListener(this.handleExtensionMessage);
}

/**
* Handle a message from the window. This implementation
* specifically handles the authResult and webAuthnResult
* commands. This facilitates single sign-on.
*
* @param event - The message event.
*/
private handleWindowMessage = (event: MessageEvent) => {
const { source, data } = event;
/**
* Handlers for window messages from the content script.
*/
const windowMessageHandlers: ContentMessageWindowEventHandlers = {
authResult: ({ data, referrer }: { data: any; referrer: string }) =>
handleAuthResultMessage(data, referrer),
webAuthnResult: ({ data, referrer }: { data: any; referrer: string }) =>
handleWebAuthnResultMessage(data, referrer),
};

if (source !== window || !data?.command) {
return;
}
/**
* Handles the auth result message from the window.
*
* @param data - Data from the window message
* @param referrer - The referrer of the window
*/
async function handleAuthResultMessage(data: ContentMessageWindowData, referrer: string) {
const { command, lastpass, code, state } = data;
await chrome.runtime.sendMessage({ command, code, state, lastpass, referrer });
}

const { command } = data;
const referrer = source.location.hostname;
/**
* Handles the webauthn result message from the window.
*
* @param data - Data from the window message
* @param referrer - The referrer of the window
*/
async function handleWebAuthnResultMessage(data: ContentMessageWindowData, referrer: string) {
const { command, remember } = data;
await chrome.runtime.sendMessage({ command, data: data.data, remember, referrer });
}

if (command === "checkIfReadyForAuthResult") {
window.postMessage({ command: "readyToReceiveAuthResult" }, "*");
}
/**
* Handles the window message event.
*
* @param event - The window message event
*/
function handleWindowMessageEvent(event: MessageEvent) {
const { source, data } = event;
if (source !== window || !data?.command) {
return;
}

if (command === "authResult") {
const { lastpass, code, state } = data;
chrome.runtime.sendMessage({ command, code, state, lastpass, referrer });
}
const referrer = source.location.hostname;
const handler = windowMessageHandlers[data.command];
if (handler) {
handler({ data, referrer });
}
}

if (command === "webAuthnResult") {
const { remember } = data;
chrome.runtime.sendMessage({ command, data: data.data, remember, referrer });
}
};
/**
* Commands to forward from this script to the extension background.
*/
const forwardCommands = new Set([
"bgUnlockPopoutOpened",
"addToLockedVaultPendingNotifications",
"unlockCompleted",
"addedCipher",
]);

/**
* Handle a message from the extension. This
* implementation forwards the message to the
* extension background so that it can be received
* in other contexts of the background script.
*
* @param message - The message from the extension.
*/
private handleExtensionMessage = (message: any) => {
if (this.forwardCommands.includes(message.command)) {
chrome.runtime.sendMessage(message);
}
};
/**
* Handles messages from the extension. Currently, this is
* used to forward messages from the background context to
* other scripts within the extension.
*
* @param message - The message from the extension
*/
async function handleExtensionMessage(message: any) {
if (forwardCommands.has(message.command)) {
await chrome.runtime.sendMessage(message);
}
}

/**
* Destroy the content message handler. Removes
* the window message listener and the chrome
* runtime message listener.
*/
destroy = () => {
window.removeEventListener("message", this.handleWindowMessage);
chrome.runtime.onMessage.removeListener(this.handleExtensionMessage);
/**
* Duplicate implementation of the same named method within `apps/browser/src/autofill/utils/index.ts`.
* This is done due to some strange observed compilation behavior present when importing the method from
* the utils file.
*
* TODO: Investigate why webpack tree shaking is not removing other methods when importing from the utils file.
* Possible cause can be seen below:
* @see https://stackoverflow.com/questions/71679366/webpack5-does-not-seem-to-tree-shake-unused-exports
*
* @param callback - Callback function to run when the extension disconnects
*/
function setupExtensionDisconnectAction(callback: (port: chrome.runtime.Port) => void) {
const port = chrome.runtime.connect({ name: "autofill-injected-script-port" });
const onDisconnectCallback = (disconnectedPort: chrome.runtime.Port) => {
callback(disconnectedPort);
port.onDisconnect.removeListener(onDisconnectCallback);
};
port.onDisconnect.addListener(onDisconnectCallback);
}

export default ContentMessageHandler;
2 changes: 0 additions & 2 deletions apps/browser/src/autofill/globals.d.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
import { AutofillInit } from "./content/abstractions/autofill-init";
import ContentMessageHandler from "./content/content-message-handler";

declare global {
interface Window {
bitwardenAutofillInit?: AutofillInit;
bitwardenContentMessageHandler?: ContentMessageHandler;
}
}
4 changes: 2 additions & 2 deletions apps/browser/src/autofill/services/autofill.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -203,11 +203,11 @@ describe("AutofillService", () => {
});
});

it("injects the bootstrap-content-message-handler script if not injecting on page load", async () => {
it("injects the content-message-handler script if not injecting on page load", async () => {
await autofillService.injectAutofillScripts(sender.tab, sender.frameId, false);

expect(BrowserApi.executeScriptInTab).toHaveBeenCalledWith(tabMock.id, {
file: "content/bootstrap-content-message-handler.js",
file: "content/content-message-handler.js",
...defaultExecuteScriptOptions,
});
});
Expand Down
2 changes: 1 addition & 1 deletion apps/browser/src/autofill/services/autofill.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ export default class AutofillService implements AutofillServiceInterface {
injectedScripts.push("autofiller.js");
} else {
await BrowserApi.executeScriptInTab(tab.id, {
file: "content/bootstrap-content-message-handler.js",
file: "content/content-message-handler.js",
runAt: "document_start",
});
}
Expand Down
12 changes: 6 additions & 6 deletions apps/browser/src/manifest.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,12 @@
"128": "images/icon128.png"
},
"content_scripts": [
{
"all_frames": false,
"js": ["content/content-message-handler.js"],
"matches": ["http://*/*", "https://*/*", "file:///*"],
"run_at": "document_start"
},
{
"all_frames": true,
"js": [
Expand All @@ -24,12 +30,6 @@
"matches": ["http://*/*", "https://*/*", "file:///*"],
"run_at": "document_start"
},
{
"all_frames": false,
"js": ["content/bootstrap-content-message-handler.js"],
"matches": ["http://*/*", "https://*/*", "file:///*"],
"run_at": "document_start"
},
{
"all_frames": true,
"css": ["content/autofill.css"],
Expand Down
8 changes: 4 additions & 4 deletions apps/browser/src/manifest.v3.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,14 @@
},
"content_scripts": [
{
"all_frames": true,
"js": ["content/trigger-autofill-script-injection.js"],
"all_frames": false,
"js": ["content/content-message-handler.js"],
"matches": ["http://*/*", "https://*/*", "file:///*"],
"run_at": "document_start"
},
{
"all_frames": false,
"js": ["content/bootstrap-content-message-handler.js"],
"all_frames": true,
"js": ["content/trigger-autofill-script-injection.js"],
"matches": ["http://*/*", "https://*/*", "file:///*"],
"run_at": "document_start"
},
Expand Down
Loading

0 comments on commit 57ccfd8

Please sign in to comment.