Skip to content

fix(windows): preserve ARM64 ACL identity lookup - #1452

Closed
Ingwannu wants to merge 1 commit into
devfrom
agent/fix-1449-windows-arm64-acl
Closed

fix(windows): preserve ARM64 ACL identity lookup#1452
Ingwannu wants to merge 1 commit into
devfrom
agent/fix-1449-windows-arm64-acl

Conversation

@Ingwannu

Copy link
Copy Markdown
Owner

Summary

  • Preserve required secret-file ACL hardening on Windows ARM64 when Bun 1.3.14 cannot load the bun:ffi System32 resolver.
  • Keep GetSystemDirectoryW authoritative on supported runtimes, then permit only the non-elevated SID lookup on win32/arm64 to use the fixed protected default PowerShell path when that exact file exists.
  • Keep environment-derived paths and account names outside the trust boundary, leave UAC and Task Scheduler resolution unchanged, and fail closed for non-default Windows roots.
  • Add regression coverage for the ARM64 selection boundary, hostile environment values, unsupported platforms/architectures, missing files, and primary-resolver precedence.
  • Document the decision and tradeoff in the config/Codex-home architecture note.

Root cause: the effective-SID query resolved PowerShell through GetSystemDirectoryW via bun:ffi. Bun 1.3.14 on Windows ARM64 cannot provide that FFI call, so the required ACL path surfaced EACLIDENTITY before 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.
  • The first CPU-limited full-suite run completed with 10,778 pass, 10 skip, and 9 unrelated failures/errors. Six occurred before the GUI dependency install step; the two reported image/bridge failures passed when rerun in isolation. The remaining codex-shim failure reproduces in an untouched test path because the fixture reads the process runtime token instead of local-secret; neither its implementation nor test differs from dev in this PR.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

@Ingwannu
Ingwannu requested review from Wibias and lidge-jun August 11, 2026 05:25
@github-actions

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: a5b23388-e40d-4baa-bf89-910820a35622

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the bug Something isn't working label Aug 11, 2026

@Wibias Wibias left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
  • GetSystemDirectoryW call 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.

@lidge-jun

Copy link
Copy Markdown
Owner

Landed on dev — closing as superseded rather than merged; the change is already there and the branch now conflicts with it.

On dev as fcb337a18 with your authorship intact, plus 4368bb352 closing the reviewer's blocker.

The blocker was right and worth the extra round: the fallback was a catch-all. On ARM64 it substituted the hard-coded C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe for any trusted-resolution failure whenever that file happened to exist — GetSystemDirectoryW failure, a non-default Windows root, a containment or integrity check failure, and anything added later. That PowerShell performs effective-SID discovery feeding secret-file ACL hardening, so it converted a trust-boundary failure into a different identity source.

WindowsSystemDirectoryFfiUnavailableError is now thrown only where loadGetSystemDirectoryW() cannot provide the FFI function, and the fallback requires that typed sentinel plus win32 plus arm64 plus the fixed executable. Everything else rethrows unchanged. The existing test that threw a generic Error("bun:ffi unavailable") was certifying the unsafe shape, so it now uses the sentinel.

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 (windows N/4 is dispatch-only, #1059); the resolver seam is injected, so these run on macOS. Thanks for the fix — the QEMU ARM64 diagnosis in the issue is what made it findable.

@lidge-jun lidge-jun closed this Aug 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants