diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 38daf8a71de..878b7e3e591 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -867,6 +867,8 @@ jobs: src/main/runtime/repo-worktree-admin-fingerprint.test.ts src/main/runtime/worktree-scan-admin-fingerprint-gate.test.ts src/shared/secure-file-fsync-flags.test.ts + src/shared/secure-path-windows-acl.win32.test.ts + src/main/runtime/unreadable-secret-store-preservation.win32.test.ts src/main/ipc/pty-codex-account-attribution.test.ts src/main/ipc/pty-spawn-env-codex-resume-provenance.test.ts src/relay/windows-port-scan.win32.test.ts diff --git a/config/scripts/pr-code-change-scope.mjs b/config/scripts/pr-code-change-scope.mjs index f83c1508c26..c99e058891f 100644 --- a/config/scripts/pr-code-change-scope.mjs +++ b/config/scripts/pr-code-change-scope.mjs @@ -237,6 +237,8 @@ const WINDOWS_PACKAGE_TESTS = [ 'src/main/runtime/repo-worktree-admin-fingerprint.test.ts', 'src/main/runtime/worktree-scan-admin-fingerprint-gate.test.ts', 'src/shared/secure-file-fsync-flags.test.ts', + 'src/shared/secure-path-windows-acl.win32.test.ts', + 'src/main/runtime/unreadable-secret-store-preservation.win32.test.ts', 'src/main/ipc/pty-codex-account-attribution.test.ts', 'src/main/ipc/pty-spawn-env-codex-resume-provenance.test.ts', 'src/relay/windows-port-scan.win32.test.ts' diff --git a/src/main/artifacts/artifact-create-intent-store.test.ts b/src/main/artifacts/artifact-create-intent-store.test.ts index f512f5ab439..08b0e03e374 100644 --- a/src/main/artifacts/artifact-create-intent-store.test.ts +++ b/src/main/artifacts/artifact-create-intent-store.test.ts @@ -1,4 +1,3 @@ -import { execFileSync } from 'node:child_process' import { mkdtemp, readFile, readdir, rm, stat, truncate, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' @@ -16,9 +15,14 @@ import { getOrCreateArtifactCreateIntent, removeArtifactCreateIntent } from './artifact-create-intent-store' +import { runProcessSync } from '../../shared/child-process/run-process' +import { __resetSecureFileWindowsUserSidForTests } from '../../shared/secure-file' import type { ArtifactShareScope } from './artifact-share-record-store' -vi.mock('node:child_process', () => ({ execFile: vi.fn(), execFileSync: vi.fn() })) +vi.mock('../../shared/child-process/run-process', () => ({ + runProcess: vi.fn(), + runProcessSync: vi.fn() +})) const createdPaths: string[] = [] const scope: ArtifactShareScope = { @@ -168,12 +172,27 @@ describe('artifact create intent store', () => { expect((await readdir(directory)).some((name) => name.endsWith('.tmp'))).toBe(false) }) - it('hardens one Windows journal directory without per-file PowerShell launches', async () => { + it('hardens one Windows journal directory without per-file ACL launches', async () => { const originalPlatform = Object.getOwnPropertyDescriptor(process, 'platform') Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) - vi.mocked(execFileSync).mockImplementation((file) => - String(file).endsWith('whoami.exe') ? '"USER","S-1-5-21-1000"' : '' - ) + const ok = { code: 0, signal: null, stdout: '', stderr: '', timedOut: false } + // Earlier cases in this file already resolved (and cached) the SID against an unstubbed mock. + __resetSecureFileWindowsUserSidForTests() + vi.mocked(runProcessSync).mockImplementation((spec) => { + if (spec.program.endsWith('whoami.exe')) { + return { ...ok, stdout: '"USER","S-1-5-21-1000"' } + } + const args = spec.args ?? [] + if (args.length > 1) { + return ok // /reset and the /grant:r pass + } + // The verify pass re-reads the DACL; answer with the three protected inheritable rules. + const rules = ['host\\me', 'NT AUTHORITY\\SYSTEM', 'BUILTIN\\Administrators'].map( + (name, index) => + index === 0 ? `${args[0]} ${name}:(OI)(CI)(F)` : ` ${name}:(OI)(CI)(F)` + ) + return { ...ok, stdout: `${rules.join('\r\n')}\r\n\r\nSuccessfully processed 1 files\r\n` } + }) try { const userDataPath = await createUserDataPath() getOrCreateArtifactCreateIntent( @@ -193,16 +212,20 @@ describe('artifact create intent store', () => { body ) - const powershellCalls = vi - .mocked(execFileSync) - .mock.calls.filter(([file]) => String(file).endsWith('powershell.exe')) - expect(powershellCalls).toHaveLength(1) - expect((powershellCalls[0]![1] as string[]).at(-1)).toBe('1') + // One harden across both intents: counted by its /reset pass, which opens each harden. + const aclCalls = vi + .mocked(runProcessSync) + .mock.calls.map(([spec]) => spec) + .filter((spec) => spec.program.endsWith('icacls.exe')) + expect(aclCalls.filter((spec) => spec.args?.includes('/reset'))).toHaveLength(1) + // The child intent files rely on inheritance, so the directory rules must carry (OI)(CI). + const grant = aclCalls.find((spec) => spec.args?.includes('/grant:r')) + expect(grant?.args?.filter((arg) => arg.endsWith(':(OI)(CI)(F)'))).toHaveLength(3) } finally { if (originalPlatform) { Object.defineProperty(process, 'platform', originalPlatform) } - vi.mocked(execFileSync).mockReset() + vi.mocked(runProcessSync).mockReset() } }) diff --git a/src/main/observability/index.ts b/src/main/observability/index.ts index f891da51418..74728a2d81b 100644 --- a/src/main/observability/index.ts +++ b/src/main/observability/index.ts @@ -48,7 +48,8 @@ import { type UploadBundleOptions, type UploadBundleResult } from './diagnostic-bundle-upload' -import { setActiveSink } from './tracer' +import { setActiveSink, startSpan } from './tracer' +import { setSecurePathHardeningReporter } from '../../shared/secure-path-hardening-report' const CI_ENV_VARS = [ 'CI', @@ -153,10 +154,37 @@ export function initObservability(): ObservabilityConsent { return c } installLocalSink() + installSecurePathHardeningReporter() return c } +/** + * Why route it here: Windows path hardening lives in `src/shared` and defaults to `console.warn`, + * which reaches nothing in a packaged build — the main process is GUI-subsystem and owns no + * console. A credential file left on inherited ACLs is exactly what a diagnostic bundle should + * show, so it becomes a span in the trace sink. + * + * `recovered` ends successfully rather than failing: a host that climbs back out of the + * rate-limited state has to be as visible as one that fell into it, or the degraded state is only + * ever half-diagnosable. + */ +function installSecurePathHardeningReporter(): void { + setSecurePathHardeningReporter((entry) => { + const span = startSpan('secure-path.windows-acl', { + attributes: { targetPath: entry.targetPath, stage: entry.stage, detail: entry.detail } + }) + if (entry.stage === 'recovered') { + span.end() + console.info('[secure-path.windows-acl] path hardening recovered', entry) + return + } + span.fail(entry.detail) + console.warn('[secure-path.windows-acl] failed to restrict path', entry) + }) +} + export async function shutdownObservability(): Promise { + setSecurePathHardeningReporter(null) // Order matters: tracer first so no new pushes arrive while the local sink // is closing and flushing buffered lines. setActiveSink(null) diff --git a/src/main/orca-profiles/profile-cloud-auth-status.ts b/src/main/orca-profiles/profile-cloud-auth-status.ts index 400498d223e..b0eb7da1978 100644 --- a/src/main/orca-profiles/profile-cloud-auth-status.ts +++ b/src/main/orca-profiles/profile-cloud-auth-status.ts @@ -29,7 +29,10 @@ export function getOrcaProfileAuthStatusFromProfile( state: 'unconfigured', persistence: session.status === 'found' ? session.persistence : 'none', cloud, - credentialError: session.status === 'decrypt-failed' ? session.error : undefined, + credentialError: + session.status === 'decrypt-failed' || session.status === 'unreadable' + ? session.error + : undefined, setupMessage: configState.setupMessage } } @@ -51,6 +54,9 @@ export function getOrcaProfileAuthStatusFromProfile( state: 'reconnect-required', persistence: 'none', cloud, - credentialError: session.status === 'decrypt-failed' ? session.error : undefined + credentialError: + session.status === 'decrypt-failed' || session.status === 'unreadable' + ? session.error + : undefined } } diff --git a/src/main/orca-profiles/profile-cloud-session-refresh.ts b/src/main/orca-profiles/profile-cloud-session-refresh.ts index 221b4908bee..273f71f016e 100644 --- a/src/main/orca-profiles/profile-cloud-session-refresh.ts +++ b/src/main/orca-profiles/profile-cloud-session-refresh.ts @@ -72,6 +72,11 @@ function clearCloudSessionIfUnchanged( if (current.status === 'found' && current.session.refreshToken !== failed.refreshToken) { return } + // A session we were denied is not a session we may delete: the token we would be clearing might + // not even be the one that failed, and `clearOrcaCloudSession` unlinks the file outright. + if (current.status === 'unreadable') { + return + } if (active.profile.cloud) { tombstoneCloudSession( cloudSessionIdentity(active.profile.id, active.profile.cloud), diff --git a/src/main/orca-profiles/profile-cloud-session-store.ts b/src/main/orca-profiles/profile-cloud-session-store.ts index 62d779772c1..d1334d96b6b 100644 --- a/src/main/orca-profiles/profile-cloud-session-store.ts +++ b/src/main/orca-profiles/profile-cloud-session-store.ts @@ -1,7 +1,7 @@ import { existsSync, readFileSync, rmSync } from 'node:fs' import { join } from 'node:path' import { safeStorage } from 'electron' -import { writeSecureJsonFile } from '../../shared/secure-file' +import { isUnreadableError, writeSecureJsonFile } from '../../shared/secure-file' import type { OrcaCloudCapabilities, OrcaCloudOrgSummary, @@ -29,6 +29,11 @@ export type OrcaCloudSessionReadResult = | { status: 'found'; session: OrcaCloudSession; persistence: OrcaCloudSessionPersistence } | { status: 'missing'; persistence: 'none' } | { status: 'decrypt-failed'; persistence: 'none'; error: string } + /** + * The file is there and this process may not read it. Distinct from `decrypt-failed` because + * that one means "read it, it was garbage" and licenses replacing it; this one licenses nothing. + */ + | { status: 'unreadable'; persistence: 'none'; error: string } type PersistedEncryptedSession = { version: 1 @@ -215,7 +220,14 @@ export function readOrcaCloudSession( return { status: 'found', session: parsed.session, persistence: 'dev-plaintext' } } return { status: 'decrypt-failed', persistence: 'none', error: 'Unsafe session format.' } - } catch { + } catch (error) { + if (isUnreadableError(error)) { + return { + status: 'unreadable', + persistence: 'none', + error: 'Cannot read the saved Orca account session: the read failed.' + } + } return { status: 'decrypt-failed', persistence: 'none', diff --git a/src/main/plugins/plugin-secrets-store.ts b/src/main/plugins/plugin-secrets-store.ts index 7f16821cfb6..fb41aba71c8 100644 --- a/src/main/plugins/plugin-secrets-store.ts +++ b/src/main/plugins/plugin-secrets-store.ts @@ -1,7 +1,7 @@ import { existsSync, readFileSync, statSync } from 'node:fs' import { join } from 'node:path' import { safeStorage } from 'electron' -import { writeSecureFile } from '../../shared/secure-file' +import { isUnreadableError, writeSecureFile } from '../../shared/secure-file' import { PLUGIN_STORAGE_KEY_LIMIT, PLUGIN_STORAGE_TOTAL_MAX_BYTES @@ -25,6 +25,8 @@ type PersistedSecretsFile = { export type PluginSecretsResult = { ok: true; value: T } | { ok: false; error: string } +const UNREADABLE_VAULT_ERROR = 'secret vault exists but could not be read; refusing to overwrite it' + export class PluginSecretsStore { private readonly filePath: string @@ -32,7 +34,8 @@ export class PluginSecretsStore { this.filePath = join(pluginDataDir(pluginsDataDir, qualifiedKey), 'secrets.json.enc') } - private read(): PersistedSecretsFile { + /** `null` means the vault exists and this process may not read it — which is never "empty". */ + private read(): PersistedSecretsFile | null { const empty: PersistedSecretsFile = { version: 1, format: 'electron-safe-storage-v1', @@ -56,7 +59,13 @@ export class PluginSecretsStore { ) { return parsed } - } catch { + } catch (error) { + // Being denied the read is not evidence the vault is corrupt. Returning `empty` here would + // make the next set() write a vault containing only that one key, silently dropping every + // secret the file still holds — and the write would succeed. + if (isUnreadableError(error)) { + return null + } // Corrupt vaults read as empty; set() rewrites a valid file. } return empty @@ -64,6 +73,9 @@ export class PluginSecretsStore { get(key: string): PluginSecretsResult { const file = this.read() + if (!file) { + return { ok: false, error: UNREADABLE_VAULT_ERROR } + } const ciphertext = file.ciphertexts[key] if (typeof ciphertext !== 'string') { return { ok: true, value: null } @@ -83,6 +95,9 @@ export class PluginSecretsStore { return { ok: false, error: 'OS-backed encryption is unavailable; secret not stored' } } const file = this.read() + if (!file) { + return { ok: false, error: UNREADABLE_VAULT_ERROR } + } if ( !Object.hasOwn(file.ciphertexts, key) && Object.keys(file.ciphertexts).length >= PLUGIN_STORAGE_KEY_LIMIT @@ -100,6 +115,10 @@ export class PluginSecretsStore { delete(key: string): void { const file = this.read() + if (!file) { + // Rewriting what we could not read would drop every other secret in the vault. + return + } if (Object.hasOwn(file.ciphertexts, key)) { delete file.ciphertexts[key] writeSecureFile(this.filePath, JSON.stringify(file, null, 2)) diff --git a/src/main/plugins/plugin-storage-store.ts b/src/main/plugins/plugin-storage-store.ts index b3949b8f6d4..94a74261e1e 100644 --- a/src/main/plugins/plugin-storage-store.ts +++ b/src/main/plugins/plugin-storage-store.ts @@ -1,6 +1,6 @@ import { existsSync, readFileSync, statSync } from 'node:fs' import { join } from 'node:path' -import { writeSecureFile } from '../../shared/secure-file' +import { isUnreadableError, writeSecureFile } from '../../shared/secure-file' import { isQualifiedPluginKey } from '../../shared/plugins/plugin-manifest' import { PLUGIN_STORAGE_KEY_LIMIT, @@ -16,6 +16,8 @@ import { * Adapted from community PR #5801's per-plugin settings store. */ +const UNREADABLE_STORE_ERROR = 'storage file exists but could not be read; refusing to overwrite it' + export function pluginDataDir(pluginsDataDir: string, qualifiedKey: string): string { if (!isQualifiedPluginKey(qualifiedKey)) { throw new Error(`unsafe plugin key: ${qualifiedKey}`) @@ -36,7 +38,8 @@ export class PluginKvStore { this.filePath = join(pluginDataDir(pluginsDataDir, qualifiedKey), fileName) } - private read(): Record { + /** `null` means the file exists and this process may not read it - which is never `{}`. */ + private read(): Record | null { try { if (!existsSync(this.filePath)) { return {} @@ -48,22 +51,27 @@ export class PluginKvStore { if (parsed && typeof parsed === 'object' && !Array.isArray(parsed)) { return parsed as Record } - } catch { + } catch (error) { + // Being denied the read is not evidence of corruption. Returning `{}` here would make + // the next set()/delete() write a file holding only that one key, dropping the rest. + if (isUnreadableError(error)) { + return null + } // Corrupt files reset to empty rather than wedging the plugin. } return {} } get(key: string): unknown { - return this.read()[key] + return this.read()?.[key] } getAll(): Record { - return this.read() + return this.read() ?? {} } keys(): string[] { - return Object.keys(this.read()) + return Object.keys(this.read() ?? {}) } set(key: string, value: unknown): PluginKvWriteResult { @@ -80,6 +88,9 @@ export class PluginKvStore { return { ok: false, error: `value exceeds ${PLUGIN_STORAGE_VALUE_MAX_BYTES} bytes` } } const settings = this.read() + if (!settings) { + return { ok: false, error: UNREADABLE_STORE_ERROR } + } if (!Object.hasOwn(settings, key) && Object.keys(settings).length >= PLUGIN_STORAGE_KEY_LIMIT) { return { ok: false, error: `storage exceeds the ${PLUGIN_STORAGE_KEY_LIMIT}-key limit` } } @@ -94,6 +105,10 @@ export class PluginKvStore { delete(key: string): void { const settings = this.read() + if (!settings) { + // Rewriting what we could not read would drop every other key in the store. + return + } if (Object.hasOwn(settings, key)) { delete settings[key] writeSecureFile(this.filePath, JSON.stringify(settings, null, 2)) diff --git a/src/main/runtime/device-registry.ts b/src/main/runtime/device-registry.ts index b5f0c7a6c8d..b2d5de8ef41 100644 --- a/src/main/runtime/device-registry.ts +++ b/src/main/runtime/device-registry.ts @@ -5,7 +5,11 @@ import { randomBytes, randomUUID } from 'node:crypto' import { existsSync, readFileSync } from 'node:fs' import { join } from 'node:path' -import { hardenExistingSecureFile, writeSecureJsonFile } from '../../shared/secure-file' +import { + hardenExistingSecureFile, + isUnreadableError, + writeSecureJsonFile +} from '../../shared/secure-file' import type { DeviceScope } from '../../shared/runtime-types' import { DEVICE_REGISTRY_FILENAME } from './mobile-pairing-files' import type { RelayDeviceBinding } from './relay/relay-revoke-outbox' @@ -54,6 +58,8 @@ const LAST_SEEN_FLUSH_DELAY_MS = 250 export class DeviceRegistry { private readonly registryPath: string private devices: DeviceEntry[] = [] + /** Set when the registry exists but could not be read, which makes `devices` a lie to save from. */ + private registryUnreadable = false private pendingLastSeenFlush: NodeJS.Timeout | null = null constructor(userDataPath: string) { @@ -293,12 +299,21 @@ export class DeviceRegistry { // LAN links), so a missing value must keep binding every interface on reconnect. pairingReach: device.pairingReach === 'this-computer' ? 'this-computer' : 'network' })) - } catch { + this.registryUnreadable = false + } catch (error) { + // "Cannot read" is not "is empty". Saving an empty list over a registry we were merely + // denied would erase every paired device's bearer token, and the write would succeed. + this.registryUnreadable = isUnreadableError(error) this.devices = [] } } private save(devices: DeviceEntry[]): void { + if (this.registryUnreadable) { + throw new Error( + `Cannot read the device registry at ${this.registryPath}: the read failed. Refusing to overwrite it, which would revoke every paired device.` + ) + } writeSecureJsonFile(this.registryPath, devices) // Why: every registry save includes the latest in-memory timestamps, so a later timer would rewrite it. this.cancelPendingLastSeenFlush() diff --git a/src/main/runtime/e2ee-keypair.ts b/src/main/runtime/e2ee-keypair.ts index 3e8b3e0aa0a..9ffb9844e86 100644 --- a/src/main/runtime/e2ee-keypair.ts +++ b/src/main/runtime/e2ee-keypair.ts @@ -4,7 +4,11 @@ import { existsSync, readFileSync, statSync } from 'node:fs' import { join } from 'node:path' import nacl from 'tweetnacl' -import { hardenExistingSecureFile, writeSecureJsonFile } from '../../shared/secure-file' +import { + hardenExistingSecureFile, + isUnreadableError, + writeSecureJsonFile +} from '../../shared/secure-file' import { E2EE_KEYPAIR_FILENAME } from './mobile-pairing-files' const KEYPAIR_FILENAME = E2EE_KEYPAIR_FILENAME @@ -42,7 +46,17 @@ export function loadOrCreateE2EEKeypair(userDataPath: string): E2EEKeypair { return { publicKey, secretKey, publicKeyB64: raw.publicKeyB64 } } } - } catch { + } catch (error) { + // A read this process is not permitted to make says nothing about the contents. Falling + // through would overwrite the only copy of the secret key — and the overwrite succeeds, so + // nothing downstream stops it. Every paired device derives its shared secret from this key, + // so regenerating silently un-pairs all of them and no old message stays decryptable. + if (isUnreadableError(error)) { + throw new Error( + `Cannot read the E2EE keypair at ${filePath}: the read failed. Refusing to regenerate it, which would invalidate every paired device.`, + { cause: error } + ) + } // Malformed file — regenerate below. } } diff --git a/src/main/runtime/relay/relay-revoke-outbox.ts b/src/main/runtime/relay/relay-revoke-outbox.ts index 473f619e134..5989af0c283 100644 --- a/src/main/runtime/relay/relay-revoke-outbox.ts +++ b/src/main/runtime/relay/relay-revoke-outbox.ts @@ -1,7 +1,11 @@ import { randomUUID } from 'node:crypto' import { existsSync, readFileSync } from 'node:fs' import { join } from 'node:path' -import { hardenExistingSecureFile, writeSecureJsonFile } from '../../../shared/secure-file' +import { + hardenExistingSecureFile, + isUnreadableError, + writeSecureJsonFile +} from '../../../shared/secure-file' export type RelayDeviceBinding = { relayHostId: string @@ -37,6 +41,8 @@ function isItem(value: unknown): value is RelayRevokeOutboxItem { export class RelayRevokeOutbox { private readonly path: string private items: RelayRevokeOutboxItem[] + /** Set when the outbox exists but could not be read, so `items` is not what is on disk. */ + private outboxUnreadable = false constructor(userDataPath: string) { this.path = join(userDataPath, OUTBOX_FILENAME) @@ -83,12 +89,20 @@ export class RelayRevokeOutbox { hardenExistingSecureFile(this.path) const parsed: unknown = JSON.parse(readFileSync(this.path, 'utf-8')) return Array.isArray(parsed) ? parsed.filter(isItem) : [] - } catch { + } catch (error) { + // An outbox we were denied is not an empty outbox. Saving [] over it would drop + // revocations that have not reached the relay, so a revoked device stays live. + this.outboxUnreadable = isUnreadableError(error) return [] } } private save(items: readonly RelayRevokeOutboxItem[]): void { + if (this.outboxUnreadable) { + throw new Error( + `Cannot read the relay revoke outbox at ${this.path}: the read failed. Refusing to overwrite it, which would drop pending revocations.` + ) + } writeSecureJsonFile(this.path, items) } } diff --git a/src/main/runtime/rpc/mobile-auth-acl-critical-path.test.ts b/src/main/runtime/rpc/mobile-auth-acl-critical-path.test.ts index 35225f72f9c..b5589aa7724 100644 --- a/src/main/runtime/rpc/mobile-auth-acl-critical-path.test.ts +++ b/src/main/runtime/rpc/mobile-auth-acl-critical-path.test.ts @@ -1,26 +1,71 @@ // Why: the E2EE auth handshake used to persist `lastSeenAt` inline, and on Windows every secure-file -// write blocks the main thread on two synchronous PowerShell ACL spawns (~1-1.5s cold each). These tests +// write blocks the main thread on synchronous icacls ACL spawns (~1-1.5s cold each). These tests // pin the spawn count on the auth critical path, not wall-clock, so they are deterministic under load. -import { execFile, execFileSync } from 'node:child_process' -import { mkdtempSync, readFileSync, rmSync } from 'node:fs' +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { WebSocket } from 'ws' +import type { ProcessResult, ProcessSpec } from '../../../shared/child-process/run-process' +import { runProcess, runProcessSync } from '../../../shared/child-process/run-process' import { DEVICE_REGISTRY_FILENAME } from '../mobile-pairing-files' import { DeviceRegistry, type DeviceEntry } from '../device-registry' import { decrypt, deriveSharedKey, encrypt, generateKeyPair } from './e2ee-crypto' import { MobileSocketWiring, type MobileSocketTransport } from './mobile-socket-wiring' -vi.mock('node:child_process', () => ({ - execFileSync: vi.fn(), - execFile: vi.fn() +// Why this module and not `node:child_process`: hardening reaches the OS only through +// runProcess/runProcessSync, and a hand-written child_process factory silently omitted the one +// function they call — so every spawn threw, hardening no-opped, and the test double hid it. +vi.mock('../../../shared/child-process/run-process', () => ({ + runProcess: vi.fn(), + runProcessSync: vi.fn() })) -// Why: stands in for the PowerShell cold start; long enough that a gated response would be obvious, +// Why: stands in for the icacls cold start; long enough that a gated response would be obvious, // short enough that the suite stays fast. Assertions use the recorded ordering, never this number. const INJECTED_SPAWN_LATENCY_MS = 5 -const POWERSHELL = 'C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe' +const USER_SID = 'S-1-5-21-1000' +const OK: ProcessResult = { code: 0, signal: null, stdout: '', stderr: '', timedOut: false } +/** + * One secure write hardens two paths: the staged temp file, fresh and still on the inherited DACL, + * costs the full verify/reset/grant/verify pass; the published file, whose protected DACL came + * along with the rename, costs only its verify. + */ +const BLOCKING_SPAWNS_PER_WRITE = 5 + +/** Paths the fake icacls has granted a protected DACL, keyed to the ACE flags the grant used. */ +const hardenedByFake = new Map() + +/** + * Stands in for icacls. `/save` really writes a UTF-16LE SDDL file, because the code under test + * reads that file back off disk to decide whether a rewrite is needed at all — which is what makes + * the spawn count per write a property of the real ACL path rather than of this double. + */ +function fakeIcacls(spec: ProcessSpec): ProcessResult { + const args = spec.args ?? [] + const path = args[0] ?? '' + const grantIndex = args.indexOf('/grant:r') + if (grantIndex !== -1) { + hardenedByFake.set(path, args[grantIndex + 1]!.includes('(OI)(CI)') ? 'OICI' : '') + return OK + } + const saveIndex = args.indexOf('/save') + if (saveIndex === -1) { + return OK // /reset + } + writeFileSync(args[saveIndex + 1]!, fakeSddl(path), 'utf16le') + return OK +} + +function fakeSddl(path: string): string { + const aceFlags = hardenedByFake.get(path) + if (aceFlags === undefined) { + // Never hardened: the inherited DACL a fresh file carries, so the first verify must fail. + return `name\r\nD:(A;ID;FA;;;SY)(A;ID;FA;;;BA)(A;ID;FA;;;${USER_SID})\r\n` + } + const ace = (sid: string): string => `(A;${aceFlags};FA;;;${sid})` + return `name\r\nD:PAI${ace('BA')}${ace('SY')}${ace(USER_SID)}\r\n` +} type TimelineEntry = 'acl-spawn' | 'e2ee_ready' | 'e2ee_authenticated' | 'other-frame' @@ -62,24 +107,23 @@ describe('mobile auth critical path', () => { process.env.SystemRoot = 'C:\\Windows' Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) userDataPath = mkdtempSync(join(tmpdir(), 'orca-auth-acl-')) - vi.mocked(execFileSync).mockReset() - vi.mocked(execFile).mockReset() - vi.mocked(execFileSync).mockImplementation((file) => { - if (String(file).endsWith('whoami.exe')) { - return '"USER","S-1-5-21-1000"' + hardenedByFake.clear() + vi.mocked(runProcessSync).mockReset() + vi.mocked(runProcess).mockReset() + vi.mocked(runProcessSync).mockImplementation((spec) => { + // Matched by suffix, not by the whole path: `windowsSystem32Binary` joins with the host + // separator, so the literal only matches when the tests happen to run on Windows. + if (spec.program.endsWith('whoami.exe')) { + return { ...OK, stdout: `"USER","${USER_SID}"` } } timeline.push('acl-spawn') // Why: the real spawn blocks the main thread, so the fake must too — and via Atomics, not a // Date.now() spin, which would never terminate under fake timers. Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, INJECTED_SPAWN_LATENCY_MS) - return '' - }) - vi.mocked(execFile).mockImplementation((_file, _args, _options, callback) => { - if (typeof callback === 'function') { - callback(null, '', '') - } - return {} as ReturnType + return fakeIcacls(spec) }) + // The directory harden stays on the async lane, so it never lands on the timeline. + vi.mocked(runProcess).mockImplementation((spec) => Promise.resolve(fakeIcacls(spec))) }) afterEach(() => { @@ -157,8 +201,11 @@ describe('mobile auth critical path', () => { registry.flushPendingLastSeen() // Hardening is deferred, never dropped: tmp file + published file, exactly as the inline path did. - expect(timeline.filter((entry) => entry === 'acl-spawn')).toHaveLength(2) - expect(vi.mocked(execFileSync).mock.lastCall?.[0]).toBe(POWERSHELL) + expect(timeline.filter((entry) => entry === 'acl-spawn')).toHaveLength( + BLOCKING_SPAWNS_PER_WRITE + ) + // The blocking work is the ACL tool itself, not some other spawn on the same lane. + expect(vi.mocked(runProcessSync).mock.lastCall?.[0].program).toMatch(/icacls\.exe$/) expect(readPersistedDevices()[0]?.lastSeenAt).toBe( registry.getDevice(device.deviceId)?.lastSeenAt ) @@ -172,7 +219,11 @@ describe('mobile auth critical path', () => { authenticate(registry, device) // Why: rotatePendingDevice drops entries disk says were never scanned, so this write stays inline. - expect(timeline).toEqual(['e2ee_ready', 'acl-spawn', 'acl-spawn', 'e2ee_authenticated']) + expect(timeline).toEqual([ + 'e2ee_ready', + ...Array(BLOCKING_SPAWNS_PER_WRITE).fill('acl-spawn'), + 'e2ee_authenticated' + ]) expect(readPersistedDevices()[0]?.lastSeenAt).toBeGreaterThan(0) }) @@ -189,7 +240,9 @@ describe('mobile auth critical path', () => { expect(timeline.filter((entry) => entry === 'acl-spawn')).toHaveLength(0) vi.advanceTimersByTime(250) - expect(timeline.filter((entry) => entry === 'acl-spawn')).toHaveLength(2) + expect(timeline.filter((entry) => entry === 'acl-spawn')).toHaveLength( + BLOCKING_SPAWNS_PER_WRITE + ) }) it('cancels the deferred rewrite when another registry save persists the timestamp', () => { @@ -201,9 +254,13 @@ describe('mobile auth critical path', () => { registry.updateLastSeenDeferred(device.deviceId) registry.addDevice('Other client', 'runtime') - expect(timeline.filter((entry) => entry === 'acl-spawn')).toHaveLength(2) + expect(timeline.filter((entry) => entry === 'acl-spawn')).toHaveLength( + BLOCKING_SPAWNS_PER_WRITE + ) vi.advanceTimersByTime(250) - expect(timeline.filter((entry) => entry === 'acl-spawn')).toHaveLength(2) + expect(timeline.filter((entry) => entry === 'acl-spawn')).toHaveLength( + BLOCKING_SPAWNS_PER_WRITE + ) }) }) diff --git a/src/main/runtime/unreadable-secret-store-preservation.win32.test.ts b/src/main/runtime/unreadable-secret-store-preservation.win32.test.ts new file mode 100644 index 00000000000..d92deccfa9a --- /dev/null +++ b/src/main/runtime/unreadable-secret-store-preservation.win32.test.ts @@ -0,0 +1,262 @@ +import { mkdirSync, mkdtempSync, readFileSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest' +import { runProcessSync } from '../../shared/child-process/run-process' +import { windowsSystem32Binary } from '../../shared/child-process/windows-system-binary' +import { removeTreeSync } from '../../shared/windows-transient-lock-removal' + +/** + * What a *successful* harden does to a reader that cannot read. + * + * Path hardening writes a protected DACL granting only the SIDs the running process holds. Where + * the data root came from somewhere else — a relocated `ORCA_USER_DATA_PATH`, a share, a roaming + * profile, a backup restored under a recreated local account, or a harden whose `/reset` landed + * and whose `/grant` did not — the file ends up granting a SID this process does not have. It then + * reads as `EPERM` while its *directory* stays writable, because file hardening is synchronous on + * the write path and directory hardening is fire-and-forget. + * + * Every store below used to treat any read failure as "malformed — regenerate", and the + * regeneration succeeds: `renameSync` over an unreadable file needs `FILE_DELETE_CHILD` on the + * parent, not `DELETE` on the file. So the healing path destroyed the thing it could not read. + * Before hardening actually applied, this failed open — the file was simply readable. + * + * These assert the file still holds its original bytes afterwards. Runs only on win32, where a + * DACL is the mechanism; skipped elsewhere. + */ + +/** Whether a DACL that omits this token actually denies it a read. */ +function readDenied(filePath: string): boolean { + try { + readFileSync(filePath, 'utf8') + return false + } catch (error) { + return /^(?:EPERM|EACCES)$/.test((error as NodeJS.ErrnoException).code ?? '') + } +} + +/** + * An elevated token logged in as the built-in Administrator reads straight through a DACL that + * grants it nothing, so on such a host every assertion here would pass while proving nothing. + * Probe once and skip rather than assert vacuously -- the same trade the ACL suite makes for its + * unelevated-only case. `isUnreadableError` has its own unit tests on every platform; this suite + * carries the stores' refusal wherever a denial is actually reproducible. + */ +function canDenyReads(): boolean { + if (process.platform !== 'win32') { + return false + } + const probeRoot = mkdtempSync(join(tmpdir(), 'orca-deny-probe-')) + const probe = join(probeRoot, 'probe.json') + try { + writeFileSync(probe, '{}') + icacls(probe, '/inheritance:r', '/q') + icacls(probe, '/grant:r', `*${FOREIGN_SID}:(F)`, '/q') + return readDenied(probe) + } finally { + icacls(probe, '/reset', '/q') + icacls(probeRoot, '/reset', '/t', '/q') + removeTreeSync(probeRoot) + } +} + +/** + * BUILTIN\Guests: a real, always-resolvable group that no interactive token is a member of. + * `S-1-5-32-544` looks foreign only until the suite meets a host that is elevated AND logged + * in as the built-in Administrator -- a CI runner -- where it grants the reader full control + * and every assertion below goes vacuous. An unresolvable SID is not an option: icacls + * rejects one with ERROR_NONE_MAPPED (1332). + */ +const FOREIGN_SID = 'S-1-5-32-546' + +function icacls(...args: string[]): number | null { + return runProcessSync({ + program: windowsSystem32Binary('icacls.exe'), + args, + timeoutMs: 10_000 + }).code +} + +/** The on-disk state a successful harden leaves for a SID this process does not hold. */ +function makeUnreadable(filePath: string): void { + // Two invocations: the combined `/inheritance:r /grant:r` form keeps %TEMP%'s inherited + // [SYSTEM, Administrators, user] as *explicit* ACEs on Windows Server, which left the file + // readable and every assertion below vacuous. Remove inheritance first, then grant. + expect(icacls(filePath, '/inheritance:r', '/q')).toBe(0) + expect(icacls(filePath, '/grant:r', `*${FOREIGN_SID}:(F)`, '/q')).toBe(0) + expect(readDenied(filePath), 'fixture should be unreadable').toBe(true) +} + +const describeOnWindows = process.platform === 'win32' && canDenyReads() ? describe : describe.skip + +describeOnWindows('a secure store that exists but cannot be read', () => { + let root: string + + beforeAll(() => { + root = mkdtempSync(join(tmpdir(), 'orca-unreadable-')) + }) + + afterAll(() => { + // Reset first: the tree is not removable while its files grant only Administrators. + icacls(root, '/reset', '/t', '/q') + removeTreeSync(root) + }) + + it('does not regenerate the E2EE keypair, which would un-pair every device', async () => { + const { loadOrCreateE2EEKeypair } = await import('./e2ee-keypair') + const { E2EE_KEYPAIR_FILENAME } = await import('./mobile-pairing-files') + const dir = join(root, 'e2ee') + mkdirSync(dir, { recursive: true }) + const filePath = join(dir, E2EE_KEYPAIR_FILENAME) + const original = JSON.stringify({ + v: 1, + publicKeyB64: Buffer.alloc(32, 7).toString('base64'), + secretKeyB64: Buffer.alloc(32, 9).toString('base64') + }) + writeFileSync(filePath, original) + makeUnreadable(filePath) + + expect(() => loadOrCreateE2EEKeypair(dir)).toThrow(/Refusing to (regenerate|overwrite)/) + + // The point: the secret key is still the one every paired phone derived its shared secret from. + icacls(filePath, '/reset', '/q') + expect(readFileSync(filePath, 'utf8')).toBe(original) + }) + + it('does not erase the device registry, which would revoke every paired token', async () => { + const { DeviceRegistry } = await import('./device-registry') + const { DEVICE_REGISTRY_FILENAME } = await import('./mobile-pairing-files') + const dir = join(root, 'devices') + mkdirSync(dir, { recursive: true }) + const filePath = join(dir, DEVICE_REGISTRY_FILENAME) + const original = JSON.stringify([ + { + deviceId: 'device-1', + name: 'Phone', + token: 'bearer-token-that-must-survive', + scope: 'mobile', + pairedAt: 1, + lastSeenAt: 2 + } + ]) + writeFileSync(filePath, original) + makeUnreadable(filePath) + + const registry = new DeviceRegistry(dir) + // Any mutator reaches save(); it must refuse rather than write the empty list it loaded. + expect(() => registry.addDevice('Another phone', 'mobile')).toThrow( + /Refusing to (regenerate|overwrite)/ + ) + + icacls(filePath, '/reset', '/q') + expect(readFileSync(filePath, 'utf8')).toBe(original) + }) + + it('does not blank the plugin secret vault on write', async () => { + vi.doMock('electron', () => ({ + safeStorage: { + isEncryptionAvailable: () => true, + encryptString: (value: string) => Buffer.from(`enc:${value}`), + decryptString: (buffer: Buffer) => buffer.toString().replace(/^enc:/, '') + } + })) + const { PluginSecretsStore } = await import('./../plugins/plugin-secrets-store') + const dir = join(root, 'plugin-secrets') + mkdirSync(dir, { recursive: true }) + const store = new PluginSecretsStore(dir, 'publisher.plugin') + // Reach the path the store computes rather than restating its layout here. + const filePath = (store as unknown as { filePath: string }).filePath + mkdirSync(join(filePath, '..'), { recursive: true }) + const original = JSON.stringify({ + version: 1, + format: 'electron-safe-storage-v1', + ciphertexts: { existing: Buffer.from('enc:keep-me').toString('base64') } + }) + writeFileSync(filePath, original) + makeUnreadable(filePath) + + expect(store.set('added', 'value')).toEqual({ ok: false, error: expect.any(String) }) + store.delete('existing') + + icacls(filePath, '/reset', '/q') + expect(readFileSync(filePath, 'utf8')).toBe(original) + vi.doUnmock('electron') + }) + it('does not blank the plugin KV store on write', async () => { + const { PluginKvStore } = await import('./../plugins/plugin-storage-store') + const dir = join(root, 'plugin-kv') + mkdirSync(dir, { recursive: true }) + const store = new PluginKvStore(dir, 'publisher.plugin', 'storage.json') + const filePath = (store as unknown as { filePath: string }).filePath + mkdirSync(join(filePath, '..'), { recursive: true }) + const original = JSON.stringify({ keep: 'me' }) + writeFileSync(filePath, original) + makeUnreadable(filePath) + + expect(store.set('added', 'value')).toEqual({ ok: false, error: expect.any(String) }) + store.delete('keep') + + icacls(filePath, '/reset', '/q') + expect(readFileSync(filePath, 'utf8')).toBe(original) + }) + + it('does not drop pending relay revocations', async () => { + const { RelayRevokeOutbox } = await import('./relay/relay-revoke-outbox') + const dir = join(root, 'relay') + mkdirSync(dir, { recursive: true }) + const filePath = join(dir, 'mobile-relay-revoke-outbox.json') + const original = JSON.stringify([ + { + relayHostId: 'host-1', + relayDeviceId: 'device-1', + ownerIdentityKey: 'owner-1', + reqId: 'req-1', + createdAt: 1 + } + ]) + writeFileSync(filePath, original) + makeUnreadable(filePath) + + const outbox = new RelayRevokeOutbox(dir) + expect(() => + outbox.enqueue({ + relayHostId: 'host-2', + relayDeviceId: 'device-2', + ownerIdentityKey: 'owner-2' + }) + ).toThrow(/Refusing to (regenerate|overwrite)/) + + icacls(filePath, '/reset', '/q') + expect(readFileSync(filePath, 'utf8')).toBe(original) + }) + + /** + * The one site that *deletes* rather than overwrites: a refresh failure plus an unreadable + * session used to fall past the `status === 'found'` guard into `clearOrcaCloudSession`. + */ + it('does not delete the account session it could not read', async () => { + vi.doMock('electron', () => ({ + safeStorage: { + isEncryptionAvailable: () => true, + encryptString: (value: string) => Buffer.from(value), + decryptString: (buffer: Buffer) => buffer.toString() + } + })) + const { readOrcaCloudSession, getOrcaCloudSessionPath } = + await import('./../orca-profiles/profile-cloud-session-store') + const dir = join(root, 'profiles') + mkdirSync(dir, { recursive: true }) + const filePath = getOrcaCloudSessionPath('profile-1', dir) + mkdirSync(join(filePath, '..'), { recursive: true }) + const original = JSON.stringify({ version: 1, format: 'dev-plaintext-v1', savedAt: 1 }) + writeFileSync(filePath, original) + makeUnreadable(filePath) + + // The status the delete path keys off: `unreadable`, never `decrypt-failed`. + expect(readOrcaCloudSession('profile-1', dir).status).toBe('unreadable') + + icacls(filePath, '/reset', '/q') + expect(readFileSync(filePath, 'utf8')).toBe(original) + vi.doUnmock('electron') + }) +}) diff --git a/src/shared/child-process/__fixtures__/child-process-import-allowlist.txt b/src/shared/child-process/__fixtures__/child-process-import-allowlist.txt index b2b82fdff69..fd8502d8913 100644 --- a/src/shared/child-process/__fixtures__/child-process-import-allowlist.txt +++ b/src/shared/child-process/__fixtures__/child-process-import-allowlist.txt @@ -183,5 +183,4 @@ src/shared/fish-binary-requirement.ts src/shared/process-table-snapshot-reader.ts src/shared/pty-slave-line-discipline-echo.ts src/shared/ripgrep-process-availability.ts -src/shared/secure-path-windows-acl.ts src/shared/shell-process-readiness.ts diff --git a/src/shared/child-process/windows-system-binary.ts b/src/shared/child-process/windows-system-binary.ts index f74b8fa7200..1efeef50c79 100644 --- a/src/shared/child-process/windows-system-binary.ts +++ b/src/shared/child-process/windows-system-binary.ts @@ -1,4 +1,7 @@ -import { join } from 'node:path' +// Why win32 and not the host `join`: these are Windows paths and are only ever spawned on Windows, +// but they are also built off-platform (tests, and any code that plans a Windows command from a +// POSIX host), where the host separator produces the mixed `C:\Windows/System32/whoami.exe`. +import { win32 as pathWin32 } from 'node:path' /** * Absolute paths for the Windows system binaries Orca shells out to. @@ -17,12 +20,12 @@ function systemRoot(env: NodeJS.ProcessEnv = process.env): string { } export function windowsPowerShellPath(env: NodeJS.ProcessEnv = process.env): string { - return join(systemRoot(env), 'System32', 'WindowsPowerShell', 'v1.0', 'powershell.exe') + return pathWin32.join(systemRoot(env), 'System32', 'WindowsPowerShell', 'v1.0', 'powershell.exe') } export function windowsSystem32Binary( fileName: string, env: NodeJS.ProcessEnv = process.env ): string { - return join(systemRoot(env), 'System32', fileName) + return pathWin32.join(systemRoot(env), 'System32', fileName) } diff --git a/src/shared/secure-file.test.ts b/src/shared/secure-file.test.ts index d7b7e9e4415..2439c6d2137 100644 --- a/src/shared/secure-file.test.ts +++ b/src/shared/secure-file.test.ts @@ -1,24 +1,73 @@ -import { execFile, execFileSync } from 'node:child_process' import { chmodSync, mkdirSync, mkdtempSync, rmSync, statSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { runProcess, runProcessSync } from './child-process/run-process' +import { mayAttemptHardening } from './secure-path-hardening-retry-budget' import { __getSecureFileHardeningCacheStateForTests, __resetSecureFileHardenedPathsForTests, __resetSecureFileWindowsUserSidForTests, hardenExistingSecureFile, hardenSecurePath, + isUnreadableError, writeSecureFile } from './secure-file' const posixModeIt = process.platform === 'win32' ? it.skip : it -vi.mock('child_process', () => ({ - execFileSync: vi.fn(), - execFile: vi.fn() +vi.mock('./child-process/run-process', () => ({ + runProcess: vi.fn(), + runProcessSync: vi.fn() })) +const OK = { code: 0, signal: null, stdout: '', stderr: '', timedOut: false } +const USER_SID = 'S-1-5-21-1000' + +type FakeSpec = { program: string; args?: readonly string[] } + +/** Paths the fake considers already hardened, with the ACE flags the grant pass used. */ +const hardenedByFake = new Map() + +/** Paths whose verify pass should answer with a DACL that is not the intended one. */ +const forcedBadSddl = new Map() + +/** + * Stands in for icacls. `/save` really writes a UTF-16LE SDDL file, because the code under test + * reads that file back off disk — which also means these tests exercise the real SDDL parser + * rather than a restatement of it. + */ +function fakeIcacls(spec: FakeSpec): typeof OK { + const args = spec.args ?? [] + const path = args[0] ?? '' + const grantIndex = args.indexOf('/grant:r') + if (grantIndex !== -1) { + const grant = args[grantIndex + 1]! + hardenedByFake.set(path, grant.includes('(OI)(CI)') ? 'OICI' : '') + return OK + } + const saveIndex = args.indexOf('/save') + if (saveIndex === -1) { + return OK // /reset + } + writeFileSync(args[saveIndex + 1]!, fakeSddl(path), 'utf16le') + return OK +} + +function fakeSddl(path: string): string { + const forced = forcedBadSddl.get(path) + if (forced) { + return `name\r\n${forced}\r\n` + } + const aceFlags = hardenedByFake.get(path) + if (aceFlags === undefined) { + // Never hardened: the inherited DACL a fresh file carries, so the first verify must fail. + return `name\r\nD:(A;ID;FA;;;SY)(A;ID;FA;;;BA)(A;ID;FA;;;${USER_SID})\r\n` + } + const ace = (sid: string): string => `(A;${aceFlags};FA;;;${sid})` + return `name\r\nD:PAI${ace('BA')}${ace('SY')}${ace(USER_SID)}\r\n` +} + describe('hardenSecurePath', () => { const originalSystemRoot = process.env.SystemRoot const originalWindir = process.env.WINDIR @@ -30,24 +79,19 @@ describe('hardenSecurePath', () => { delete process.env.WINDIR __resetSecureFileWindowsUserSidForTests() __resetSecureFileHardenedPathsForTests() - vi.mocked(execFileSync).mockReset() - vi.mocked(execFile).mockReset() - // execFileSync handles whoami.exe (SID lookup) and the SYNCHRONOUS PowerShell file-ACL - // path used by writeSecureFile. The directory + read-path re-harden use async execFile. - vi.mocked(execFileSync).mockImplementation((file) => { - if (file === 'C:\\Windows\\System32\\whoami.exe') { - return '"USER","S-1-5-21-1000"' + vi.mocked(runProcessSync).mockReset() + vi.mocked(runProcess).mockReset() + hardenedByFake.clear() + forcedBadSddl.clear() + // runProcessSync serves whoami.exe (SID lookup) and the SYNCHRONOUS icacls file-ACL path + // used by writeSecureFile. Directory + read-path re-hardens use async runProcess. + vi.mocked(runProcessSync).mockImplementation((spec) => { + if (spec.program === 'C:\\Windows\\System32\\whoami.exe') { + return { ...OK, stdout: `"USER","${USER_SID}"` } } - // Synchronous PowerShell ACL apply succeeds (returns empty stdout). - return '' - }) - // Directory + read-path PowerShell is called asynchronously; simulate immediate success - vi.mocked(execFile).mockImplementation((_file, _args, _opts, callback) => { - if (typeof callback === 'function') { - callback(null, '', '') - } - return {} as ReturnType + return fakeIcacls(spec) }) + vi.mocked(runProcess).mockImplementation((spec) => Promise.resolve(fakeIcacls(spec))) }) afterEach(() => { @@ -71,64 +115,358 @@ describe('hardenSecurePath', () => { } }) - it('rewrites Windows ACLs through the system PowerShell path', () => { + it('rewrites Windows ACLs through icacls, purging explicit ACEs before granting', async () => { hardenSecurePath('C:\\Users\\me\\.orca\\secret.json', { isDirectory: false, platform: 'win32' }) + await flushAsyncAcl() // whoami.exe called synchronously to obtain SID - expect(execFileSync).toHaveBeenNthCalledWith( - 1, - 'C:\\Windows\\System32\\whoami.exe', - ['/user', '/fo', 'csv', '/nh'], - expect.objectContaining({ encoding: 'utf-8' }) - ) - // PowerShell called asynchronously - const [powershellFile, powershellArgs, powershellOptions] = vi.mocked(execFile).mock.calls[0]! - expect(powershellFile).toBe('C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe') - expect(powershellArgs).toEqual( - expect.arrayContaining([ - '-NoProfile', - '-NonInteractive', - '-ExecutionPolicy', - 'Bypass', - 'C:\\Users\\me\\.orca\\secret.json', - 'S-1-5-21-1000', - '0' - ]) - ) - const script = (powershellArgs as string[])[5]! - expect(script).toContain('SetAccessRuleProtection($true, $false)') - expect(script).toContain('RemoveAccessRuleSpecific') - expect(script).toContain('Unexpected ACL entry') - expect(powershellOptions).toEqual(expect.objectContaining({ windowsHide: true, timeout: 5000 })) - }) - - it('adds inheritable rules when hardening a Windows directory', () => { - hardenSecurePath('C:\\Users\\me\\.orca', { isDirectory: true, platform: 'win32' }) - - const powershellArgs = vi.mocked(execFile).mock.calls[0]![1] as string[] - expect(powershellArgs.at(-1)).toBe('1') - expect(powershellArgs[5]).toContain('ContainerInherit') - expect(powershellArgs[5]).toContain('ObjectInherit') - }) - - it('keeps Windows hardening best-effort when ACL rewriting fails', () => { - // Simulate async PowerShell failure — the callback receives an error - vi.mocked(execFile).mockImplementationOnce((_file, _args, _opts, callback) => { - if (typeof callback === 'function') { - callback(new Error('access denied'), '', '') - } - return {} as ReturnType + expect(vi.mocked(runProcessSync).mock.calls[0]![0]).toMatchObject({ + program: 'C:\\Windows\\System32\\whoami.exe', + args: ['/user', '/fo', 'csv', '/nh'] }) + const specs = vi.mocked(runProcess).mock.calls.map(([spec]) => spec) + expect(specs.every((spec) => spec.program === 'C:\\Windows\\System32\\icacls.exe')).toBe(true) + // Verify runs first, so an already-correct DACL is never rewritten. + expect(specs[0]!.args?.slice(0, 2)).toEqual(['C:\\Users\\me\\.orca\\secret.json', '/save']) + expect(specs[1]!.args).toEqual(['C:\\Users\\me\\.orca\\secret.json', '/reset', '/q']) + expect(specs[2]!.args).toEqual([ + 'C:\\Users\\me\\.orca\\secret.json', + '/inheritance:r', + '/grant:r', + `*${USER_SID}:(F)`, + '/grant:r', + '*S-1-5-18:(F)', + '/grant:r', + '*S-1-5-32-544:(F)', + '/q' + ]) + // The apply is read back: a loosened ACL has to be detectable, not just overwritten. + expect(specs[3]!.args?.slice(0, 2)).toEqual(['C:\\Users\\me\\.orca\\secret.json', '/save']) + expect(specs[2]!.timeoutMs).toBe(5000) + }) + + // BLOCKING 1: re-running /reset on an already-correct DACL restores the inherited (broader) one + // for the few ms until the grant pass lands, for no gain. A correct DACL must be left alone. + it('leaves an already-correct ACL untouched instead of rewriting it', async () => { + const target = 'C:\\Users\\me\\.orca\\secret.json' + hardenedByFake.set(target, '') + + hardenSecurePath(target, { isDirectory: false, platform: 'win32' }) + await flushAsyncAcl() + + const specs = vi.mocked(runProcess).mock.calls.map(([spec]) => spec) + expect(specs).toHaveLength(1) + expect(specs[0]!.args).toContain('/save') + expect(specs.some((spec) => spec.args?.includes('/reset'))).toBe(false) + expect(specs.some((spec) => spec.args?.includes('/grant:r'))).toBe(false) + }) + + // BLOCKING 3: the verify pass must check *identity*, not just rule count, inheritance and rights. + // Granting Everyone full control satisfies all three of those and is the failure it exists for. + it.each([ + ['full control to Everyone', 'D:PAI(A;;FA;;;BA)(A;;FA;;;SY)(A;;FA;;;WD)', 'S-1-1-0'], + ['a deny rule', `D:PAI(D;;FA;;;BA)(A;;FA;;;SY)(A;;FA;;;${USER_SID})`, 'unexpected D rule'], + ['an unprotected DACL', `D:AI(A;;FA;;;BA)(A;;FA;;;SY)(A;;FA;;;${USER_SID})`, 'not protected'], + [ + 'a surviving inherited rule', + `D:PAI(A;ID;FA;;;BA)(A;;FA;;;SY)(A;;FA;;;${USER_SID})`, + 'inherited' + ], + ['read-only rights', `D:PAI(A;;FR;;;BA)(A;;FA;;;SY)(A;;FA;;;${USER_SID})`, 'not full control'] + ])('rejects a verified DACL granting %s', async (_label, sddl, expected) => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const target = 'C:\\Users\\me\\.orca\\secret.json' + forcedBadSddl.set(target, sddl) + + hardenSecurePath(target, { isDirectory: false, platform: 'win32' }) + await flushAsyncAcl() + + expect(warn).toHaveBeenCalledWith( + '[secure-path.windows-acl] failed to restrict path', + expect.objectContaining({ + stage: 'verify', + detail: expect.stringContaining(expected) + }) + ) + warn.mockRestore() + }) + + /** + * Evicting the cache on every failed apply is the #4901 storm wearing a different hat: the env + * store re-hardens on the *read* path at ~2/s, so on a host where hardening legitimately cannot + * work (FAT32, network path, restricted token) that is two icacls spawns and two warnings a + * second, forever. + * + * The curve itself is pinned in secure-path-hardening-retry-budget.test.ts; what matters here is + * that the read path is actually wired to it. + */ + it('collapses a failing read-path poll to a single attempt', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + const targetPath = writeFailingHardenTarget() + + for (let read = 0; read < 25; read++) { + hardenExistingSecureFile(targetPath) + await flushAsyncAcl() + } + + expect(attemptsFor(targetPath)).toHaveLength(1) + warn.mockRestore() + }) + + /** + * A budget that expires rather than latching: three transient failures used to abandon a path + * for the life of the process, so one AV scan or momentary lock left every later credential + * write unprotected on a host where hardening would now succeed. + */ + it('re-probes a long-failing path once its backoff has elapsed', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + const targetPath = writeFailingHardenTarget() + let clock = performance.now() + const now = vi.spyOn(performance, 'now').mockImplementation(() => clock) + + // A day of failing, well past any fixed cap, stepping by more than the 30-minute ceiling. + for (let step = 0; step < 48; step++) { + hardenExistingSecureFile(targetPath) + await flushAsyncAcl() + clock += 31 * 60_000 + } + + expect(attemptsFor(targetPath)).toHaveLength(48) + now.mockRestore() + warn.mockRestore() + }) + + it('reports recovery when a previously throttled path hardens again', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const info = vi.spyOn(console, 'info').mockImplementation(() => {}) + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + const targetPath = writeFailingHardenTarget() + let clock = performance.now() + const now = vi.spyOn(performance, 'now').mockImplementation(() => clock) + + // Three failures to reach the announced degraded state, each past its own backoff. + for (const wait of [0, 61_000, 121_000]) { + clock += wait + hardenExistingSecureFile(targetPath) + await flushAsyncAcl() + } + expect(throttleReports(warn, targetPath)).toHaveLength(1) + + // The transient condition clears; the next re-probe must notice. + clock += 5 * 60_000 + vi.mocked(runProcess).mockImplementation((spec) => Promise.resolve(fakeIcacls(spec))) + hardenExistingSecureFile(targetPath) + await flushAsyncAcl() + + expect(info).toHaveBeenCalledWith( + '[secure-path.windows-acl] path hardening recovered', + expect.objectContaining({ targetPath, stage: 'recovered' }) + ) + now.mockRestore() + info.mockRestore() + warn.mockRestore() + }) + + /** + * The write path is exempt from the budget, but it was also invisible to it: a successful write + * left the failure record standing, so the read path went on backing off for up to 30 minutes + * after the host had demonstrably recovered, and no `recovered` transition came from this lane. + */ + it('clears the read-path backoff when the exempt write path succeeds', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const info = vi.spyOn(console, 'info').mockImplementation(() => {}) + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + const targetPath = writeFailingHardenTarget() + let clock = performance.now() + const now = vi.spyOn(performance, 'now').mockImplementation(() => clock) + + // Three read-path failures: the path is throttled and its next re-probe is minutes away. + for (const wait of [0, 61_000, 121_000]) { + clock += wait + hardenExistingSecureFile(targetPath) + await flushAsyncAcl() + } + expect(mayAttemptHardening(targetPath)).toBe(false) + + // The host recovers and a credential is written. The synchronous apply succeeds (runProcessSync + // was never made to fail), so the read path must stop backing off. + writeSecureFile(targetPath, 'contents') + + expect(mayAttemptHardening(targetPath)).toBe(true) + expect(info).toHaveBeenCalledWith( + '[secure-path.windows-acl] path hardening recovered', + expect.objectContaining({ targetPath, stage: 'recovered' }) + ) + now.mockRestore() + info.mockRestore() + warn.mockRestore() + }) + + /** + * The SID lookup's own one-minute latch, which is the read-path budget's twin and strictly + * worse: a failed lookup makes the plan null, disabling the *synchronous write* path too — so + * the write-path exemption that recovers the budget cannot recover this. Measured against the + * wall clock, a backwards step held it shut for the whole length of the step. + */ + it('re-probes the user SID after a backwards clock step', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + let clock = performance.now() + const now = vi.spyOn(performance, 'now').mockImplementation(() => clock) + let wallClock = Date.parse('2026-01-01T00:00:00Z') + const wallNow = vi.spyOn(Date, 'now').mockImplementation(() => wallClock) + const userDataPath = mkdtempSync(join(tmpdir(), 'orca-secure-file-')) + tempDirs.push(userDataPath) + const targetPath = join(userDataPath, 'secret.json') + let sidLookupFails = true + vi.mocked(runProcessSync).mockImplementation((spec) => { + if (spec.program === 'C:\\Windows\\System32\\whoami.exe') { + return sidLookupFails ? { ...OK, code: 1 } : { ...OK, stdout: `"USER","${USER_SID}"` } + } + return fakeIcacls(spec) + }) + + // No SID, so no plan, so hardening is off entirely — not merely throttled. + expect(writeSecureFile(targetPath, 'first')).toBe(false) + + // A minute of real time passes while the wall clock steps back a year. + clock += 61_000 + wallClock -= 365 * 24 * 60 * 60_000 + sidLookupFails = false + + expect(writeSecureFile(targetPath, 'second')).toBe(true) + wallNow.mockRestore() + now.mockRestore() + warn.mockRestore() + }) + + // Scoped to one path: the parent directory is hardened too, and reports its own transition. + function throttleReports(warn: ReturnType, targetPath: string): unknown[] { + return warn.mock.calls.filter((call) => { + const entry = call[1] as { stage?: string; targetPath?: string } | undefined + return entry?.stage === 'throttled' && entry.targetPath === targetPath + }) + } + + function writeFailingHardenTarget(): string { + const userDataPath = mkdtempSync(join(tmpdir(), 'orca-secure-file-')) + tempDirs.push(userDataPath) + const targetPath = join(userDataPath, 'secret.json') + writeFileSync(targetPath, '{}') + vi.mocked(runProcess).mockResolvedValue({ ...OK, code: 5, stderr: 'Access is denied.' }) + return targetPath + } + + function attemptsFor(targetPath: string): { args?: readonly string[] }[] { + return getHardenAclCalls().filter((spec) => getAclTarget(spec) === targetPath) + } + + // /c makes icacls exit 0 while printing "Failed processing 1 files" — a silent no-op by another route. + it('never passes the icacls /c continue-on-error flag', async () => { + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + const userDataPath = mkdtempSync(join(tmpdir(), 'orca-secure-file-')) + tempDirs.push(userDataPath) + // Cover both runners: the write path is synchronous, the directory re-harden is not. + writeSecureFile(join(userDataPath, 'secret.json'), 'contents') + hardenSecurePath('C:\\Users\\me\\.orca\\other.json', { + isDirectory: false, + platform: 'win32' + }) + await flushAsyncAcl() + + const specs = [ + ...vi.mocked(runProcess).mock.calls.map(([spec]) => spec), + ...vi.mocked(runProcessSync).mock.calls.map(([spec]) => spec) + ] + expect(specs.length).toBeGreaterThan(4) + for (const spec of specs) { + expect(spec.args).not.toContain('/c') + } + }) + + it('adds inheritable rules when hardening a Windows directory', async () => { + hardenSecurePath('C:\\Users\\me\\.orca', { isDirectory: true, platform: 'win32' }) + await flushAsyncAcl() + + const grantArgs = vi + .mocked(runProcess) + .mock.calls.map(([spec]) => spec.args as string[]) + .find((args) => args.includes('/grant:r'))! + expect(grantArgs).toContain(`*${USER_SID}:(OI)(CI)(F)`) + expect(grantArgs).toContain('*S-1-5-18:(OI)(CI)(F)') + }) + + it('keeps Windows hardening best-effort when ACL rewriting fails', async () => { + vi.mocked(runProcess).mockRejectedValue(new Error('access denied')) + expect(() => hardenSecurePath('C:\\Users\\me\\.orca\\secret.json', { isDirectory: false, platform: 'win32' }) ).not.toThrow() + await expect(flushAsyncAcl()).resolves.toBeUndefined() + }) + + // The old PowerShell command line never reached the grant step at all, so a failure had to be + // visible somewhere; "best effort" may not mean "undetectable". + it('logs when a Windows ACL apply fails instead of swallowing it', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + vi.mocked(runProcess).mockResolvedValue({ ...OK, code: 5, stderr: 'Access is denied.' }) + + hardenSecurePath('C:\\Users\\me\\.orca\\secret.json', { + isDirectory: false, + platform: 'win32' + }) + await flushAsyncAcl() + + expect(warn).toHaveBeenCalledWith( + '[secure-path.windows-acl] failed to restrict path', + expect.objectContaining({ + targetPath: 'C:\\Users\\me\\.orca\\secret.json', + stage: 'reset', + detail: 'Access is denied.' + }) + ) + warn.mockRestore() + }) + + it('reports a failed synchronous ACL apply to the caller and the log', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + vi.mocked(runProcessSync).mockImplementation((spec) => { + if (spec.program === 'C:\\Windows\\System32\\whoami.exe') { + return { ...OK, stdout: '"USER","S-1-5-21-1000"' } + } + return { ...OK, code: 5, stderr: 'Access is denied.' } + }) + const userDataPath = mkdtempSync(join(tmpdir(), 'orca-secure-file-')) + tempDirs.push(userDataPath) + + writeSecureFile(join(userDataPath, 'secret.json'), 'contents') + + expect(warn).toHaveBeenCalledWith( + '[secure-path.windows-acl] failed to restrict path', + expect.objectContaining({ stage: 'reset', detail: 'Access is denied.' }) + ) + warn.mockRestore() + }) + + // Paths past MAX_PATH make icacls report "cannot find the path specified"; the extended prefix is the escape. + it('uses the extended-length prefix for paths past MAX_PATH', async () => { + const longPath = `C:\\Users\\me\\.orca\\${'d'.repeat(300)}\\secret.json` + hardenSecurePath(longPath, { isDirectory: false, platform: 'win32' }) + await flushAsyncAcl() + + for (const [spec] of vi.mocked(runProcess).mock.calls) { + expect(spec.args![0]).toBe(`\\\\?\\${longPath}`) + } }) it('caches successful existing-file hardening within a process', () => { @@ -142,8 +480,8 @@ describe('hardenSecurePath', () => { hardenExistingSecureFile(targetPath) // dir hardened once (path-cached), file hardened once (metadata-cached) — 2 total - expect(getPowerShellCalls()).toHaveLength(2) - expect(getPowerShellCalls().map(getPowerShellTarget)).toEqual([userDataPath, targetPath]) + expect(getHardenAclCalls()).toHaveLength(2) + expect(getHardenAclCalls().map(getAclTarget)).toEqual([userDataPath, targetPath]) }) it('LRU-evicts Windows file hardening entries and safely re-hardens an evicted path', () => { @@ -165,8 +503,8 @@ describe('hardenSecurePath', () => { hardenExistingSecureFile(paths[0]!) - const fileTargets = getPowerShellCalls() - .map(getPowerShellTarget) + const fileTargets = getHardenAclCalls() + .map(getAclTarget) .filter((path) => paths.includes(path)) expect(fileTargets).toEqual([...paths, paths[0]]) expect(__getSecureFileHardeningCacheStateForTests().paths).toMatchObject({ @@ -196,8 +534,8 @@ describe('hardenSecurePath', () => { hardenExistingSecureFile(files[0]!) - const directoryTargets = getPowerShellCalls() - .map(getPowerShellTarget) + const directoryTargets = getHardenAclCalls() + .map(getAclTarget) .filter((path) => directories.includes(path)) expect(directoryTargets).toEqual([...directories, directories[0]]) expect(__getSecureFileHardeningCacheStateForTests().directories).toMatchObject({ @@ -218,12 +556,8 @@ describe('hardenSecurePath', () => { hardenExistingSecureFile(targetPath) // call 1: dir + file. call 2: dir skipped (path-cached), file re-hardened (new mtime) - expect(getPowerShellCalls()).toHaveLength(3) - expect(getPowerShellCalls().map(getPowerShellTarget)).toEqual([ - userDataPath, - targetPath, - targetPath - ]) + expect(getHardenAclCalls()).toHaveLength(3) + expect(getHardenAclCalls().map(getAclTarget)).toEqual([userDataPath, targetPath, targetPath]) }) it('keeps post-rename target hardening on every write while caching the directory', () => { @@ -236,19 +570,19 @@ describe('hardenSecurePath', () => { writeSecureFile(targetPath, 'second') // The DIRECTORY is hardened async + path-cached: exactly once across both writes. - const asyncTargets = getPowerShellCalls().map(getPowerShellTarget) + const asyncTargets = getHardenAclCalls().map(getAclTarget) expect(asyncTargets).toEqual([userDataPath]) // The credential FILES (tmpFile + renamed target) are hardened SYNCHRONOUSLY on each write. // write 1: tmpFile(1) + targetFile(1) = 2; write 2: tmpFile(1) + targetFile(1) = 2; total 4. - const syncTargets = getSyncPowerShellCalls().map(getPowerShellTarget) + const syncTargets = getSyncHardenAclCalls().map(getAclTarget) expect(syncTargets).toHaveLength(4) expect(syncTargets.filter((entry) => entry === targetPath)).toHaveLength(2) // No directory should be hardened via the synchronous path. expect(syncTargets.filter((entry) => entry === userDataPath)).toHaveLength(0) }) - // Regression test: #4901 — env-store reads at ~2×/s caused a PowerShell storm because the + // Regression test: #4901 — env-store reads at ~2×/s caused an ACL-spawn storm because the // parent directory mtime churned (every secure write updates it), so the mtime-keyed cache // never matched. Directories must be path-cached for the process lifetime. it('does not re-harden the parent directory when its mtime changes between reads', async () => { @@ -268,9 +602,7 @@ describe('hardenSecurePath', () => { hardenExistingSecureFile(targetPath) // The parent directory must be hardened exactly ONCE despite its mtime changing - const dirCalls = getPowerShellCalls().filter( - (call) => getPowerShellTarget(call) === userDataPath - ) + const dirCalls = getHardenAclCalls().filter((call) => getAclTarget(call) === userDataPath) expect(dirCalls).toHaveLength(1) }) @@ -285,27 +617,25 @@ describe('hardenSecurePath', () => { hardenExistingSecureFile(targetPath) hardenExistingSecureFile(targetPath) - const fileCalls = getPowerShellCalls().filter( - (call) => getPowerShellTarget(call) === targetPath - ) + const fileCalls = getHardenAclCalls().filter((call) => getAclTarget(call) === targetPath) expect(fileCalls).toHaveLength(1) }) - it('applies the read-path ACL asynchronously without blocking (async execFile)', () => { + it('applies the read-path ACL asynchronously without blocking (async runProcess)', () => { hardenSecurePath('C:\\Users\\me\\.orca\\secret.json', { isDirectory: false, platform: 'win32' }) - // The default (read/dir) path must launch PowerShell via execFile (async), never sync. - expect(getSyncPowerShellCalls()).toHaveLength(0) - expect(getPowerShellCalls()).toHaveLength(1) + // The default (read/dir) path must launch icacls via runProcess (async), never sync. + expect(getSyncHardenAclCalls()).toHaveLength(0) + expect(getHardenAclCalls()).toHaveLength(1) }) // Security regression guard (#5006 review finding): writeSecureFile must restrict the // credential FILE's ACL SYNCHRONOUSLY before returning. On Windows writeFileSync({mode}) // is a no-op, so an async file ACL would leave the credential briefly readable under the - // parent's inherited (broader) ACL for the ~1-1.5s PowerShell cold-start window. + // parent's inherited (broader) ACL for the duration of the spawn. it('hardens the credential file synchronously while keeping the directory async', () => { Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) const userDataPath = mkdtempSync(join(tmpdir(), 'orca-secure-file-')) @@ -315,13 +645,13 @@ describe('hardenSecurePath', () => { writeSecureFile(targetPath, 'contents') // Directory: async only. - expect(getPowerShellCalls().map(getPowerShellTarget)).toEqual([userDataPath]) + expect(getHardenAclCalls().map(getAclTarget)).toEqual([userDataPath]) // File (tmpFile + renamed target): synchronous only — no async file ACL window. - const syncTargets = getSyncPowerShellCalls().map(getPowerShellTarget) + const syncTargets = getSyncHardenAclCalls().map(getAclTarget) expect(syncTargets).toContain(targetPath) expect(syncTargets.filter((entry) => entry === userDataPath)).toHaveLength(0) // The final published target's ACL must have been applied via the synchronous path. - expect(getPowerShellCalls().map(getPowerShellTarget)).not.toContain(targetPath) + expect(getHardenAclCalls().map(getAclTarget)).not.toContain(targetPath) }) // Nit #1 (review): the synchronous file path must cache as hardened ONLY on confirmed @@ -333,30 +663,30 @@ describe('hardenSecurePath', () => { tempDirs.push(userDataPath) const targetPath = join(userDataPath, 'secret.json') - // First write: the synchronous PowerShell ACL apply throws for every powershell call. - vi.mocked(execFileSync).mockImplementation((file) => { - if (file === 'C:\\Windows\\System32\\whoami.exe') { - return '"USER","S-1-5-21-1000"' + // First write: the synchronous icacls ACL apply throws for every icacls call. + vi.mocked(runProcessSync).mockImplementation((spec) => { + if (spec.program === 'C:\\Windows\\System32\\whoami.exe') { + return { ...OK, stdout: '"USER","S-1-5-21-1000"' } } throw new Error('access denied') }) expect(() => writeSecureFile(targetPath, 'first')).not.toThrow() - const firstWriteTargetCalls = getSyncPowerShellCalls() - .map(getPowerShellTarget) + const firstWriteTargetCalls = getSyncHardenAclCalls() + .map(getAclTarget) .filter((entry) => entry === targetPath) expect(firstWriteTargetCalls).toHaveLength(1) // Second write: ACL apply now succeeds. Because the failed apply was NOT cached, the // target file is hardened again rather than skipped. - vi.mocked(execFileSync).mockImplementation((file) => { - if (file === 'C:\\Windows\\System32\\whoami.exe') { - return '"USER","S-1-5-21-1000"' + vi.mocked(runProcessSync).mockImplementation((spec) => { + if (spec.program === 'C:\\Windows\\System32\\whoami.exe') { + return { ...OK, stdout: '"USER","S-1-5-21-1000"' } } - return '' + return OK }) writeSecureFile(targetPath, 'second') - const allTargetCalls = getSyncPowerShellCalls() - .map(getPowerShellTarget) + const allTargetCalls = getSyncHardenAclCalls() + .map(getAclTarget) .filter((entry) => entry === targetPath) expect(allTargetCalls).toHaveLength(2) }) @@ -374,15 +704,13 @@ describe('hardenSecurePath', () => { writeSecureFile(join(userDataPath, `secret-${i}.json`), `contents-${i}`) } - const dirCalls = getPowerShellCalls().filter( - (call) => getPowerShellTarget(call) === userDataPath - ) + const dirCalls = getHardenAclCalls().filter((call) => getAclTarget(call) === userDataPath) expect(dirCalls).toHaveLength(1) }) - // win32-only guard: on non-win32 platforms no PowerShell is ever spawned (sync or async); + // win32-only guard: on non-win32 platforms no icacls is ever spawned (sync or async); // POSIX hardening uses chmodSync only. - it('never spawns PowerShell on non-win32 platforms', () => { + it('never spawns icacls on non-win32 platforms', () => { Object.defineProperty(process, 'platform', { configurable: true, value: 'linux' }) const userDataPath = mkdtempSync(join(tmpdir(), 'orca-secure-file-')) tempDirs.push(userDataPath) @@ -391,8 +719,8 @@ describe('hardenSecurePath', () => { writeSecureFile(targetPath, 'contents') hardenExistingSecureFile(targetPath) - expect(getPowerShellCalls()).toHaveLength(0) - expect(getSyncPowerShellCalls()).toHaveLength(0) + expect(getHardenAclCalls()).toHaveLength(0) + expect(getSyncHardenAclCalls()).toHaveLength(0) }) posixModeIt('re-hardens a POSIX directory when its metadata changes after caching', () => { @@ -440,22 +768,51 @@ describe('hardenSecurePath', () => { }) }) -const POWERSHELL_SUFFIX = 'WindowsPowerShell\\v1.0\\powershell.exe' - -// Async PowerShell calls (directory hardening + read-path file re-harden). -function getPowerShellCalls(): unknown[][] { - return vi.mocked(execFile).mock.calls.filter(([file]) => String(file).endsWith(POWERSHELL_SUFFIX)) +/** + * Every harden opens with a `/save` verify; one that has work to do then runs `/reset`, `/grant:r` + * and a closing `/save`. Counting only the *opening* verify keeps "one harden = one entry" + * regardless of which of the two shapes it took. + */ +function hardenInitiations(specs: FakeSpec[]): { args?: readonly string[] }[] { + const initiations: { args?: readonly string[] }[] = [] + const awaitingClosingVerify = new Set() + for (const spec of specs) { + if (!spec.program.endsWith('icacls.exe')) { + continue + } + const path = spec.args?.[0] ?? '' + if (spec.args?.includes('/grant:r')) { + awaitingClosingVerify.add(path) + } else if (spec.args?.includes('/save')) { + if (awaitingClosingVerify.has(path)) { + awaitingClosingVerify.delete(path) + } else { + initiations.push(spec) + } + } + } + return initiations } -// Synchronous PowerShell calls (credential-file ACL on the write path). -function getSyncPowerShellCalls(): unknown[][] { - return vi - .mocked(execFileSync) - .mock.calls.filter(([file]) => String(file).endsWith(POWERSHELL_SUFFIX)) +// Async icacls calls (directory hardening + read-path file re-harden). +function getHardenAclCalls(): { args?: readonly string[] }[] { + return hardenInitiations(vi.mocked(runProcess).mock.calls.map(([spec]) => spec)) } -function getPowerShellTarget(call: unknown[]): string { - return (call[1] as string[])[6]! +// Synchronous icacls calls (credential-file ACL on the write path). +function getSyncHardenAclCalls(): { args?: readonly string[] }[] { + return hardenInitiations(vi.mocked(runProcessSync).mock.calls.map(([spec]) => spec)) +} + +function getAclTarget(spec: { args?: readonly string[] }): string { + return spec.args![0]! +} + +// The async harden awaits three icacls passes, so let the chain settle before asserting on it. +async function flushAsyncAcl(): Promise { + for (let i = 0; i < 4; i++) { + await new Promise((resolve) => setTimeout(resolve, 0)) + } } async function waitForFileTimestampTick(): Promise { @@ -465,3 +822,37 @@ async function waitForFileTimestampTick(): Promise { function statMode(path: string): number { return statSync(path).mode & 0o777 } + +describe('isUnreadableError', () => { + const withCode = (code: string): NodeJS.ErrnoException => Object.assign(new Error(code), { code }) + + // The hardened-DACL case this predicate was written for. + it('reports a denied read', () => { + expect(isUnreadableError(withCode('EPERM'))).toBe(true) + expect(isUnreadableError(withCode('EACCES'))).toBe(true) + }) + + /** + * The likelier half on Windows: antivirus holding a credential open during startup yields + * EBUSY, and fd exhaustion yields EMFILE. Neither says the bytes were read, so neither may + * license a regenerate-and-overwrite. + */ + it('reports a read that never reached the contents for any other reason', () => { + expect(isUnreadableError(withCode('EBUSY'))).toBe(true) + expect(isUnreadableError(withCode('EMFILE'))).toBe(true) + expect(isUnreadableError(withCode('ENFILE'))).toBe(true) + expect(isUnreadableError(withCode('EIO'))).toBe(true) + }) + + /** + * The other side of the distinction, and the reason this is an allow list rather than + * "everything except ENOENT": a missing file licenses creating one, and bytes that were read + * and did not parse are the self-heal these stores exist to perform. + */ + it('does not report a missing file or a parse failure', () => { + expect(isUnreadableError(withCode('ENOENT'))).toBe(false) + expect(isUnreadableError(new SyntaxError('Unexpected end of JSON input'))).toBe(false) + expect(isUnreadableError(withCode('EISDIR'))).toBe(false) + expect(isUnreadableError(undefined)).toBe(false) + }) +}) diff --git a/src/shared/secure-file.ts b/src/shared/secure-file.ts index 639630c7c65..c04ea9afe5b 100644 --- a/src/shared/secure-file.ts +++ b/src/shared/secure-file.ts @@ -13,9 +13,15 @@ import { } from 'node:fs' import { dirname } from 'node:path' import { + DEFAULT_HARDENING_CACHE_BOUNDS, SecurePathHardeningCache, type SecurePathHardeningCacheBounds } from './secure-path-hardening-cache' +import { + configureHardeningRetryBudget, + mayAttemptHardening, + recordHardeningOutcome +} from './secure-path-hardening-retry-budget' import { bestEffortRestrictWindowsPath, resetSecureFileWindowsUserSidForTests, @@ -33,24 +39,14 @@ type HardenedPathCacheEntry = { birthtimeMs: number } -export const SECURE_PATH_HARDENING_CACHE_MAX_ENTRIES = 1024 -export const SECURE_PATH_HARDENING_CACHE_KEY_MAX_BYTES = 64 * 1024 -export const SECURE_PATH_HARDENING_CACHE_KEYS_MAX_BYTES = 512 * 1024 - -const DEFAULT_HARDENING_CACHE_BOUNDS: SecurePathHardeningCacheBounds = { - maxEntries: SECURE_PATH_HARDENING_CACHE_MAX_ENTRIES, - maxKeyBytes: SECURE_PATH_HARDENING_CACHE_KEY_MAX_BYTES, - maxTotalKeyBytes: SECURE_PATH_HARDENING_CACHE_KEYS_MAX_BYTES -} - const UNSUPPORTED_DIRECTORY_FSYNC_CODES = new Set(['EINVAL', 'ENOTSUP', 'EOPNOTSUPP']) -// Why: PowerShell hardening (~1-1.5s) stalls the main thread, so cache idempotent re-hardens per process. +// Why: hardening spawns icacls synchronously (once when the DACL already verifies, four times when it must be rewritten), so cache idempotent re-hardens per process. let hardenedPathsThisProcess = new SecurePathHardeningCache( DEFAULT_HARDENING_CACHE_BOUNDS ) -// Why: child writes constantly bump a dir's mtime, so cache dirs by path (not metadata) to avoid a PowerShell spawn every read (#4901). +// Why: child writes constantly bump a dir's mtime, so cache dirs by path (not metadata) to avoid an icacls spawn every read (#4901). // Limitation: a dir deleted+recreated in-process won't re-harden; fine since we never delete our secure dirs at runtime. let hardenedDirectoryPathsThisProcess = new SecurePathHardeningCache( DEFAULT_HARDENING_CACHE_BOUNDS @@ -61,9 +57,13 @@ function hardenSecureDirectoryOnce(dirPath: string): void { if (hardenedDirectoryPathsThisProcess.get(dirPath)) { return } - applySecurePathRestriction(dirPath, true, process.platform, false) - // Cache even though the async ACL may still be in flight — dir restriction is best-effort, no retry. + // Cache before the ACL lands so concurrent writes don't restorm; a failure drops it, under the retry budget. hardenedDirectoryPathsThisProcess.set(dirPath, true) + applySecurePathRestriction(dirPath, true, process.platform, false, (restricted) => { + if (!restricted) { + hardenedDirectoryPathsThisProcess.delete(dirPath) + } + }) } function hardenSecurePathOnce(targetPath: string, isDirectory: boolean): boolean { @@ -81,26 +81,50 @@ function hardenSecurePathOnce(targetPath: string, isDirectory: boolean): boolean return true } // Why: async re-harden is safe here — read path hardens each file at most once/process; new files harden synchronously on the write path. - if (applySecurePathRestriction(targetPath, isDirectory, process.platform, false)) { + const outcome = applySecurePathRestriction( + targetPath, + isDirectory, + process.platform, + false, + (restricted) => { + if (!restricted) { + hardenedPathsThisProcess.delete(targetPath) + } + } + ) + if (outcome !== 'failed') { rememberHardenedPath(targetPath, isDirectory) return true } return false } -export function writeSecureJsonFile(targetPath: string, value: unknown): void { - writeSecureFile(targetPath, JSON.stringify(value, null, 2)) +/** Returns false when the file was written but its permissions could not be restricted. */ +export function writeSecureJsonFile(targetPath: string, value: unknown): boolean { + return writeSecureFile(targetPath, JSON.stringify(value, null, 2)) } -export function writeDurableSecureJsonFile(targetPath: string, value: unknown): void { - writeSecureFile(targetPath, JSON.stringify(value, null, 2), { durable: true }) +/** Returns false when the file was written but its permissions could not be restricted. */ +export function writeDurableSecureJsonFile(targetPath: string, value: unknown): boolean { + return writeSecureFile(targetPath, JSON.stringify(value, null, 2), { durable: true }) } +/** + * Writes `contents` and restricts the result to the current user. + * + * Returns whether the restriction actually took. Hardening stays best-effort — it fails + * legitimately on FAT32, network paths and restricted tokens, and must not break a write — but + * the outcome is now reported rather than assumed, so a caller storing a credential can react. + * + * The return value covers the *file* only. The parent directory is hardened fire-and-forget — on + * Windows that lane is async and answers `pending` regardless — so a `true` here says nothing + * about the directory's ACL. + */ export function writeSecureFile( targetPath: string, contents: string, options: { durable?: boolean } = {} -): void { +): boolean { const dir = dirname(targetPath) if (!existsSync(dir)) { mkdirSync(dir, { recursive: true, mode: 0o700 }) @@ -118,15 +142,18 @@ export function writeSecureFile( fsyncFileSync(tmpFile) } // Why: writeFileSync mode is a no-op on Windows, so restrict the credential's ACL synchronously before the rename publishes it under inherited ACLs. - applySecurePathRestriction(tmpFile, false, process.platform, true) + const stagedOutcome = applySecurePathRestriction(tmpFile, false, process.platform, true) renameSync(tmpFile, targetPath) // Why: these hold auth credentials, so the published path must stay current-user only; cache only on confirmed success so failures retry. - if (applySecurePathRestriction(targetPath, false, process.platform, true)) { + // The staged file's protected DACL survives the rename, so this pass usually just verifies it. + const publishedOutcome = applySecurePathRestriction(targetPath, false, process.platform, true) + if (publishedOutcome === 'applied') { rememberHardenedPath(targetPath, false) } if (options.durable) { bestEffortFsyncDirectorySync(dir) } + return stagedOutcome === 'applied' && publishedOutcome === 'applied' } catch (error) { rmSync(tmpFile, { force: true }) throw error @@ -164,6 +191,34 @@ export function bestEffortFsyncDirectorySync(directory: string): void { } } +/** + * Errors that mean the contents were never seen, so they say nothing about what the file holds. + * + * `ENOENT` is deliberately absent: "there is no file" genuinely licenses creating one. So is a + * parse failure, which means the bytes WERE read and were garbage - the self-heal these stores + * were built for. The distinction is "could not read it" versus "read it and it was garbage". + * + * Why it matters: a reader that treats every failure as corruption regenerates the file, and the + * regeneration succeeds - `renameSync` over an unreadable file needs `FILE_DELETE_CHILD` on the + * parent, not `DELETE` on the file - so the original is destroyed by the code meant to heal it. + * `EPERM`/`EACCES` is the hardened-DACL case: a file granting a SID this process does not hold, + * reachable through a relocated user-data path, a share or roaming profile, a restored backup + * under a new local SID, or a half-applied harden. The rest are transient and, on Windows, more + * likely than that: `EBUSY` is what antivirus produces by holding a file open at the moment of a + * read, which for a credential read on the startup path is an ordinary Tuesday. + */ +export function isUnreadableError(error: unknown): boolean { + const code = (error as NodeJS.ErrnoException | null)?.code + return ( + code === 'EPERM' || + code === 'EACCES' || + code === 'EBUSY' || + code === 'EMFILE' || + code === 'ENFILE' || + code === 'EIO' + ) +} + export function hardenExistingSecureFile(targetPath: string): void { const dir = dirname(targetPath) if (existsSync(dir)) { @@ -191,24 +246,48 @@ export function hardenSecurePath( ) } -/** Applies hardening; async Windows calls only report that best-effort ACL work was accepted. */ +/** + * `pending` is the honest answer for the async Windows branch: it has not happened yet, and + * reporting it as `applied` is what let a dead ACL look like a working one. The real outcome + * arrives through `onAsyncSettled`. + */ +type HardeningOutcome = 'applied' | 'pending' | 'failed' + function applySecurePathRestriction( targetPath: string, isDirectory: boolean, platform: NodeJS.Platform, - sync: boolean -): boolean { + sync: boolean, + onAsyncSettled?: (restricted: boolean) => void +): HardeningOutcome { if (platform === 'win32') { if (sync) { + // Why no retry floor here: the write path is user-driven, not polled, and a failed apply + // must still be retried on the next write of the same credential. // Why: apply the ACL synchronously so the credential file isn't briefly readable under inherited ACLs (writeFileSync mode is a no-op on Windows). - return restrictWindowsPathSync(targetPath, isDirectory) + const restricted = restrictWindowsPathSync(targetPath, isDirectory) + if (restricted) { + // Success only: this is how a recovered host clears the read path's backoff (and reports + // `recovered`). Recording a failure here would put the exempt lane back under the budget. + recordHardeningOutcome(targetPath, true) + } + return restricted ? 'applied' : 'failed' } - // Why: dir/read-path re-harden runs async to avoid blocking the main thread (#4901); return true optimistically since it's best-effort. - bestEffortRestrictWindowsPath(targetPath, isDirectory) - return true + // Why the floor: this is the read path, polled at ~2/s (#4901). Retrying every failure there + // is the same storm the cache exists to prevent. + if (!mayAttemptHardening(targetPath)) { + onAsyncSettled?.(false) + return 'failed' + } + // Why: dir/read-path re-harden runs async to avoid blocking the main thread (#4901). + bestEffortRestrictWindowsPath(targetPath, isDirectory, (restricted) => { + recordHardeningOutcome(targetPath, restricted) + onAsyncSettled?.(restricted) + }) + return 'pending' } chmodSync(targetPath, isDirectory ? 0o700 : 0o600) - return true + return 'applied' } /** Caches the current metadata snapshot for a just-hardened path, or clears it if the path is gone. */ @@ -275,6 +354,7 @@ export function __resetSecureFileHardenedPathsForTests( ): void { hardenedPathsThisProcess = new SecurePathHardeningCache(bounds) hardenedDirectoryPathsThisProcess = new SecurePathHardeningCache(bounds) + configureHardeningRetryBudget(bounds) } export function __getSecureFileHardeningCacheStateForTests(): { diff --git a/src/shared/secure-path-hardening-cache.ts b/src/shared/secure-path-hardening-cache.ts index 3ec32de4f8b..eb1f7200bf9 100644 --- a/src/shared/secure-path-hardening-cache.ts +++ b/src/shared/secure-path-hardening-cache.ts @@ -4,6 +4,21 @@ export type SecurePathHardeningCacheBounds = { maxTotalKeyBytes: number } +export const SECURE_PATH_HARDENING_CACHE_MAX_ENTRIES = 1024 +export const SECURE_PATH_HARDENING_CACHE_KEY_MAX_BYTES = 64 * 1024 +export const SECURE_PATH_HARDENING_CACHE_KEYS_MAX_BYTES = 512 * 1024 + +/** + * The bounds every hardening cache uses unless a caller overrides them. They live here rather + * than with one consumer so a cache can default itself instead of depending on some other module + * being imported first. + */ +export const DEFAULT_HARDENING_CACHE_BOUNDS: SecurePathHardeningCacheBounds = { + maxEntries: SECURE_PATH_HARDENING_CACHE_MAX_ENTRIES, + maxKeyBytes: SECURE_PATH_HARDENING_CACHE_KEY_MAX_BYTES, + maxTotalKeyBytes: SECURE_PATH_HARDENING_CACHE_KEYS_MAX_BYTES +} + type RetainedSecurePath = { value: T keyBytes: number diff --git a/src/shared/secure-path-hardening-report.ts b/src/shared/secure-path-hardening-report.ts new file mode 100644 index 00000000000..45408da0317 --- /dev/null +++ b/src/shared/secure-path-hardening-report.ts @@ -0,0 +1,43 @@ +/** + * Where path-hardening outcomes are announced, kept apart from the code that applies them so the + * retry budget can report degradation and recovery without importing the Windows ACL lane. + */ +export type SecurePathHardeningReport = { + targetPath: string + /** + * `throttled` and `recovered` mark entering and leaving the rate-limited degraded state; + * `settle` is the async lane's own callback failing, which is a caller bug rather than a host one. + */ + stage: 'sid-lookup' | 'reset' | 'grant' | 'verify' | 'settle' | 'throttled' | 'recovered' + detail: string +} + +/** + * Why a hook: hardening runs in the Electron main process, which is GUI-subsystem on Windows and + * owns no console, so `console.warn` reaches nothing in a packaged build. The main process + * installs a reporter that routes into the diagnostic trace; the console default keeps dev runs + * and the CLI readable. + */ +const consoleReporter = (entry: SecurePathHardeningReport): void => { + if (entry.stage === 'recovered') { + console.info('[secure-path.windows-acl] path hardening recovered', entry) + return + } + console.warn('[secure-path.windows-acl] failed to restrict path', entry) +} + +let reportEntry: (entry: SecurePathHardeningReport) => void = consoleReporter + +export function setSecurePathHardeningReporter( + reporter: ((entry: SecurePathHardeningReport) => void) | null +): void { + reportEntry = reporter ?? consoleReporter +} + +export function reportSecurePathHardening( + targetPath: string, + stage: SecurePathHardeningReport['stage'], + detail: string +): void { + reportEntry({ targetPath, stage, detail: detail.trim().slice(0, 500) }) +} diff --git a/src/shared/secure-path-hardening-retry-budget.test.ts b/src/shared/secure-path-hardening-retry-budget.test.ts new file mode 100644 index 00000000000..e2b76b7cec1 --- /dev/null +++ b/src/shared/secure-path-hardening-retry-budget.test.ts @@ -0,0 +1,174 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + configureHardeningRetryBudget, + hardeningRetryDelayMs, + mayAttemptHardening, + recordHardeningOutcome +} from './secure-path-hardening-retry-budget' +import { + setSecurePathHardeningReporter, + type SecurePathHardeningReport +} from './secure-path-hardening-report' + +const PATH = 'C:\\Users\\me\\.orca\\secret.json' +const OTHER = 'C:\\Users\\me\\.orca\\other.json' +const MINUTE = 60_000 + +describe('secure path hardening retry budget', () => { + /** Elapsed monotonic time, which is what the budget measures. */ + let clock = 0 + /** The wall clock, which it must not measure: it steps backwards on real machines. */ + let wallClock = 0 + let reports: SecurePathHardeningReport[] = [] + + beforeEach(() => { + clock = 1_000_000 + wallClock = Date.parse('2026-01-01T00:00:00Z') + vi.spyOn(performance, 'now').mockImplementation(() => clock) + vi.spyOn(Date, 'now').mockImplementation(() => wallClock) + reports = [] + setSecurePathHardeningReporter((entry) => reports.push(entry)) + configureHardeningRetryBudget({ + maxEntries: 64, + maxKeyBytes: 4096, + maxTotalKeyBytes: 65_536 + }) + }) + + afterEach(() => { + setSecurePathHardeningReporter(null) + vi.restoreAllMocks() + }) + + /** Drives the loop the read path drives: attempt when allowed, record the failure, advance. */ + function pollUntil(elapsedMs: number, stepMs: number, restricted = false): number[] { + const attemptedAt: number[] = [] + const startedAt = clock + while (clock - startedAt <= elapsedMs) { + if (mayAttemptHardening(PATH)) { + attemptedAt.push(clock - startedAt) + recordHardeningOutcome(PATH, restricted) + } + clock += stepMs + wallClock += stepMs + } + return attemptedAt + } + + it('doubles the delay after each consecutive failure, up to the ceiling', () => { + expect(hardeningRetryDelayMs(1)).toBe(1 * MINUTE) + expect(hardeningRetryDelayMs(2)).toBe(2 * MINUTE) + expect(hardeningRetryDelayMs(3)).toBe(4 * MINUTE) + expect(hardeningRetryDelayMs(4)).toBe(8 * MINUTE) + expect(hardeningRetryDelayMs(5)).toBe(16 * MINUTE) + // Ceiling reached, and it stays there however long the host has been broken. + expect(hardeningRetryDelayMs(6)).toBe(30 * MINUTE) + expect(hardeningRetryDelayMs(50)).toBe(30 * MINUTE) + expect(hardeningRetryDelayMs(5000)).toBe(30 * MINUTE) + }) + + it('allows the first attempt for a path it has never seen', () => { + expect(mayAttemptHardening(PATH)).toBe(true) + }) + + // The #4901 condition: the env store re-hardens on the read path about twice a second. + it('collapses a read-path poll to a single attempt in the first minute', () => { + const attemptedAt = pollUntil(55_000, 500) + + expect(attemptedAt).toEqual([0]) + }) + + it('re-probes on the documented curve rather than on every read', () => { + // Six hours of polling every 30s: 720 reads, and only the backoff decides how many run. + const attemptedAt = pollUntil(6 * 60 * MINUTE, 30_000) + + // 0, +1, +2, +4, +8, +16, then every 30 minutes forever. + expect(attemptedAt.slice(0, 6).map((ms) => ms / MINUTE)).toEqual([0, 1, 3, 7, 15, 31]) + const trailingGaps = attemptedAt + .slice(-4) + .map((ms, index, all) => (all[index + 1]! - ms) / MINUTE) + expect(trailingGaps.slice(0, -1)).toEqual([30, 30, 30]) + }) + + // The failure this replaced: three transient failures used to disable a path until restart. + it('never abandons a path, however long it has been failing', () => { + pollUntil(30 * 24 * 60 * MINUTE, 15 * MINUTE) + + // A month of failures later, the very next elapsed ceiling still re-probes. + clock += 31 * MINUTE + expect(mayAttemptHardening(PATH)).toBe(true) + }) + + /** + * The same latch by another route. NTP corrections, VM snapshot restores and a user changing the + * clock all step `Date.now()` backwards; measured against the wall clock that makes the elapsed + * time negative, so the path stayed below its delay for the whole length of the step — a year, + * here — which is exactly the permanent abandonment the backoff exists to remove. + */ + it('re-probes after a backwards clock step rather than waiting for the wall clock', () => { + pollUntil(6 * 60 * MINUTE, 30_000) + + wallClock -= 365 * 24 * 60 * MINUTE + clock += 31 * MINUTE + + expect(mayAttemptHardening(PATH)).toBe(true) + }) + + it('announces the degraded state once, not once per failure', () => { + pollUntil(6 * 60 * MINUTE, 30_000) + + const throttled = reports.filter((entry) => entry.stage === 'throttled') + expect(throttled).toHaveLength(1) + expect(throttled[0]).toMatchObject({ targetPath: PATH, stage: 'throttled' }) + }) + + it('announces recovery when a throttled path hardens again, and resets the curve', () => { + pollUntil(10 * MINUTE, 30_000) + expect(reports.filter((entry) => entry.stage === 'throttled')).toHaveLength(1) + + recordHardeningOutcome(PATH, true) + + expect(reports.filter((entry) => entry.stage === 'recovered')).toMatchObject([ + { targetPath: PATH, stage: 'recovered' } + ]) + // Record cleared: the next failure starts at the floor rather than the ceiling. + expect(mayAttemptHardening(PATH)).toBe(true) + recordHardeningOutcome(PATH, false) + clock += 59_000 + expect(mayAttemptHardening(PATH)).toBe(false) + clock += 2_000 + expect(mayAttemptHardening(PATH)).toBe(true) + }) + + it('stays silent about recovery for a path that never reached the degraded state', () => { + recordHardeningOutcome(PATH, false) + recordHardeningOutcome(PATH, true) + + expect(reports).toEqual([]) + }) + + it('budgets each path separately', () => { + recordHardeningOutcome(PATH, false) + + expect(mayAttemptHardening(PATH)).toBe(false) + expect(mayAttemptHardening(OTHER)).toBe(true) + }) + + /** + * The state every other test here configures away: a module instance nobody has called + * `configureHardeningRetryBudget` on, which is what a second importer gets. It used to throw, + * and from the async lane that throw is an unhandled rejection rather than a caught error, so + * "the budget is unconfigured" surfaced as a dead main process. Nothing is imported here but + * the module itself — importing `secure-file.ts` is what used to hide this. + */ + it('defaults its bounds when nothing configured it', async () => { + vi.resetModules() + const budget = await import('./secure-path-hardening-retry-budget.js') + + expect(budget.mayAttemptHardening(PATH)).toBe(true) + expect(() => budget.recordHardeningOutcome(PATH, false)).not.toThrow() + // Proof it recorded into a real cache rather than merely not throwing. + expect(budget.mayAttemptHardening(PATH)).toBe(false) + expect(budget.mayAttemptHardening(OTHER)).toBe(true) + }) +}) diff --git a/src/shared/secure-path-hardening-retry-budget.ts b/src/shared/secure-path-hardening-retry-budget.ts new file mode 100644 index 00000000000..9dfd93f02f3 --- /dev/null +++ b/src/shared/secure-path-hardening-retry-budget.ts @@ -0,0 +1,100 @@ +import { + DEFAULT_HARDENING_CACHE_BOUNDS, + SecurePathHardeningCache, + type SecurePathHardeningCacheBounds +} from './secure-path-hardening-cache' +import { reportSecurePathHardening } from './secure-path-hardening-report' + +type HardeningFailureRecord = { at: number; attempts: number } + +/** + * How often a path whose hardening keeps failing may be retried. + * + * Why throttle at all: the env store re-hardens on the *read* path at ~2/s (#4901), so retrying + * every failure is an icacls-and-log storm on hosts where hardening cannot work — FAT32/exFAT have + * no ACLs, and network paths, redirected profiles and restricted tokens refuse. + * + * Why exponential and not a cap: a cap that never expires latches a *transient* failure — one AV + * scan or momentary lock and the path is abandoned for the life of the process, which can be days. + * Backoff bounds the rate without ever bounding the lifetime. It settles at ~2 attempts/hour on a + * permanently incapable host, which matters because the budget is per path and there are several + * secure files; a fixed one-minute floor would leave a standing five-figure daily spawn count for + * work that will never succeed. + * + * Why slowing it down is close to free: the synchronous write path is deliberately *not* + * throttled, so a host that recovers hardens on its very next credential write. This read-path + * re-probe is a backstop, not the recovery mechanism. + */ +const HARDENING_RETRY_FLOOR_MS = 60_000 +const HARDENING_RETRY_CEILING_MS = 30 * 60_000 + +/** + * Why not `Date.now`: an NTP correction, a VM snapshot restore or a user changing the clock steps + * the wall clock backwards, which made the elapsed time negative and held every path below its + * delay until the clock caught up — a year, for a year-long step. That is the permanent latch this + * backoff exists to remove. Elapsed monotonic time cannot go backwards. + */ +const monotonicNowMs = (): number => performance.now() + +/** Consecutive failures before the degraded state is announced. */ +const HARDENING_THROTTLE_ANNOUNCE_AFTER = 3 + +/** Exported so the tests pin the real curve rather than a copy of it. */ +export function hardeningRetryDelayMs(attempts: number): number { + return Math.min(HARDENING_RETRY_FLOOR_MS * 2 ** (attempts - 1), HARDENING_RETRY_CEILING_MS) +} + +let hardeningFailures: SecurePathHardeningCache | null = null + +/** + * Why it defaults instead of throwing: this used to require `configureHardeningRetryBudget` first, + * and the only thing keeping that contract was import order — one module configured it at module + * scope and happened to be the sole importer. Any second importer got a throw, and from the async + * lane that throw lands in a `.then` handler as an unhandled rejection, which takes the Electron + * main process down. A retry budget is not worth a crash, and a default is not worth a caller. + */ +function failures(): SecurePathHardeningCache { + hardeningFailures ??= new SecurePathHardeningCache( + DEFAULT_HARDENING_CACHE_BOUNDS + ) + return hardeningFailures +} + +/** Overrides the default bounds. Optional: nothing has to call this before the budget is used. */ +export function configureHardeningRetryBudget(bounds: SecurePathHardeningCacheBounds): void { + hardeningFailures = new SecurePathHardeningCache(bounds) +} + +export function mayAttemptHardening(targetPath: string): boolean { + const failure = failures().get(targetPath) + if (!failure) { + return true + } + // No cap: once the backoff elapses the path is re-probed, however long it has been failing. + return monotonicNowMs() - failure.at >= hardeningRetryDelayMs(failure.attempts) +} + +export function recordHardeningOutcome(targetPath: string, restricted: boolean): void { + const previous = failures().get(targetPath) + if (restricted) { + failures().delete(targetPath) + if (previous && previous.attempts >= HARDENING_THROTTLE_ANNOUNCE_AFTER) { + reportSecurePathHardening( + targetPath, + 'recovered', + `hardening succeeded again after ${previous.attempts} consecutive failures` + ) + } + return + } + const attempts = (previous?.attempts ?? 0) + 1 + failures().set(targetPath, { at: monotonicNowMs(), attempts }) + // Fires exactly once: attempts only rises, and a success clears the record entirely. + if (attempts === HARDENING_THROTTLE_ANNOUNCE_AFTER) { + reportSecurePathHardening( + targetPath, + 'throttled', + `hardening failed ${attempts} times; backing off toward one retry per ${HARDENING_RETRY_CEILING_MS / 60_000} minutes until it succeeds` + ) + } +} diff --git a/src/shared/secure-path-windows-acl.ts b/src/shared/secure-path-windows-acl.ts index 4d61dff79e1..ac0aac0d1ce 100644 --- a/src/shared/secure-path-windows-acl.ts +++ b/src/shared/secure-path-windows-acl.ts @@ -1,144 +1,351 @@ -import { execFile, execFileSync } from 'node:child_process' -import { win32 as pathWin32 } from 'node:path' +import { randomBytes } from 'node:crypto' +import { readFileSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join, win32 as pathWin32 } from 'node:path' +import { runProcess, runProcessSync } from './child-process/run-process' +import { windowsSystem32Binary } from './child-process/windows-system-binary' +import { + reportSecurePathHardening, + type SecurePathHardeningReport +} from './secure-path-hardening-report' +import { localDomainSidOf, parseSddlDacl } from './windows-security-descriptor' -let cachedWindowsUserSid: string | null | undefined +const ACL_TIMEOUT_MS = 5000 -function buildWindowsRestrictAclArgs( - targetPath: string, - currentUserSid: string, +/** SYSTEM and the local Administrators group: they can take ownership regardless, so denying them buys nothing. */ +const LOCAL_SYSTEM_SID = 'S-1-5-18' +const BUILTIN_ADMINISTRATORS_SID = 'S-1-5-32-544' + +const WINDOWS_SID_PATTERN = /^S-1-\d+(?:-\d+)+$/ + +type AclPlan = { + program: string + /** The path as icacls must receive it, already extended-length prefixed when needed. */ + icaclsPath: string isDirectory: boolean -): string[] { - return [ - '-NoProfile', - '-NonInteractive', - '-ExecutionPolicy', - 'Bypass', - '-Command', - WINDOWS_RESTRICT_ACL_SCRIPT, - targetPath, - currentUserSid, - isDirectory ? '1' : '0' - ] + allowedSids: string[] + /** Resolves the machine-relative aliases `/save` emits; null when the user SID is not one. */ + localDomainSid: string | null + resetArgs: string[] + grantArgs: string[] } -export function bestEffortRestrictWindowsPath(targetPath: string, isDirectory: boolean): void { - const currentUserSid = getCurrentWindowsUserSid() - if (!currentUserSid) { +function buildAclPlan(targetPath: string, currentUserSid: string, isDirectory: boolean): AclPlan { + const icaclsPath = toIcaclsPath(targetPath) + // Directories propagate to children (artifact-intent files rely on inheritance); files take no flags. + const rights = isDirectory ? '(OI)(CI)(F)' : '(F)' + const allowedSids = [...new Set([currentUserSid, LOCAL_SYSTEM_SID, BUILTIN_ADMINISTRATORS_SID])] + return { + program: windowsSystem32Binary('icacls.exe'), + icaclsPath, + isDirectory, + allowedSids, + localDomainSid: localDomainSidOf(currentUserSid), + // `/reset` purges explicit ACEs, which `/inheritance:r` leaves in place — a planted + // `Everyone:(R)` survives the grant pass otherwise. The two cannot be combined in one call. + resetArgs: [icaclsPath, '/reset', '/q'], + // Never add /c: it makes icacls exit 0 on "Failed processing 1 files", a silent no-op by another route. + grantArgs: [ + icaclsPath, + '/inheritance:r', + ...allowedSids.flatMap((sid) => ['/grant:r', `*${sid}:${rights}`]), + '/q' + ] + } +} + +function verifyArgs(plan: AclPlan, savePath: string): string[] { + return [plan.icaclsPath, '/save', savePath, '/q'] +} + +function sddlSavePath(): string { + return join(tmpdir(), `orca-acl-${process.pid}-${randomBytes(6).toString('hex')}.sddl`) +} + +/** + * Judges a `/save` result. Returns a failure reason, or null when the DACL on disk is exactly the + * intended one — protected, granting full control to the allowed SIDs and to nobody else. + */ +function evaluateSavedAcl( + plan: AclPlan, + result: { code: number | null; stderr: string }, + savePath: string +): string | null { + if (result.code !== 0) { + return result.stderr.trim() || `icacls exited ${result.code}` + } + let sddl: string + try { + // icacls writes the descriptor as UTF-16LE, which sidesteps the OEM codepage its stdout uses. + sddl = readFileSync(savePath, 'utf16le') + } catch { + return 'icacls saved no security descriptor' + } + return validateHardenedDacl(sddl, plan) +} + +function validateHardenedDacl(sddl: string, plan: AclPlan): string | null { + const dacl = parseSddlDacl(sddl, plan.localDomainSid ?? undefined) + if (!dacl) { + return 'no DACL in the saved security descriptor' + } + if (!dacl.isProtected) { + return 'DACL is not protected; the parent still propagates into it' + } + // Exactly these, in any order: a directory's rules must be inheritable and nothing else. + const expectedFlags = plan.isDirectory ? ['OI', 'CI'] : [] + const observed = new Set() + for (const ace of dacl.aces) { + if (ace.type !== 'A') { + return `unexpected ${ace.type} rule for ${ace.sid}` + } + if (ace.flags.includes('ID')) { + return `inherited rule survived for ${ace.sid}` + } + if (ace.rights !== 'FA') { + return `rule for ${ace.sid} grants ${ace.rights || 'nothing'}, not full control` + } + // The whole set, not just OI. (OI) without (CI) leaves subdirectories unprotected, and + // adding (IO) makes every rule inherit-only, so the directory object itself grants nobody + // anything and Orca cannot even write into it. Both used to be repaired blindly on every + // pass; since hardening short-circuits on a DACL that verifies, whatever this accepts stays. + if ( + ace.flags.length !== expectedFlags.length || + !expectedFlags.every((flag) => ace.flags.includes(flag)) + ) { + return `wrong inheritance flags (${ace.flags.join('') || 'none'}) for ${ace.sid}` + } + observed.add(ace.sid) + } + // Identity, not just shape: a count check alone accepts a granted SID swapped for another. + // Unexpected principals are reported before missing ones — "Everyone has full control" is the + // headline, and a substitution always produces both. + for (const sid of observed) { + if (!plan.allowedSids.includes(sid)) { + return `unexpected rule for ${sid}` + } + } + for (const sid of plan.allowedSids) { + if (!observed.has(sid)) { + return `missing rule for ${sid}` + } + } + return null +} + +/** + * icacls resolves through the MAX_PATH-limited API and fails with "cannot find the path + * specified" past 259 characters; the extended prefix is the documented escape. + */ +function toIcaclsPath(targetPath: string): string { + if (targetPath.length < 260 || targetPath.startsWith('\\\\?\\')) { + return targetPath + } + const normalized = pathWin32.normalize(targetPath) + if (/^[A-Za-z]:\\/.test(normalized)) { + return `\\\\?\\${normalized}` + } + if (normalized.startsWith('\\\\')) { + return `\\\\?\\UNC\\${normalized.slice(2)}` + } + return targetPath +} + +function report( + targetPath: string, + stage: SecurePathHardeningReport['stage'], + detail: string +): void { + reportSecurePathHardening(targetPath, stage, detail) +} + +/** + * Applies the ACL without blocking. `onSettled` reports the real outcome, which the return value + * cannot: the caller's cache must not keep claiming a path is hardened when the apply failed. + */ +export function bestEffortRestrictWindowsPath( + targetPath: string, + isDirectory: boolean, + onSettled?: (restricted: boolean) => void +): void { + const plan = planFor(targetPath, isDirectory) + if (!plan) { + onSettled?.(false) return } - // Why: async to avoid blocking the main thread — sync PowerShell cold-start (~1-1.5s) on the frequent read path stormed it (#4901). - execFile( - getWindowsSystemToolPath('WindowsPowerShell\\v1.0\\powershell.exe'), - buildWindowsRestrictAclArgs(targetPath, currentUserSid, isDirectory), - { - windowsHide: true, - timeout: 5000 - }, - () => { - // Why: ignore errors — hardening is best-effort; PowerShell ACL APIs may be unavailable or locked down. + // Why async: hardening runs on the read path, and blocking it on a spawn stormed the main thread (#4901). + // Why both arms and a terminal catch: a bare `void p.then(fn)` makes a rejected `restrictAsync` + // *and* a throw from `onSettled` itself an unhandled rejection, which Node's default turns into + // a main-process crash — the exact opposite of what the reporter hook exists for. `false` is the + // right value on the error arm: it drops the path from the caller's cache and leaves it retryable. + void restrictAsync(targetPath, plan) + .then(onSettled, () => onSettled?.(false)) + .catch((error: unknown) => reportSettlementThrow(targetPath, error)) +} + +/** + * The last frame before an unhandled rejection, so it must not throw either — and the reporter it + * calls is a caller-installed hook, which is the one thing here that plausibly does. + */ +function reportSettlementThrow(targetPath: string, error: unknown): void { + try { + report(targetPath, 'settle', `hardening settlement callback threw: ${String(error)}`) + } catch { + // Nothing left to report through; losing one diagnostic beats crashing the main process. + } +} + +async function restrictAsync(targetPath: string, plan: AclPlan): Promise { + // Verify first: a path that already reads back correct needs no write at all. Re-running + // `/reset` on a correct DACL would briefly restore the inherited (broader) one for no gain. + if ((await verifyAsync(plan)) === null) { + return true + } + for (const [stage, args] of [ + ['reset', plan.resetArgs], + ['grant', plan.grantArgs] + ] as const) { + try { + const result = await runProcess({ program: plan.program, args, timeoutMs: ACL_TIMEOUT_MS }) + if (result.code !== 0) { + report(targetPath, stage, result.stderr || `icacls exited ${result.code}`) + return false + } + } catch (error) { + report(targetPath, stage, String(error)) + return false } - ) + } + const invalid = await verifyAsync(plan) + if (invalid) { + report(targetPath, 'verify', invalid) + return false + } + return true +} + +async function verifyAsync(plan: AclPlan): Promise { + const savePath = sddlSavePath() + try { + const result = await runProcess({ + program: plan.program, + args: verifyArgs(plan, savePath), + timeoutMs: ACL_TIMEOUT_MS + }) + return evaluateSavedAcl(plan, result, savePath) + } catch (error) { + return String(error) + } finally { + discard(savePath) + } } export function restrictWindowsPathSync(targetPath: string, isDirectory: boolean): boolean { + const plan = planFor(targetPath, isDirectory) + if (!plan) { + return false + } + // Why sync: the file must not be published until its ACL is actually restricted (read path stays async, #4901). + if (verifySync(plan) === null) { + return true + } + for (const [stage, args] of [ + ['reset', plan.resetArgs], + ['grant', plan.grantArgs] + ] as const) { + try { + const result = runProcessSync({ program: plan.program, args, timeoutMs: ACL_TIMEOUT_MS }) + if (result.code !== 0) { + report(targetPath, stage, result.stderr || `icacls exited ${result.code}`) + return false + } + } catch (error) { + // Why not fatal: a failed ACL apply must not crash the write; false leaves the path uncached to retry later. + report(targetPath, stage, String(error)) + return false + } + } + const invalid = verifySync(plan) + if (invalid) { + report(targetPath, 'verify', invalid) + return false + } + return true +} + +function verifySync(plan: AclPlan): string | null { + const savePath = sddlSavePath() + try { + const result = runProcessSync({ + program: plan.program, + args: verifyArgs(plan, savePath), + timeoutMs: ACL_TIMEOUT_MS + }) + return evaluateSavedAcl(plan, result, savePath) + } catch (error) { + return String(error) + } finally { + discard(savePath) + } +} + +function discard(savePath: string): void { + try { + rmSync(savePath, { force: true }) + } catch { + // The descriptor holds no secrets; a leftover temp file is not worth reporting. + } +} + +function planFor(targetPath: string, isDirectory: boolean): AclPlan | null { const currentUserSid = getCurrentWindowsUserSid() if (!currentUserSid) { - return false - } - // Why: file must not be published until its ACL is actually restricted, so block and report real success (read path stays async, #4901). - try { - execFileSync( - getWindowsSystemToolPath('WindowsPowerShell\\v1.0\\powershell.exe'), - buildWindowsRestrictAclArgs(targetPath, currentUserSid, isDirectory), - { - stdio: ['ignore', 'ignore', 'ignore'], - windowsHide: true, - timeout: 5000 - } - ) - return true - } catch { - // Why: best-effort — a failed ACL apply must not crash the write; false leaves the path uncached to retry later. - return false + report(targetPath, 'sid-lookup', 'could not resolve the current user SID') + return null } + return buildAclPlan(targetPath, currentUserSid, isDirectory) } -const WINDOWS_RESTRICT_ACL_SCRIPT = ` -$ErrorActionPreference = 'Stop' -$path = $args[0] -$currentUserSid = $args[1] -$isDirectory = $args[2] -eq '1' -$allowedSidTexts = @($currentUserSid, 'S-1-5-18', 'S-1-5-32-544') -$allowedSids = @{} -foreach ($sidText in $allowedSidTexts) { - $allowedSids[$sidText] = $true -} -$acl = Get-Acl -LiteralPath $path -$acl.SetAccessRuleProtection($true, $false) -foreach ($rule in @($acl.Access)) { - [void]$acl.RemoveAccessRuleSpecific($rule) -} -$inheritanceFlags = [System.Security.AccessControl.InheritanceFlags]::None -if ($isDirectory) { - $inheritanceFlags = [System.Security.AccessControl.InheritanceFlags]::ContainerInherit -bor [System.Security.AccessControl.InheritanceFlags]::ObjectInherit -} -foreach ($sidText in $allowedSidTexts) { - $sid = [System.Security.Principal.SecurityIdentifier]::new($sidText) - $rule = [System.Security.AccessControl.FileSystemAccessRule]::new( - $sid, - [System.Security.AccessControl.FileSystemRights]::FullControl, - $inheritanceFlags, - [System.Security.AccessControl.PropagationFlags]::None, - [System.Security.AccessControl.AccessControlType]::Allow - ) - [void]$acl.AddAccessRule($rule) -} -Set-Acl -LiteralPath $path -AclObject $acl -$verifiedAcl = Get-Acl -LiteralPath $path -if (-not $verifiedAcl.AreAccessRulesProtected) { - throw 'ACL inheritance is still enabled' -} -$fullControl = [System.Security.AccessControl.FileSystemRights]::FullControl -foreach ($rule in @($verifiedAcl.Access)) { - $sid = $rule.IdentityReference.Translate([System.Security.Principal.SecurityIdentifier]).Value - if (-not $allowedSids.ContainsKey($sid)) { - throw "Unexpected ACL entry $sid" - } - if ($rule.AccessControlType -ne [System.Security.AccessControl.AccessControlType]::Allow) { - throw "Unexpected ACL deny entry $sid" - } - if (($rule.FileSystemRights -band $fullControl) -ne $fullControl) { - throw "ACL entry $sid does not grant FullControl" - } -} -`.trim() +let cachedWindowsUserSid: string | null = null +let sidLookupFailedAt: number | null = null +const SID_LOOKUP_RETRY_MS = 60_000 +/** + * Why monotonic and not `Date.now`: a backwards wall-clock step held this window open until the + * clock caught up, and this latch is worse than the read-path budget's — a failed lookup makes + * `planFor` return null, which disables the synchronous *write* path too, so the write-path + * exemption that recovers from that one cannot recover from this. + */ +const monotonicNowMs = (): number => performance.now() + +/** + * Only a well-formed SID is cached for the process lifetime. A failure is cached for a minute: + * caching it forever let one transient `whoami` hiccup disable hardening until restart. + */ function getCurrentWindowsUserSid(): string | null { - if (cachedWindowsUserSid !== undefined) { + if (cachedWindowsUserSid) { return cachedWindowsUserSid } - try { - const output = execFileSync( - getWindowsSystemToolPath('whoami.exe'), - ['/user', '/fo', 'csv', '/nh'], - { - encoding: 'utf-8', - stdio: ['ignore', 'pipe', 'ignore'], - windowsHide: true, - timeout: 5000 - } - ).trim() - const columns = parseCsvLine(output) - cachedWindowsUserSid = columns[1] ?? null - } catch { - cachedWindowsUserSid = null + if (sidLookupFailedAt !== null && monotonicNowMs() - sidLookupFailedAt < SID_LOOKUP_RETRY_MS) { + return null } - return cachedWindowsUserSid -} - -function getWindowsSystemToolPath(relativeSystem32Path: string): string { - const systemRoot = process.env.SystemRoot || process.env.WINDIR || 'C:\\Windows' - return pathWin32.join(systemRoot, 'System32', relativeSystem32Path) + try { + const result = runProcessSync({ + program: windowsSystem32Binary('whoami.exe'), + args: ['/user', '/fo', 'csv', '/nh'], + timeoutMs: ACL_TIMEOUT_MS + }) + const candidate = result.code === 0 ? parseCsvLine(result.stdout.trim())[1] : undefined + if (candidate && WINDOWS_SID_PATTERN.test(candidate)) { + cachedWindowsUserSid = candidate + sidLookupFailedAt = null + return candidate + } + } catch { + // Fall through to the failure record below. + } + sidLookupFailedAt = monotonicNowMs() + return null } function parseCsvLine(line: string): string[] { @@ -146,5 +353,6 @@ function parseCsvLine(line: string): string[] { } export function resetSecureFileWindowsUserSidForTests(): void { - cachedWindowsUserSid = undefined + cachedWindowsUserSid = null + sidLookupFailedAt = null } diff --git a/src/shared/secure-path-windows-acl.win32.test.ts b/src/shared/secure-path-windows-acl.win32.test.ts new file mode 100644 index 00000000000..7ae193d70c5 --- /dev/null +++ b/src/shared/secure-path-windows-acl.win32.test.ts @@ -0,0 +1,404 @@ +import { mkdirSync, mkdtempSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest' +import { runProcessSync } from './child-process/run-process' +import { windowsSystem32Binary } from './child-process/windows-system-binary' +import { + setSecurePathHardeningReporter, + type SecurePathHardeningReport +} from './secure-path-hardening-report' +import { + bestEffortRestrictWindowsPath, + resetSecureFileWindowsUserSidForTests, + restrictWindowsPathSync +} from './secure-path-windows-acl' +import { removeTreeSync } from './windows-transient-lock-removal' + +/** + * The half of the proof a mocked argv test cannot give. + * + * The shipped bug was not a wrong argv — it was an argv the *callee* never + * received: `powershell.exe -Command