-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix(desktop): browser pane popups, fingerprinting, and tab title/favicon bleed #6928
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -200,7 +200,27 @@ class BrowserManager extends EventEmitter { | |
| // throttled+hidden guest stops presenting frames and CDP input and | ||
| // screenshots silently break. | ||
| this.applyThrottling(paneId, wc); | ||
| wc.setWindowOpenHandler(({ url }) => { | ||
| wc.setWindowOpenHandler(({ url, disposition, features }) => { | ||
| // Popup-shaped requests — a non-empty features string (the | ||
| // `width=…,height=…` shape OAuth "Sign in with …" buttons use) or | ||
| // Chromium's NEW_POPUP disposition, which Electron reports as | ||
| // "new-window" — must become a real child window. Denying them | ||
| // makes `window.open()` return null and severs `window.opener`, so | ||
| // postMessage/`popup.closed` handshakes never complete. | ||
| const isPopup = disposition === "new-window" || features !== ""; | ||
| if (isPopup && isAllowedGuestUrl(url)) { | ||
| return { | ||
| action: "allow" as const, | ||
| overrideBrowserWindowOptions: { | ||
| // Share the pane's session so the popup sees the same | ||
| // cookies/storage as its opener. | ||
| webPreferences: { partition: "persist:superset" }, | ||
| }, | ||
| }; | ||
|
Comment on lines
+211
to
+219
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: printf '%s\n' '--- applicable repository conventions ---'
find /tmp/coderabbit-repo-knowledge/superset-sh-superset-c3450498 -type f -name '*.md' -maxdepth 3 -print
printf '%s\n' '--- browser manager structure and relevant symbols ---'
ast-grep outline apps/desktop/src/main/lib/browser/browser-manager.ts
printf '%s\n' '--- navigation and child-window handlers ---'
rg -n -C 8 'setWindowOpenHandler|setupNavigationGuard|web-contents-created|will-navigate|will-redirect|isAllowedGuestUrl|BrowserWindow' apps/desktop/src/main/lib/browser/browser-manager.tsRepository: superset-sh/superset Length of output: 10404 🏁 Script executed: set -o pipefail
printf '%s\n' '--- convention files covering desktop/main ---'
find /tmp/coderabbit-repo-knowledge/superset-sh-superset-c3450498 -type f -path '*/**/*.md' -print | sort | head -100
printf '%s\n' '--- relevant source references ---'
rg -n -C 10 'setWindowOpenHandler|setupNavigationGuard|web-contents-created|will-navigate|will-redirect|isAllowedGuestUrl|BrowserWindow' apps/desktop/src/main/lib/browser/browser-manager.tsRepository: superset-sh/superset Length of output: 9415 🏁 Script executed: printf '%s\n' '--- knowledge scope names ---'
find /tmp/coderabbit-repo-knowledge/superset-sh-superset-c3450498 -type f -name '*.md' -print | sort
printf '%s\n' '--- exact navigation policy definitions and callers ---'
rg -n -C 12 'setupNavigationGuard|webContents|web-contents-created|will-navigate|will-redirect|setWindowOpenHandler|isAllowedGuestUrl' apps/desktop/src/main/lib/browserRepository: superset-sh/superset Length of output: 50376 🏁 Script executed: printf '%s\n' '--- desktop conventions ---'
cat /tmp/coderabbit-repo-knowledge/superset-sh-superset-c3450498/conventions/apps-desktop.md
printf '%s\n' '--- desktop main learnings ---'
cat /tmp/coderabbit-repo-knowledge/superset-sh-superset-c3450498/learnings/apps-desktop-src-main.md
printf '%s\n' '--- process-wide webContents and navigation controls ---'
rg -n -C 6 'web-contents-created|webContentsCreated|will-navigate|will-redirect|setWindowOpenHandler' apps/desktop/src apps/desktop/electron* apps/desktop/package.json 2>/dev/null | head -240Repository: superset-sh/superset Length of output: 14513 🏁 Script executed: printf '%s\n' '--- bounded source check for any global child-webContents guard ---'
rg -n 'web-contents-created|webContentsCreated|will-navigate|will-redirect' apps/desktop/src/main apps/desktop/src | head -160
printf '%s\n' '--- BrowserWindow construction in desktop main source ---'
rg -n -C 8 'new BrowserWindow|BrowserWindow\(' apps/desktop/src/main | head -200Repository: superset-sh/superset Length of output: 1788 🏁 Script executed: printf '%s\n' '--- bounded global-handler search result ---'
rg -n 'web-contents-created|webContentsCreated|will-navigate|will-redirect' apps/desktop/src/main apps/desktop/src | head -160Repository: superset-sh/superset Length of output: 1707 🏁 Script executed: sed -n '1,75p' apps/desktop/src/lib/electron-app/factories/app/setup.tsRepository: superset-sh/superset Length of output: 2438 Security Misconfiguration (CWE-693) Reachability: External · Exploitability: Moderate Apply the guest navigation allowlist to allowed child windows. The process-wide 🤖 Prompt for AI AgentsSource: MCP tools |
||
| } | ||
| // Tab-shaped requests (target="_blank" links, featureless | ||
| // window.open) stay in-app: deny the native window and let the | ||
| // renderer open the URL as a new browser split. | ||
| if (url && url !== "about:blank") { | ||
| this.emit(`new-window:${paneId}`, url); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,119 @@ | ||
| import { join } from "node:path"; | ||
| import { type Session, session } from "electron"; | ||
| import { | ||
| chromeMajorVersion, | ||
| clientHintsBrandList, | ||
| clientHintsPlatform, | ||
| formatBrandVersionList, | ||
| } from "shared/client-hints"; | ||
|
|
||
| const BROWSER_PARTITION = "persist:superset"; | ||
|
|
||
| /** | ||
| * The frozen platform token real desktop Chrome sends per OS. Chrome stopped | ||
| * exposing the actual OS version in the UA (macOS is pinned to 10_15_7 even on | ||
| * Apple Silicon, Windows to NT 10.0), so these literals are what an ordinary | ||
| * Chrome install reports today. | ||
| */ | ||
| function chromePlatformToken(): string { | ||
| switch (process.platform) { | ||
| case "darwin": | ||
| return "Macintosh; Intel Mac OS X 10_15_7"; | ||
| case "win32": | ||
| return "Windows NT 10.0; Win64; x64"; | ||
| default: | ||
| return "X11; Linux x86_64"; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * A user-agent string matching what real desktop Chrome sends on this OS for | ||
| * the Chromium bundled with this Electron build. The major version comes from | ||
| * `process.versions.chrome` — never hardcode one: Client Hints (`sec-ch-ua`, | ||
| * `navigator.userAgentData`) are generated by the engine itself, and a UA | ||
| * claiming a different version than the hints report is a stronger bot signal | ||
| * than no override at all. Real Chrome's UA is version-reduced to | ||
| * `<major>.0.0.0` (the full build number only appears in Client Hints), so | ||
| * ours is too. | ||
| */ | ||
| export function buildChromeUserAgent(): string { | ||
| const chromeMajor = chromeMajorVersion(process.versions.chrome); | ||
| return `Mozilla/5.0 (${chromePlatformToken()}) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/${chromeMajor}.0.0.0 Safari/537.36`; | ||
| } | ||
|
|
||
| /** | ||
| * Rewrite the `sec-ch-ua*` request headers to match the UA string. | ||
| * `setUserAgent()` doesn't touch Chromium's UserAgentMetadata, which generates | ||
| * these headers independently — and Electron's brand list has "Chromium" but | ||
| * no "Google Chrome" entry, so without this every request carries Client | ||
| * Hints contradicting the UA: a stronger bot signal than the stock Electron | ||
| * UA ever was. | ||
| */ | ||
| function configureClientHintHeaders(browserSession: Session): void { | ||
| const chromeVersion = process.versions.chrome; | ||
| // Real Chrome sends the low-entropy hints on every request to a secure | ||
| // origin; the full-version ones only after a site opts in via Accept-CH, | ||
| // which Chromium tracks for us — so those are only replaced when the | ||
| // engine chose to send them. | ||
| const lowEntropyHeaders: Record<string, string> = { | ||
| "sec-ch-ua": formatBrandVersionList( | ||
| clientHintsBrandList(chromeVersion, "major"), | ||
| ), | ||
| "sec-ch-ua-mobile": "?0", | ||
| "sec-ch-ua-platform": `"${clientHintsPlatform(process.platform)}"`, | ||
| }; | ||
| const optInHeaders: Record<string, string> = { | ||
| "sec-ch-ua-full-version-list": formatBrandVersionList( | ||
| clientHintsBrandList(chromeVersion, "full"), | ||
| ), | ||
| "sec-ch-ua-full-version": `"${chromeMajorVersion(chromeVersion)}.0.0.0"`, | ||
| }; | ||
|
|
||
| // Electron keeps a single onBeforeSendHeaders listener per session — a | ||
| // second registration elsewhere would silently replace this one. | ||
| browserSession.webRequest.onBeforeSendHeaders((details, callback) => { | ||
| const headers = details.requestHeaders; | ||
| const present = new Set<string>(); | ||
| for (const name of Object.keys(headers)) { | ||
| const lower = name.toLowerCase(); | ||
| if ( | ||
| Object.hasOwn(lowEntropyHeaders, lower) || | ||
| Object.hasOwn(optInHeaders, lower) | ||
| ) { | ||
| present.add(lower); | ||
| delete headers[name]; | ||
| } | ||
| } | ||
| const secureOrigin = | ||
| details.url.startsWith("https:") || details.url.startsWith("wss:"); | ||
| for (const [name, value] of Object.entries(lowEntropyHeaders)) { | ||
| if (secureOrigin || present.has(name)) headers[name] = value; | ||
| } | ||
| for (const [name, value] of Object.entries(optInHeaders)) { | ||
| if (present.has(name)) headers[name] = value; | ||
| } | ||
| callback({ requestHeaders: headers }); | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Make the browser pane's session fingerprint as ordinary Chrome, in all | ||
| * three places bot-detection scripts (Cloudflare, DataDome, PerimeterX, …) | ||
| * cross-check: the UA string, the `sec-ch-ua*` request headers, and the | ||
| * `navigator.userAgentData` object page JS reads. All are derived from the | ||
| * same `process.versions.chrome`, so they can never disagree. A mismatch | ||
| * between any two can get a user's own ordinary browsing wrongly challenged | ||
| * or blocked. | ||
| */ | ||
| export function configureBrowserUserAgent(): void { | ||
| const browserSession = session.fromPartition(BROWSER_PARTITION); | ||
| browserSession.setUserAgent(buildChromeUserAgent()); | ||
| configureClientHintHeaders(browserSession); | ||
| // Headers alone don't change what live scripts read from | ||
| // navigator.userAgentData; a session preload rebuilds it in every frame | ||
| // before page scripts run. | ||
| browserSession.registerPreloadScript({ | ||
| type: "frame", | ||
| filePath: join(__dirname, "../preload/browser-client-hints.js"), | ||
| }); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| import { contextBridge } from "electron"; | ||
| import { | ||
| buildUserAgentDataOverride, | ||
| type UserAgentDataOverride, | ||
| } from "shared/client-hints"; | ||
|
|
||
| /** | ||
| * Session preload for the browser pane (registered in | ||
| * main/lib/browser/user-agent.ts), running at document-start in every frame. | ||
| * Rewriting the `sec-ch-ua*` headers doesn't change what page JS reads from | ||
| * `navigator.userAgentData` — Chromium builds that from the same metadata | ||
| * `setUserAgent()` can't touch, so scripts would still see a brand list with | ||
| * no "Google Chrome" entry contradicting the UA string. Replace it in the | ||
| * main world before any page script can read it. | ||
| */ | ||
|
|
||
| const override = buildUserAgentDataOverride({ | ||
| chromeVersion: process.versions.chrome, | ||
| platform: process.platform, | ||
| arch: process.arch, | ||
| osVersion: process.getSystemVersion(), | ||
| }); | ||
|
|
||
| contextBridge.executeInMainWorld({ | ||
| // Serialized and re-executed in the page's world: only `data` and globals | ||
| // are in scope here. | ||
| func: (data: UserAgentDataOverride) => { | ||
| // Real Chrome exposes navigator.userAgentData only in secure contexts. | ||
| if (!globalThis.isSecureContext) return; | ||
| const uaData = { | ||
| brands: data.brands, | ||
| mobile: data.mobile, | ||
| platform: data.platform, | ||
| getHighEntropyValues: async (_hints?: string[]) => ({ | ||
| brands: data.brands, | ||
| mobile: data.mobile, | ||
| ...data.highEntropy, | ||
| }), | ||
|
Comment on lines
+34
to
+38
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: printf '%s\n' '--- repository conventions ---'
find /tmp/coderabbit-repo-knowledge/superset-sh-superset-c3450498 -maxdepth 2 -type f -name '*.md' -print
printf '%s\n' '--- candidate file ---'
cat -n apps/desktop/src/preload/browser-client-hints.ts
printf '%s\n' '--- direct references ---'
rg -n -C 3 'buildUserAgentDataOverride|userAgentData|getHighEntropyValues|highEntropy' apps/desktop/src/preload apps/desktop/src/sharedRepository: superset-sh/superset Length of output: 13847 🏁 Script executed: cat /tmp/coderabbit-repo-knowledge/superset-sh-superset-c3450498/conventions/apps-desktop.md
printf '\n--- repo-wide conventions ---\n'
cat /tmp/coderabbit-repo-knowledge/superset-sh-superset-c3450498/conventions/repo-wide.md
printf '\n--- scoped learnings ---\n'
for f in /tmp/coderabbit-repo-knowledge/superset-sh-superset-c3450498/learnings/apps-desktop.md /tmp/coderabbit-repo-knowledge/superset-sh-superset-c3450498/learnings/repo-wide.md; do
[ -f "$f" ] && { echo "--- $f ---"; cat "$f"; }
doneRepository: superset-sh/superset Length of output: 5612 🌐 Web query:
💡 Result: The ch-ua-high-entropy-values directive is a policy-controlled feature defined in the WICG User-Agent Client Hints specification that governs access to high-entropy information via the NavigatorUAData.getHighEntropyValues JavaScript API [1][2]. Key details regarding this Permissions Policy include: Purpose: It allows top-level sites to restrict which documents (including third-party iframes) are permitted to retrieve high-entropy user-agent data [1][3][4]. This provides site owners with greater control over data exposure, effectively enabling them to disable or limit the collection of high-entropy hints to reduce potential for passive fingerprinting [1][5][6]. Behavior: If a document is not allowed to use the ch-ua-high-entropy-values feature, any call to getHighEntropyValues will resolve with only the available low-entropy data (typically brands, mobile status, and platform) and omit the requested high-entropy properties [1][7][8]. Default Configuration: The policy has a default allowlist of '*', meaning that by default, all content within a document and its nested browsing contexts is permitted to use the getHighEntropyValues method [1][3]. Implementation: Site owners can configure this via the HTTP Permissions-Policy header or the allow attribute on iframe elements [7][3]. Example Usage (HTTP Header): Permissions-Policy: ch-ua-high-entropy-values=("self" "https://example.com") [7][3] Example Usage (iframe): <iframe src="https://example.com" allow="ch-ua-high-entropy-values"></iframe> [3] Note that while this policy controls the overall access to the getHighEntropyValues method, individual client hints may also be managed by their own specific Permissions Policy directives [3][4]. Citations:
Sensitive Data Exposure (CWE-200): Exposure of Sensitive Information to an Unauthorized Actor Reachability: External · Exploitability: Trivial Honor the requested hints and When policy denies 🤖 Prompt for AI Agents |
||
| toJSON: () => ({ | ||
| brands: data.brands, | ||
| mobile: data.mobile, | ||
| platform: data.platform, | ||
| }), | ||
| }; | ||
| // Redefine the prototype accessor (where the real property lives) | ||
| // rather than adding an own property to the navigator instance. | ||
| Object.defineProperty(Navigator.prototype, "userAgentData", { | ||
| get: () => uaData, | ||
| configurable: true, | ||
| enumerable: true, | ||
| }); | ||
| }, | ||
| args: [override], | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: superset-sh/superset
Length of output: 49541
🏁 Script executed:
Repository: superset-sh/superset
Length of output: 26282
🌐 Web query:
Electron 41.10.3 WebContents setWindowOpenHandler WindowOpenDisposition NEW_POPUP NEW_WINDOW shift-click new-window💡 Result:
In Electron, webContents.setWindowOpenHandler is the recommended API for managing windows created by the renderer process (e.g., via window.open or link clicks) [1][2]. The details object passed to the handler includes a disposition property, which corresponds to the Chromium WindowOpenDisposition and indicates how a link was opened [3]. For user interactions involving modifier keys or specific link targets, the disposition will be set as follows: 1. new-window: This disposition corresponds to a shift + left click [3]. 2. NEW_POPUP and NEW_WINDOW: These are specific Chromium WindowOpenDisposition values [3]. In the context of Electron's setWindowOpenHandler, these are typically categorized under the new-window disposition or the general event flow when a user explicitly requests a new top-level window [3]. Since Electron 12 and later, setWindowOpenHandler is explicitly invoked for these actions (including shift-clicks and target=_blank links) [4]. You can use this handler to decide whether to allow or deny the window, and optionally override BrowserWindow construction options [1][5]. Example usage: mainWindow.webContents.setWindowOpenHandler((details) => { // details.disposition contains values like 'new-window', 'foreground-tab', etc. if (details.disposition === 'new-window') { return { action: 'allow', overrideBrowserWindowOptions: { width: 800, height: 600 } }; } return { action: 'deny' }; }); By returning { action: 'allow', overrideBrowserWindowOptions: {... } }, you maintain full control over the newly created window's security and behavior, which is more privileged and robust than relying on the renderer-provided feature string [1][5][6].
Citations:
🌐 Web query:
site:github.com/electron/electron v41.10.3 "NEW_POPUP" "NEW_WINDOW" "new-window" setWindowOpenHandler💡 Result:
In the context of Electron,
NEW_POPUPandNEW_WINDOWare ChromiumWindowOpenDispositionvalues that represent the intent to open a new window or popup [1][2]. Within Electron’s internal C++ implementation, these specific dispositions are mapped to the string value"new-window"when surfaced to the developer [1][2]. The methodwebContents.setWindowOpenHandler()is the modern, recommended API used to intercept and handle these requests [3][4][5]. It replaces the deprecatednew-windowevent [4][5]. Key Technical Details: - API Purpose:setWindowOpenHandlerallows developers to customize or block the creation of new windows (including popups) triggered bywindow.openor other navigation paths before they are created [3][5]. - Implementation: When a renderer attempts to open a new window, the handler is invoked in the main process [3][6]. Returning{ action: 'deny' }cancels the operation, while{ action: 'allow', overrideBrowserWindowOptions: { ... } }allows it with customBrowserWindowconfigurations [3][4]. - Electron v41.10.3 Update: Version 41.10.3 included a security fix (GHSA-9f4c-93c8-jc8g) addressing a vulnerability where sandboxed iframes could bypassallow-popupsrestrictions via theOpenURLnavigation path [7]. This fix ensures thatsetWindowOpenHandleris correctly triggered even in these scenarios, making it a critical security control for filtering untrusted content [7]. By usingsetWindowOpenHandler, you gain full control over theBrowserWindowConstructorOptionsfor any new window, which is more secure and flexible than relying on the legacynew-windowevent or standard window feature strings [3][4][5].Citations:
Use a popup-specific signal instead of
disposition === "new-window". Electron reports shift-clicked links as"new-window". A shift-click on an ordinary_blanklink can therefore create a native child window instead of emittingnew-window:${paneId}for the renderer split flow. Add Electron 41.10.3 regression coverage for script popups and shift-clicked links.🤖 Prompt for AI Agents
Source: MCP tools