fix(windows): restrict secure files in-process instead of spawning icacls by OrcaWin · Pull Request #28135 · stablyai/orca

OrcaWin · 2026-10-13T01:27:15Z

	Files	Added	Deleted	Net
Test	32	$\color{#1a7f37}{\Huge{\mathbf{+}}}$2051	$\color{#cf222e}{\Huge{\mathbf{−}}}$2094	$\color{#cf222e}{\Huge{\mathbf{−}}}$43
Prod	51	$\color{#1a7f37}{\Huge{\mathbf{+}}}$1709	$\color{#cf222e}{\Huge{\mathbf{−}}}$1197	$\color{#1a7f37}{\Huge{\mathbf{+}}}$512

ELI5

On Windows, Orca keeps secrets (pairing keys, device tokens, the local runtime token, plugin secrets) in ordinary files and locks each one so only you, the system, and administrators can read it. Until now it did that locking by starting a Windows tool, icacls.exe, five times for every save. Each start froze Orca's window until it finished, and on PCs with corporate security software that can take seconds. Security sandboxes also flag the exact icacls commands Orca ran as ransomware behaviour.

Orca now asks Windows directly, from inside its own process, using a small native module. A secret file is created already locked. No programs are started, and a save costs about half a millisecond.

What Changed

What users see

Before: on Windows, every secure save froze Orca's main thread. That was 94–138 ms on a fast machine and several seconds under endpoint security. There are about 23 secure stores, among them paired devices, plugin storage and secrets, the runtime metadata written at every start, and the API key stores. The most frequent caller, the notification-dismissal store saved on each prompt, has already moved off secure files (perf(notifications): stop every Windows notification from freezing Orca #27731), so the remaining freezes come from these less frequent saves and from startup.
After: no freeze, and no icacls.exe or whoami.exe process is ever started.

How it works

New native module, native/windows-security (@orca/windows-security). A small N-API addon over advapi32, packaged and built the same way as @orca/windows-registry. It has five jobs:
- read the current user's SID;
- read a file's access list as plain structure (each entry's type, flags, rights and raw SID), so no text format is parsed;
- replace a file's access list, either on the calling thread or on a background thread;
- create a new file that already carries a given access list;
- add one grant to an existing access list (Windows keeps the entries in the order it requires; a "NULL" access list, which already allows everyone, is left alone).
The module is loaded once, together with the current user's SID, and only from inside the app: from the copy orcad ships beside its own files, or from the app's own node_modules. It never follows Node's usual search up parent folders, which could reach a folder like C:\node_modules that any local user can create. Every path it receives gets the long-path (\\?\) prefix, and background permission changes to the same folder run one after another.

Deciding what the access list should be, and checking it, stays in TypeScript.
Writing a secret (writeSecureFile). The temporary file is created already locked to the user, SYSTEM and Administrators. Orca reads the lock back once, then renames the file into place; a rename keeps the lock. No access list is changed after the file exists, so a secret is never readable under the folder's broader permissions, not even briefly. Reading the lock back is what catches FAT32 and some network shares, which silently ignore it.
Locking a folder. Each folder is checked once per process. It is rewritten only when the check fails, and the rewrite runs on a background thread. Windows applies a folder change to every file inside it, which took about 2 s on a real 29,000-file app data folder, so this must never block a save or startup. Folders that earlier versions already locked pass the check, so they are never rewritten.
The check uses OpenSSH's ACE rule for Windows: only the user, SYSTEM and Administrators may be granted anything, whether directly or inherited. Orca adds two of its own: the item must be protected from its parent folder, so loosening the parent later cannot reach it, and the user must have full control of it. (OpenSSH also checks the owner; Orca does not.) Any entry that is not a plain allow or deny (conditional, callback or object entries), and a NULL access list, fails the check and gets re-locked. Files and folders locked by the old icacls code pass unchanged, so there is no migration. A file that only inherits its permissions is re-locked once.
If the module cannot load, the save still happens, reports "not restricted" as before, and logs once. There is no icacls or PowerShell fallback. If the module is loaded but creating the locked file fails, the save fails with an error naming the Windows call and error code, rather than quietly writing the secret unlocked.
Packaging fails a Windows build when the native module binary that ships (the copy under node_modules) is missing or built for the wrong CPU type, so the problem can't first appear on a user's machine.
Every other runtime icacls/whoami caller now uses the same module:
- grantDirAcl and its async version (the permission-error retry paths);
- the startup app-data grant;
- the install-folder permission probe and repair.
The artifact-intent store drops its own temp-file writer, folder cache and synchronous folder re-lock, which existed only to avoid per-save launches, and saves through writeSecureFile like every other secure store.

The repair's copy-paste commands for administrators are still shown as text; nothing runs them. The probe reads the same structure as the check, so an entry it cannot evaluate makes the whole reading "unreadable" instead of being skipped. The repair's second pass now grants every file but only the folders whose own permissions are protected (inheritably, so their contents get it too): every other folder already inherits the first pass's grant, and re-granting it re-applied the change across its whole subtree (3.2 s for 1,093 folders in a 7.6k-entry install).
orcad (the headless server) on Windows ships the module next to its bundles. It is built per architecture (x64 and arm64) and its CPU type is checked when packaged. orcad refuses to start if it cannot lock its data folder, so a Windows orcad without this module would never start.

Why

Two problems shared one cause: changing a file's permissions on Windows meant starting another program.

[Bug]: Windows: UI freezes for seconds on every agent prompt — secure file writes run icacls synchronously on the main thread #26027: the program was started synchronously on the main thread, on every save.
[Bug]: Windows credential ACL hardening spawns icacls /reset + /inheritance:r, scored Malicious by behavioural sandboxes #25583: the program's commands are what security sandboxes score as malicious.

Alternatives considered:

Keep icacls, but run it asynchronously or in a worker. This removes the freeze but not the flagged behaviour. Running it asynchronously also publishes the secret before it is locked, which gives up the current guarantee.
PowerShell Set-Acl. Slower to start than icacls (about 1–1.5 s cold), and a PowerShell launch is itself something security tools flag.
Lock only the user's profile folder and trust what files inherit from it. Not enough when the data folder is moved (for example C:\orca-data, which inherits Authenticated Users: Modify from C:\), or when the profile is roaming or redirected. Locking that folder still has to happen in-process.
Encrypt with safeStorage instead of locking. Windows encrypts per user, which protects against other accounts but not against another program running as the same user. Several of these files must also stay readable by the CLI and orcad, which cannot use Electron's safeStorage.

Locking in-process keeps today's guarantees (verify first, never publish an unlocked secret, report failure) and starts no programs at all.

Linked Issue

Fixes #26027
Fixes #25583

Visual Proof

N/A. The only visible change is that the UI no longer freezes; this is measured below rather than recorded.

Testing

Tested on Windows 11 x64 (10.0.26200). macOS and Linux behaviour is unchanged (they still use chmod; the module is never loaded there).

Spawns and blocking time per secure write (same harness, every child process counted; temp folder and a moved data folder under C:\):
- Before: 5–7 icacls/whoami starts per write, 94–138 ms blocking, plus 6 more icacls starts afterwards to lock the folder.
- After: 0 starts, about 0.5 ms (4 ms for the first write, which loads the module).
No moment of exposure. A second thread read the permissions of every temporary and final file while the main thread saved continuously for 1 s into a folder under C:\ that inherits Authenticated Users: Modify: 1,587 saves, 28,343 reads, 1 distinct access list, 0 broader than allowed.
First launch after upgrade, default install. %APPDATA%\orca and its key files already pass the check, so nothing is rewritten. The startup-grant marker still matches, so no grant runs. Cost: module load about 2 ms, check about 1 ms. When a folder does need locking it happens in the background: 4,000 entries took about 220 ms while the save returned in 1.2 ms. A folder that already passes is not rewritten on the next launch.
Real-permission tests on Windows (registered in both Windows CI lists):
- windows-security-addon.win32.test.ts: the module itself. This includes that inherited entries passed in are dropped and recomputed from the parent, and that a grant keeps Windows' required order (deny, then allow, then inherited).
- secure-path-windows-acl.win32.test.ts: real files and folders, the moved data folder, paths longer than 260 characters, and old-format locks left unchanged.
- The rewritten real-permission tests for the install-folder repair and for unreadable-store preservation.
Unit tests for the check, the save and read paths (with a fake module), the startup grant, grantDirAcl, the install-folder grant and repair, and orcad packaging. Each process-starting path asserts it starts nothing.
Native and packaging checks:
- ensure-native-runtime for Node and Electron, including a negative check: delete the built module and it is detected and rebuilt.
- The module loads under Electron 43.7.5.
- orcad built for win32-x64 with the module included; both x64 (0x8664) and arm64 (0xaa64) builds pass the CPU-type check.
- A bundle laid out like orcad, with no node_modules, loads the module and locks its data folder.
pnpm tc, typecheck:node, typecheck:web, typecheck:cli, oxlint, the anti-slop lint, check:code-quality:changed, the max-lines and process-host-import ratchets.
I manually tested these changes locally
Automated tests added/updated

Notes

Behaviour change: if a store file Orca owns has been locked to some other account, reading it now takes it back for the user right away. Before, the fix ran later, so the first read failed and Orca refused to regenerate the file. Files Orca does not own (a restored backup, a share) still fail as before, so their contents are never overwritten.
Conflicts with fix(runtime): persist mobile notification dismissals without blocking Windows #26754 in secure-file.ts. That PR's background save path is no longer needed: perf(notifications): stop every Windows notification from freezing Orca #27731 already moved its store off secure files, and this PR makes the remaining secure saves cheap.
Relation to perf(notifications): stop every Windows notification from freezing Orca #27731: that PR stopped locking one file that holds no secrets. This one keeps locking every real secret and makes the locking itself spawn-free.

…acls Every secure write on Windows spawned icacls.exe five times on the calling thread (the Electron main thread for most stores): verify, /reset, /inheritance:r /grant:r, verify, verify. Process creation under endpoint security costs ~0.6 s each, so each write froze the UI for seconds (#26027). The /reset + /inheritance:r pair is also what behavioural EDR scores as ransomware tradecraft (#25583). Add native/windows-security (@orca/windows-security): a small N-API addon over advapi32 that reads and writes DACLs in-process. It exposes the token user SID, a DACL read as SDDL, a DACL write (sync and on the thread pool), a create-with-DACL file write, and an additive grant. Policy and verification stay in TypeScript. - writeSecureFile creates each temp file already carrying the protected user/SYSTEM/Administrators DACL (CreateFileW + SECURITY_ATTRIBUTES), reads it back once, and renames it. No ACL is modified on the write path and no process starts. Before: 5-7 spawns, 94-138 ms per write. After: 0 spawns, ~0.5 ms. - A secure directory is verified once per process. It is rewritten only when verification fails, on the thread pool, because the write walks every descendant (~2 s on a 29k-entry userData). - The verifier follows OpenSSH's rule: only the user, SYSTEM and Administrators may be granted anything, and the path must be protected (or, for a file, inherit from a secure parent). DACLs written by the icacls-era hardening pass unchanged, so there is no migration. - If the addon cannot load, the write still happens and reports unrestricted, logged once. There is no icacls or PowerShell fallback. - grantDirAcl, the startup userData grant, and the install-dir probe and package repair use the same addon, so whoami.exe and icacls.exe are no longer spawned anywhere at runtime. - orcad's Windows slots ship the addon beside their bundles (built per arch, PE-machine checked). Its data-root privacy check fails closed, so without the addon a Windows orcad would not start. Fixes #26027 Fixes #25583

… lane The read path called restrictWindowsPathSync directly and recorded success itself; the existing sync lane already does both. One call site fewer.

Conflicts resolved: - .gitignore: kept both sides (main's native/windows-agent-hook/target/ and this branch's native/windows-security build outputs). - src/main/runtime/mobile-notification-dismissal-store.ts: took main's side (#27731 moved the store to AtomicSnapshotWriter; this branch only touched a comment there). Content changes inside this merge: - config/scripts/pr-code-change-scope.mjs: the native/windows-registry and native/windows-security sources are classified through NATIVE_CACHE_PREFIXES instead of eight literal entries, which keeps the merged file under its 600-line limit. - config/main-sync-spawn-baseline.txt: pruned. src/main/win32-utils.ts (2 -> 0) and src/shared/secure-path-windows-acl.ts (3 -> 0) no longer launch icacls/whoami.

coderabbitai · 2026-10-13T01:33:49Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 9c929345-9da1-444d-a9e0-ddae3fcf7ad4

📥 Commits

Reviewing files that changed from the base of the PR and between fb3e9b5 and 5a83dfe.

📒 Files selected for processing (39)

config/main-sync-spawn-baseline.txt
config/scripts/check-main-sync-spawn-ratchet.mjs
config/scripts/check-main-sync-spawn-ratchet.test.mjs
native/windows-security/index.d.ts
native/windows-security/index.js
native/windows-security/src/addon.cc
src/main/artifacts/artifact-create-intent-store.test.ts
src/main/orcad/orcad-managed-stop.test.ts
src/main/runtime/unreadable-secret-store-preservation.win32.test.ts
src/main/startup/gpu-lifecycle-install-dir-acl-guard.test.ts
src/main/startup/windows-install-dir-acl-probe.test.ts
src/main/startup/windows-install-dir-acl-probe.ts
src/main/startup/windows-install-dir-acl-recovery.test.ts
src/main/startup/windows-install-dir-acl-repair.win32.test.ts
src/main/startup/windows-install-dir-acl.test-fixture.ts
src/main/startup/windows-install-dir-package-acl-grant.test.ts
src/main/startup/windows-install-dir-package-acl-grant.ts
src/main/startup/windows-install-dir-package-acl-repair.test.ts
src/main/startup/windows-install-dir-package-acl-repair.ts
src/main/startup/windows-user-data-acl.ts
src/main/win32-utils-async-acl.test.ts
src/main/win32-utils.ts
src/main/zcode/zcode-plan-api-key-store-native-windows.test.ts
src/main/zcode/zcode-plan-api-key-store-plaintext-rollback.test.ts
src/main/zcode/zcode-plan-api-key-store-publication-race.test.ts
src/shared/secure-file-fsync-flags.test.ts
src/shared/secure-file-windows.test.ts
src/shared/secure-file.ts
src/shared/secure-path-hardening-report.ts
src/shared/secure-path-windows-acl.ts
src/shared/secure-path-windows-acl.win32.test.ts
src/shared/windows-secure-dacl.test.ts
src/shared/windows-secure-dacl.ts
src/shared/windows-security-addon.test-fixture.ts
src/shared/windows-security-addon.test.ts
src/shared/windows-security-addon.ts
src/shared/windows-security-addon.win32.test.ts
src/shared/windows-security-descriptor.test.ts
src/shared/windows-security-descriptor.ts

💤 Files with no reviewable changes (2)

src/shared/windows-security-descriptor.test.ts
src/shared/windows-security-descriptor.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

The change adds a Windows native addon for SID lookup, DACL inspection and updates, restricted file creation, and access grants. Windows secure-file handling and startup ACL probes and grants now use the addon instead of icacls subprocesses. Build, packaging, cache, CI, and ORCAD artifact paths include the addon, with checks for its binary architecture. Tests and runtime checks were updated for the new addon and ACL paths.

Priority: ➖ Normal

Severity of issue fixed: Medium

Merge Risk: 🟡 Moderate · up to 5a83d

Two earlier review concerns remain open. User-data ACL repair may skip existing protected nested entries and still be marked as done. Secure-file directory hardening may not notice later ACL drift. Resolve or accept both before merging.

Pre-merge checks · 4 · 1

Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 39.47% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 152 functions across 69 files. (1 skipped…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check		The active direct issue is `#25583`. The PR replaces runtime `icacls.exe` and `whoami.exe` ACL operations with `@orca/windows-security`. `writeRestrictedWindowsFile` creates a new file with the target D…
Out of Scope Changes check		The native addon, secure-file integration, ACL grant and repair migration, trusted packaging, `orcad` staging, and related tests support `#25583` by removing runtime ACL process launches and ensuring th…
Title check		The title clearly and concisely describes the primary change: replacing icacls-based Windows secure-file restriction with in-process ACL handling.
Description check		The description is comprehensive. It covers the user impact, implementation, rationale, linked issues, visual proof, testing, platform coverage, and relevant notes. Some optional template sections, su…

Full details: Docstring Coverage

Explanation

Docstring coverage is 39.47% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 152 functions across 69 files. (1 skipped: 1 unsupported.)

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

Commit to this branch
Create a new PR

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.}

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/main/startup/windows-install-dir-package-acl-repair.ts (1)

62-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Fix the duplicated article in the comment.

Line 62 reads "or an a repair". Remove "an".

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 04d7120e-6462-43d6-9a37-44a71ff8b76a

📥 Commits

Reviewing files that changed from the base of the PR and between 20e4221 and 3b71ed9.

⛔ Files ignored due to path filters (1)

pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml

📒 Files selected for processing (72)

.github/actions/prepare-native-runtime/action.yml
.github/workflows/ci-cache-warmup.yml
.github/workflows/node-server-tests.yml
.github/workflows/pr.yml
.github/workflows/ssh-windows-hosts.yml
.gitignore
config/electron-builder.config.cjs
config/main-sync-spawn-baseline.txt
config/packaged-runtime-node-modules.cjs
config/process-host-direct-import-baseline.txt
config/scripts/build-orcad.mjs
config/scripts/build-windows-security-addon.mjs
config/scripts/ci-native-cache.test.mjs
config/scripts/ensure-native-runtime.mjs
config/scripts/ensure-native-runtime.test.mjs
config/scripts/node-server-change-scope.mjs
config/scripts/orcad-windows-security.mjs
config/scripts/orcad-windows-security.test.mjs
config/scripts/package-electron-runtime-contract.test.mjs
config/scripts/pr-code-change-scope.mjs
config/scripts/rebuild-native-deps-node-pty.test.mjs
config/scripts/rebuild-native-deps-test-fixtures.mjs
config/scripts/rebuild-native-deps.mjs
native/windows-security/binding.gyp
native/windows-security/index.d.ts
native/windows-security/index.js
native/windows-security/package.json
native/windows-security/src/addon.cc
package.json
pnpm-workspace.yaml
src/main/agent-hooks/installer-utils.ts
src/main/artifacts/artifact-create-intent-store.test.ts
src/main/codex-accounts/fs-utils.ts
src/main/orcad/orcad-data-root-privacy.ts
src/main/runtime/device-registry.ts
src/main/runtime/rpc/mobile-auth-acl-critical-path.test.ts
src/main/runtime/unreadable-secret-store-preservation.win32.test.ts
src/main/startup/gpu-lifecycle-install-dir-acl-guard.test.ts
src/main/startup/main-window-actions.ts
src/main/startup/windows-install-dir-acl-poison-marker.ts
src/main/startup/windows-install-dir-acl-probe.test.ts
src/main/startup/windows-install-dir-acl-probe.ts
src/main/startup/windows-install-dir-acl-recovery.test.ts
src/main/startup/windows-install-dir-acl-recovery.ts
src/main/startup/windows-install-dir-acl-repair.win32.test.ts
src/main/startup/windows-install-dir-acl.test-fixture.ts
src/main/startup/windows-install-dir-package-acl-grant.test.ts
src/main/startup/windows-install-dir-package-acl-grant.ts
src/main/startup/windows-install-dir-package-acl-repair.test.ts
src/main/startup/windows-install-dir-package-acl-repair.ts
src/main/startup/windows-user-data-acl.test.ts
src/main/startup/windows-user-data-acl.ts
src/main/win32-utils-async-acl.test.ts
src/main/win32-utils.ts
src/main/zcode/zcode-plan-api-key-store-native-windows.test.ts
src/main/zcode/zcode-plan-api-key-store-plaintext-rollback.test.ts
src/main/zcode/zcode-plan-api-key-store-publication-race.test.ts
src/shared/orcad-artifacts.test.ts
src/shared/orcad-artifacts.ts
src/shared/secure-file-windows.test.ts
src/shared/secure-file.test.ts
src/shared/secure-file.ts
src/shared/secure-path-hardening-report.ts
src/shared/secure-path-hardening-retry-budget.ts
src/shared/secure-path-metadata-snapshot.ts
src/shared/secure-path-windows-acl.ts
src/shared/secure-path-windows-acl.win32.test.ts
src/shared/windows-secure-dacl.test.ts
src/shared/windows-secure-dacl.ts
src/shared/windows-security-addon.test-fixture.ts
src/shared/windows-security-addon.ts
src/shared/windows-security-addon.win32.test.ts

💤 Files with no reviewable changes (2)

config/process-host-direct-import-baseline.txt
config/main-sync-spawn-baseline.txt

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

…#28135) - The descriptor parser is anchored, so a conditional ACE (its body holds parentheses) or anything else it cannot read fails closed instead of verifying the ACEs before it. - findSecureDaclViolation is one flat rule: protected, allow rules only for the user, SYSTEM and Administrators (OpenSSH's ACE rule), and full control for the user. The inherits-from-a-secure-parent exception is gone: it accepted exactly what loosening the parent reaches. - The addon and the user SID load once, behind one wrapper that adds the extended-length prefix to every path and runs async DACL writes to one path in order, so a startup grant cannot land after a re-lock and restore the broader DACL. - With the addon loaded, a failed born-restricted create throws with the matching Node error code instead of falling back to a plain write. - A grant leaves a NULL DACL untouched instead of turning 'everyone' into 'only this SID'. WriteFile runs once and fails on a short write. - The artifact create-intent store writes through writeSecureFile and drops its own temp writer, directory LRU and synchronous directory re-lock. - Test seams the global fake makes redundant are gone.

…missing or foreign-arch The @orca/windows-* workspace links exist on every host, so their presence proved nothing: a missing or wrong-architecture .node only surfaced at runtime, as unrestricted secure files or an orcad that refuses to start. beforePack now reads each addon's PE machine against the target arch.

coderabbitai

Actionable comments posted: 2

🧹 Nitpick comments (1)

src/shared/secure-path-windows-acl.win32.test.ts (1)

63-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Remove the duplicate resetWindowsSecurityLoadForTests() call.

Line 64 repeats the call on Line 63. The second call does nothing, so delete it.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 5e0fca5b-6398-48f1-aa14-a43c59b10952

📥 Commits

Reviewing files that changed from the base of the PR and between 3b71ed9 and 9c07e52.

📒 Files selected for processing (36)

config/packaged-runtime-node-modules.cjs
config/scripts/electron-builder-config.test.mjs
config/scripts/orcad-windows-security.mjs
config/scripts/orcad-windows-security.test.mjs
native/windows-security/index.d.ts
native/windows-security/src/addon.cc
src/main/artifacts/artifact-create-intent-store.test.ts
src/main/artifacts/artifact-create-intent-store.ts
src/main/runtime/device-registry.ts
src/main/runtime/rpc/mobile-auth-acl-critical-path.test.ts
src/main/runtime/unreadable-secret-store-preservation.win32.test.ts
src/main/startup/windows-install-dir-acl-probe.test.ts
src/main/startup/windows-install-dir-acl-probe.ts
src/main/startup/windows-install-dir-acl-recovery.test.ts
src/main/startup/windows-install-dir-acl-repair.win32.test.ts
src/main/startup/windows-install-dir-acl.test-fixture.ts
src/main/startup/windows-install-dir-package-acl-grant.test.ts
src/main/startup/windows-install-dir-package-acl-grant.ts
src/main/startup/windows-user-data-acl.test.ts
src/main/startup/windows-user-data-acl.ts
src/main/win32-utils-async-acl.test.ts
src/main/win32-utils.ts
src/shared/secure-file-coarse-ctime.test.ts
src/shared/secure-file-windows.test.ts
src/shared/secure-file.test.ts
src/shared/secure-file.ts
src/shared/secure-path-hardening-report.ts
src/shared/secure-path-windows-acl.ts
src/shared/secure-path-windows-acl.win32.test.ts
src/shared/windows-secure-dacl.test.ts
src/shared/windows-secure-dacl.ts
src/shared/windows-security-addon.test-fixture.ts
src/shared/windows-security-addon.ts
src/shared/windows-security-addon.win32.test.ts
src/shared/windows-security-descriptor.test.ts
src/shared/windows-security-descriptor.ts

🚧 Files skipped from review as they are similar to previous changes (1)

src/main/runtime/device-registry.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

#28135) - Sync-spawn ratchet: hardenSecurePath and the sync-flag exemption it alone used are gone, and the advice and baseline header no longer say secure writers run icacls. - The install-dir probe reads ACEs through parseSddlDacl, so an ACE it cannot parse fails the whole reading closed instead of being skipped; AC is a resolved alias there now. - The packaging check reads the addon binary through node_modules, the copy that ships. - No winCode-to-errno table: nothing reads .code off a failed secure write. - The load memo is not mutated; secure-path keeps its own log-once flag. - grantInstallDirPackageAcl is a plain function, and secure-file restricts a Windows directory and a POSIX path directly instead of through one two-platform helper.

coderabbitai

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to GitHub limitations.

⚠️ Outside diff range comments (1)

🟠 Major · Do not publish after ACL verification fails. · secure-file.ts:138-175

src/shared/secure-file.ts:138-175
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Do not publish after ACL verification fails.

writeRestrictedWindowsFile returns false when the addon verifies an invalid DACL. writeSecureFile still calls renameSync, so a credential can be published with the inherited broad DACL before the caller handles the false result. Preserve the addon-unavailable fallback, but distinguish it from verification failure at the publication boundary.

Suggested fix

-    const restricted =
+    const windowsRestriction =
       process.platform === 'win32'
         ? writeRestrictedWindowsFile(tmpFile, contents)
-        : writeModeRestrictedFile(tmpFile, contents)
+        : null
+    const restricted = windowsRestriction?.restricted ?? writeModeRestrictedFile(tmpFile, contents)
...
     if (options.shouldPublish && !options.shouldPublish()) {
       rmSync(tmpFile, { force: true })
       return false
     }
+    if (windowsRestriction && windowsRestriction.addonAvailable && !windowsRestriction.restricted) {
+      rmSync(tmpFile, { force: true })
+      return false
+    }
     renameSync(tmpFile, targetPath)

-export function writeRestrictedWindowsFile(targetPath: string, contents: string): boolean {
+export function writeRestrictedWindowsFile(
+  targetPath: string,
+  contents: string
+): { restricted: boolean; addonAvailable: boolean } {
   const security = securityFor(targetPath)
   if (!security) {
     writeFileSync(targetPath, contents, { encoding: 'utf-8', mode: 0o600 })
-    return false
+    return { restricted: false, addonAvailable: false }
   }
...
-  return verifyAfterApply(security, targetPath, false)
+  return {
+    restricted: verifyAfterApply(security, targetPath, false),
+    addonAvailable: true
+  }
}

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 190e2976-7146-46a7-8b46-44d5e06fe17f

📥 Commits

Reviewing files that changed from the base of the PR and between 9c07e52 and 5618d3f.

📒 Files selected for processing (15)

config/main-sync-spawn-baseline.txt
config/packaged-runtime-node-modules.cjs
config/scripts/check-main-sync-spawn-ratchet.mjs
config/scripts/check-main-sync-spawn-ratchet.test.mjs
config/scripts/electron-builder-config.test.mjs
src/main/startup/windows-install-dir-acl-probe.test.ts
src/main/startup/windows-install-dir-acl-probe.ts
src/main/startup/windows-install-dir-package-acl-grant.test.ts
src/main/startup/windows-install-dir-package-acl-grant.ts
src/main/startup/windows-install-dir-package-acl-repair.ts
src/shared/secure-file-windows.test.ts
src/shared/secure-file.ts
src/shared/secure-path-windows-acl.ts
src/shared/windows-security-addon.ts
src/shared/windows-security-descriptor.ts

🚧 Files skipped from review as they are similar to previous changes (1)

config/main-sync-spawn-baseline.txt

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

…ative # Conflicts: # config/scripts/check-main-sync-spawn-ratchet.mjs

…e close fails - The GitHub Windows runner is the built-in Administrator (RID 500), whose SID reads back as LA. Production already resolves it through localDomainSidOf; the win32 test helpers parsed without it. One shared readDaclOf resolves aliases for every real-ACL suite, and the verifier's unit test covers an LA-aliased directory and LA as a stranger. - createFileWithDacl treats a failed CloseHandle like a failed write (lost data on a network volume) and deletes the torn file. - Drop a duplicate test reset and a typo.

…tructure (review #28135) - Loader: orcad's staged binary first, then the workspace package only if its real path is inside the app (process.resourcesPath, or this checkout). A bare specifier otherwise walks every parent up to C:\node_modules, which any local user may create, and orcad holds credentials. - The addon reads a DACL as { protected, aces: [{ type, flags, mask, sid }] } via GetAce, with a SID only for plain allow/deny ACEs and null for a NULL DACL. findSecureDaclViolation and the install-dir probe check that structure, so conditional, callback and object ACEs fail closed. The SDDL parser, its alias table, LA/localDomainSid resolution and getDaclSddl are gone; SDDL is only the write format. - Install-dir tree pass: files flagless as before, but a directory only when its own DACL is protected (inheritably). Re-granting inheriting directories re-propagated each subtree (3.2 s for 1,093 directories). The root is no longer granted, or counted, twice. The two passes are plain functions behind a { root, tree } seam. - One namespacing (path.win32.toNamespacedPath) at the addon boundary, also the per-path ordering key; the comment states the real sync-grant invariant. - restrictWindowsDirectory(dir): Promise<boolean> replaces the callback lane; the reporter is guarded once, so the 'settle' stage is gone. - Sync-spawn ratchet: the secure writers no longer spawn or block, so they leave the gate (109 -> 61 uses). - Errors name the Win32 call and code only; a failed GetSecurityDescriptorControl fails the write instead of defaulting to unprotected.

…old chmod order writeSecureFile now chmods the temp file to 0o600 before fsyncing it, so the fsync also covers the mode change and the file is owner-writable when opened. Assert what matters under umask 0o200: the write succeeds, the temp file is fsynced read-only, and the published file is exactly 0o600.

path.win32.toNamespacedPath rewrote POSIX temp paths (/tmp/x -> \tmp\x) in the Linux runs of the Windows-logic unit tests, so the fake addon wrote to the wrong place. The addon only runs on Windows, where the host function is the win32 one; elsewhere it is the identity. The long-path assertion runs on Windows only.

…ndows only The prefix comes from the host's toNamespacedPath, which is the identity off Windows; the Linux run of this suite expected the win32 escape.

…not be listed (review #28135) A readdir failure on userData aborted the whole grant, so the root grant that could repair a DACL refusing the listing was never attempted. The listing failure is now recorded, the root is still granted, and the marker stays withheld so the next launch retries.

…SIDs (review #28135) - The large-relock test waited only for the parent to turn protected, which happens before propagation reaches the children, then read a child. It now waits on the parent and both ends of the child range. - The token-user test rejected S-1-12-1 (Entra ID) accounts, failing the suite on Entra-joined dev machines.

Orca Worker added 3 commits October 11, 2026 22:57

refactor(secure-file): harden existing Windows files through the sync…

2b7f3eb

… lane The read path called restrictWindowsPathSync directly and recorded success itself; the existing sync lane already does both. One call site fewer.

coderabbitai Bot reviewed Oct 13, 2026

View reviewed changes

Comment thread native/windows-security/src/addon.cc Resolved

Orca Worker added 2 commits October 12, 2026 18:56

coderabbitai Bot reviewed Oct 13, 2026

View reviewed changes

Comment thread src/main/startup/windows-user-data-acl.ts Resolved

coderabbitai Bot reviewed Oct 13, 2026

View reviewed changes

Comment thread src/shared/secure-file.ts Resolved

Orca Worker added 8 commits October 12, 2026 20:46

Merge remote-tracking branch 'origin/main' into OrcaWin/windows-acl-n…

3701a6d

…ative # Conflicts: # config/scripts/check-main-sync-spawn-ratchet.mjs

test(windows-security): assert the install-dir long-path prefix on Wi…

4497096

…ndows only The prefix comes from the host's toNamespacedPath, which is the identity off Windows; the Linux run of this suite expected the win32 escape.

OrcaWin merged commit 4559c25 into main Oct 13, 2026
78 of 79 checks passed

This was referenced Oct 13, 2026

fix(secure-file): create the session key and daemon token already restricted #28276

Open

[Bug] Windows: console windows still flash and steal keyboard focus on 1.4.224 (git, netstat, icacls, gh, wsl spawned without windowsHide) #27390

Closed

nwparker mentioned this pull request Oct 14, 2026

perf(windows): keep the one-time first-launch ACL grant from lagging the whole machine #5134

Closed

Navigation Menu

Repository navigation

fix(windows): restrict secure files in-process instead of spawning icacls - #28135

fix(windows): restrict secure files in-process instead of spawning icacls#28135
OrcaWin merged 14 commits into
mainfrom
OrcaWin/windows-acl-native

OrcaWin commented Oct 13, 2026 •

edited by github-actions Bot

Loading

Uh oh!

coderabbitai Bot commented Oct 13, 2026 •

edited

Loading

Reviews paused

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

OrcaWin commented Oct 13, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ELI5

What Changed

Why

Linked Issue

Visual Proof

Testing

Notes

Uh oh!

coderabbitai Bot commented Oct 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

OrcaWin commented Oct 13, 2026 •

edited by github-actions Bot

Loading

coderabbitai Bot commented Oct 13, 2026 •

edited

Loading