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; +}