fix(windows): preserve ARM64 ACL identity lookup - #1452
Conversation
|
✅ Deterministic PR hygiene checks passed. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Wibias
left a comment
There was a problem hiding this comment.
Request changes on current head 47271935fb973070216a7cb16f933303e527a07a.
Merge blocker: the new Windows ARM64 PowerShell fallback is triggered by every failure from the trusted resolver, not only by the specific Bun/FFI-unavailable condition this PR is meant to work around.
The current shape is effectively:
try {
return resolution.resolveTrusted();
} catch (error) {
if (
resolution.platform !== "win32" ||
resolution.arch !== "arm64" ||
!resolution.pathExists(DEFAULT_WINDOWS_ARM64_POWERSHELL)
) {
throw error;
}
return DEFAULT_WINDOWS_ARM64_POWERSHELL;
}That catch-all weakens the existing trust boundary. On ARM64 it can replace any trusted-resolution or validation failure with the hard-coded C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe path merely because that file exists. This includes failures from GetSystemDirectoryW, non-default Windows roots, missing/invalid trusted PowerShell resolution, containment/integrity checks, or future security checks.
This matters because the selected PowerShell is used for effective SID/principal discovery feeding secret-file ACL hardening. A trusted resolver failure must not be silently converted into a different identity source.
Please narrow the fallback to the one explicitly expected failure mode, e.g. a typed/sentinel bun:ffi / System32-resolution-unavailable error. All other resolver and validation failures must continue to fail closed.
Required regressions should prove at minimum:
- win32/arm64 + explicit FFI-unavailable condition -> fixed fallback may be used when the expected file exists;
GetSystemDirectoryWcall failure -> no fallback;- invalid/non-default trusted system directory -> no fallback;
- trusted PowerShell validation/containment failure -> no fallback;
- arbitrary resolver error -> no fallback;
- successful trusted resolver -> primary result always wins.
Also preserve the stated non-default-Windows-root contract. existsSync("C:\\Windows\\...") alone is not evidence that C:\\Windows is the active protected Windows root, so the fallback must not override an authoritative OS-derived non-default root.
Exact-head CI was green in the previous review, so this request is about the trust/fail-closed contract rather than a failing test.
|
Landed on On The blocker was right and worth the extra round: the fallback was a catch-all. On ARM64 it substituted the hard-coded
Ablation: removing the sentinel gate turns four regressions red — API-call failure, unusable non-default directory, validation failure, and an arbitrary resolver error each returned the fixed System32 path. 27 pass / 0 fail with the gate in place. No real ARM64 Windows behavior was exercised ( |
Summary
bun:ffiSystem32 resolver.GetSystemDirectoryWauthoritative on supported runtimes, then permit only the non-elevated SID lookup onwin32/arm64to use the fixed protected default PowerShell path when that exact file exists.Root cause: the effective-SID query resolved PowerShell through
GetSystemDirectoryWviabun:ffi. Bun 1.3.14 on Windows ARM64 cannot provide that FFI call, so the required ACL path surfacedEACLIDENTITYbefore PowerShell started.Closes #1449
Verification
taskset -c 0,1 nice -n 10 bun test tests/windows-user-principal.test.ts tests/windows-secret-acl.test.ts tests/windows-elevation.test.ts tests/windows-elevation-spawn.test.ts tests/windows-popup-fix.test.ts— 238 pass, 0 fail on exact PR head.taskset -c 0,1 nice -n 10 bun run typecheck— passed on exact PR head.taskset -c 0,1 nice -n 10 bun run privacy:scan— passed on exact PR head.git diff --check— passed.codex-shimfailure reproduces in an untouched test path because the fixture reads the process runtime token instead oflocal-secret; neither its implementation nor test differs fromdevin this PR.Checklist