From 2440c6d2443e67dc6a2904f2da207614632eb29d Mon Sep 17 00:00:00 2001 From: Anders Kaseorg Date: Mon, 30 Sep 2024 15:04:14 -0700 Subject: [PATCH] electron_bridge: Harden against hypothetical DOM clobbering attacks. Signed-off-by: Anders Kaseorg --- tools/test-js-with-node | 2 +- web/src/activity.ts | 5 +++-- web/src/desktop_integration.js | 17 ++++++++--------- web/src/desktop_notifications.ts | 7 ++++--- ...{electron_bridge.d.ts => electron_bridge.ts} | 6 +++++- web/src/narrow_title.ts | 7 +++---- web/src/portico/desktop-login.ts | 6 ++++-- web/src/server_events_dispatch.js | 15 ++++----------- web/tests/activity.test.js | 5 +++-- web/tests/dispatch.test.js | 5 +++-- web/tests/notifications.test.js | 1 + 11 files changed, 39 insertions(+), 37 deletions(-) rename web/src/{electron_bridge.d.ts => electron_bridge.ts} (83%) diff --git a/tools/test-js-with-node b/tools/test-js-with-node index 2ad2d2f5ca..5e5225068f 100755 --- a/tools/test-js-with-node +++ b/tools/test-js-with-node @@ -102,7 +102,7 @@ EXEMPT_FILES = make_set( "web/src/drafts_overlay_ui.js", "web/src/dropdown_widget.ts", "web/src/echo.ts", - "web/src/electron_bridge.d.ts", + "web/src/electron_bridge.ts", "web/src/email_pill.ts", "web/src/emoji_picker.ts", "web/src/emojisets.ts", diff --git a/web/src/activity.ts b/web/src/activity.ts index 9a0b28b527..e1dab5ad27 100644 --- a/web/src/activity.ts +++ b/web/src/activity.ts @@ -3,6 +3,7 @@ import assert from "minimalistic-assert"; import {z} from "zod"; import * as channel from "./channel"; +import {electron_bridge} from "./electron_bridge"; import {page_params} from "./page_params"; import * as presence from "./presence"; import * as watchdog from "./watchdog"; @@ -84,8 +85,8 @@ export function compute_active_status(): ActivityState { // // The check for `get_idle_on_system === undefined` is feature // detection; older desktop app releases never set that property. - if (window.electron_bridge?.get_idle_on_system !== undefined) { - if (window.electron_bridge.get_idle_on_system()) { + if (electron_bridge?.get_idle_on_system !== undefined) { + if (electron_bridge.get_idle_on_system()) { return ActivityState.IDLE; } return ActivityState.ACTIVE; diff --git a/web/src/desktop_integration.js b/web/src/desktop_integration.js index bb7aef8b99..b90ef173d0 100644 --- a/web/src/desktop_integration.js +++ b/web/src/desktop_integration.js @@ -2,6 +2,7 @@ import $ from "jquery"; import * as browser_history from "./browser_history"; import * as channel from "./channel"; +import {electron_bridge} from "./electron_bridge"; import * as feedback_widget from "./feedback_widget"; import {$t} from "./i18n"; import * as message_store from "./message_store"; @@ -9,19 +10,19 @@ import * as message_view from "./message_view"; import * as stream_data from "./stream_data"; export function initialize() { - if (window.electron_bridge === undefined) { + if (electron_bridge === undefined) { return; } - window.electron_bridge.on_event("logout", () => { + electron_bridge.on_event("logout", () => { $("#logout_form").trigger("submit"); }); - window.electron_bridge.on_event("show-keyboard-shortcuts", () => { + electron_bridge.on_event("show-keyboard-shortcuts", () => { browser_history.go_to_location("keyboard-shortcuts"); }); - window.electron_bridge.on_event("show-notification-settings", () => { + electron_bridge.on_event("show-notification-settings", () => { browser_history.go_to_location("settings/notifications"); }); @@ -29,10 +30,8 @@ export function initialize() { // is often referred to as inline reply feature. This is done so desktop app doesn't // have to depend on channel.post for setting crsf_token and message_view.narrow_by_topic // to narrow to the message being sent. - if (window.electron_bridge.set_send_notification_reply_message_supported !== undefined) { - window.electron_bridge.set_send_notification_reply_message_supported(true); - } - window.electron_bridge.on_event("send_notification_reply_message", (message_id, reply) => { + electron_bridge.set_send_notification_reply_message_supported?.(true); + electron_bridge.on_event("send_notification_reply_message", (message_id, reply) => { const message = message_store.get(message_id); const data = { type: message.type, @@ -56,7 +55,7 @@ export function initialize() { } function error(error) { - window.electron_bridge.send_event("send_notification_reply_message_failed", { + electron_bridge.send_event("send_notification_reply_message_failed", { data, message_id, error, diff --git a/web/src/desktop_notifications.ts b/web/src/desktop_notifications.ts index 7f2ca66be8..368385aa0b 100644 --- a/web/src/desktop_notifications.ts +++ b/web/src/desktop_notifications.ts @@ -1,6 +1,7 @@ import $ from "jquery"; import assert from "minimalistic-assert"; +import {electron_bridge} from "./electron_bridge"; import type {Message} from "./message_store"; type NoticeMemory = Map< @@ -33,8 +34,8 @@ export class ElectronBridgeNotification extends EventTarget { constructor(title: string, options: NotificationOptions) { super(); - assert(window.electron_bridge?.new_notification !== undefined); - const notification_data = window.electron_bridge.new_notification( + assert(electron_bridge?.new_notification !== undefined); + const notification_data = electron_bridge.new_notification( title, options, (type, eventInit) => this.dispatchEvent(new Event(type, eventInit)), @@ -63,7 +64,7 @@ export class ElectronBridgeNotification extends EventTarget { } } -if (window.electron_bridge?.new_notification) { +if (electron_bridge?.new_notification) { NotificationAPI = ElectronBridgeNotification; } else if (window.Notification) { NotificationAPI = window.Notification; diff --git a/web/src/electron_bridge.d.ts b/web/src/electron_bridge.ts similarity index 83% rename from web/src/electron_bridge.d.ts rename to web/src/electron_bridge.ts index bf68caeb7a..6c4c056fbc 100644 --- a/web/src/electron_bridge.d.ts +++ b/web/src/electron_bridge.ts @@ -40,6 +40,10 @@ export type ElectronBridge = { declare global { // eslint-disable-next-line @typescript-eslint/consistent-type-definitions interface Window { - electron_bridge?: ElectronBridge; + electron_bridge?: ElectronBridge | Element; } } + +// Check for Element for extra defense against DOM clobbering attacks +export const electron_bridge: ElectronBridge | undefined = + window.electron_bridge instanceof Element ? undefined : window.electron_bridge; diff --git a/web/src/narrow_title.ts b/web/src/narrow_title.ts index 35d7ef7e84..5c3b9d39c2 100644 --- a/web/src/narrow_title.ts +++ b/web/src/narrow_title.ts @@ -1,6 +1,7 @@ import _ from "lodash"; import assert from "minimalistic-assert"; +import {electron_bridge} from "./electron_bridge"; import * as favicon from "./favicon"; import type {Filter} from "./filter"; import {$t} from "./i18n"; @@ -115,11 +116,9 @@ export function update_unread_counts(counts: FullUnreadCountsData): void { favicon.update_favicon(unread_count, pm_count); // Notify the current desktop app's UI about the new unread count. - if (window.electron_bridge !== undefined) { - window.electron_bridge.send_event("total_unread_count", unread_count); - } + electron_bridge?.send_event("total_unread_count", unread_count); - // TODO: Add a `window.electron_bridge.updateDirectMessageCount(new_pm_count);` call? + // TODO: Add a `electron_bridge.updateDirectMessageCount(new_pm_count);` call? redraw_title(); } diff --git a/web/src/portico/desktop-login.ts b/web/src/portico/desktop-login.ts index 386b2c0095..a3d818e004 100644 --- a/web/src/portico/desktop-login.ts +++ b/web/src/portico/desktop-login.ts @@ -1,3 +1,5 @@ +import {electron_bridge} from "../electron_bridge"; + document.querySelector("form#form")!.addEventListener("submit", () => { document.querySelector("p#bad-token")!.hidden = false; }); @@ -42,8 +44,8 @@ void (async () => { // key and a promise; as soon as something encrypted to that key is copied // to the clipboard, the app decrypts it and resolves the promise to the // plaintext. This lets us skip the manual paste step. - const {key, pasted} = window.electron_bridge?.decrypt_clipboard - ? window.electron_bridge.decrypt_clipboard(1) + const {key, pasted} = electron_bridge?.decrypt_clipboard + ? electron_bridge.decrypt_clipboard(1) : await decrypt_manual(); const keyHex = [...key].map((b) => b.toString(16).padStart(2, "0")).join(""); diff --git a/web/src/server_events_dispatch.js b/web/src/server_events_dispatch.js index a1a0f56547..13acdb2d09 100644 --- a/web/src/server_events_dispatch.js +++ b/web/src/server_events_dispatch.js @@ -16,6 +16,7 @@ import * as compose_closed_ui from "./compose_closed_ui"; import * as compose_pm_pill from "./compose_pm_pill"; import * as compose_recipient from "./compose_recipient"; import * as compose_state from "./compose_state"; +import {electron_bridge} from "./electron_bridge"; import * as emoji from "./emoji"; import * as emoji_picker from "./emoji_picker"; import * as gear_menu from "./gear_menu"; @@ -261,8 +262,8 @@ export function dispatch_normal_event(event) { realm_settings[event.property](); settings_org.sync_realm_settings(event.property); - if (event.property === "name" && window.electron_bridge !== undefined) { - window.electron_bridge.send_event("realm_name", event.value); + if (event.property === "name") { + electron_bridge?.send_event("realm_name", event.value); } if (event.property === "invite_to_realm_policy") { @@ -326,15 +327,7 @@ export function dispatch_normal_event(event) { realm.realm_icon_url = event.data.icon_url; realm.realm_icon_source = event.data.icon_source; realm_icon.rerender(); - { - const electron_bridge = window.electron_bridge; - if (electron_bridge !== undefined) { - electron_bridge.send_event( - "realm_icon_url", - event.data.icon_url, - ); - } - } + electron_bridge?.send_event("realm_icon_url", event.data.icon_url); break; case "logo": realm.realm_logo_url = event.data.logo_url; diff --git a/web/tests/activity.test.js b/web/tests/activity.test.js index ec207ab538..eca784d49b 100644 --- a/web/tests/activity.test.js +++ b/web/tests/activity.test.js @@ -26,6 +26,7 @@ const _document = { }; const channel = mock_esm("../src/channel"); +const electron_bridge = mock_esm("../src/electron_bridge"); const padded_widget = mock_esm("../src/padded_widget"); const pm_list = mock_esm("../src/pm_list"); const popovers = mock_esm("../src/popovers"); @@ -878,7 +879,7 @@ test("electron_bridge", ({override_rewire}) => { function with_bridge_idle(bridge_idle, f) { with_overrides(({override}) => { - override(window, "electron_bridge", { + override(electron_bridge, "electron_bridge", { get_idle_on_system: () => bridge_idle, }); return f(); @@ -893,7 +894,7 @@ test("electron_bridge", ({override_rewire}) => { }); with_overrides(({override}) => { - override(window, "electron_bridge", undefined); + override(electron_bridge, "electron_bridge", undefined); activity.mark_client_idle(); assert.equal(activity.compute_active_status(), "idle"); activity.mark_client_active(); diff --git a/web/tests/dispatch.test.js b/web/tests/dispatch.test.js index 431d4bbb49..015e65f186 100644 --- a/web/tests/dispatch.test.js +++ b/web/tests/dispatch.test.js @@ -30,6 +30,9 @@ const audible_notifications = mock_esm("../src/audible_notifications"); const bot_data = mock_esm("../src/bot_data"); const compose_banner = mock_esm("../src/compose_banner"); const compose_pm_pill = mock_esm("../src/compose_pm_pill"); +const {electron_bridge} = mock_esm("../src/electron_bridge", { + electron_bridge: {}, +}); const theme = mock_esm("../src/theme"); const emoji_picker = mock_esm("../src/emoji_picker"); const gear_menu = mock_esm("../src/gear_menu"); @@ -99,8 +102,6 @@ const overlays = mock_esm("../src/overlays"); mock_esm("../src/giphy"); const {Filter} = zrequire("filter"); -const electron_bridge = set_global("electron_bridge", {}); - message_lists.update_recipient_bar_background_color = noop; message_lists.current = { get_row: noop, diff --git a/web/tests/notifications.test.js b/web/tests/notifications.test.js index 54ee383336..b58dc815c2 100644 --- a/web/tests/notifications.test.js +++ b/web/tests/notifications.test.js @@ -7,6 +7,7 @@ const {run_test} = require("./lib/test"); const $ = require("./lib/zjquery"); const {current_user, page_params, user_settings} = require("./lib/zpage_params"); +mock_esm("../src/electron_bridge"); mock_esm("../src/spoilers", {hide_spoilers_in_notification() {}}); const user_topics = zrequire("user_topics");