From 064de661dacbe1db20dac546149254e85fc6aa87 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 17 Aug 2026 22:15:46 -0700 Subject: [PATCH] fix(crash-reporting): evaluate the ACL signature per target and flag locale risk Three readiness-review findings: - the signature was merged across targets, so a grant on the directory masked its absence on the module file - the exact per-file state the probe exists to detect - the well-known package name check is English-only and icacls localizes it, so a non-English box could false-positive silently; report whether the check could be trusted - the serve-mode test omitted platform, so the gate was never exercised Also switch to the durable recorder (this runs after initObservability, so the span lands in the diagnostics bundle) and correct two comments that misstated where the probe runs. --- .../windows-install-dir-acl-probe.test.ts | 36 ++++++++++++++++- .../startup/windows-install-dir-acl-probe.ts | 39 +++++++++++++------ 2 files changed, 63 insertions(+), 12 deletions(-) diff --git a/src/main/startup/windows-install-dir-acl-probe.test.ts b/src/main/startup/windows-install-dir-acl-probe.test.ts index e36c6e6a548..367c42615a7 100644 --- a/src/main/startup/windows-install-dir-acl-probe.test.ts +++ b/src/main/startup/windows-install-dir-acl-probe.test.ts @@ -122,6 +122,40 @@ describe('probeWindowsInstallDirAcl', () => { expect(data.matchesPoisonSignature).toBe(true) }) + it('does not let a grant on one target mask its absence on another', async () => { + const data = await probe({ + spawnFn: fakeSpawn((target) => + target.endsWith('ffmpeg.dll') + ? dacl(target, ORPHAN) + : dacl(target, RESTRICTED_GRANT, ORPHAN) + ).spawnFn + }) + expect(data.hasWellKnownPackageGrant).toBe(true) + expect(data.matchesPoisonSignature).toBe(true) + }) + + it('reports whether the well-known name check could be trusted', async () => { + const english = await probeWith(ORPHAN) + expect(english.wellKnownNameCheckReliable).toBe(true) + const localized = await new Promise>((resolve) => { + resetWindowsInstallDirAclProbeForTest() + probeWindowsInstallDirAcl({ + platform: 'win32', + installDir: INSTALL_DIR, + fileExists: () => false, + spawnFn: fakeSpawn( + (target) => `${target} ${ORPHAN}\r\n AUTORITE NT\\Systeme:(I)(F)` + ).spawnFn, + recordBreadcrumb: (_name, d) => { + resolve(d as Record) + return undefined + } + }) + }) + expect(localized.matchesPoisonSignature).toBe(true) + expect(localized.wellKnownNameCheckReliable).toBe(false) + }) + it('matches the well-known SIDs exactly, not by prefix', async () => { const data = await probeWith('S-1-15-2-1234567890:(OI)(CI)(RX)') expect(data.orphanPackageSidCount).toBe(1) @@ -159,7 +193,7 @@ describe('probeWindowsInstallDirAcl', () => { it.each([ ['darwin', { platform: 'darwin' as NodeJS.Platform }], - ['serve mode', { isServeMode: true }] + ['serve mode', { platform: 'win32' as NodeJS.Platform, isServeMode: true }] ])('does no work on %s', async (_label, options) => { const fake = fakeSpawn((target) => dacl(target, ORPHAN)) const record = vi.fn() diff --git a/src/main/startup/windows-install-dir-acl-probe.ts b/src/main/startup/windows-install-dir-acl-probe.ts index e37c60d110d..e99f577dbbf 100644 --- a/src/main/startup/windows-install-dir-acl-probe.ts +++ b/src/main/startup/windows-install-dir-acl-probe.ts @@ -5,7 +5,7 @@ import { sanitizeCrashReportString, type CrashReportBreadcrumbData } from '../../shared/crash-reporting' -import { recordCrashBreadcrumb } from '../crash-reporting/crash-breadcrumb-store' +import { recordDurableCrashBreadcrumb } from '../crash-reporting/durable-crash-breadcrumb' import { getIcaclsExePath } from '../win32-utils' /** @@ -33,8 +33,12 @@ const PROBE_BUDGET_MS = 5_000 /** Raw S-1-15-2-* means icacls could not resolve it — locale-independent. */ const RAW_PACKAGE_SID = /\bS-1-15-2-[0-9-]+\b/i const WELL_KNOWN_PACKAGE_SIDS = new Set(['s-1-15-2-1', 's-1-15-2-2']) -// icacls localizes these; the raw SID form is never printed for them. +// icacls localizes these; the raw SID form is never printed for them. Orphan +// detection stays SID-form and locale-independent, but this check is not — so we +// report whether the output looked English at all, making a locale-induced +// false positive recognizable instead of silent. const WELL_KNOWN_PACKAGE_NAMES = /ALL (RESTRICTED )?APPLICATION PACKAGES/i +const ENGLISH_PRINCIPAL = /\b(NT AUTHORITY|BUILTIN|APPLICATION PACKAGE AUTHORITY)\b/i // Why: an ACE that denies, or only propagates to children, grants nothing on this // object — so it cannot satisfy an orphan the way the reproduced fix did. const NON_GRANTING_FLAGS = /\((?:DENY|IO)\)/i @@ -46,13 +50,14 @@ export type WindowsInstallDirAclProbeOptions = { /** Test seams. */ spawnFn?: typeof spawn fileExists?: (path: string) => boolean - recordBreadcrumb?: typeof recordCrashBreadcrumb + recordBreadcrumb?: typeof recordDurableCrashBreadcrumb onDone?: (data: CrashReportBreadcrumbData) => void } type AclFacts = { orphanPackageSids: string[] hasWellKnownPackageGrant: boolean + sawEnglishPrincipal: boolean } function readDacl(spawnFn: typeof spawn, target: string, deadlineMs: number): Promise { @@ -91,7 +96,11 @@ function readDacl(spawnFn: typeof spawn, target: string, deadlineMs: number): Pr function collectAclFacts(daclOutput: string): AclFacts { const orphanPackageSids: string[] = [] let hasWellKnownPackageGrant = false + let sawEnglishPrincipal = false for (const line of daclOutput.split(/\r?\n/)) { + if (ENGLISH_PRINCIPAL.test(line)) { + sawEnglishPrincipal = true + } // icacls glues the echoed path onto the first principal with no separator, so // match the principal:(flags) tail rather than trying to split the line. const ace = /([^\s:][^:]*):(\([^\s]*\))\s*$/.exec(line.trim()) @@ -109,7 +118,7 @@ function collectAclFacts(daclOutput: string): AclFacts { hasWellKnownPackageGrant = true } } - return { orphanPackageSids, hasWellKnownPackageGrant } + return { orphanPackageSids, hasWellKnownPackageGrant, sawEnglishPrincipal } } function resolveTargets(installDir: string, fileExists: (path: string) => boolean): string[] { @@ -118,7 +127,7 @@ function resolveTargets(installDir: string, fileExists: (path: string) => boolea } async function runProbe(options: WindowsInstallDirAclProbeOptions): Promise { - const record = options.recordBreadcrumb ?? recordCrashBreadcrumb + const record = options.recordBreadcrumb ?? recordDurableCrashBreadcrumb let data: CrashReportBreadcrumbData try { const installDir = options.installDir ?? dirname(process.execPath) @@ -134,6 +143,12 @@ async function runProbe(options: WindowsInstallDirAclProbeOptions): Promise f.orphanPackageSids))] const hasWellKnownPackageGrant = facts.some((f) => f.hasWellKnownPackageGrant) + // Why per target: a grant on the directory does not grant on the module file, + // and the reproduced failure is a per-file content read. Merging would let a + // grant on one target mask its absence on the other. + const poisoned = facts.some( + (f) => f.orphanPackageSids.length > 0 && !f.hasWellKnownPackageGrant + ) data = outputs.every((out) => out === '') ? { status: 'failed', reason: 'all-targets-unreadable' } : { @@ -144,7 +159,10 @@ async function runProbe(options: WindowsInstallDirAclProbeOptions): Promise 0 && !hasWellKnownPackageGrant + // False positives are possible on a non-English Windows, where the + // well-known ACE resolves to a localized name this cannot match. + wellKnownNameCheckReliable: facts.some((f) => f.sawEnglishPrincipal), + matchesPoisonSignature: poisoned } } catch (error) { data = { status: 'failed', reason: sanitizeCrashReportString(`probe: ${String(error)}`, 200) } @@ -163,8 +181,8 @@ export function resetWindowsInstallDirAclProbeForTest(): void { /** * Fire-and-forget; returns before any spawn. win32 only — no spawn and no fs I/O - * anywhere else. Records into the retained breadcrumb ring, which is what - * process-gone-recorder snapshots into every crash report. + * anywhere else. Called from openMainWindow, which runs after initObservability, + * so the durable record also emits a span into the diagnostics bundle. */ export function probeWindowsInstallDirAcl(options: WindowsInstallDirAclProbeOptions = {}): void { if ((options.platform ?? process.platform) !== 'win32' || options.isServeMode === true) { @@ -174,9 +192,8 @@ export function probeWindowsInstallDirAcl(options: WindowsInstallDirAclProbeOpti return } probeStarted = true - // Why the try: this runs at the caller's module scope before app.whenReady, so - // anything thrown here would abort main-process init and no window would ever - // be created. A diagnostic must never be able to do that. + // Why the try: this runs inline in openMainWindow, so anything thrown here + // propagates into window creation. A diagnostic must never be able to do that. try { setImmediate(() => { void runProbe(options).catch(() => undefined)