fix(windows-sandbox): allow deletion in writable roots#31138
fix(windows-sandbox): allow deletion in writable roots#31138fcoury-oai wants to merge 3 commits into
Conversation
|
It seems crazy that this is taking so long to be merged, considering how small the fix is, how big of a deal the issue is, and how long it's already been broken for. The amount of compute wasted by this bug alone at scale is not insignificant. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0119f3bfc0
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0916a34301
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
fcoury-oai
force-pushed
the
codex/fix-windows-sandbox-delete-acl
branch
from
3f97310 to
767540e
Compare
fcoury-oai
changed the title
2 participants
Why
The legacy unelevated Windows sandbox allowed tools to create and update files in workspace-write roots, but it could not delete files that already existed there. This breaks operations such as
apply_patchfile deletion and replacement in the workspace,TEMP, andTMP.The delete grant must also preserve deny-write carveouts. Granting
FILE_DELETE_CHILDon a writable parent would let the sandbox remove protected children such as.gitor an explicit read-only subpath even when those children have direct deny ACEs.This addresses the delete-failure variant reported in #30009 and #30712. It does not claim to fix their separate split-root setup, elevated-helper, or proxy-related failure modes.
What Changed
DELETErights without granting parent-levelFILE_DELETE_CHILD, so descendants can be removed while protected children remain protected.FILE_DELETE_CHILD, and make elevated setup detect and refresh that unsafe legacy state.TEMP, andTMP, plus protected.gitand outside-root controls.The core ACL behavior is in
acl.rs, stale-ACE detection is insetup_main/win.rs, and the end-to-end regression is inunified_exec/tests.rs.How to Test
On Windows:
TEMP, andTMP; also create a sibling file outside the writable roots and a protected.gitdirectory.apply_patch..gitdirectory remain intact.Targeted tests:
just test -p codex-windows-sandboxlegacy_workspace_write_delete_is_limited_to_writable_rootswrite_root_refresh_replaces_stale_delete_child_grantThe final SHA passed all 31 required checks, including the Windows Bazel test matrix, in run 28886245161.