fix(windows): eliminate console windows from proxy-internal identity & process lookups - #1279
fix(windows): eliminate console windows from proxy-internal identity & process lookups#1279wade19990814-hue wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughWindows identity resolution now uses trusted ChangesWindows identity and profile resolution
WMIC discovery and record parsing
WMIC-first process introspection
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ProcessProbe
participant WMIC
participant PowerShellCIM
participant ProcessState
ProcessProbe->>WMIC: Enumerate process records
WMIC-->>ProcessProbe: Return validated records
alt WMIC unavailable
ProcessProbe->>PowerShellCIM: Query filtered process data
PowerShellCIM-->>ProcessProbe: Return process data
end
ProcessProbe->>ProcessState: Store process identity and start times
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/codex/app-server-processes.ts`:
- Around line 512-525: Update the WMIC query flow in the function containing
resolveWmicExe and readWindowsProcStartMsViaPowerShell so both a missing WMIC
executable and any WMIC query failure fall back to
readWindowsProcStartMsViaPowerShell(pid). Preserve the existing successful WMIC
parsing behavior, but replace the failure path that returns null with the
PowerShell fallback.
- Around line 582-600: Update the Windows branch around resolveWmicExe and
execFileSync to discard unusable PIDs (non-finite, non-integer, or non-positive)
before constructing WQL, while preserving null results for rejected inputs.
Split the remaining valid PIDs into bounded chunks, execute and parse each WMIC
query independently, merge creation times into out, and assign null for any
valid PID missing from all successful chunk results so one oversized query or
invalid input cannot discard every result.
In `@src/codex/native-profile-processes.ts`:
- Around line 98-110: Update the record-counting loop in the WMIC process
enumeration to explicitly skip the WMIC helper process, matching the guard used
by listWindowsSnapshotsViaWmic. Identify that helper by its command line before
applying WINDOWS_CODEX_NAME_RE or WINDOWS_CODEX_CMDLINE_RE, while preserving the
existing selfPid exclusion and invalid-empty-list behavior.
In `@src/codex/user-identity.ts`:
- Around line 50-67: Replace the inline Bun.spawnSync logic in whoamiValue with
runWhoamiSync(WHOAMI_TIMEOUT_MS) from windows-whoami.ts. Handle timedOut and
unsuccessful results through refuse, set stderr to "ignore" before decoding
result.stdout, and preserve the existing SID parsing and invalid-SID rejection.
- Around line 147-162: Replace the spread-based String.fromCharCode conversion
in the profile-resolution flow with TextDecoder("utf-16le") decoding of the
null-terminated Uint16Array, preserving surrogate pairs and avoiding
argument-limit failures. In the retry logic around
libraries.getUserProfileDirectoryW, retry only when GetLastError() reports
ERROR_INSUFFICIENT_BUFFER (122); otherwise preserve the original failure path
and message.
- Around line 107-125: Update the Win32 handle declarations in the windows
profile library setup to use u64 for GetCurrentProcess’s return value and
OpenProcessToken’s process and token pointer parameters. Preserve token handles
as bigint throughout the related wrappers and update the associated type
aliases, including getCurrentProcess, openProcessToken, and downstream
token-handling functions, so pseudo-handles pass correctly through bun:ffi.
In `@src/lib/windows-elevation.ts`:
- Around line 216-223: Update resolveTrustedWindowsWmicExe to reject
empty-string test overrides while preserving null as the explicit absent value,
and route an existing candidate through assertTrustedSystemExecutable when
existsSync confirms the file exists. Keep returning null when WMIC is absent and
retain the sibling resolver validation pattern.
In `@src/lib/windows-whoami.ts`:
- Line 15: Make WINDOWS_SID_PATTERN the sole SID validation rule: import and use
it in the consumers’ SID checks, including windows-user-principal.ts and the
SID_PATTERN usages in user-identity.ts, then remove their local SID_PATTERN
declarations. Preserve all existing validation behavior while eliminating
duplicate regular expressions.
- Around line 59-89: Update runWhoamiAsync to use Bun.spawn’s native timeout
option and proc.timedOut, removing the setTimeout and manual proc.kill logic.
Ensure stdout is consumed in the finally path even when proc.exited rejects, and
derive the returned success, exitCode, and timedOut fields from the native
timeout state after output consumption.
In `@src/lib/windows-wmic.ts`:
- Around line 124-138: Move the resolveTrustedWindowsWhoamiExe() call inside the
try block in currentWindowsAccount() so resolver failures also return null,
preserving the documented failure contract and existing fail-closed behavior.
- Around line 102-121: Update wmicGetOwner to accept an optional resolved wmic
path parameter and use it when provided, falling back to resolveWmicExe only
when omitted. Update listWindowsSnapshotsViaWmic to pass its already-resolved
path into each wmicGetOwner call, preserving the existing null handling and
owner lookup behavior.
- Around line 74-94: Update parseWmicCreationDate to validate the parsed year,
month, day, hour, minute, second, and timezone offset ranges before calling
Date.UTC. Return null for any out-of-range component, preserving conversion for
valid WMIC values and the existing null-on-unparseable contract.
In `@tests/codex-app-server-processes.test.ts`:
- Around line 516-533: In the cleanup blocks at
tests/codex-app-server-processes.test.ts:516-533 and :582-593, retain the
marker-verified taskkill using survivor, then add the same unconditional
taskkill tree fallback for child.pid inside each existing try/catch. The
fallback must force-kill the spawned child and its process tree with
Windows-hidden, ignored stdio options so cleanup still occurs when WMIC
verification produces no output.
In `@tests/native-profile-processes.test.ts`:
- Around line 34-63: Extend the existing “uses WMIC with shell-free bounded
execution and counts Codex processes” test’s mocked WMIC output with a WMIC.exe
record whose CommandLine contains the WQL literals, then keep the expected busy
count at 2. Ensure the regression covers the explicit helper-record exclusion in
probeNativeCodexProcesses without changing the existing invocation assertions.
In `@tests/windows-user-principal.test.ts`:
- Around line 28-31: Add focused regression tests in the appropriate test layers
for parseWindowsSidFromWhoami, covering localized whoami /user output, empty or
missing SID output, and CRLF line endings; also cover windowsProfileDirectory
and localAppDataValue behavior for CRLF input. Keep the existing
trusted-executable test cleanup unchanged.
In `@tests/windows-wmic.test.ts`:
- Around line 86-94: Add a test alongside the existing parseWmicCreationDate
offset tests using a negative offset such as -300, and assert that the result
follows the same utcMs minus offsetMinutes conversion, confirming
western-hemisphere timestamps are adjusted in the correct direction.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 138eeb14-0f4a-4868-97ac-8437e55d6c0a
📒 Files selected for processing (11)
src/codex/app-server-processes.tssrc/codex/native-profile-processes.tssrc/codex/user-identity.tssrc/lib/windows-elevation.tssrc/lib/windows-user-principal.tssrc/lib/windows-whoami.tssrc/lib/windows-wmic.tstests/codex-app-server-processes.test.tstests/native-profile-processes.test.tstests/windows-user-principal.test.tstests/windows-wmic.test.ts
| const wmic = resolveWmicExe(); | ||
| if (!wmic) return readWindowsProcStartMsViaPowerShell(pid); | ||
| try { | ||
| const out = execFileSync("powershell.exe", [ | ||
| const out = execFileSync( | ||
| wmic, | ||
| ["process", "where", `ProcessId=${pid}`, "get", "CreationDate", "/format:list"], | ||
| { encoding: "utf-8", stdio: ["ignore", "pipe", "ignore"], timeout: 5_000, windowsHide: true }, | ||
| ); | ||
| const match = /^CreationDate=(.*)$/m.exec(out.replace(/\r/g, "")); | ||
| return parseWmicCreationDate(match?.[1]); | ||
| } catch { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Fall back to PowerShell when the WMIC start-time query fails, not only when WMIC is absent.
Line 513 routes to readWindowsProcStartMsViaPowerShell only when resolveWmicExe() returns null. If WMIC exists but the call fails (timeout on a contended host, WMI repository error, /format:list returning no CreationDate line), the catch at Line 522 returns null and the working PowerShell path is never tried. The caller then treats the process start time as unknown, which weakens the PID-reuse guard that readProcessStartMs exists to support.
Reuse the fallback for both conditions.
🛡️ Proposed fallback on failure
const match = /^CreationDate=(.*)$/m.exec(out.replace(/\r/g, ""));
- return parseWmicCreationDate(match?.[1]);
+ return parseWmicCreationDate(match?.[1]) ?? readWindowsProcStartMsViaPowerShell(pid);
} catch {
- return null;
+ return readWindowsProcStartMsViaPowerShell(pid);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const wmic = resolveWmicExe(); | |
| if (!wmic) return readWindowsProcStartMsViaPowerShell(pid); | |
| try { | |
| const out = execFileSync("powershell.exe", [ | |
| const out = execFileSync( | |
| wmic, | |
| ["process", "where", `ProcessId=${pid}`, "get", "CreationDate", "/format:list"], | |
| { encoding: "utf-8", stdio: ["ignore", "pipe", "ignore"], timeout: 5_000, windowsHide: true }, | |
| ); | |
| const match = /^CreationDate=(.*)$/m.exec(out.replace(/\r/g, "")); | |
| return parseWmicCreationDate(match?.[1]); | |
| } catch { | |
| return null; | |
| } | |
| } | |
| const wmic = resolveWmicExe(); | |
| if (!wmic) return readWindowsProcStartMsViaPowerShell(pid); | |
| try { | |
| const out = execFileSync( | |
| wmic, | |
| ["process", "where", `ProcessId=${pid}`, "get", "CreationDate", "/format:list"], | |
| { encoding: "utf-8", stdio: ["ignore", "pipe", "ignore"], timeout: 5_000, windowsHide: true }, | |
| ); | |
| const match = /^CreationDate=(.*)$/m.exec(out.replace(/\r/g, "")); | |
| return parseWmicCreationDate(match?.[1]) ?? readWindowsProcStartMsViaPowerShell(pid); | |
| } catch { | |
| return readWindowsProcStartMsViaPowerShell(pid); | |
| } | |
| } |
🧰 Tools
🪛 OpenGrep (1.26.0)
[ERROR] 520-520: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/codex/app-server-processes.ts` around lines 512 - 525, Update the WMIC
query flow in the function containing resolveWmicExe and
readWindowsProcStartMsViaPowerShell so both a missing WMIC executable and any
WMIC query failure fall back to readWindowsProcStartMsViaPowerShell(pid).
Preserve the existing successful WMIC parsing behavior, but replace the failure
path that returns null with the PowerShell fallback.
| if (platform === "win32") { | ||
| const wmic = resolveWmicExe(); | ||
| if (!wmic) { | ||
| for (const pid of pids) out.set(pid, null); | ||
| return out; | ||
| } | ||
| try { | ||
| const filter = pids.map(pid => `ProcessId=${pid}`).join(" OR "); | ||
| const stdout = execFileSync("powershell.exe", [ | ||
| "-NoProfile", "-NoLogo", "-NonInteractive", "-WindowStyle", "Hidden", | ||
| "-Command", | ||
| `Get-CimInstance Win32_Process -Filter "${filter}" | ForEach-Object { "$($_.ProcessId)\t$($_.CreationDate.ToUniversalTime().ToString("o"))" }`, | ||
| ], { encoding: "utf-8", stdio: ["ignore", "pipe", "ignore"], timeout: 5_000, windowsHide: true }); | ||
| const byPid = new Map<number, number>(); | ||
| for (const line of stdout.split(/\r?\n/)) { | ||
| const tab = line.indexOf("\t"); | ||
| if (tab <= 0) continue; | ||
| const pid = Number(line.slice(0, tab)); | ||
| const parsed = Date.parse(line.slice(tab + 1).trim()); | ||
| if (Number.isSafeInteger(pid) && Number.isFinite(parsed)) byPid.set(pid, parsed); | ||
| const filter = pids.map(pid => `ProcessId=${pid}`).join(" or "); | ||
| const stdout = execFileSync( | ||
| wmic, | ||
| ["process", "where", filter, "get", "ProcessId,CreationDate", "/format:list"], | ||
| { encoding: "utf-8", stdio: ["ignore", "pipe", "ignore"], timeout: 5_000, windowsHide: true }, | ||
| ); | ||
| for (const record of parseWmicListRecords(stdout)) { | ||
| if (record.creationDate !== undefined) { | ||
| out.set(record.processId, parseWmicCreationDate(record.creationDate)); | ||
| } | ||
| } | ||
| for (const pid of pids) out.set(pid, byPid.get(pid) ?? null); | ||
| for (const pid of pids) if (!out.has(pid)) out.set(pid, null); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Chunk the WMIC batch filter and reject unusable PIDs before building the WQL.
Two concrete failure modes exist in this branch:
- Line 589 builds one WQL clause per PID with no upper bound.
pidsis an unboundedreadonly number[]. Each term costs about 15 characters, so a few thousand PIDs push the WMIC command line past the Windows limit (about 32767 characters).execFileSyncthen fails, thecatchat Line 602 maps every PID tonull, and all start times become unknown in one shot. - No PID validation occurs. A single
NaN, negative, or fractional entry produces a term such asProcessId=NaN, which makes WMIC reject the whole query. One bad input therefore discards the result for every valid PID in the batch.
Chunk the query and filter the PIDs first. The single-PID path at Lines 511-525 has the same lack of validation, but it fails in isolation there.
🛡️ Proposed chunking and validation
try {
- const filter = pids.map(pid => `ProcessId=${pid}`).join(" or ");
- const stdout = execFileSync(
- wmic,
- ["process", "where", filter, "get", "ProcessId,CreationDate", "/format:list"],
- { encoding: "utf-8", stdio: ["ignore", "pipe", "ignore"], timeout: 5_000, windowsHide: true },
- );
- for (const record of parseWmicListRecords(stdout)) {
- if (record.creationDate !== undefined) {
- out.set(record.processId, parseWmicCreationDate(record.creationDate));
- }
- }
+ const usable = pids.filter(pid => Number.isSafeInteger(pid) && pid > 1);
+ const WMIC_BATCH_SIZE = 200;
+ for (let i = 0; i < usable.length; i += WMIC_BATCH_SIZE) {
+ const chunk = usable.slice(i, i + WMIC_BATCH_SIZE);
+ const filter = chunk.map(pid => `ProcessId=${pid}`).join(" or ");
+ const stdout = execFileSync(
+ wmic,
+ ["process", "where", filter, "get", "ProcessId,CreationDate", "/format:list"],
+ { encoding: "utf-8", stdio: ["ignore", "pipe", "ignore"], timeout: 5_000, windowsHide: true },
+ );
+ for (const record of parseWmicListRecords(stdout)) {
+ if (record.creationDate !== undefined) {
+ out.set(record.processId, parseWmicCreationDate(record.creationDate));
+ }
+ }
+ }
for (const pid of pids) if (!out.has(pid)) out.set(pid, null);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (platform === "win32") { | |
| const wmic = resolveWmicExe(); | |
| if (!wmic) { | |
| for (const pid of pids) out.set(pid, null); | |
| return out; | |
| } | |
| try { | |
| const filter = pids.map(pid => `ProcessId=${pid}`).join(" OR "); | |
| const stdout = execFileSync("powershell.exe", [ | |
| "-NoProfile", "-NoLogo", "-NonInteractive", "-WindowStyle", "Hidden", | |
| "-Command", | |
| `Get-CimInstance Win32_Process -Filter "${filter}" | ForEach-Object { "$($_.ProcessId)\t$($_.CreationDate.ToUniversalTime().ToString("o"))" }`, | |
| ], { encoding: "utf-8", stdio: ["ignore", "pipe", "ignore"], timeout: 5_000, windowsHide: true }); | |
| const byPid = new Map<number, number>(); | |
| for (const line of stdout.split(/\r?\n/)) { | |
| const tab = line.indexOf("\t"); | |
| if (tab <= 0) continue; | |
| const pid = Number(line.slice(0, tab)); | |
| const parsed = Date.parse(line.slice(tab + 1).trim()); | |
| if (Number.isSafeInteger(pid) && Number.isFinite(parsed)) byPid.set(pid, parsed); | |
| const filter = pids.map(pid => `ProcessId=${pid}`).join(" or "); | |
| const stdout = execFileSync( | |
| wmic, | |
| ["process", "where", filter, "get", "ProcessId,CreationDate", "/format:list"], | |
| { encoding: "utf-8", stdio: ["ignore", "pipe", "ignore"], timeout: 5_000, windowsHide: true }, | |
| ); | |
| for (const record of parseWmicListRecords(stdout)) { | |
| if (record.creationDate !== undefined) { | |
| out.set(record.processId, parseWmicCreationDate(record.creationDate)); | |
| } | |
| } | |
| for (const pid of pids) out.set(pid, byPid.get(pid) ?? null); | |
| for (const pid of pids) if (!out.has(pid)) out.set(pid, null); | |
| if (platform === "win32") { | |
| const wmic = resolveWmicExe(); | |
| if (!wmic) { | |
| for (const pid of pids) out.set(pid, null); | |
| return out; | |
| } | |
| try { | |
| const usable = pids.filter(pid => Number.isSafeInteger(pid) && pid > 1); | |
| const WMIC_BATCH_SIZE = 200; | |
| for (let i = 0; i < usable.length; i += WMIC_BATCH_SIZE) { | |
| const chunk = usable.slice(i, i + WMIC_BATCH_SIZE); | |
| const filter = chunk.map(pid => `ProcessId=${pid}`).join(" or "); | |
| const stdout = execFileSync( | |
| wmic, | |
| ["process", "where", filter, "get", "ProcessId,CreationDate", "/format:list"], | |
| { encoding: "utf-8", stdio: ["ignore", "pipe", "ignore"], timeout: 5_000, windowsHide: true }, | |
| ); | |
| for (const record of parseWmicListRecords(stdout)) { | |
| if (record.creationDate !== undefined) { | |
| out.set(record.processId, parseWmicCreationDate(record.creationDate)); | |
| } | |
| } | |
| } | |
| for (const pid of pids) if (!out.has(pid)) out.set(pid, null); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/codex/app-server-processes.ts` around lines 582 - 600, Update the Windows
branch around resolveWmicExe and execFileSync to discard unusable PIDs
(non-finite, non-integer, or non-positive) before constructing WQL, while
preserving null results for rejected inputs. Split the remaining valid PIDs into
bounded chunks, execute and parse each WMIC query independently, merge creation
times into out, and assign null for any valid PID missing from all successful
chunk results so one oversized query or invalid input cannot discard every
result.
| const records = parseWmicListRecords(output); | ||
| // A valid enumeration always includes at least our own process (the proxy | ||
| // command line contains "opencodex"), so an empty result is a failure. | ||
| if (records.length === 0) throw new Error("invalid process list"); | ||
| let count = 0; | ||
| for (const record of records) { | ||
| if (record.processId === selfPid) continue; | ||
| const nameMatch = record.name !== undefined && WINDOWS_CODEX_NAME_RE.test(record.name); | ||
| const commandLineMatch = record.commandLine !== undefined | ||
| && WINDOWS_CODEX_CMDLINE_RE.test(record.commandLine); | ||
| if (nameMatch || commandLineMatch) count += 1; | ||
| } | ||
| return count; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Exclude the WMIC helper process explicitly rather than relying on quote characters.
The WMIC child's own CommandLine contains the WQL literals 'codex%' and '%codex%', so WMIC returns its own process for this query. Today it escapes the count only because WINDOWS_CODEX_CMDLINE_RE requires the character before codex to be one of [\\/"\s], and the actual characters are ' and %. Any later relaxation of that character class silently adds a phantom Codex process and flips a clear probe to busy, which blocks profile writes.
listWindowsSnapshotsViaWmic in src/codex/app-server-processes.ts Lines 349-352 already guards this case explicitly. Mirror that guard here.
🛡️ Proposed explicit exclusion
for (const record of records) {
if (record.processId === selfPid) continue;
+ // WMIC's own CommandLine embeds the WQL "codex" literals, so skip it.
+ if (record.commandLine?.toLowerCase().includes(wmic.toLowerCase())) continue;
const nameMatch = record.name !== undefined && WINDOWS_CODEX_NAME_RE.test(record.name);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const records = parseWmicListRecords(output); | |
| // A valid enumeration always includes at least our own process (the proxy | |
| // command line contains "opencodex"), so an empty result is a failure. | |
| if (records.length === 0) throw new Error("invalid process list"); | |
| let count = 0; | |
| for (const record of records) { | |
| if (record.processId === selfPid) continue; | |
| const nameMatch = record.name !== undefined && WINDOWS_CODEX_NAME_RE.test(record.name); | |
| const commandLineMatch = record.commandLine !== undefined | |
| && WINDOWS_CODEX_CMDLINE_RE.test(record.commandLine); | |
| if (nameMatch || commandLineMatch) count += 1; | |
| } | |
| return count; | |
| const records = parseWmicListRecords(output); | |
| // A valid enumeration always includes at least our own process (the proxy | |
| // command line contains "opencodex"), so an empty result is a failure. | |
| if (records.length === 0) throw new Error("invalid process list"); | |
| let count = 0; | |
| for (const record of records) { | |
| if (record.processId === selfPid) continue; | |
| // WMIC's own CommandLine embeds the WQL "codex" literals, so skip it. | |
| if (record.commandLine?.toLowerCase().includes(wmic.toLowerCase())) continue; | |
| const nameMatch = record.name !== undefined && WINDOWS_CODEX_NAME_RE.test(record.name); | |
| const commandLineMatch = record.commandLine !== undefined | |
| && WINDOWS_CODEX_CMDLINE_RE.test(record.commandLine); | |
| if (nameMatch || commandLineMatch) count += 1; | |
| } | |
| return count; |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/codex/native-profile-processes.ts` around lines 98 - 110, Update the
record-counting loop in the WMIC process enumeration to explicitly skip the WMIC
helper process, matching the guard used by listWindowsSnapshotsViaWmic. Identify
that helper by its command line before applying WINDOWS_CODEX_NAME_RE or
WINDOWS_CODEX_CMDLINE_RE, while preserving the existing selfPid exclusion and
invalid-empty-list behavior.
| function whoamiValue(): string { | ||
| let result: ReturnType<typeof Bun.spawnSync>; | ||
| try { | ||
| result = Bun.spawnSync([ | ||
| "powershell.exe", | ||
| "-NoLogo", | ||
| "-NoProfile", | ||
| "-NonInteractive", | ||
| "-Command", | ||
| expression, | ||
| ], { | ||
| result = Bun.spawnSync([resolveTrustedWindowsWhoamiExe(), "/user"], { | ||
| stdin: "ignore", | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| windowsHide: true, | ||
| }); | ||
| } catch (cause) { | ||
| refuse("Windows effective-account lookup could not start.", cause); | ||
| } | ||
| if (result.exitCode !== 0) refuse("Windows effective-account lookup failed."); | ||
| const value = new TextDecoder().decode(result.stdout).trim(); | ||
| if (!value) refuse("Windows effective-account lookup returned an empty value."); | ||
| return value; | ||
| const output = new TextDecoder().decode(result.stdout); | ||
| const sid = parseWindowsSidFromWhoami(output); | ||
| if (!sid) refuse("Windows effective-account lookup returned an invalid SID."); | ||
| return sid; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm `refuse` returns `never`, locate existing identity timeout constants, and list whoami spawn sites.
set -euo pipefail
fd -t f 'user-identity.ts' src --exec ast-grep outline {} --items all
rg -nP -C3 'function refuse\b' src
rg -nP -C2 'TIMEOUT_MS|DEADLINE_MS' src/codex src/lib
rg -nP -C4 'whoami' src --type=tsRepository: lidge-jun/opencodex
Length of output: 24817
🏁 Script executed:
#!/bin/bash
# Inspect the shared whoami runner and the identity module around the inlined spawn.
set -euo pipefail
printf '\n--- src/lib/windows-whoami.ts lines 1-80 ---\n'
cat -n src/lib/windows-whoami.ts | sed -n '1,80p'
printf '\n--- src/codex/user-identity.ts lines 1-110 ---\n'
cat -n src/codex/user-identity.ts | sed -n '1,110p'
printf '\n--- src/codex/user-identity.ts lines 170-190 ---\n'
cat -n src/codex/user-identity.ts | sed -n '170,190p'Repository: lidge-jun/opencodex
Length of output: 9435
Reuse runWhoamiSync from src/lib/windows-whoami.ts in src/codex/user-identity.ts.
whoamiValue() inlines a Bun.spawnSync without timeout, while src/lib/windows-whoami.ts line 46 uses Math.max(1, timeoutMs). On Windows, resolveEffectiveUserIdentity() calls resolveWindowsSid() during startup/configuration paths; if whoami.exe stalls waiting on domain/account resolution, this unbounded child blocks until it exits. Use runWhoamiSync(WHOAMI_TIMEOUT_MS) and handle timedOut plus !success; also set stderr: "ignore" before reading result.stdout.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/codex/user-identity.ts` around lines 50 - 67, Replace the inline
Bun.spawnSync logic in whoamiValue with runWhoamiSync(WHOAMI_TIMEOUT_MS) from
windows-whoami.ts. Handle timedOut and unsuccessful results through refuse, set
stderr to "ignore" before decoding result.stdout, and preserve the existing SID
parsing and invalid-SID rejection.
| export function currentWindowsAccount(): string | null { | ||
| const whoami = resolveTrustedWindowsWhoamiExe(); | ||
| let output: string; | ||
| try { | ||
| output = execFileSync( | ||
| whoami, | ||
| [], | ||
| { encoding: "utf-8", stdio: ["ignore", "pipe", "ignore"], timeout: 5_000, windowsHide: true }, | ||
| ); | ||
| } catch { | ||
| return null; | ||
| } | ||
| const line = output.split(/\r?\n/).map(line => line.trim()).find(Boolean); | ||
| return line && /\\/.test(line) ? line : null; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Document that resolveTrustedWindowsWhoamiExe() can throw outside the try block.
The doc comment states the function returns null on failure. The resolver call at Line 125 sits outside the try, so a trust-assertion failure propagates as an exception instead of null. The direct consumer listWindowsSnapshotsViaWmic treats both outcomes as enumeration failure, so behavior stays fail-closed. Still, other callers may assume the null contract.
Either move the call inside the try, or state the throwing behavior in the comment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/windows-wmic.ts` around lines 124 - 138, Move the
resolveTrustedWindowsWhoamiExe() call inside the try block in
currentWindowsAccount() so resolver failures also return null, preserving the
documented failure contract and existing fail-closed behavior.
| try { | ||
| if (survivor) { | ||
| // Double-check the current CommandLine still carries the unique | ||
| // marker before taskkill. If the PID was recycled or the marker is | ||
| // gone, do NOT kill anything. | ||
| const verify = spawnSync( | ||
| "wmic", | ||
| ["process", "where", `ProcessId=${survivor}`, "get", "CommandLine", "/value"], | ||
| { encoding: "utf8", stdio: ["ignore", "pipe", "ignore"], windowsHide: true, timeout: 5_000 }, | ||
| ); | ||
| const current = (verify.stdout ?? "").replace(/\r/g, ""); | ||
| if (/^CommandLine=.*$/m.test(current) && current.includes(PROBE_MARKER)) { | ||
| spawnSync("taskkill", ["/F", "/PID", String(survivor)], { stdio: "ignore", windowsHide: true }); | ||
| } | ||
| } | ||
| } catch { | ||
| /* already exited */ | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Both Windows integration tests gate cleanup on a PATH-resolved wmic that may be absent. The shared root cause: marker verification runs through bare "wmic", and on a host without the deprecated WMIC component the verification returns empty output, so taskkill is skipped and the 45-second ping payload survives the test run.
tests/codex-app-server-processes.test.ts#L516-L533: keep the marker-verifiedtaskkillfor the precise kill, then add an unconditional tree kill of the spawned child as a fallback, for examplespawnSync("taskkill", ["/F", "/T", "/PID", String(child.pid)], { stdio: "ignore", windowsHide: true })inside the existingtry/catch.tests/codex-app-server-processes.test.ts#L582-L593: apply the identical tree-kill fallback here; this test explicitly simulates a WMIC-absent host, so it is the most likely place to leak.
📍 Affects 1 file
tests/codex-app-server-processes.test.ts#L516-L533(this comment)tests/codex-app-server-processes.test.ts#L582-L593
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/codex-app-server-processes.test.ts` around lines 516 - 533, In the
cleanup blocks at tests/codex-app-server-processes.test.ts:516-533 and :582-593,
retain the marker-verified taskkill using survivor, then add the same
unconditional taskkill tree fallback for child.pid inside each existing
try/catch. The fallback must force-kill the spawned child and its process tree
with Windows-hidden, ignored stdio options so cleanup still occurs when WMIC
verification produces no output.
| test("uses WMIC with shell-free bounded execution and counts Codex processes", async () => { | ||
| const calls: Parameters<NativeProcessExecutor>[] = []; | ||
| const execFile: NativeProcessExecutor = async (file, args, options) => { | ||
| calls.push([file, args, options]); | ||
| return "2\n"; | ||
| // Real WMIC /format:list shape: keys in alphabetical order per block, | ||
| // blank lines between records (ProcessId CLOSES a record). | ||
| return [ | ||
| "CommandLine=codex app-server --serve", | ||
| "Name=codex.exe", | ||
| "ProcessId=41", | ||
| "", | ||
| 'CommandLine="C:\\tools\\codex.cmd" serve', | ||
| "Name=cmd.exe", | ||
| "ProcessId=42", | ||
| "", | ||
| "CommandLine=bun D:\\tools\\opencodex\\src\\cli\\index.ts start", | ||
| "Name=bun.exe", | ||
| "ProcessId=99", | ||
| ].join("\n"); | ||
| }; | ||
| const script = [ | ||
| "$ErrorActionPreference='Stop';", | ||
| "$self=$PID;", | ||
| "$items=Get-CimInstance Win32_Process | Where-Object { $_.ProcessId -ne $self -and ($_.Name -match '^(?i:codex)(?:\\.exe)?$' -or $_.CommandLine -match '(?i)(?:^|[\\\\/\"\\s])codex(?:\\.exe|\\.cmd)?(?:[\"\\s]|$)') };", | ||
| "@($items).Count", | ||
| ].join(" "); | ||
| await withTrustedWindowsPowerShell(async powershell => { | ||
| await withTrustedWindowsWmic(async wmic => { | ||
| await expect(probeNativeCodexProcesses({ | ||
| platform: "win32", | ||
| execFile, | ||
| pid: 99, | ||
| })).resolves.toEqual({ status: "busy", count: 2 }); | ||
|
|
||
| expect(calls).toEqual([[ | ||
| powershell, | ||
| ["-NoLogo", "-NoProfile", "-NonInteractive", "-Command", script], | ||
| wmic, | ||
| ["process", "where", "(Name like 'codex%' or CommandLine like '%codex%')", "get", "ProcessId,Name,CommandLine", "/format:list"], |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Add a regression test for exclusion of the WMIC helper record.
The WMIC child's own record reaches the counter, and it is currently excluded only by the character class in WINDOWS_CODEX_CMDLINE_RE. No test pins that behavior. Add a record whose Name is WMIC.exe and whose CommandLine embeds the WQL literals, then assert the count stays unchanged. This test also locks the explicit guard suggested on src/codex/native-profile-processes.ts Lines 98-110.
As per path instructions: "A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/native-profile-processes.test.ts` around lines 34 - 63, Extend the
existing “uses WMIC with shell-free bounded execution and counts Codex
processes” test’s mocked WMIC output with a WMIC.exe record whose CommandLine
contains the WQL literals, then keep the expected busy count at 2. Ensure the
regression covers the explicit helper-record exclusion in
probeNativeCodexProcesses without changing the existing invocation assertions.
Source: Path instructions
| test("builds a hidden command from the trusted whoami path", () => { | ||
| const trusted = "C:\\trusted-system32\\whoami.exe"; | ||
| setTrustedWindowsElevationExecutablesForTests({ whoami: trusted }); | ||
| expect(windowsPrincipalCommandForTests()).toEqual([trusted, "/user"]); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check override cleanup and locate tests for the new whoami parser and token-based profile resolution.
set -euo pipefail
fd -t f 'windows-user-principal.test.ts' tests --exec cat -n {}
rg -nP -C2 'setTrustedWindowsElevationExecutablesForTests\s*\(\s*null' tests
rg -nP -C3 'parseWindowsSidFromWhoami|runWhoamiSync|runWhoamiAsync' tests
rg -nP -C3 'GetUserProfileDirectoryW|windowsProfileDirectory|localAppDataValue' tests
fd -t f 'windows-whoami' -E node_modulesRepository: lidge-jun/opencodex
Length of output: 8451
🏁 Script executed:
#!/bin/bash
set -euf pipefail
echo "== candidate source files =="
fd -t f '(windows-whoami|windows-user-principal|windows-elevation|user-identity)\.(ts|tsx)$' src
echo
echo "== outlines =="
for f in $(fd -t f '(windows-whoami|windows-user-principal|windows-elevation|user-identity)\.(ts|tsx)$' src); do
echo "--- $f ($(wc -l < "$f") lines) ---"
ast-grep outline "$f" || true
done
echo
echo "== relevant tests containing identity/profile/windows whoami concepts =="
rg -nP -C3 'whoami|windowsProfileDirectory|GetUserProfileDirectoryW|localAppData|RunToken|token-based|parseWindowsSidFromWhoami|EACLIDENTITY|windows-user-principal|windows-elevation|user-identity' tests src | sed -n '1,260p'Repository: lidge-jun/opencodex
Length of output: 28134
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== imports and key functions in windows-user-principal.ts =="
sed -n '1,120p' src/lib/windows-user-principal.ts
sed -n '120,215p' src/lib/windows-user-principal.ts
sed -n '215,260p' src/lib/windows-user-principal.ts
echo
echo "== windows whoami implementation =="
sed -n '1,100p' src/lib/windows-whoami.ts
echo
echo "== user-identity Windows identity/profile functions =="
sed -n '1,190p' src/codex/user-identity.ts
echo
echo "== test focused paths under tests =="
fd -t f 'windows-profile|user-identity|windows-whoami|windows-user-principal|windows-elevation' tests -d 2 | sortRepository: lidge-jun/opencodex
Length of output: 21118
Add focused coverage for the Windows identity parser and profile resolver.
tests/windows-user-principal.test.ts already resets setTrustedWindowsElevationExecutablesForTests(null) in afterEach, so no cleanup change is needed. This behavior change still needs regression tests for parseWindowsSidFromWhoami and for windowsProfileDirectory/localAppDataValue in the appropriate test layer; add cases for localized whoami /user output, empty/no SID output, and CRLF line endings.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/windows-user-principal.test.ts` around lines 28 - 31, Add focused
regression tests in the appropriate test layers for parseWindowsSidFromWhoami,
covering localized whoami /user output, empty or missing SID output, and CRLF
line endings; also cover windowsProfileDirectory and localAppDataValue behavior
for CRLF input. Keep the existing trusted-executable test cleanup unchanged.
Source: Path instructions
| test("converts a WMIC CreationDate with offset to epoch ms", () => { | ||
| const value = parseWmicCreationDate("20260808120000.000000+480"); | ||
| expect(value).toBe(Date.UTC(2026, 7, 8, 12, 0, 0, 0) - 480 * 60_000); | ||
| }); | ||
|
|
||
| test("handles fractional seconds and missing offset", () => { | ||
| const value = parseWmicCreationDate("20260808120000.123456"); | ||
| expect(value).toBe(Date.UTC(2026, 7, 8, 12, 0, 0, 123)); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Add a negative-offset case to lock the sign convention.
Both offset tests use +480 or no offset. The sign handling in parseWmicCreationDate (utcMs - offsetMinutes * 60_000) is the part most likely to regress, and a sign inversion would still pass the current suite for the +480 case only if the subtraction direction changed. A western-hemisphere value such as -300 (UTC−5) pins the convention.
💚 Proposed additional test
test("handles fractional seconds and missing offset", () => {
const value = parseWmicCreationDate("20260808120000.123456");
expect(value).toBe(Date.UTC(2026, 7, 8, 12, 0, 0, 123));
});
+
+ test("applies a negative UTC offset", () => {
+ const value = parseWmicCreationDate("20260808120000.000000-300");
+ expect(value).toBe(Date.UTC(2026, 7, 8, 12, 0, 0, 0) + 300 * 60_000);
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("converts a WMIC CreationDate with offset to epoch ms", () => { | |
| const value = parseWmicCreationDate("20260808120000.000000+480"); | |
| expect(value).toBe(Date.UTC(2026, 7, 8, 12, 0, 0, 0) - 480 * 60_000); | |
| }); | |
| test("handles fractional seconds and missing offset", () => { | |
| const value = parseWmicCreationDate("20260808120000.123456"); | |
| expect(value).toBe(Date.UTC(2026, 7, 8, 12, 0, 0, 123)); | |
| }); | |
| test("converts a WMIC CreationDate with offset to epoch ms", () => { | |
| const value = parseWmicCreationDate("20260808120000.000000+480"); | |
| expect(value).toBe(Date.UTC(2026, 7, 8, 12, 0, 0, 0) - 480 * 60_000); | |
| }); | |
| test("handles fractional seconds and missing offset", () => { | |
| const value = parseWmicCreationDate("20260808120000.123456"); | |
| expect(value).toBe(Date.UTC(2026, 7, 8, 12, 0, 0, 123)); | |
| }); | |
| test("applies a negative UTC offset", () => { | |
| const value = parseWmicCreationDate("20260808120000.000000-300"); | |
| expect(value).toBe(Date.UTC(2026, 7, 8, 12, 0, 0, 0) + 300 * 60_000); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/windows-wmic.test.ts` around lines 86 - 94, Add a test alongside the
existing parseWmicCreationDate offset tests using a negative offset such as
-300, and assert that the result follows the same utcMs minus offsetMinutes
conversion, confirming western-hemisphere timestamps are adjusted in the correct
direction.
|
@Ingwannu I wanted to clarify how this PR relates to #1268 so there's no confusion — they fix two different bugs, and they complement each other. There are two distinct console-window-popup bugs on Windows. Same symptom (a visible console window), but different root cause, spawn site, and fix. 1. The launcher boundary — #1236 → PR #1268
2. Inside the proxy — #1278 → this PR (#1279)
They complement each other
Verified on a real Windows 11 machine: enumerated the running proxy process and confirmed it owns no console window (ConsoleWindowClass EnumWindows + GetWindowThreadProcessId), so the transient popup was the proxy's internal spawn, not the launcher window. A standalone probe with the bundled Bun 1.3.14 also confirmed the missing |
|
@wade19990814-hue Approved the pending Cross-platform CI at your exact head ( It hasn't gone green yet, and I want to be straight about what I can and can't tell you. The run ends The honest read is that this is probably not yours. The same shard-hang pattern hit The reason I'm not simply calling it flake and moving on: this PR changes process and identity lookups, and a hang immediately after a proxy server starts is exactly the shape a blocking child-process call would take. I have no Windows host to reproduce on, so I can't rule it out from here — and the affected shard runs on Linux, where your Concretely, if you want to close it out from your side: check that every new lookup path has a timeout and cannot block when the underlying command is unavailable, particularly anything reached during server startup. If that's already true, this is base-branch instability and I'll keep rerunning until it lands green. Not asking you to push anything blind. Tell me if you'd rather I keep rerunning. |
|
Cross-platform CI is green at The remaining Over to you for the readiness checklist in the description — those four boxes are your attestation and I won't tick them on your behalf. Once they're ticked the gate marks this ready for review automatically. |
…& process lookups Replace PowerShell child processes with hidden windows-native alternatives: - Identity SID: whoami.exe (hidden) instead of powershell.exe (no windowsHide) - LocalAppData: token-based FFI (GetUserProfileDirectoryW) — no child process, environment-independent, verified more reliable than .NET GetFolderPath which expands %USERPROFILE% from env on some hosts - Windows principal ACL lookup: whoami.exe /user instead of powershell.exe - Process snapshots & count probes: WMIC first (faster, no .NET), hidden PowerShell CIM fallback for hosts where the deprecated WMIC is absent - Fix parseWmicListRecords for real WMIC /format:list key ordering (alphabetical — ProcessId closes each record, not opens it) Co-developed from @wade19990814-hue's in-progress fix branch. Closes lidge-jun#1278
43b6b82 to
9574d45
Compare
|
@lidge-jun Thanks again for chasing down the shard instability earlier — much appreciated. One heads-up on why the head moved: right around your last comment, The new head is Once it's green I'll tick the final "local CI green" box. Sorry for the extra rerun this created — and if you'd rather I'd handled the dev-sync differently, just say the word. |
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
I reviewed exact head 9574d451e16e5744a999ef41f2121c514fa9475d for functional bugs, leaks/privacy, edge cases, security, usefulness/scope, current CI, unresolved review threads, and maintainer feedback.
The core bug is real and worth fixing: replacing the unhidden PowerShell identity lookup removes the transient Windows console popup and moving identity lookups onto trusted System32 executables/token APIs is directionally better. However, this head also bundles a broader WMIC-first process-introspection rewrite and currently contains several regressions that should block merge.
1. macOS batch start-time lookup is broken
In readProcessStartMsBatch, the Darwin branch still parses valid ps results into byPid, but the final loop no longer reads byPid:
for (const pid of pids) if (!out.has(pid)) out.set(pid, null);out is empty there, so every requested macOS PID is returned as null. The previous code correctly used byPid.get(pid) ?? null.
Impact: whenever a Codex app-server is running on macOS, catalog-state collection treats its start time as unreadable and collapses to state: "unknown", disabling the fresh/stale determination. This is a cross-platform regression unrelated to the Windows popup fix.
Please restore the byPid copy and add a focused Darwin batch regression test.
2. Windows hosts without WMIC lose the existing start-time fallback
The new Windows batch path does this when WMIC is absent:
if (!wmic) {
for (const pid of pids) out.set(pid, null);
return out;
}That is materially worse than the base implementation, which used the hidden PowerShell CIM query for the same batch. The new windows-wmic.ts documentation itself acknowledges that WMIC is a deprecated optional component and is absent on many modern Windows hosts.
Impact: on exactly those supported hosts, every running app-server gets an unknown start time, so catalog freshness/staleness becomes unknown instead of working through the existing hidden PowerShell fallback.
Please retain a hidden trusted-PowerShell fallback for the batch path when WMIC is unavailable, with a regression test for the no-WMIC case.
3. “Fallback” only handles WMIC absence, not WMIC failure
The same pattern exists across the new WMIC-first paths:
listWindowsSnapshots()chooses WMIC whenever the executable exists. If the WMIC query times out/fails,currentWindowsAccount()fails, or anywmicGetOwner()lookup fails, the path throws instead of trying the existing PowerShell implementation.probeNativeCodexProcesses()likewise returnsunknownon an installed-but-broken WMIC rather than attempting PowerShell.readWindowsProcStartMs()returnsnullon a WMIC query/parse failure instead of trying its PowerShell helper.
So an installed but unhealthy WMI/WMIC stack turns previously working functionality into unknown/failure even though the fallback backend is available. Please treat WMIC execution/parse failure the same way as WMIC absence and fall back before failing closed.
There is also a latency issue here: snapshot enumeration now performs a synchronous current-account lookup, one synchronous WMIC enumeration, then another synchronous wmic getowner call per candidate. In a degraded WMI environment that is roughly 5s + 8s + 5s * candidateCount, which no longer matches the documented aggregate Windows bound and can block the event loop substantially longer than the previous single bounded PowerShell enumeration. Avoid serial per-candidate 5-second worst cases or enforce a shared deadline.
4. The rewritten startup identity path still has an unbounded child
user-identity.ts::whoamiValue() directly calls Bun.spawnSync(... /user ...) without a timeout, even though this PR introduces runWhoamiSync(timeoutMs) specifically with a bounded timeout.
This is particularly relevant to the maintainer discussion: lidge-jun explicitly asked that every new lookup path reached around startup be checked for a timeout/non-blocking failure mode after the earlier shard-hang investigation. This direct call is still unbounded and is reached by startup/config/shutdown write coordination.
Please reuse the bounded helper (or otherwise bound this lookup) rather than maintaining a second unbounded implementation.
5. Security-sensitive SID parsing should validate the trailing SID token
parseWindowsSidFromWhoami() documents that the SID is the trailing token, but currently returns the first SID-looking substring anywhere on a line:
/S-1-(?:\d+-)+\d+/i.exec(line.trim())That parser feeds both the ACL principal path and the coordinator identity path. An account/domain string containing a SID-shaped substring can therefore be mistaken for the actual token SID. On the ACL path, choosing the wrong principal is security-sensitive because that value is later used for the file ACL grant; on the coordinator path it can select the wrong namespace.
Please parse/validate the final whitespace-delimited token (or anchor the SID pattern to the end of the line) and add a regression for a SID-shaped username/domain preceding the real SID.
Other valid cleanup / edge cases
I would also address these before final approval, though they are lower severity than the blockers above:
parseWmicCreationDate()should reject impossible date/time components rather than lettingDate.UTC()silently normalize them.- The Windows integration-test cleanup verifies a survivor with bare PATH-resolved
wmic; on a WMIC-absent host the spawnedpingchild can survive the test. Cleanup should have an unconditional safe tree-kill fallback for the test-owned child. - The new profile-path implementation deliberately ignores redirected LocalAppData and constructs
<token-profile>\AppData\Local. That may be an acceptable security trade-off, but the PR should explicitly acknowledge the compatibility/migration effect rather than describe it as universally resolving the “real LocalAppData”.
Security / privacy / leaks
I did not find a new credential/token leak, auth bypass, shell-injection path, or secret-persistence vulnerability in this diff. Positive changes include trusted System32 resolution for whoami/PowerShell, corrected u64 Win32 HANDLE FFI usage, environment-poisoning resistance for identity resolution, and generally fail-closed ownership handling. The WMIC containment comment is worth cleaning up, but I do not consider the current constructed System32\wbem\WMIC.exe path a merge-blocking exploit by itself.
Usefulness and scope
The popup fix itself is useful and should land. But the linked issue is specifically the unhidden PowerShell identity spawn. The pre-existing process-enumeration and native-profile PowerShell calls already used windowsHide: true, so the WMIC-first process rewrite is not necessary to solve #1278. It also accounts for most of the new failure surface above.
My recommendation is to either:
- keep this PR focused on the identity/profile popup fix and split the WMIC optimization into a follow-up PR, or
- keep WMIC here only after restoring robust PowerShell fallback, shared deadlines, and focused Windows/macOS regression coverage.
Maintainer / CI state
- Exact-head React Doctor: green.
- Exact-head Cross-platform CI: green, including typecheck, GUI tests, privacy scan, macOS, npm-global, and keyring jobs.
- The full Windows test shard is conditionally skipped, so the new real-Windows
skipIf(process.platform !== "win32")integration paths are not independently exercised by this CI run. - lidge-jun’s prior comments mainly covered the earlier CI shard instability and specifically asked for bounded lookup paths; there is no submitted human maintainer approval on this head yet.
- Multiple current CodeRabbit threads remain unresolved. Some are low-value/nit-level, but the WMIC failure fallback and unbounded identity lookup findings are valid.
Once the macOS regression, no-WMIC/failing-WMIC fallback behavior, bounded identity lookup, and SID parsing are fixed with focused tests, this will be much closer to approval.
|
The popup bug is real and we want the fix, but the bundled process-enumeration rewrite introduces a macOS regression. In Windows fallback also gets weaker in specific paths. The console-window problem you traced is legitimate, and the trusted-executable plus Closing this version because a small Windows UX fix is coupled to platform changes that currently break macOS correctness. Please do resubmit the bounded fix — the bug itself is worth closing. |
…ups (lidge-jun#1236) The desktop proxy parent runs without a console, so every console-subsystem child spawned without CREATE_NO_WINDOW gets a fresh visible console window. user-identity's SID and LocalAppData lookups spawned powershell.exe with no windowsHide, which surfaced as popups at startup, on config writes, and on shutdown. Focused fix per the lidge-jun#1279 review: harden the existing identity and process-lookup spawn sites only — no enumeration rewrite, no POSIX changes. - user-identity: spawn the identity lookups hidden (windowsHide plus -WindowStyle Hidden), under an 8s bounded timeout, and from the trusted System32 PowerShell (never PATH). A hung child now fails the lookup instead of wedging startup. - app-server-processes: resolve the three enumeration/start-time PowerShell sites through resolveTrustedWindowsPowerShellExe(); windowsHide and timeouts were already in place there. - windows-user-principal and native-profile-processes were already hardened on dev and are untouched. Adds tests/windows-popup-fix.test.ts regression coverage for the hidden, trusted, bounded spawn shape plus a real-token check on Windows hosts.
3 participants
Summary
Eliminate all visible console-window popups from the proxy's internal child processes on Windows:
user-identity.ts):powershell.exespawn withoutwindowsHide→whoami.exe /user(hidden, token-based, resolved from trusted System32)user-identity.ts): PowerShell.NET GetFolderPath(which was also not environment-independent on hosts with%USERPROFILE%in the registry) → pure FFIOpenProcessToken + GetUserProfileDirectoryW— token-based, no child process at all, truly environment-independentwindows-user-principal.ts):powershell.exe→whoami.exe /userapp-server-processes.ts): PowerShell-only → WMIC first (faster cold start, no .NET runtime), hidden PowerShell CIM fallback for hosts without WMICnative-profile-processes.ts): same WMIC-first + PowerShell fallbackparseWmicListRecords(windows-wmic.ts): rewritten to parse WMIC/format:listblocks correctly — real WMIC outputs keys in alphabetical order,ProcessIdat the end of each record; the previous implementation attributedCommandLineto the wrong recordNew files:
src/lib/windows-whoami.ts— hiddenwhoamiasync/sync runner & SID parsersrc/lib/windows-wmic.ts— WMIC record parser,getowner, creation-date converter, current-account resolvertests/windows-wmic.test.ts— parser regression tests (block order, continuation lines, invalid ProcessIds)Verification
bun run typecheck— passedbun teston all affected test files — 237 passed, 0 failedwindowsHide, matching the installed release's launch mode) → identity resolution lands on the realLocalAppDatadirectory (no env-var dependency), no PowerShell window appears — the popup is verified eliminatedpowershell.exefrom a console-less parent withoutwindowsHidecreates a visible console window; withwindowsHide: true→hwnd = 0— the root cause is the missing flag, and the installed runtime honors it correctlyChecklist
resolveTrustedWindowsSystemDirectorycontainmentReview readiness
devcommit — rebased onto14e94852, 0 commits behindu64, notptr) and Minor (UTF-16TextDecoderdecode) findings are appliedCloses #1278
Summary by CodeRabbit