Do not grant Windows sandbox ACLs on USERPROFILE#18443
Conversation
6192da7 to
ab952b1
Compare
ab952b1 to
d4f84ad
Compare
d4f84ad to
2f71016
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f71016caf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
| }; | ||
| let write_roots = expand_user_profile_root(write_roots); | ||
| let write_roots = filter_user_profile_root(write_roots); |
There was a problem hiding this comment.
Preserve USERPROFILE when it is the working-directory root
build_payload_roots always applies filter_user_profile_root to write roots. If command_cwd == USERPROFILE, gather_write_roots intentionally includes CWD, but this filter removes it. The payload then grants only existing child entries, so writes that require permission on the home root itself (e.g., creating a new top-level file/dir) can fail when running from home.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is intended. Tradeoff is necessary to not break things like SSH as perms granted at top level percolate downward.
Stack
USERPROFILEroots.Bug
On Windows, Codex can grant the sandbox ACL access to the whole user profile directory.
That means the sandbox ACL can be applied under paths like:
This breaks SSH. Windows OpenSSH checks permissions on SSH config and key material. If Codex adds a sandbox group ACL to those files, OpenSSH can reject the config or keys.
The bad interaction is:
USERPROFILE.Why this happens more now
Codex now has more flows that naturally start in the user profile:
Those are valid user actions. The bug is that
USERPROFILEis too broad a sandbox root.Change
This PR keeps the useful behavior of starting from the user profile without granting the profile root itself.
The new flow is:
USERPROFILE, replace it with the direct children ofUSERPROFILEUSERPROFILEitself from the final root list.tshand.brevto that exclusion listSo this input:
becomes roots like:
and does not include:
If
USERPROFILEcannot be listed, expansion falls back to the profile root and the later filter removes it. That keeps the failure mode closed for this bug.Why this shape
The sandbox still gets access to ordinary profile folders when the user starts from home.
The sandbox no longer grants access to the profile root itself.
All filtering happens after expansion, for both read and write roots. That gives us one simple rule: expand broad profile grants first, then remove roots the sandbox must not own.
Tests
just fmtcargo test -p codex-windows-sandboxjust fix -p codex-windows-sandboxgit diff --check