From 8c975b9d15fda7990c65e044267ec157a48d3c2d Mon Sep 17 00:00:00 2001 From: OrcaWin Date: Mon, 10 Aug 2026 19:55:29 -0700 Subject: [PATCH] fix(terminal): avoid Windows PATH registry subprocesses (#13485) --- src/main/cli/cli-installer.test.ts | 2 +- .../cli/windows-user-path-registry.test.ts | 2 +- src/main/cli/windows-user-path-registry.ts | 41 ++----- ...eflight-windows-path-refresh.repro.test.ts | 65 ++--------- ...windows-environment-path-main-loop.test.ts | 80 +++++++++++++ src/main/pty/windows-environment-path.test.ts | 110 ++++++++---------- src/main/pty/windows-environment-path.ts | 86 +++++++++----- .../pty/windows-path-registry-reader.test.ts | 70 +++++++++++ src/main/pty/windows-path-registry-reader.ts | 67 +++++++++++ src/main/windows-native-registry.ts | 25 ++++ 10 files changed, 371 insertions(+), 177 deletions(-) create mode 100644 src/main/pty/windows-environment-path-main-loop.test.ts create mode 100644 src/main/pty/windows-path-registry-reader.test.ts create mode 100644 src/main/pty/windows-path-registry-reader.ts create mode 100644 src/main/windows-native-registry.ts diff --git a/src/main/cli/cli-installer.test.ts b/src/main/cli/cli-installer.test.ts index 9c17fd21116..180611c1590 100644 --- a/src/main/cli/cli-installer.test.ts +++ b/src/main/cli/cli-installer.test.ts @@ -489,7 +489,7 @@ describe('CliInstaller', () => { const registryReader = new WindowsUserPathRegistryReader({ platform: 'win32', registryLoader: async () => ({ - HK: { CU: 0x80000001 }, + HK: { CU: 0x80000001, LM: 0x80000002 }, getRegistryKey: () => ({ Path: { name: 'Path', type: 2, value: registryPath } }) diff --git a/src/main/cli/windows-user-path-registry.test.ts b/src/main/cli/windows-user-path-registry.test.ts index 12beca13852..7f919694626 100644 --- a/src/main/cli/windows-user-path-registry.test.ts +++ b/src/main/cli/windows-user-path-registry.test.ts @@ -7,7 +7,7 @@ import { function registryModule(pathValue?: string, type = 2) { return { - HK: { CU: 0x80000001 }, + HK: { CU: 0x80000001, LM: 0x80000002 }, REG: { SZ: 1, EXPAND_SZ: 2 }, getRegistryKey: vi.fn(() => pathValue === undefined ? {} : { Path: { name: 'Path', type, value: pathValue } } diff --git a/src/main/cli/windows-user-path-registry.ts b/src/main/cli/windows-user-path-registry.ts index d3e493fd87d..90e8ba7863e 100644 --- a/src/main/cli/windows-user-path-registry.ts +++ b/src/main/cli/windows-user-path-registry.ts @@ -1,26 +1,17 @@ -import { createRequire } from 'node:module' +import { + loadWindowsNativeRegistry, + WINDOWS_REG_EXPAND_SZ, + WINDOWS_REG_SZ, + type WindowsNativeRegistryModule +} from '../windows-native-registry' export type WindowsUserPathReadResult = | { state: 'success'; value: string | null; expandable: boolean } | { state: 'unknown'; detail: string } -type RegistryValue = { - name?: unknown - type?: unknown - value?: unknown -} - -type WindowsRegistryModule = { - HK: { CU: number } - getRegistryKey: ( - root: number, - path: string - ) => Record | null | undefined -} - type WindowsUserPathRegistryReaderOptions = { platform?: NodeJS.Platform - registryLoader?: () => Promise + registryLoader?: () => Promise now?: () => number cacheTtlMs?: number } @@ -28,19 +19,9 @@ type WindowsUserPathRegistryReaderOptions = { const DEFAULT_CACHE_TTL_MS = 1_000 const USER_ENVIRONMENT_KEY = 'Environment' const USER_PATH_VALUE = 'Path' -const REG_SZ = 1 -const REG_EXPAND_SZ = 2 -const requireFromMain = createRequire(__filename) - -async function loadWindowsRegistryModule(): Promise { - // Why: the optional dependency is not present in non-Windows installs, so - // TypeScript and the main bundle must not resolve it eagerly. - return requireFromMain('windows-native-registry') as WindowsRegistryModule -} - export class WindowsUserPathRegistryReader { private readonly platform: NodeJS.Platform - private readonly registryLoader: () => Promise + private readonly registryLoader: () => Promise private readonly now: () => number private readonly cacheTtlMs: number private cached: { readAt: number; result: WindowsUserPathReadResult } | null = null @@ -49,7 +30,7 @@ export class WindowsUserPathRegistryReader { constructor(options: WindowsUserPathRegistryReaderOptions = {}) { this.platform = options.platform ?? process.platform - this.registryLoader = options.registryLoader ?? loadWindowsRegistryModule + this.registryLoader = options.registryLoader ?? (async () => loadWindowsNativeRegistry()) this.now = options.now ?? Date.now this.cacheTtlMs = options.cacheTtlMs ?? DEFAULT_CACHE_TTL_MS } @@ -123,7 +104,7 @@ export class WindowsUserPathRegistryReader { return { state: 'success', value: null, expandable: false } } if ( - (pathEntry.type !== REG_SZ && pathEntry.type !== REG_EXPAND_SZ) || + (pathEntry.type !== WINDOWS_REG_SZ && pathEntry.type !== WINDOWS_REG_EXPAND_SZ) || typeof pathEntry.value !== 'string' ) { return { @@ -134,7 +115,7 @@ export class WindowsUserPathRegistryReader { return { state: 'success', value: pathEntry.value || null, - expandable: pathEntry.type === REG_EXPAND_SZ + expandable: pathEntry.type === WINDOWS_REG_EXPAND_SZ } } catch { return { diff --git a/src/main/ipc/preflight-windows-path-refresh.repro.test.ts b/src/main/ipc/preflight-windows-path-refresh.repro.test.ts index 38e135d4558..916ab35ab63 100644 --- a/src/main/ipc/preflight-windows-path-refresh.repro.test.ts +++ b/src/main/ipc/preflight-windows-path-refresh.repro.test.ts @@ -3,45 +3,11 @@ import { join } from 'node:path' import { tmpdir } from 'node:os' import { afterEach, describe, expect, it, vi } from 'vitest' -const { registryQueryAsyncMock, registryQuerySyncMock } = vi.hoisted(() => ({ - registryQueryAsyncMock: vi.fn(), - registryQuerySyncMock: vi.fn() -})) - -vi.mock('node:child_process', async (importOriginal) => { - const original = await importOriginal>() - const originalExecFile = original.execFile as ( - command: string, - commandArgs: string[], - commandOptions: unknown, - commandCallback: (error: Error | null, stdout: string, stderr: string) => void - ) => unknown - const registryAwareExecFile = ( - file: string, - args: string[], - options: unknown, - callback: (error: Error | null, stdout: string, stderr: string) => void - ): unknown => { - if (file.toLowerCase().endsWith('\\reg.exe')) { - return registryQueryAsyncMock(file, args, options, callback) - } - return originalExecFile(file, args, options, callback) - } - const customPromisify = Symbol.for('nodejs.util.promisify.custom') - Object.defineProperty(registryAwareExecFile, customPromisify, { - value: (originalExecFile as unknown as Record)[customPromisify] - }) - return { - ...original, - execFile: registryAwareExecFile, - execFileSync: registryQuerySyncMock - } -}) - import { __resetPersistedWindowsPathCacheForTests, mergePersistedWindowsPathAsync } from '../pty/windows-environment-path' +import { __setWindowsPathRegistryLoaderForTests } from '../pty/windows-path-registry-reader' import { execLocalPreflightCommand } from './preflight-command-exec' describe.runIf(process.platform === 'win32')('Windows preflight Path refresh reproduction', () => { @@ -50,8 +16,7 @@ describe.runIf(process.platform === 'win32')('Windows preflight Path refresh rep afterEach(() => { process.env.Path = originalPath - registryQueryAsyncMock.mockReset() - registryQuerySyncMock.mockReset() + __setWindowsPathRegistryLoaderForTests() __resetPersistedWindowsPathCacheForTests() for (const directory of fixtureDirs.splice(0)) { rmSync(directory, { recursive: true, force: true }) @@ -68,22 +33,13 @@ describe.runIf(process.platform === 'win32')('Windows preflight Path refresh rep ) let persistedUserPath = '' - registryQuerySyncMock.mockImplementation((_file, args: string[]) => { - const value = String(args[1]).startsWith('HKCU') ? persistedUserPath : '' - return ` Path REG_SZ ${value}\r\n` - }) - registryQueryAsyncMock.mockImplementation( - ( - _file: string, - args: string[], - _options: unknown, - callback: (error: Error | null, stdout: string, stderr: string) => void - ) => { - const value = String(args[1]).startsWith('HKCU') ? persistedUserPath : '' - callback(null, ` Path REG_SZ ${value}\r\n`, '') - return {} as never - } - ) + const getRegistryKey = vi.fn((root: number) => ({ + Path: { type: 1, value: root === 2 ? persistedUserPath : '' } + })) + __setWindowsPathRegistryLoaderForTests(() => ({ + HK: { LM: 1, CU: 2 }, + getRegistryKey + })) __resetPersistedWindowsPathCacheForTests() await expect(execLocalPreflightCommand(command, ['/?'])).rejects.toMatchObject({ @@ -97,7 +53,6 @@ describe.runIf(process.platform === 'win32')('Windows preflight Path refresh rep await expect(execLocalPreflightCommand(command, ['/?'])).resolves.toMatchObject({ stdout: expect.any(String) }) - expect(registryQuerySyncMock).toHaveBeenCalledTimes(2) - expect(registryQueryAsyncMock).toHaveBeenCalledTimes(2) + expect(getRegistryKey).toHaveBeenCalledTimes(4) }) }) diff --git a/src/main/pty/windows-environment-path-main-loop.test.ts b/src/main/pty/windows-environment-path-main-loop.test.ts new file mode 100644 index 00000000000..fbe185479cc --- /dev/null +++ b/src/main/pty/windows-environment-path-main-loop.test.ts @@ -0,0 +1,80 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { + __resetPersistedWindowsPathCacheForTests, + readPersistedWindowsPathSegments +} from './windows-environment-path' +import { __setWindowsPathRegistryLoaderForTests } from './windows-path-registry-reader' + +const CREATE_PROCESS_DELAY_MS = 160 +const delayedExecFileSync = vi.hoisted(() => + vi.fn(() => { + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, CREATE_PROCESS_DELAY_MS) + return ' Path REG_EXPAND_SZ C:\\Delayed' + }) +) + +vi.mock('node:child_process', () => ({ + execFile: vi.fn(), + execFileSync: delayedExecFileSync +})) + +describe('persisted Windows PATH process creation', () => { + afterEach(() => { + __resetPersistedWindowsPathCacheForTests() + __setWindowsPathRegistryLoaderForTests() + delayedExecFileSync.mockClear() + }) + + async function measureMainLoopGap(run: () => T): Promise<{ + callMs: number + maxGapMs: number + result: T + }> { + let lastTick = performance.now() + let maxGapMs = 0 + const timer = setInterval(() => { + const now = performance.now() + maxGapMs = Math.max(maxGapMs, now - lastTick - 5) + lastTick = now + }, 5) + await new Promise((resolve) => setTimeout(resolve, 20)) + + const startedAt = performance.now() + const result = run() + const callMs = performance.now() - startedAt + await new Promise((resolve) => setTimeout(resolve, 20)) + clearInterval(timer) + return { callMs, maxGapMs, result } + } + + it('proves delayed process creation blocks while native registry reads create no process', async () => { + const legacy = await measureMainLoopGap(() => + readPersistedWindowsPathSegments({ + platform: 'win32', + env: { SystemRoot: 'C:\\Windows' }, + execFileSync: delayedExecFileSync as never + }) + ) + + expect(legacy.result).toEqual(['C:\\Delayed', 'C:\\Delayed']) + expect(delayedExecFileSync).toHaveBeenCalledTimes(2) + expect(legacy.callMs).toBeGreaterThanOrEqual(CREATE_PROCESS_DELAY_MS * 2) + expect(legacy.maxGapMs).toBeGreaterThanOrEqual(CREATE_PROCESS_DELAY_MS) + + __resetPersistedWindowsPathCacheForTests() + delayedExecFileSync.mockClear() + __setWindowsPathRegistryLoaderForTests(() => ({ + HK: { LM: 1, CU: 2 }, + getRegistryKey: (root) => ({ + Path: { type: 1, value: root === 1 ? 'C:\\Machine' : 'C:\\User' } + }) + })) + const native = readPersistedWindowsPathSegments({ + platform: 'win32', + env: { SystemRoot: 'C:\\Windows' } + }) + + expect(native).toEqual(['C:\\Machine', 'C:\\User']) + expect(delayedExecFileSync).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/pty/windows-environment-path.test.ts b/src/main/pty/windows-environment-path.test.ts index d639ee604ce..e1ace3350ca 100644 --- a/src/main/pty/windows-environment-path.test.ts +++ b/src/main/pty/windows-environment-path.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' const { defaultExecFileMock, defaultExecFileSyncMock } = vi.hoisted(() => ({ defaultExecFileMock: vi.fn(), @@ -18,8 +18,26 @@ import { readPersistedWindowsPathSegmentsAsync, resolvePathEnvKey } from './windows-environment-path' +import { __setWindowsPathRegistryLoaderForTests } from './windows-path-registry-reader' -type ExecCallback = (error: Error | null, stdout: string, stderr: string) => void +const registryGetKeyMock = vi.fn() + +function registryPath(value: string): Record { + return { Path: { type: 1, value } } +} + +beforeEach(() => { + registryGetKeyMock.mockReset() + __setWindowsPathRegistryLoaderForTests(() => ({ + HK: { LM: 1, CU: 2 }, + getRegistryKey: registryGetKeyMock + })) +}) + +afterEach(() => { + __resetPersistedWindowsPathCacheForTests() + __setWindowsPathRegistryLoaderForTests() +}) describe('readPersistedWindowsPathSegments', () => { it('reads machine and user Path values from the Windows registry', () => { @@ -67,18 +85,17 @@ describe('readPersistedWindowsPathSegments', () => { it('caches production registry reads briefly', () => { const originalPlatform = process.platform Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) - defaultExecFileSyncMock - .mockReturnValueOnce(' Path REG_SZ C:\\Machine\r\n') - .mockReturnValueOnce(' Path REG_SZ C:\\User\r\n') + registryGetKeyMock + .mockReturnValueOnce(registryPath('C:\\Machine')) + .mockReturnValueOnce(registryPath('C:\\User')) __resetPersistedWindowsPathCacheForTests() try { expect(readPersistedWindowsPathSegments()).toEqual(['C:\\Machine', 'C:\\User']) expect(readPersistedWindowsPathSegments()).toEqual(['C:\\Machine', 'C:\\User']) - expect(defaultExecFileSyncMock).toHaveBeenCalledTimes(2) + expect(registryGetKeyMock).toHaveBeenCalledTimes(2) } finally { __resetPersistedWindowsPathCacheForTests() - defaultExecFileSyncMock.mockReset() Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) } }) @@ -86,27 +103,26 @@ describe('readPersistedWindowsPathSegments', () => { it('force-refreshes the production registry cache', () => { const originalPlatform = process.platform Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) - defaultExecFileSyncMock - .mockReturnValueOnce(' Path REG_SZ C:\\Machine\r\n') - .mockReturnValueOnce(' Path REG_SZ C:\\User\r\n') - .mockReturnValueOnce(' Path REG_SZ C:\\Machine\r\n') - .mockReturnValueOnce(' Path REG_SZ C:\\User;C:\\Program Files\\GitHub CLI\r\n') + registryGetKeyMock + .mockReturnValueOnce(registryPath('C:\\Machine')) + .mockReturnValueOnce(registryPath('C:\\User')) + .mockReturnValueOnce(registryPath('C:\\Machine')) + .mockReturnValueOnce(registryPath('C:\\User;C:\\Program Files\\GitHub CLI')) __resetPersistedWindowsPathCacheForTests() try { expect(readPersistedWindowsPathSegments()).toEqual(['C:\\Machine', 'C:\\User']) expect(readPersistedWindowsPathSegments()).toEqual(['C:\\Machine', 'C:\\User']) - expect(defaultExecFileSyncMock).toHaveBeenCalledTimes(2) + expect(registryGetKeyMock).toHaveBeenCalledTimes(2) expect(readPersistedWindowsPathSegments({ forceRefresh: true })).toEqual([ 'C:\\Machine', 'C:\\User', 'C:\\Program Files\\GitHub CLI' ]) - expect(defaultExecFileSyncMock).toHaveBeenCalledTimes(4) + expect(registryGetKeyMock).toHaveBeenCalledTimes(4) } finally { __resetPersistedWindowsPathCacheForTests() - defaultExecFileSyncMock.mockReset() Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) } }) @@ -114,9 +130,9 @@ describe('readPersistedWindowsPathSegments', () => { it('keeps the last good segments when a forced read hits a blocked registry', () => { const originalPlatform = process.platform Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) - defaultExecFileSyncMock - .mockReturnValueOnce(' Path REG_SZ C:\\Machine\r\n') - .mockReturnValueOnce(' Path REG_SZ C:\\User\r\n') + registryGetKeyMock + .mockReturnValueOnce(registryPath('C:\\Machine')) + .mockReturnValueOnce(registryPath('C:\\User')) .mockImplementation(() => { throw new Error('ERROR: Access is denied.') }) @@ -131,7 +147,6 @@ describe('readPersistedWindowsPathSegments', () => { expect(readPersistedWindowsPathSegments()).toEqual(['C:\\Machine', 'C:\\User']) } finally { __resetPersistedWindowsPathCacheForTests() - defaultExecFileSyncMock.mockReset() Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) } }) @@ -139,10 +154,10 @@ describe('readPersistedWindowsPathSegments', () => { it('still clears the cache when the registry reports an empty Path', () => { const originalPlatform = process.platform Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) - defaultExecFileSyncMock - .mockReturnValueOnce(' Path REG_SZ C:\\Machine\r\n') - .mockReturnValueOnce(' Path REG_SZ C:\\User\r\n') - .mockReturnValue(' Path REG_SZ \r\n') + registryGetKeyMock + .mockReturnValueOnce(registryPath('C:\\Machine')) + .mockReturnValueOnce(registryPath('C:\\User')) + .mockReturnValue(registryPath('')) __resetPersistedWindowsPathCacheForTests() try { @@ -154,7 +169,6 @@ describe('readPersistedWindowsPathSegments', () => { expect(readPersistedWindowsPathSegments()).toEqual([]) } finally { __resetPersistedWindowsPathCacheForTests() - defaultExecFileSyncMock.mockReset() Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) } }) @@ -162,13 +176,9 @@ describe('readPersistedWindowsPathSegments', () => { it('deduplicates concurrent forced asynchronous refreshes and merges each environment', async () => { const originalPlatform = process.platform Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) - const callbacks: ExecCallback[] = [] - defaultExecFileMock.mockImplementation( - (_file: string, _args: string[], _options: unknown, callback: ExecCallback) => { - callbacks.push(callback) - return {} as never - } - ) + registryGetKeyMock + .mockReturnValueOnce(registryPath('C:\\Machine')) + .mockReturnValueOnce(registryPath('C:\\User')) __resetPersistedWindowsPathCacheForTests() try { @@ -177,41 +187,25 @@ describe('readPersistedWindowsPathSegments', () => { const first = mergePersistedWindowsPathAsync(firstEnv, { forceRefresh: true }) const second = mergePersistedWindowsPathAsync(secondEnv, { forceRefresh: true }) - expect(defaultExecFileMock).toHaveBeenCalledTimes(2) - callbacks[0]?.(null, ' Path REG_SZ C:\\Machine\r\n', '') - callbacks[1]?.(null, ' Path REG_SZ C:\\User\r\n', '') await Promise.all([first, second]) expect(firstEnv.Path).toBe('C:\\First;C:\\Machine;C:\\User') expect(secondEnv.Path).toBe('C:\\Second;C:\\Machine;C:\\User') + expect(registryGetKeyMock).toHaveBeenCalledTimes(2) } finally { __resetPersistedWindowsPathCacheForTests() - defaultExecFileMock.mockReset() Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) } }) - it('keeps the last good cache when bounded asynchronous reads time out', async () => { + it('keeps the last good cache when native registry reads fail', async () => { const originalPlatform = process.platform Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) - defaultExecFileMock - .mockImplementationOnce( - (_file: string, _args: string[], _options: unknown, callback: ExecCallback) => { - callback(null, ' Path REG_SZ C:\\Machine\r\n', '') - return {} as never - } - ) - .mockImplementationOnce( - (_file: string, _args: string[], _options: unknown, callback: ExecCallback) => { - callback(null, ' Path REG_SZ C:\\User\r\n', '') - return {} as never - } - ) - .mockImplementation( - (_file: string, _args: string[], _options: unknown, callback: ExecCallback) => { - callback(Object.assign(new Error('timed out'), { code: 'ETIMEDOUT' }), '', '') - return {} as never - } - ) + registryGetKeyMock + .mockReturnValueOnce(registryPath('C:\\Machine')) + .mockReturnValueOnce(registryPath('C:\\User')) + .mockImplementation(() => { + throw new Error('registry unavailable') + }) __resetPersistedWindowsPathCacheForTests() try { @@ -223,7 +217,7 @@ describe('readPersistedWindowsPathSegments', () => { 'C:\\Machine', 'C:\\User' ]) - expect(defaultExecFileMock).toHaveBeenCalledTimes(2) + expect(registryGetKeyMock).toHaveBeenCalledTimes(2) await expect(readPersistedWindowsPathSegmentsAsync({ forceRefresh: true })).resolves.toEqual([ 'C:\\Machine', 'C:\\User' @@ -232,11 +226,9 @@ describe('readPersistedWindowsPathSegments', () => { 'C:\\Machine', 'C:\\User' ]) - expect(defaultExecFileMock).toHaveBeenCalledTimes(4) - expect(defaultExecFileMock.mock.calls[2]?.[2]).toMatchObject({ timeout: 5_000 }) + expect(registryGetKeyMock).toHaveBeenCalledTimes(4) } finally { __resetPersistedWindowsPathCacheForTests() - defaultExecFileMock.mockReset() Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) } }) diff --git a/src/main/pty/windows-environment-path.ts b/src/main/pty/windows-environment-path.ts index 36032da9aec..8fad453cab5 100644 --- a/src/main/pty/windows-environment-path.ts +++ b/src/main/pty/windows-environment-path.ts @@ -1,9 +1,10 @@ -import { execFile, execFileSync } from 'node:child_process' +import type { execFile, execFileSync } from 'node:child_process' import { expandWindowsEnvironmentVariables, expandWindowsPathEnvironmentVariables } from '../../shared/windows-environment-expansion' import { getRegExePath } from '../win32-utils' +import { readWindowsPathRegistry } from './windows-path-registry-reader' type ExecFile = typeof execFile type ExecFileSync = typeof execFileSync @@ -16,10 +17,7 @@ type ReadWindowsPathOptions = { platform?: NodeJS.Platform } -type RegistryPathRead = { - failed: boolean - segments: string[] -} +type RegistryPathRead = { failed: boolean; segments: string[] } const WINDOWS_PATH_REGISTRY_KEYS = [ ['HKLM\\SYSTEM\\CurrentControlSet\\Control\\Session Manager\\Environment', 'Path'], @@ -71,6 +69,19 @@ function registryOutputSegments( : [] } +function readNativeRegistryPaths( + env: NodeJS.ProcessEnv, + pathDelimiter: string +): RegistryPathRead[] { + return readWindowsPathRegistry().map((read) => ({ + failed: read.failed, + segments: + read.value === null + ? [] + : splitPathSegments(expandWindowsEnvironmentVariables(read.value, env), pathDelimiter) + })) +} + function keepLastGoodSegments(segments: string[], failedReads: number): string[] { if (failedReads === WINDOWS_PATH_REGISTRY_KEYS.length && persistedWindowsPathCache) { return [...persistedWindowsPathCache.segments] @@ -145,34 +156,39 @@ export function readPersistedWindowsPathSegments(options: ReadWindowsPathOptions return [...persistedWindowsPathCache.segments] } - const run = options.execFileSync ?? execFileSync const env = options.env ?? process.env const pathDelimiter = getPathDelimiter(platform) - const segments: string[] = [] - let failedReads = 0 - - for (const [key, valueName] of WINDOWS_PATH_REGISTRY_KEYS) { - try { - const output = run(getRegExePath(env), ['query', key, '/v', valueName], { - encoding: 'utf8', - timeout: PERSISTED_WINDOWS_PATH_QUERY_TIMEOUT_MS, - windowsHide: true + const reads = options.execFileSync + ? WINDOWS_PATH_REGISTRY_KEYS.map(([key, valueName]) => { + try { + const output = options.execFileSync!( + getRegExePath(env), + ['query', key, '/v', valueName], + { + encoding: 'utf8', + timeout: PERSISTED_WINDOWS_PATH_QUERY_TIMEOUT_MS, + windowsHide: true + } + ) + return { + failed: false, + segments: registryOutputSegments(output, valueName, env, pathDelimiter) + } + } catch { + return { failed: true, segments: [] } + } }) - segments.push(...registryOutputSegments(output, valueName, env, pathDelimiter)) - } catch { - // Registry access can fail in stripped test containers or remote-like - // Windows contexts. Existing PATH remains the fallback in those cases. - failedReads += 1 - } - } + : readNativeRegistryPaths(env, pathDelimiter) + const segments = reads.flatMap((read) => read.segments) + const failedReads = reads.filter((read) => read.failed).length if (!useProductionCache) { return segments } - // Why: local PTY spawn is a hot path on Windows, and each uncached read - // runs two synchronous `reg.exe query` subprocesses. A short TTL keeps - // terminal bursts cheap while still picking up newly installed CLIs soon. + // Why: local PTY spawn is a hot path on Windows, and each uncached refresh performs two + // synchronous native registry reads. A short TTL keeps terminal bursts cheap while still + // picking up newly installed CLIs soon. return cachePersistedWindowsPathSegments(segments, failedReads) } @@ -202,14 +218,22 @@ export async function readPersistedWindowsPathSegmentsAsync( return [...(await pendingPersistedWindowsPathRefresh)] } - const run = options.execFile ?? execFile const env = options.env ?? process.env const pathDelimiter = getPathDelimiter(platform) - const executable = getRegExePath(env) - const refresh = Promise.all( - WINDOWS_PATH_REGISTRY_KEYS.map((registryValue) => - readRegistryPathAsync(run, executable, registryValue, env, pathDelimiter) - ) + const refresh = ( + options.execFile + ? Promise.all( + WINDOWS_PATH_REGISTRY_KEYS.map((registryValue) => + readRegistryPathAsync( + options.execFile!, + getRegExePath(env), + registryValue, + env, + pathDelimiter + ) + ) + ) + : Promise.resolve(readNativeRegistryPaths(env, pathDelimiter)) ).then((reads) => { const segments = reads.flatMap((read) => read.segments) if (!useProductionCache) { diff --git a/src/main/pty/windows-path-registry-reader.test.ts b/src/main/pty/windows-path-registry-reader.test.ts new file mode 100644 index 00000000000..634cd30e72a --- /dev/null +++ b/src/main/pty/windows-path-registry-reader.test.ts @@ -0,0 +1,70 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { + __setWindowsPathRegistryLoaderForTests, + readWindowsPathRegistry +} from './windows-path-registry-reader' + +describe('readWindowsPathRegistry', () => { + afterEach(() => __setWindowsPathRegistryLoaderForTests()) + + it('reads machine and user PATH values without creating a process', () => { + const getRegistryKey = vi + .fn() + .mockReturnValueOnce({ PATH: { type: 2, value: '%SystemRoot%\\System32' } }) + .mockReturnValueOnce({ Path: { type: 1, value: 'C:\\Users\\me\\bin' } }) + __setWindowsPathRegistryLoaderForTests(() => ({ + HK: { LM: 1, CU: 2 }, + getRegistryKey + })) + + expect(readWindowsPathRegistry()).toEqual([ + { failed: false, value: '%SystemRoot%\\System32' }, + { failed: false, value: 'C:\\Users\\me\\bin' } + ]) + expect(getRegistryKey).toHaveBeenNthCalledWith( + 1, + 1, + 'SYSTEM\\CurrentControlSet\\Control\\Session Manager\\Environment' + ) + expect(getRegistryKey).toHaveBeenNthCalledWith(2, 2, 'Environment') + }) + + it('distinguishes an empty PATH from a failed registry read', () => { + const getRegistryKey = vi + .fn() + .mockReturnValueOnce({ Path: { type: 1, value: '' } }) + .mockReturnValueOnce({ Path: { type: 4, value: 1 } }) + __setWindowsPathRegistryLoaderForTests(() => ({ + HK: { LM: 1, CU: 2 }, + getRegistryKey + })) + + expect(readWindowsPathRegistry()).toEqual([ + { failed: false, value: '' }, + { failed: true, value: null } + ]) + }) + + it('treats a missing PATH value as a failed query', () => { + __setWindowsPathRegistryLoaderForTests(() => ({ + HK: { LM: 1, CU: 2 }, + getRegistryKey: vi.fn(() => ({})) + })) + + expect(readWindowsPathRegistry()).toEqual([ + { failed: true, value: null }, + { failed: true, value: null } + ]) + }) + + it('fails closed when the optional native module is unavailable', () => { + __setWindowsPathRegistryLoaderForTests(() => { + throw new Error('native module unavailable') + }) + + expect(readWindowsPathRegistry()).toEqual([ + { failed: true, value: null }, + { failed: true, value: null } + ]) + }) +}) diff --git a/src/main/pty/windows-path-registry-reader.ts b/src/main/pty/windows-path-registry-reader.ts new file mode 100644 index 00000000000..6f98c45e390 --- /dev/null +++ b/src/main/pty/windows-path-registry-reader.ts @@ -0,0 +1,67 @@ +import { + loadWindowsNativeRegistry, + WINDOWS_REG_EXPAND_SZ, + WINDOWS_REG_SZ, + type WindowsNativeRegistryModule +} from '../windows-native-registry' + +export type WindowsPathRegistryRead = { + failed: boolean + value: string | null +} + +const MACHINE_ENVIRONMENT_KEY = 'SYSTEM\\CurrentControlSet\\Control\\Session Manager\\Environment' +const USER_ENVIRONMENT_KEY = 'Environment' +const PATH_VALUE = 'Path' + +let windowsRegistryLoader = loadWindowsNativeRegistry + +function readRegistryPath( + registry: WindowsNativeRegistryModule, + root: number, + key: string +): WindowsPathRegistryRead { + try { + const values = registry.getRegistryKey(root, key) + if (!values || typeof values !== 'object') { + return { failed: true, value: null } + } + const entry = Object.entries(values).find( + ([name]) => name.toLowerCase() === PATH_VALUE.toLowerCase() + )?.[1] + if (!entry) { + return { failed: true, value: null } + } + if ( + (entry.type !== WINDOWS_REG_SZ && entry.type !== WINDOWS_REG_EXPAND_SZ) || + typeof entry.value !== 'string' + ) { + return { failed: true, value: null } + } + return { failed: false, value: entry.value } + } catch { + return { failed: true, value: null } + } +} + +export function readWindowsPathRegistry(): WindowsPathRegistryRead[] { + let registry: WindowsNativeRegistryModule + try { + registry = windowsRegistryLoader() + } catch { + return [ + { failed: true, value: null }, + { failed: true, value: null } + ] + } + return [ + readRegistryPath(registry, registry.HK.LM, MACHINE_ENVIRONMENT_KEY), + readRegistryPath(registry, registry.HK.CU, USER_ENVIRONMENT_KEY) + ] +} + +export function __setWindowsPathRegistryLoaderForTests( + loader?: () => WindowsNativeRegistryModule +): void { + windowsRegistryLoader = loader ?? loadWindowsNativeRegistry +} diff --git a/src/main/windows-native-registry.ts b/src/main/windows-native-registry.ts new file mode 100644 index 00000000000..e3a2d77889d --- /dev/null +++ b/src/main/windows-native-registry.ts @@ -0,0 +1,25 @@ +import { createRequire } from 'node:module' + +export type WindowsNativeRegistryValue = { + name?: unknown + type?: unknown + value?: unknown +} + +export type WindowsNativeRegistryModule = { + HK: { CU: number; LM: number } + getRegistryKey: ( + root: number, + path: string + ) => Record | null | undefined +} + +export const WINDOWS_REG_SZ = 1 +export const WINDOWS_REG_EXPAND_SZ = 2 + +const requireFromMain = createRequire(__filename) + +export function loadWindowsNativeRegistry(): WindowsNativeRegistryModule { + // Why: non-Windows installs omit this optional dependency, so never resolve it at module load. + return requireFromMain('windows-native-registry') as WindowsNativeRegistryModule +}