From e66a1f37b5a31513bef5d26b7c145b01eaa40a07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=9C=A8=20Audrey=20=E2=9C=A8?= Date: Thu, 19 Feb 2026 08:45:24 -0500 Subject: [PATCH] Extract urlOriginsMatch utility and refactor senderIsInternal (#19076) Adds urlOriginsMatch to @bitwarden/platform, which compares two URLs by scheme, host, and port. Uses `protocol + "//" + host` rather than `URL.origin` because non-special schemes (e.g. chrome-extension://) return the opaque string "null" from .origin, making equality comparison unreliable. URLs without a host (file:, data:) are explicitly rejected to prevent hostless schemes from comparing equal. Refactors senderIsInternal to delegate to urlOriginsMatch and to derive the extension URL via BrowserApi.getRuntimeURL("") rather than inline chrome/browser API detection. Adds full test coverage for senderIsInternal. The previous string-based comparison used startsWith after stripping trailing slashes, which was safe in senderIsInternal where inputs are tightly constrained. As a general utility accepting arbitrary URLs, startsWith can produce false positives (e.g. "https://example.com" matching "https://example.com.evil.com"). Structural host comparison is the correct contract for unrestricted input. --- .../src/platform/browser/browser-api.spec.ts | 100 ++++++++++++++++++ .../src/platform/browser/browser-api.ts | 36 ++++--- libs/platform/src/index.ts | 1 + libs/platform/src/util.spec.ts | 54 ++++++++++ libs/platform/src/util.ts | 53 ++++++++++ 5 files changed, 227 insertions(+), 17 deletions(-) create mode 100644 libs/platform/src/util.spec.ts create mode 100644 libs/platform/src/util.ts diff --git a/apps/browser/src/platform/browser/browser-api.spec.ts b/apps/browser/src/platform/browser/browser-api.spec.ts index f7561b2b50b..d8a8fe52570 100644 --- a/apps/browser/src/platform/browser/browser-api.spec.ts +++ b/apps/browser/src/platform/browser/browser-api.spec.ts @@ -1,5 +1,7 @@ import { mock } from "jest-mock-extended"; +import { LogService } from "@bitwarden/logging"; + import { BrowserApi } from "./browser-api"; type ChromeSettingsGet = chrome.types.ChromeSetting["get"]; @@ -29,6 +31,104 @@ describe("BrowserApi", () => { }); }); + describe("senderIsInternal", () => { + const EXTENSION_ORIGIN = "chrome-extension://id"; + + beforeEach(() => { + jest.spyOn(BrowserApi, "getRuntimeURL").mockReturnValue(`${EXTENSION_ORIGIN}/`); + }); + + it("returns false when sender is undefined", () => { + const result = BrowserApi.senderIsInternal(undefined); + + expect(result).toBe(false); + }); + + it("returns false when sender has no origin", () => { + const result = BrowserApi.senderIsInternal({ id: "abc" } as any); + + expect(result).toBe(false); + }); + + it("returns false when the extension URL cannot be determined", () => { + jest.spyOn(BrowserApi, "getRuntimeURL").mockReturnValue(""); + + const result = BrowserApi.senderIsInternal({ origin: EXTENSION_ORIGIN }); + + expect(result).toBe(false); + }); + + it.each([ + ["an external origin", "https://evil.com"], + ["a subdomain of the extension origin", "chrome-extension://id.evil.com"], + ["a file: URL (opaque origin)", "file:///home/user/page.html"], + ["a data: URL (opaque origin)", "data:text/html,

hi

"], + ])("returns false when sender origin is %s", (_, senderOrigin) => { + const result = BrowserApi.senderIsInternal({ origin: senderOrigin }); + + expect(result).toBe(false); + }); + + it("returns false when sender is from a non-top-level frame", () => { + const result = BrowserApi.senderIsInternal({ origin: EXTENSION_ORIGIN, frameId: 5 }); + + expect(result).toBe(false); + }); + + it("returns true when sender origin matches and no frameId is present (popup)", () => { + const result = BrowserApi.senderIsInternal({ origin: EXTENSION_ORIGIN }); + + expect(result).toBe(true); + }); + + it("returns true when sender origin matches and frameId is 0 (top-level frame)", () => { + const result = BrowserApi.senderIsInternal({ origin: EXTENSION_ORIGIN, frameId: 0 }); + + expect(result).toBe(true); + }); + + it("calls logger.warning when sender has no origin", () => { + const logger = mock(); + + BrowserApi.senderIsInternal({} as any, logger); + + expect(logger.warning).toHaveBeenCalledWith(expect.stringContaining("no origin")); + }); + + it("calls logger.warning when the extension URL cannot be determined", () => { + jest.spyOn(BrowserApi, "getRuntimeURL").mockReturnValue(""); + const logger = mock(); + + BrowserApi.senderIsInternal({ origin: EXTENSION_ORIGIN }, logger); + + expect(logger.warning).toHaveBeenCalledWith(expect.stringContaining("extension URL")); + }); + + it("calls logger.warning when origin does not match", () => { + const logger = mock(); + + BrowserApi.senderIsInternal({ origin: "https://evil.com" }, logger); + + expect(logger.warning).toHaveBeenCalledWith(expect.stringContaining("does not match")); + }); + + it("calls logger.warning when sender is from a non-top-level frame", () => { + const logger = mock(); + + BrowserApi.senderIsInternal({ origin: EXTENSION_ORIGIN, frameId: 5 }, logger); + + expect(logger.warning).toHaveBeenCalledWith(expect.stringContaining("top-level frame")); + }); + + it("calls logger.info when sender is confirmed internal", () => { + const logger = mock(); + + BrowserApi.senderIsInternal({ origin: EXTENSION_ORIGIN }, logger); + + expect(logger.info).toHaveBeenCalledWith(expect.stringContaining("internal")); + }); + }); + describe("getWindow", () => { it("will get the current window if a window id is not provided", () => { // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling. diff --git a/apps/browser/src/platform/browser/browser-api.ts b/apps/browser/src/platform/browser/browser-api.ts index feefd527636..1b0f7639d1d 100644 --- a/apps/browser/src/platform/browser/browser-api.ts +++ b/apps/browser/src/platform/browser/browser-api.ts @@ -6,7 +6,7 @@ import { BrowserClientVendors } from "@bitwarden/common/autofill/constants"; import { BrowserClientVendor } from "@bitwarden/common/autofill/types"; import { DeviceType } from "@bitwarden/common/enums"; import { LogService } from "@bitwarden/logging"; -import { isBrowserSafariApi } from "@bitwarden/platform"; +import { isBrowserSafariApi, urlOriginsMatch } from "@bitwarden/platform"; import { TabMessage } from "../../types/tab-messages"; import { BrowserPlatformUtilsService } from "../services/platform-utils/browser-platform-utils.service"; @@ -34,12 +34,20 @@ export class BrowserApi { } /** - * Helper method that attempts to distinguish whether a message sender is internal to the extension or not. + * Returns `true` if the message sender appears to originate from within this extension. * - * Currently this is done through source origin matching, and frameId checking (only top-level frames are internal). - * @param sender a message sender - * @param logger an optional logger to log validation results - * @returns whether or not the sender appears to be internal to the extension + * Returns `false` when: + * - `sender` is absent or has no `origin` property + * - The extension's own URL cannot be determined at runtime + * - The sender's origin does not match the extension's origin (compared by scheme, host, and port; + * senders without a host such as `file:` or `data:` URLs are always rejected) + * - The message comes from a sub-frame rather than the top-level frame + * + * Note: this is a best-effort check that relies on the browser correctly populating `sender.origin`. + * + * @param sender - The message sender to validate. `undefined` or a sender without `origin` returns `false`. + * @param logger - Optional logger; rejections are reported at `warning` level, acceptance at `info`. + * @returns `true` if the sender appears to be internal to the extension; `false` otherwise. */ static senderIsInternal( sender: chrome.runtime.MessageSender | undefined, @@ -49,28 +57,22 @@ export class BrowserApi { logger?.warning("[BrowserApi] Message sender has no origin"); return false; } - const extensionUrl = - (typeof chrome !== "undefined" && chrome.runtime?.getURL("")) || - (typeof browser !== "undefined" && browser.runtime?.getURL("")) || - ""; + // Empty path yields the extension's base URL; coalesce to empty string so the guard below fires on a missing runtime. + const extensionUrl = BrowserApi.getRuntimeURL("") ?? ""; if (!extensionUrl) { logger?.warning("[BrowserApi] Unable to determine extension URL"); return false; } - // Normalize both URLs by removing trailing slashes - const normalizedOrigin = sender.origin.replace(/\/$/, "").toLowerCase(); - const normalizedExtensionUrl = extensionUrl.replace(/\/$/, "").toLowerCase(); - - if (!normalizedOrigin.startsWith(normalizedExtensionUrl)) { + if (!urlOriginsMatch(extensionUrl, sender.origin)) { logger?.warning( - `[BrowserApi] Message sender origin (${normalizedOrigin}) does not match extension URL (${normalizedExtensionUrl})`, + `[BrowserApi] Message sender origin (${sender.origin}) does not match extension URL (${extensionUrl})`, ); return false; } - // We only send messages from the top-level frame, but frameId is only set if tab is set, which for popups it is not. + // frameId is absent for popups, so use an 'in' check rather than direct comparison. if ("frameId" in sender && sender.frameId !== 0) { logger?.warning("[BrowserApi] Message sender is not from the top-level frame"); return false; diff --git a/libs/platform/src/index.ts b/libs/platform/src/index.ts index 3fabe3fad1a..9c9dac0c684 100644 --- a/libs/platform/src/index.ts +++ b/libs/platform/src/index.ts @@ -1,2 +1,3 @@ export * from "./services/browser-service"; export * from "./background-sync"; +export * from "./util"; diff --git a/libs/platform/src/util.spec.ts b/libs/platform/src/util.spec.ts new file mode 100644 index 00000000000..fda563db7ea --- /dev/null +++ b/libs/platform/src/util.spec.ts @@ -0,0 +1,54 @@ +import { urlOriginsMatch } from "./util"; + +describe("urlOriginsMatch", () => { + it.each([ + ["string/string, same origin", "https://example.com", "https://example.com"], + ["URL/URL, same origin", new URL("https://example.com"), new URL("https://example.com")], + ["string canonical, URL suspect", "https://example.com", new URL("https://example.com/path")], + ["URL canonical, string suspect", new URL("https://example.com/path"), "https://example.com"], + [ + "paths and query differ but origin same", + "https://example.com/foo", + "https://example.com/bar?baz=1", + ], + ["explicit default port matches implicit", "https://example.com", "https://example.com:443"], + [ + "non-special scheme with matching host", + "chrome-extension://abc123/popup.html", + "chrome-extension://abc123/bg.js", + ], + ])("returns true when %s", (_, canonical, suspect) => { + expect(urlOriginsMatch(canonical as string | URL, suspect as string | URL)).toBe(true); + }); + + it.each([ + ["hosts differ", "https://example.com", "https://evil.com"], + ["schemes differ", "https://example.com", "http://example.com"], + ["ports differ", "https://example.com:8080", "https://example.com:9090"], + [ + "suspect is a subdomain of the canonical host", + "https://example.com", + "https://sub.example.com", + ], + ["non-special scheme hosts differ", "chrome-extension://abc123/", "chrome-extension://xyz789/"], + ])("returns false when %s", (_, canonical, suspect) => { + expect(urlOriginsMatch(canonical, suspect)).toBe(false); + }); + + it.each([ + ["canonical is an invalid string", "not a url", "https://example.com"], + ["suspect is an invalid string", "https://example.com", "not a url"], + ])("returns false when %s", (_, canonical, suspect) => { + expect(urlOriginsMatch(canonical, suspect)).toBe(false); + }); + + it.each([ + ["canonical is a file: URL", "file:///home/user/a.txt", "https://example.com"], + ["suspect is a file: URL", "https://example.com", "file:///home/user/a.txt"], + ["both are file: URLs", "file:///home/user/a.txt", "file:///home/other/b.txt"], + ["canonical is a data: URL", "data:text/plain,foo", "https://example.com"], + ["suspect is a data: URL", "https://example.com", "data:text/plain,foo"], + ])("returns false when %s (no host)", (_, canonical, suspect) => { + expect(urlOriginsMatch(canonical, suspect)).toBe(false); + }); +}); diff --git a/libs/platform/src/util.ts b/libs/platform/src/util.ts new file mode 100644 index 00000000000..b59e713fba3 --- /dev/null +++ b/libs/platform/src/util.ts @@ -0,0 +1,53 @@ +function toURL(input: string | URL): URL | null { + if (input instanceof URL) { + return input; + } + try { + return new URL(input); + } catch { + return null; + } +} + +function effectiveOrigin(url: URL): string | null { + // The URL spec returns "null" for .origin on non-special schemes + // (e.g. chrome-extension://) so we build the origin from protocol + host instead. + // An empty host means no meaningful origin can be compared (file:, data:, etc.). + if (!url.host) { + return null; + } + return `${url.protocol}//${url.host}`; +} + +/** + * Compares two URLs to determine whether the suspect URL originates from the + * same host as the canonical URL. + * + * Both arguments accept either a string or an existing {@link URL} object. + * + * Returns `false` when: + * - Either argument cannot be parsed as a valid URL + * - Either URL has no host (e.g. `file:`, `data:` schemes), since URLs without + * a meaningful host cannot be distinguished by origin + * + * @param canonical - The reference URL whose origin acts as the baseline. + * @param suspect - The URL being tested against the canonical origin. + * @returns `true` if both URLs share the same scheme, host, and port; `false` otherwise. + */ +export function urlOriginsMatch(canonical: string | URL, suspect: string | URL): boolean { + const canonicalUrl = toURL(canonical); + const suspectUrl = toURL(suspect); + + if (!canonicalUrl || !suspectUrl) { + return false; + } + + const canonicalOrigin = effectiveOrigin(canonicalUrl); + const suspectOrigin = effectiveOrigin(suspectUrl); + + if (!canonicalOrigin || !suspectOrigin) { + return false; + } + + return canonicalOrigin === suspectOrigin; +}