diff --git a/src/main/host/deferred-secret-protection-report.test.ts b/src/main/host/deferred-secret-protection-report.test.ts index 67ee0eb755b..ce0a301e433 100644 --- a/src/main/host/deferred-secret-protection-report.test.ts +++ b/src/main/host/deferred-secret-protection-report.test.ts @@ -2,7 +2,7 @@ import { mkdtempSync, rmSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -import { setSecretStore } from '../../shared/secret-store' +import { _resetSecretStoreForTests, setSecretStore } from '../../shared/secret-store' type Listener = (...args: unknown[]) => void @@ -69,6 +69,7 @@ describe('scheduleSecretProtectionGapReport', () => { afterEach(() => { vi.useRealTimers() + _resetSecretStoreForTests() rmSync(dir, { recursive: true, force: true }) }) @@ -126,12 +127,38 @@ describe('scheduleSecretProtectionGapReport', () => { schedule() const window = createWindow() window.reveal() + drain() + // Why assert the timer is gone and not just the probe count: the once-guard alone makes + // the count right, so without this the fallback could stay armed for 15s past the reveal + // and nothing here would notice. + expect(vi.getTimerCount()).toBe(0) vi.runAllTimers() vi.advanceTimersByTime(60_000) vi.runAllTimers() expect(probes).toBe(1) }) + it('does not let the fallback timer hold the process open', () => { + // Why: the report is diagnostics; a referenced 15s timer would keep a quitting app alive + // for up to 15s after its last window closed. + const timers: ReturnType[] = [] + const original = globalThis.setTimeout + const spy = vi.spyOn(globalThis, 'setTimeout').mockImplementation((( + ...args: Parameters + ) => { + const timer = original(...args) + timers.push(timer) + return timer + }) as typeof setTimeout) + try { + schedule() + } finally { + spy.mockRestore() + } + expect(timers).toHaveLength(1) + expect(timers[0]?.hasRef()).toBe(false) + }) + it('does not probe again when the window reveals after the fallback already reported', () => { // Why this order and not the reverse: a GPU that presents late reveals the window // after the fallback has already reported, and a second blocking keyring probe would @@ -146,6 +173,26 @@ describe('scheduleSecretProtectionGapReport', () => { expect(probes).toBe(1) }) + it('does not turn a throwing report into a fatal main-process error', () => { + // Why: deferred, this runs off whenReady's promise chain, where a throw is an + // uncaughtException the main-process guard re-throws fatally (#9441) — not the + // unhandled rejection the app survives. A diagnostic must never end the process. + let thrown = 0 + scheduleSecretProtectionGapReport({ + dataFile, + log: () => { + thrown += 1 + throw new Error('log sink is broken') + }, + deferUntilFirstWindow: true + }) + const window = createWindow() + window.reveal() + expect(() => drain()).not.toThrow() + expect(probes).toBe(1) + expect(thrown).toBeGreaterThan(0) + }) + it('reports inline in headless serve, before the runtime is advertised as ready', () => { // Why not deferred: serve opens no window, so the fallback would fire only after // clients could already have paired, and a frozen main thread reads as a dead host. diff --git a/src/main/host/deferred-secret-protection-report.ts b/src/main/host/deferred-secret-protection-report.ts index 01a7277ec61..4b12ad7e563 100644 --- a/src/main/host/deferred-secret-protection-report.ts +++ b/src/main/host/deferred-secret-protection-report.ts @@ -12,6 +12,12 @@ import { reportSecretProtectionGap } from './secret-protection-report' * (STA-5765). Run before the first window, that reads to the user as "the app will not * open"; the window is gated behind a diagnostic whose result nothing on the startup * path consumes. + * + * Why every platform and not just Linux: deferring costs nothing here — no caller reads the + * result, so the only thing that moves is when a console line appears. macOS pays the same + * shape against the Keychain, and a probe that has to answer before a window exists is wrong + * on any host. A deferral that withholds a real secret has to be scoped to the platform that + * needs it; this one withholds nothing. */ // Why a fallback as well as the window event: `ready-to-show` can fail to fire at all @@ -40,6 +46,25 @@ export function scheduleSecretProtectionGapReport({ return } + // Why swallow here and not in reportSecretProtectionGap: deferred, this no longer runs on + // whenReady's promise chain, where a throw was an unhandled rejection the app survives + // (#9441). Off that chain it is an uncaughtException, and installUncaughtPipeErrorGuard + // re-throws those fatally — killing the app over a diagnostic the module documents as + // deliberately not fatal. Serve still reports inline and keeps the old posture. + const report = (): void => { + try { + reportSecretProtectionGap(options) + } catch (error) { + try { + ;(options.log ?? console.warn)( + `[secrets] could not run the deferred protection report: ${String(error)}` + ) + } catch { + // A throwing log sink must not be what ends the process either. + } + } + } + let ran = false const run = (): void => { if (ran) { @@ -49,9 +74,7 @@ export function scheduleSecretProtectionGapReport({ clearTimeout(fallback) // Why setImmediate: keep the blocking keyring probe off the event handler that // reveals the window, so the reveal paints first. - setImmediate(() => { - reportSecretProtectionGap(options) - }) + setImmediate(report) } const fallback = setTimeout(run, REPORT_FALLBACK_MS) diff --git a/src/main/startup/secret-protection-report-deferral-wiring.test.ts b/src/main/startup/secret-protection-report-deferral-wiring.test.ts new file mode 100644 index 00000000000..4f0d45c2b97 --- /dev/null +++ b/src/main/startup/secret-protection-report-deferral-wiring.test.ts @@ -0,0 +1,63 @@ +import { readFileSync } from 'node:fs' +import { join } from 'node:path' +import { describe, expect, it } from 'vitest' + +/** + * Guards the one line that decides whether the OS keyring gates the first window. + * + * The report is diagnostics nothing on the startup path reads, but `describeProtectionGap()` + * is a blocking D-Bus round trip on Linux, and a present-but-locked keyring never answers — + * 76s to first window on Ubuntu 24.04 (STA-5765). Both directions of the one flag here are + * silent: `true` gives headless serve a deferral it must never have (serve opens no window, + * so only the fallback fires, after clients may have paired, and a frozen main thread reads + * as a dead host); `false` puts the blocking probe back in front of the window. Deleting the + * call entirely restores the original regression. + * + * Source-level because that is the property: this runs once inside `app.whenReady()` during + * startup, so there is no seam to assert against at runtime. + */ +describe('secret protection report deferral wiring', () => { + const source = readFileSync(join(process.cwd(), 'src/main/index.ts'), 'utf8') + + const SCHEDULE = 'scheduleSecretProtectionGapReport({' + + it('arms the deferred report exactly once and never calls the blocking one directly', () => { + expect(source.split(SCHEDULE).length - 1, `${SCHEDULE} should appear exactly once`).toBe(1) + expect(source).toContain( + "import { scheduleSecretProtectionGapReport } from './host/deferred-secret-protection-report'" + ) + // Why also assert the absence: re-importing the blocking entry point reinstates the + // pre-window probe without touching the call site the next test pins. Note the scheduling + // name is `...GapReport(`, so it does not match this substring. + expect(source.split('reportSecretProtectionGap(').length - 1).toBe(0) + }) + + it('defers on desktop and reports inline in headless serve', () => { + const start = source.indexOf(SCHEDULE) + // Why bound every anchor: an unresolved one is -1, and the slice below would then run to + // EOF and pass against unrelated code. + expect(start).toBeGreaterThanOrEqual(0) + const end = source.indexOf('\n })', start) + expect(end).toBeGreaterThan(start) + const call = source.slice(start, end) + + expect(call).toContain('deferUntilFirstWindow: !isServeMode') + // Why assert the constants are absent too: `!isServeMode` being present does not stop a + // later property in the same literal from overriding it. + expect(call).not.toContain('deferUntilFirstWindow: true') + expect(call).not.toContain('deferUntilFirstWindow: false') + }) + + it('arms the report after the profile exists and inside app readiness', () => { + // Why: the report remembers what it last said beside the profile data file, so arming it + // before the profile is resolved would key the state off a path that does not exist yet. + // Anchored on code, never a comment — a reworded comment silently becomes -1. + const ready = source.indexOf('app.whenReady().then(') + const profile = source.indexOf('const activeOrcaProfile = ensureActiveOrcaProfile()') + const schedule = source.indexOf(SCHEDULE) + + expect(ready).toBeGreaterThanOrEqual(0) + expect(profile).toBeGreaterThan(ready) + expect(schedule).toBeGreaterThan(profile) + }) +})