From cc0eccabe6f5b0f6a8fda65f40eae95c78bd87f8 Mon Sep 17 00:00:00 2001 From: multica-agent Date: Tue, 28 Jul 2026 15:53:51 +0800 Subject: [PATCH] fix(diagnostics): close the kill switch, egress and multi-window gaps (MUL-5345) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review findings on #6026, all in failure paths the tests didn't reach. The kill switch didn't reliably revoke. `coolDebuggerChannel` only detached after `Debugger.disable` resolved, so a failed disable left the channel attached — the exact state the switch exists to exit. Disable is a courtesy to the renderer; detach is the contract, so it now runs in `finally`. Warming had the mirror bug: an attach we made and could not enable returned false while leaving the channel open, stranding a debugger on a renderer nothing tracks. That attach is rolled back now, and only that one — a channel someone else owns (DevTools) is left alone. Stack frames were sanitized at capture and then forwarded verbatim at egress. Between those two points they cross an on-disk breadcrumb that `readFreezeBreadcrumb` barely validates, by design: it only has to survive version skew. So "sanitized once" was not a property the flush side could rely on — an older build, a corrupt file or a future writer could put a `scopeChain` handle or an absolute install path in there and it would ship. Both ends now rebuild frames through one shared whitelist, which also makes them impossible to drift apart. The url reduction is idempotent so re-running it costs nothing. The control flag was global, and that does not survive multiple windows. Every renderer publishes `false` before its own config lands, so a window opened while capture was on either never warmed (the global value never changed, so nothing warmed the new webContents) or cooled every other window on its way up. State is per renderer now; they converge on the same value because they read the same config, but each on its own schedule. Regression tests for each: detach after a throwing disable and rollback after a throwing enable, a frame carrying `scopeChain` / `this` / an absolute path reaching the flush side, and a second window warming while the first is already on without revoking it. Co-Authored-By: Claude Opus 5 Co-authored-by: multica-agent --- .../main/diagnostics-control-registry.test.ts | 103 +++++++++++++++ .../src/main/diagnostics-control-registry.ts | 58 +++++++++ apps/desktop/src/main/index.ts | 80 ++++-------- .../src/main/renderer-stack-capture.test.ts | 106 +++++++++++---- .../src/main/renderer-stack-capture.ts | 90 +++++-------- .../src/renderer/src/freeze-flush.test.ts | 48 +++++++ apps/desktop/src/renderer/src/freeze-flush.ts | 32 +++-- apps/desktop/src/shared/hang-stack.test.ts | 123 ++++++++++++++++++ apps/desktop/src/shared/hang-stack.ts | 77 +++++++++++ 9 files changed, 565 insertions(+), 152 deletions(-) create mode 100644 apps/desktop/src/main/diagnostics-control-registry.test.ts create mode 100644 apps/desktop/src/main/diagnostics-control-registry.ts create mode 100644 apps/desktop/src/shared/hang-stack.test.ts create mode 100644 apps/desktop/src/shared/hang-stack.ts diff --git a/apps/desktop/src/main/diagnostics-control-registry.test.ts b/apps/desktop/src/main/diagnostics-control-registry.test.ts new file mode 100644 index 0000000000..5e4e6beaaa --- /dev/null +++ b/apps/desktop/src/main/diagnostics-control-registry.test.ts @@ -0,0 +1,103 @@ +/** + * Review finding (MUL-5345): a single global control value does not survive + * multiple windows. Every renderer publishes `false` before its config lands, + * so with one shared value a window opened while capture was on would either + * never warm (the global value never changed) or would cool every other + * window on its way up. Both are pinned here. + */ +import { describe, expect, it, vi } from "vitest"; + +import { createDiagnosticsControlRegistry } from "./diagnostics-control-registry"; + +function setup() { + const warm = vi.fn(); + const cool = vi.fn(); + const registry = createDiagnosticsControlRegistry({ warm, cool }); + // Stand-ins for webContents; identity is all the registry uses. + return { registry, warm, cool, mainWindow: {}, issueWindow: {} }; +} + +describe("per-renderer control", () => { + it("warms a newly opened window even though another window is already on", () => { + const { registry, warm, mainWindow, issueWindow } = setup(); + registry.apply(mainWindow, { stackCaptureEnabled: true }); + warm.mockClear(); + + // The new window boots, publishes its pre-config default, then the real + // value. A global flag would have swallowed this as "no change". + registry.apply(issueWindow, { stackCaptureEnabled: false }); + registry.apply(issueWindow, { stackCaptureEnabled: true }); + + expect(warm).toHaveBeenCalledTimes(1); + expect(warm).toHaveBeenCalledWith(issueWindow); + expect(registry.isStackCaptureEnabled(issueWindow)).toBe(true); + }); + + it("does not let a new window's pre-config default revoke another window", () => { + const { registry, cool, mainWindow, issueWindow } = setup(); + registry.apply(mainWindow, { stackCaptureEnabled: true }); + + registry.apply(issueWindow, { stackCaptureEnabled: false }); + + expect(cool).not.toHaveBeenCalledWith(mainWindow); + expect(registry.isStackCaptureEnabled(mainWindow)).toBe(true); + expect(registry.isStackCaptureEnabled(issueWindow)).toBe(false); + }); + + it("cools only the window that revoked", () => { + const { registry, cool, mainWindow, issueWindow } = setup(); + registry.apply(mainWindow, { stackCaptureEnabled: true }); + registry.apply(issueWindow, { stackCaptureEnabled: true }); + + registry.apply(mainWindow, { stackCaptureEnabled: false }); + + expect(cool).toHaveBeenCalledTimes(1); + expect(cool).toHaveBeenCalledWith(mainWindow); + expect(registry.isStackCaptureEnabled(issueWindow)).toBe(true); + }); + + it("does not re-warm a window that repeats the same value", () => { + const { registry, warm, mainWindow } = setup(); + registry.apply(mainWindow, { stackCaptureEnabled: true }); + registry.apply(mainWindow, { stackCaptureEnabled: true }); + + expect(warm).toHaveBeenCalledTimes(1); + }); + + it("does not cool a window that was never warm", () => { + const { registry, cool, mainWindow } = setup(); + registry.apply(mainWindow, { stackCaptureEnabled: false }); + + expect(cool).not.toHaveBeenCalled(); + }); +}); + +describe("fail-closed", () => { + it("starts disabled for a renderer that has never reported", () => { + const { registry, mainWindow } = setup(); + expect(registry.isStackCaptureEnabled(mainWindow)).toBe(false); + }); + + it.each([ + ["a malformed payload", { stackCaptureEnabled: "true" }], + ["null", null], + ["undefined", undefined], + ["an empty object", {}], + ])("treats %s as off", (_label, payload) => { + const { registry, warm, mainWindow } = setup(); + registry.apply(mainWindow, payload); + + expect(warm).not.toHaveBeenCalled(); + expect(registry.isStackCaptureEnabled(mainWindow)).toBe(false); + }); + + it("revokes when a warm renderer later sends garbage", () => { + const { registry, cool, mainWindow } = setup(); + registry.apply(mainWindow, { stackCaptureEnabled: true }); + + registry.apply(mainWindow, "nonsense"); + + expect(cool).toHaveBeenCalledWith(mainWindow); + expect(registry.isStackCaptureEnabled(mainWindow)).toBe(false); + }); +}); diff --git a/apps/desktop/src/main/diagnostics-control-registry.ts b/apps/desktop/src/main/diagnostics-control-registry.ts new file mode 100644 index 0000000000..fa7d64b146 --- /dev/null +++ b/apps/desktop/src/main/diagnostics-control-registry.ts @@ -0,0 +1,58 @@ +import { + parseDiagnosticsControl, + type DiagnosticsControl, +} from "../shared/diagnostics-control"; + +/** + * Which renderers are allowed to be interrogated, tracked per renderer. + * + * A single global flag looked right — the value is one server-side switch, so + * every renderer reports the same thing — but it does not survive multiple + * windows. Every renderer publishes `false` before its config arrives, so a + * window opened while capture was already on would either be skipped (the + * global value never changed, so nothing warmed it) or, worse, would revoke + * capture for every other window on the way up. Windows come and go + * independently, so the state has to be per renderer. + * + * Each renderer therefore owns its own entry: its report warms or cools its + * own channel and nothing else. They converge on the same value because they + * read the same config, but they get there on their own schedules. + */ +export interface DiagnosticsControlRegistry { + /** Apply a raw control payload from one renderer. Fail-closed on garbage. */ + apply: (target: T, rawControl: unknown) => void; + /** Whether this renderer may currently be interrogated. */ + isStackCaptureEnabled: (target: T) => boolean; +} + +export interface DiagnosticsControlHandlers { + /** Open the debugger channel for this renderer. */ + warm: (target: T) => void; + /** Close the debugger channel for this renderer. */ + cool: (target: T) => void; +} + +export function createDiagnosticsControlRegistry({ + warm, + cool, +}: DiagnosticsControlHandlers): DiagnosticsControlRegistry { + // Keyed by the renderer itself, so a closed window's entry disappears with + // it rather than pinning the object alive. + const controls = new WeakMap(); + + const enabled = (target: T) => + controls.get(target)?.stackCaptureEnabled === true; + + return { + apply(target, rawControl) { + const next = parseDiagnosticsControl(rawControl); + const was = enabled(target); + controls.set(target, next); + if (next.stackCaptureEnabled === was) return; + if (next.stackCaptureEnabled) warm(target); + else cool(target); + }, + + isStackCaptureEnabled: enabled, + }; +} diff --git a/apps/desktop/src/main/index.ts b/apps/desktop/src/main/index.ts index 6aa5ccd1f3..b71805ef38 100644 --- a/apps/desktop/src/main/index.ts +++ b/apps/desktop/src/main/index.ts @@ -37,12 +37,8 @@ import { coolDebuggerChannel, warmDebuggerChannel, } from "./renderer-stack-capture"; -import { - DIAGNOSTICS_CONTROL_CHANNEL, - DIAGNOSTICS_CONTROL_OFF, - parseDiagnosticsControl, - type DiagnosticsControl, -} from "../shared/diagnostics-control"; +import { DIAGNOSTICS_CONTROL_CHANNEL } from "../shared/diagnostics-control"; +import { createDiagnosticsControlRegistry } from "./diagnostics-control-registry"; import { loadWindowState, resolveWindowOptions, @@ -155,57 +151,31 @@ const rendererRouteContexts = new WeakMap< >(); // Hang stack capture is off until the backend says otherwise, and stays off in -// dev. Reading a stack means holding a debugger channel open on every -// renderer, so it has to be revocable without shipping a release — the flag -// arrives with /api/config, which is also why this cannot be decided at window -// creation: no renderer has fetched config yet at that point. -let diagnosticsControl: DiagnosticsControl = DIAGNOSTICS_CONTROL_OFF; -const debuggerWarmedWindows = new WeakSet(); - -function stackCaptureAllowed(): boolean { - return !is.dev && diagnosticsControl.stackCaptureEnabled; -} - -/** - * Open the debugger channel for a healthy renderer. A hang can only be - * interrogated through a channel that already exists — a command sent after - * the main thread is stuck is never dispatched (measured on Electron 39.8.7). - * Warming is therefore driven by the flag arriving, not by window creation. - */ -function warmStackCaptureFor(webContents: Electron.WebContents): void { - if (!stackCaptureAllowed()) return; - if (webContents.isDestroyed()) return; - if (debuggerWarmedWindows.has(webContents)) return; - debuggerWarmedWindows.add(webContents); - void warmDebuggerChannel(webContents.debugger).then((warmed) => { - if (!warmed) debuggerWarmedWindows.delete(webContents); - }); -} - -function warmStackCaptureForAllWindows(): void { - for (const window of BrowserWindow.getAllWindows()) { - if (!window.isDestroyed()) warmStackCaptureFor(window.webContents); - } -} - -/** - * Revoking the flag has to close the channels too. Skipping the next capture - * would leave every renderer running with a debugger attached, which is the - * state the kill switch exists to be able to exit. - */ -function detachStackCaptureFromAllWindows(): void { - for (const window of BrowserWindow.getAllWindows()) { - if (window.isDestroyed()) continue; - debuggerWarmedWindows.delete(window.webContents); - void coolDebuggerChannel(window.webContents.debugger); - } -} +// dev. Reading a stack means holding a debugger channel open on a renderer, so +// it has to be revocable without shipping a release — the flag arrives with +// /api/config, which is also why this cannot be decided at window creation: no +// renderer has fetched config yet at that point. +// +// State is per renderer, not global: every window publishes `false` before its +// own config lands, so one global value would let a newly opened window revoke +// capture for the others (see diagnostics-control-registry). +const diagnosticsControl = createDiagnosticsControlRegistry({ + warm: (webContents) => { + if (is.dev || webContents.isDestroyed()) return; + void warmDebuggerChannel(webContents.debugger); + }, + cool: (webContents) => { + if (webContents.isDestroyed()) return; + void coolDebuggerChannel(webContents.debugger); + }, +}); /** Read the hung renderer's JS stack, or null when capture is not permitted. */ async function captureStackIfEnabled( webContents: Electron.WebContents, ): Promise { - if (!stackCaptureAllowed()) return null; + if (is.dev) return null; + if (!diagnosticsControl.isStackCaptureEnabled(webContents)) return null; if (webContents.isDestroyed()) return null; return captureHangStack(webContents.debugger); } @@ -801,11 +771,7 @@ if (!gotTheLock) { // the debugger channels we are holding. ipcMain.on(DIAGNOSTICS_CONTROL_CHANNEL, (event, control: unknown) => { if (!BrowserWindow.fromWebContents(event.sender)) return; - const next = parseDiagnosticsControl(control); - if (next.stackCaptureEnabled === diagnosticsControl.stackCaptureEnabled) return; - diagnosticsControl = next; - if (next.stackCaptureEnabled) warmStackCaptureForAllWindows(); - else detachStackCaptureFromAllWindows(); + diagnosticsControl.apply(event.sender, control); }); // Sync IPC: preload exposes the validated runtime config before renderer diff --git a/apps/desktop/src/main/renderer-stack-capture.test.ts b/apps/desktop/src/main/renderer-stack-capture.test.ts index 43ec4070b2..cabda6c911 100644 --- a/apps/desktop/src/main/renderer-stack-capture.test.ts +++ b/apps/desktop/src/main/renderer-stack-capture.test.ts @@ -5,8 +5,7 @@ import { join } from "node:path"; import { ALLOWED_CDP_METHODS, captureHangStack, - redactFrameUrl, - sanitizeCallFrames, + coolDebuggerChannel, sendDebuggerCommand, warmDebuggerChannel, type CdpDebugger, @@ -30,14 +29,19 @@ function makeDebugger( const sent: string[] = []; let attached = options.attached ?? true; + const attach = vi.fn(() => { + if (options.failOn === "attach") throw new Error("boom: attach"); + attached = true; + }); + const detach = vi.fn(() => { + if (options.failOn === "detach") throw new Error("boom: detach"); + attached = false; + }); + const dbg: CdpDebugger = { isAttached: () => attached, - attach: () => { - attached = true; - }, - detach: () => { - attached = false; - }, + attach, + detach, sendCommand: vi.fn(async (method: string) => { sent.push(method); if (options.failOn === method) throw new Error(`boom: ${method}`); @@ -53,7 +57,14 @@ function makeDebugger( off: (_event, listener) => listeners.delete(listener as MessageListener), }; - return { dbg, sent, listenerCount: () => listeners.size }; + return { + dbg, + sent, + attach, + detach, + isAttached: () => attached, + listenerCount: () => listeners.size, + }; } const frame = { @@ -190,25 +201,74 @@ describe("warmDebuggerChannel", () => { }); }); -describe("frame sanitizing", () => { - it("labels an anonymous frame rather than dropping it", () => { - expect(sanitizeCallFrames([{ functionName: "", location: {} }], 10)).toEqual([ - { functionName: "(anonymous)", url: "", lineNumber: 0, columnNumber: 0 }, - ]); +// Review finding (MUL-5345): the kill switch has to actually revoke. A detach +// that only runs on the happy path leaves a debugger attached to a renderer +// after the flag is turned off, which is the state the switch exists to exit. +describe("the channel always closes", () => { + it("detaches even when Debugger.disable throws", async () => { + const { dbg, detach, isAttached } = makeDebugger({ failOn: "Debugger.disable" }); + + await coolDebuggerChannel(dbg); + + expect(detach).toHaveBeenCalledTimes(1); + expect(isAttached()).toBe(false); }); - it("returns null for a missing or empty frame list", () => { - expect(sanitizeCallFrames(undefined, 10)).toBeNull(); - expect(sanitizeCallFrames([], 10)).toBeNull(); + it("detaches on the normal path too", async () => { + const { dbg, sent, detach, isAttached } = makeDebugger(); + + await coolDebuggerChannel(dbg); + + expect(sent).toEqual(["Debugger.disable"]); + expect(detach).toHaveBeenCalledTimes(1); + expect(isAttached()).toBe(false); }); - it("strips the install path, which can contain the OS username", () => { - expect( - redactFrameUrl("file:///Users/someone/Applications/Multica.app/out/renderer/main.js"), - ).toBe("renderer/main.js"); + it("does nothing when the channel was never open", async () => { + const { dbg, detach, sent } = makeDebugger({ attached: false }); + + await coolDebuggerChannel(dbg); + + expect(sent).toEqual([]); + expect(detach).not.toHaveBeenCalled(); }); - it("tolerates a frame with no url", () => { - expect(redactFrameUrl(undefined)).toBe(""); + it("survives a detach that itself throws", async () => { + const { dbg } = makeDebugger({ failOn: "detach" }); + + await expect(coolDebuggerChannel(dbg)).resolves.toBeUndefined(); + }); + + // Warming is the other half: an attach we made and could not enable must be + // rolled back, or we report failure while leaving a channel open that + // nothing is tracking. + it("rolls the attach back when Debugger.enable throws", async () => { + const { dbg, attach, detach, isAttached } = makeDebugger({ + attached: false, + failOn: "Debugger.enable", + }); + + expect(await warmDebuggerChannel(dbg)).toBe(false); + + expect(attach).toHaveBeenCalledTimes(1); + expect(detach).toHaveBeenCalledTimes(1); + expect(isAttached()).toBe(false); + }); + + it("leaves an already-attached channel alone when enable throws", async () => { + // Someone else owns the debugger (DevTools). We did not attach it, so we + // must not detach it out from under them. + const { dbg, detach } = makeDebugger({ attached: true, failOn: "Debugger.enable" }); + + expect(await warmDebuggerChannel(dbg)).toBe(false); + + expect(detach).not.toHaveBeenCalled(); + }); + + it("reports failure without throwing when attach itself throws", async () => { + const { dbg, detach } = makeDebugger({ attached: false, failOn: "attach" }); + + expect(await warmDebuggerChannel(dbg)).toBe(false); + expect(detach).not.toHaveBeenCalled(); }); }); diff --git a/apps/desktop/src/main/renderer-stack-capture.ts b/apps/desktop/src/main/renderer-stack-capture.ts index fe7b38eec3..d169e4aa2b 100644 --- a/apps/desktop/src/main/renderer-stack-capture.ts +++ b/apps/desktop/src/main/renderer-stack-capture.ts @@ -30,6 +30,14 @@ // * BEST EFFORT: every failure resolves to null. A diagnostic must never be // the reason the app stays broken. +import { + HANG_STACK_MAX_FRAMES, + sanitizeHangStackFrames, + type HangStackFrame, +} from "../shared/hang-stack"; + +export type { HangStackFrame }; + /** Minimal CDP debugger surface — matches Electron's `webContents.debugger`. */ export interface CdpDebugger { isAttached(): boolean; @@ -54,13 +62,6 @@ export const ALLOWED_CDP_METHODS = [ "Debugger.resume", ] as const; -export interface HangStackFrame { - functionName: string; - url: string; - lineNumber: number; - columnNumber: number; -} - export interface CaptureHangStackOptions { /** Give up waiting for `Debugger.paused` after this long. */ timeoutMs?: number; @@ -69,7 +70,6 @@ export interface CaptureHangStackOptions { } const DEFAULT_TIMEOUT_MS = 2000; -const DEFAULT_MAX_FRAMES = 20; /** * Send a single CDP command, enforcing the allowlist. This is the ONLY path to @@ -96,12 +96,20 @@ export function sendDebuggerCommand( * healthy — that is the entire point. Returns whether the channel is usable. */ export async function warmDebuggerChannel(dbg: CdpDebugger): Promise { + let attachedHere = false; try { - if (!dbg.isAttached()) dbg.attach("1.3"); + if (!dbg.isAttached()) { + dbg.attach("1.3"); + attachedHere = true; + } await sendDebuggerCommand(dbg, "Debugger.enable"); return true; } catch { // Another client owns the debugger, or the renderer is already gone. + // An attach we made and could not enable has to be rolled back: reporting + // failure while leaving the channel open would strand a debugger on a + // renderer nothing is tracking. + if (attachedHere) detachQuietly(dbg); return false; } } @@ -112,12 +120,23 @@ export async function warmDebuggerChannel(dbg: CdpDebugger): Promise { * revoke the channel too rather than merely skipping the next capture. */ export async function coolDebuggerChannel(dbg: CdpDebugger): Promise { + if (!dbg.isAttached()) return; try { - if (!dbg.isAttached()) return; await sendDebuggerCommand(dbg, "Debugger.disable"); + } catch { + // Disable is a courtesy to the renderer; detach is the contract. A failed + // disable must not leave the channel attached, or revoking the kill switch + // would not actually revoke anything. + } finally { + detachQuietly(dbg); + } +} + +function detachQuietly(dbg: CdpDebugger): void { + try { dbg.detach(); } catch { - // Already detached / renderer gone — nothing to close. + // Already detached / renderer gone. } } @@ -133,7 +152,7 @@ export async function captureHangStack( options: CaptureHangStackOptions = {}, ): Promise { const timeoutMs = options.timeoutMs ?? DEFAULT_TIMEOUT_MS; - const maxFrames = options.maxFrames ?? DEFAULT_MAX_FRAMES; + const maxFrames = options.maxFrames ?? HANG_STACK_MAX_FRAMES; if (!dbg.isAttached()) return null; @@ -146,7 +165,7 @@ export async function captureHangStack( const paused = new Promise((resolve) => { onMessage = (_event, method, params) => { if (method !== "Debugger.paused") return; - resolve(sanitizeCallFrames(params.callFrames, maxFrames)); + resolve(sanitizeHangStackFrames(params.callFrames, maxFrames)); }; dbg.on("message", onMessage); timer = setTimeout(() => resolve(null), timeoutMs); @@ -172,48 +191,3 @@ export async function captureHangStack( } } } - -/** - * Copy the four scalar fields we report and drop everything else the protocol - * sends — notably `scopeChain`, whose object handles could be dereferenced - * into user data. - */ -export function sanitizeCallFrames( - rawFrames: unknown, - maxFrames: number, -): HangStackFrame[] | null { - if (!Array.isArray(rawFrames) || rawFrames.length === 0) return null; - - const frames: HangStackFrame[] = []; - for (const raw of rawFrames.slice(0, maxFrames)) { - if (!raw || typeof raw !== "object") continue; - const frame = raw as Record; - const location = (frame.location ?? {}) as Record; - frames.push({ - functionName: - typeof frame.functionName === "string" && frame.functionName - ? frame.functionName - : "(anonymous)", - url: redactFrameUrl(frame.url), - lineNumber: toNumber(location.lineNumber), - columnNumber: toNumber(location.columnNumber), - }); - } - return frames.length > 0 ? frames : null; -} - -/** - * Keep only the bundle-relative tail of a script URL. The full value is a - * `file://` path into the install location and can carry the OS username. - */ -export function redactFrameUrl(url: unknown): string { - if (typeof url !== "string" || !url) return ""; - const [withoutQuery = ""] = url.split(/[?#]/); - const segments = withoutQuery.split("/").filter(Boolean); - if (segments.length === 0) return ""; - return segments.slice(-2).join("/"); -} - -function toNumber(value: unknown): number { - return typeof value === "number" && Number.isFinite(value) ? value : 0; -} diff --git a/apps/desktop/src/renderer/src/freeze-flush.test.ts b/apps/desktop/src/renderer/src/freeze-flush.test.ts index 003145ab40..35698220fb 100644 --- a/apps/desktop/src/renderer/src/freeze-flush.test.ts +++ b/apps/desktop/src/renderer/src/freeze-flush.test.ts @@ -161,6 +161,54 @@ describe("telemetry props carry no raw identifiers", () => { expect(props).not.toHaveProperty("windowUrl"); }); + // Review finding (MUL-5345): the flush side used to forward `context.stack` + // wholesale. That array crosses an on-disk breadcrumb which is barely + // validated on read, so an older build, a corrupt file or a future writer + // could put a live handle in it and this would ship it. + it("rebuilds stack frames instead of forwarding whatever the file held", () => { + const props = buildFreezeEventProps({ + ...hang, + context: { + stack: [ + { + functionName: "blockMainThread", + url: "file:///Users/someone/Applications/Multica.app/out/renderer/main.js", + location: { lineNumber: 12, columnNumber: 3 }, + scopeChain: [{ type: "local", object: { objectId: "{secret-scope}" } }], + this: { objectId: "{secret-this}" }, + returnValue: "raw-value", + }, + ], + }, + }); + + const serialized = JSON.stringify(props); + expect(serialized).not.toContain("secret"); + expect(serialized).not.toContain("raw-value"); + // The absolute install path carries the OS username; only the tail ships. + expect(serialized).not.toContain("someone"); + expect(props.stack).toEqual([ + { + functionName: "blockMainThread", + url: "renderer/main.js", + lineNumber: 12, + columnNumber: 3, + }, + ]); + expect(props.stack_function).toBe("blockMainThread"); + expect(props.stack_url).toBe("renderer/main.js"); + expect(props.stack_line).toBe(12); + }); + + it("omits the stack fields when the file held nothing usable", () => { + for (const stack of [undefined, [], "not-an-array", [null]]) { + const props = buildFreezeEventProps({ ...hang, context: { stack } }); + expect(props).not.toHaveProperty("stack"); + expect(props).not.toHaveProperty("stack_depth"); + expect(props).not.toHaveProperty("stack_function"); + } + }); + it("drops an unknown context key rather than forwarding it", () => { const props = buildFreezeEventProps({ ...hang, diff --git a/apps/desktop/src/renderer/src/freeze-flush.ts b/apps/desktop/src/renderer/src/freeze-flush.ts index 259d0391b7..e9814fb2f2 100644 --- a/apps/desktop/src/renderer/src/freeze-flush.ts +++ b/apps/desktop/src/renderer/src/freeze-flush.ts @@ -1,5 +1,6 @@ import type { CaptureEventOptions } from "@multica/core/analytics"; import type { FreezeBreadcrumb } from "../../shared/freeze-breadcrumb"; +import { sanitizeHangStackFrames } from "../../shared/hang-stack"; // Reporting a failure the previous session couldn't report itself. // @@ -111,26 +112,29 @@ function routeProps(value: unknown): Record { } /** - * Frames are already reduced to code locations by the capture side. The top - * frame is also flattened onto the event so a hang can be grouped by the + * Frames are rebuilt here rather than forwarded. + * + * The capture side already whitelists, but its output travels through an + * on-disk breadcrumb that `readFreezeBreadcrumb` barely validates — it only + * has to survive version skew. So the array reaching this function could come + * from an older build, a corrupt file, or a future writer, and shipping it + * as-is would put whatever it contains (a `scopeChain` handle, an absolute + * install path) straight into telemetry. Re-sanitizing is cheap; trusting the + * file is not. + * + * The top frame is also flattened onto the event so a hang groups by the * function that blocked the thread without unpacking the array. */ function stackProps(value: unknown): Record { - if (!Array.isArray(value) || value.length === 0) return {}; - const frames = value as Array>; - const top = frames[0]; + const frames = sanitizeHangStackFrames(value); + if (!frames) return {}; + const top = frames[0]!; return { stack: frames, stack_depth: frames.length, - ...(top && typeof top.functionName === "string" - ? { stack_function: top.functionName } - : {}), - ...(top && typeof top.url === "string" && top.url - ? { stack_url: top.url } - : {}), - ...(top && typeof top.lineNumber === "number" - ? { stack_line: top.lineNumber } - : {}), + stack_function: top.functionName, + ...(top.url ? { stack_url: top.url } : {}), + stack_line: top.lineNumber, }; } diff --git a/apps/desktop/src/shared/hang-stack.test.ts b/apps/desktop/src/shared/hang-stack.test.ts new file mode 100644 index 0000000000..411e386c85 --- /dev/null +++ b/apps/desktop/src/shared/hang-stack.test.ts @@ -0,0 +1,123 @@ +/** + * Frames are rebuilt at both ends — capture and egress — because the on-disk + * breadcrumb between them is barely validated. These cases pin what "rebuilt" + * means: four scalars, and nothing that could be dereferenced into user data. + */ +import { describe, expect, it } from "vitest"; + +import { + HANG_STACK_MAX_FRAMES, + redactFrameUrl, + sanitizeHangStackFrames, +} from "./hang-stack"; + +const cdpFrame = { + functionName: "parseMarkdownChunked", + url: "file:///Applications/Multica.app/Contents/Resources/app.asar/out/renderer/assets/index-abc.js", + location: { lineNumber: 412, columnNumber: 17 }, + // Present on every real paused frame; both dereference into live values. + scopeChain: [{ type: "local", object: { objectId: "{secret-scope}" } }], + this: { objectId: "{secret-this}" }, +}; + +describe("sanitizeHangStackFrames", () => { + it("keeps the four scalar fields and reduces the url", () => { + expect(sanitizeHangStackFrames([cdpFrame])).toEqual([ + { + functionName: "parseMarkdownChunked", + url: "assets/index-abc.js", + lineNumber: 412, + columnNumber: 17, + }, + ]); + }); + + it("drops scope handles that could be dereferenced into user data", () => { + const frames = sanitizeHangStackFrames([cdpFrame]); + + expect(JSON.stringify(frames)).not.toContain("secret"); + expect(frames?.[0]).not.toHaveProperty("scopeChain"); + expect(frames?.[0]).not.toHaveProperty("this"); + }); + + it("drops any key a future protocol version might add", () => { + const frames = sanitizeHangStackFrames([ + { ...cdpFrame, functionLocation: { scriptId: "7" }, returnValue: "raw-value" }, + ]); + + expect(JSON.stringify(frames)).not.toContain("raw-value"); + expect(frames?.[0]).not.toHaveProperty("returnValue"); + }); + + // A breadcrumb frame has already been flattened; a CDP frame nests position + // under `location`. Re-sanitizing an already-sanitized frame must be lossless + // or the egress-side rebuild would zero out every line number. + it("accepts an already-flattened frame without losing its position", () => { + const flattened = { + functionName: "setContent", + url: "assets/index-abc.js", + lineNumber: 90, + columnNumber: 4, + }; + + expect(sanitizeHangStackFrames([flattened])).toEqual([flattened]); + expect(sanitizeHangStackFrames(sanitizeHangStackFrames([cdpFrame]))).toEqual( + sanitizeHangStackFrames([cdpFrame]), + ); + }); + + it("labels an anonymous frame rather than dropping it", () => { + expect(sanitizeHangStackFrames([{ functionName: "", location: {} }])).toEqual([ + { functionName: "(anonymous)", url: "", lineNumber: 0, columnNumber: 0 }, + ]); + }); + + it("returns null for a missing or empty frame list", () => { + expect(sanitizeHangStackFrames(undefined)).toBeNull(); + expect(sanitizeHangStackFrames([])).toBeNull(); + expect(sanitizeHangStackFrames("not-an-array")).toBeNull(); + }); + + it("keeps the top of the stack when it is deeper than the cap", () => { + const deep = Array.from({ length: 50 }, (_, i) => ({ + ...cdpFrame, + functionName: `fn${i}`, + })); + + expect(sanitizeHangStackFrames(deep, 3).map((f) => f!.functionName)).toEqual([ + "fn0", + "fn1", + "fn2", + ]); + expect(sanitizeHangStackFrames(deep)).toHaveLength(HANG_STACK_MAX_FRAMES); + }); + + it("skips a non-object entry instead of failing the whole stack", () => { + const frames = sanitizeHangStackFrames([null, cdpFrame, "junk"]); + expect(frames).toHaveLength(1); + }); +}); + +describe("redactFrameUrl", () => { + it("strips the install path, which can contain the OS username", () => { + expect( + redactFrameUrl("file:///Users/someone/Applications/Multica.app/out/renderer/main.js"), + ).toBe("renderer/main.js"); + }); + + it("is idempotent, so re-sanitizing does not erode the value", () => { + const once = redactFrameUrl( + "file:///Users/someone/Applications/Multica.app/out/renderer/main.js", + ); + expect(redactFrameUrl(once)).toBe(once); + }); + + it("drops query and hash", () => { + expect(redactFrameUrl("assets/index.js?v=1#L2")).toBe("assets/index.js"); + }); + + it("tolerates a frame with no url", () => { + expect(redactFrameUrl(undefined)).toBe(""); + expect(redactFrameUrl("")).toBe(""); + }); +}); diff --git a/apps/desktop/src/shared/hang-stack.ts b/apps/desktop/src/shared/hang-stack.ts new file mode 100644 index 0000000000..04c433fe63 --- /dev/null +++ b/apps/desktop/src/shared/hang-stack.ts @@ -0,0 +1,77 @@ +/** + * The shape a hang stack is allowed to have, and the only code that builds it. + * + * Frames cross three boundaries between capture and telemetry: main writes + * them into an on-disk breadcrumb, preload hands that file back over IPC, and + * the renderer turns it into an event. The breadcrumb is barely validated on + * read (it only has to survive version skew), so "sanitized once at capture" + * is not a property the egress side can rely on — an older file, a corrupt + * file, or a future main-process change could put anything in there. + * + * Both ends therefore rebuild frames through this module rather than trusting + * the other. Whitelisting twice is cheap; a `scopeChain` handle reaching + * PostHog is not. + */ + +export interface HangStackFrame { + functionName: string; + url: string; + lineNumber: number; + columnNumber: number; +} + +/** Deepest frames are the least useful; keep the top of the stack. */ +export const HANG_STACK_MAX_FRAMES = 20; + +/** + * Rebuild frames from untrusted input, keeping only the four scalar fields. + * + * Everything else is dropped by construction — notably `scopeChain` and + * `this`, whose object handles can be dereferenced into live user data, and + * any key a future protocol version or a future writer might add. Returns null + * when there is nothing usable, so callers can omit the field entirely rather + * than ship an empty array. + */ +export function sanitizeHangStackFrames( + rawFrames: unknown, + maxFrames: number = HANG_STACK_MAX_FRAMES, +): HangStackFrame[] | null { + if (!Array.isArray(rawFrames) || rawFrames.length === 0) return null; + + const frames: HangStackFrame[] = []; + for (const raw of rawFrames.slice(0, maxFrames)) { + if (!raw || typeof raw !== "object") continue; + const frame = raw as Record; + // CDP nests the position under `location`; a breadcrumb frame has already + // been flattened. Accept both so a re-sanitize is lossless. + const location = (frame.location ?? frame) as Record; + frames.push({ + functionName: + typeof frame.functionName === "string" && frame.functionName + ? frame.functionName + : "(anonymous)", + url: redactFrameUrl(frame.url), + lineNumber: toNumber(location.lineNumber), + columnNumber: toNumber(location.columnNumber), + }); + } + return frames.length > 0 ? frames : null; +} + +/** + * Keep only the bundle-relative tail of a script URL. The full value is a + * `file://` path into the install location and can carry the OS username. + * Idempotent, so running it again on an already-reduced value is a no-op — + * which is what lets the egress side re-apply it to a frame of unknown origin. + */ +export function redactFrameUrl(url: unknown): string { + if (typeof url !== "string" || !url) return ""; + const [withoutQuery = ""] = url.split(/[?#]/); + const segments = withoutQuery.split("/").filter(Boolean); + if (segments.length === 0) return ""; + return segments.slice(-2).join("/"); +} + +function toNumber(value: unknown): number { + return typeof value === "number" && Number.isFinite(value) ? value : 0; +}