Skip to content

windows-sandbox: drive write roots from resolved permissions#22923

Merged
bolinfest merged 1 commit into
mainfrom
pr22923
May 20, 2026
Merged

windows-sandbox: drive write roots from resolved permissions#22923
bolinfest merged 1 commit into
mainfrom
pr22923

Conversation

@bolinfest
Copy link
Copy Markdown
Collaborator

@bolinfest bolinfest commented May 15, 2026 •
edited
Loading

Why

This is the third PR in the Windows sandbox SandboxPolicy -> PermissionProfile migration stack.

#22896 introduced ResolvedWindowsSandboxPermissions, and #22918 moved elevated runner IPC to carry PermissionProfile. This PR starts moving the remaining setup/spawn helpers away from asking legacy enum questions like “is this WorkspaceWrite?” and toward resolved runtime permission questions like “does this profile require write capability roots?”

What changed

  • Added resolved-permissions helpers for network identity and write-capability detection.
  • Moved setup write-root gathering to operate on ResolvedWindowsSandboxPermissions, with the legacy SandboxPolicy wrapper left in place for existing call sites.
  • Updated identity setup, elevated capture setup, and world-writable audit denies to use resolved write roots.
  • Updated spawn preparation to carry resolved permissions in SpawnContext and use them for network blocking, setup write roots, elevated capability SID selection, and legacy capability roots.
  • Removed a now-unused legacy write-root helper.

Verification

Stack created with Sapling. Best reviewed with ReviewStack.

@bolinfest bolinfest changed the base branch from main to pr22918 May 15, 2026 23:13
This was referenced May 15, 2026
Merged
Merged
@chatgpt-codex-connector
Copy link
Copy Markdown
Contributor

chatgpt-codex-connector Bot commented May 15, 2026

💡 Codex Review

codex/codex-rs/windows-sandbox-rs/src/spawn_prep.rs

Line 51 in a4be788

pub(crate) uses_write_capabilities: bool,

P1 Badge Update remaining SpawnContext field uses

On Windows this struct no longer exposes is_workspace_write, but the legacy capture/session code still reads common.is_workspace_write (for example in lib.rs and unified_exec/backends/legacy.rs). That makes the Windows crate fail to compile for the restricted-token backend as soon as this commit is built for target_os = "windows"; those call sites need to use the new uses_write_capabilities field or keep a compatibility field.

codex/codex-rs/windows-sandbox-rs/src/allow.rs

Lines 21 to 22 in a4be788

let permissions =
ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd(policy, command_cwd);

P1 Badge Use policy_cwd when resolving workspace roots

When sandbox_policy_cwd is the session/workspace root but the command runs with cwd set to a subdirectory, this now resolves the legacy workspace-write policy against command_cwd and ignores _policy_cwd. Core passes those as separate values, so the setup/capability roots shrink to the command subdirectory and writes elsewhere in the approved workspace start failing under the Windows sandbox; this should resolve the permission profile with the policy cwd and only use command_cwd as the process cwd.

ℹ️ 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".

@bolinfest bolinfest force-pushed the pr22923 branch 5 times, most recently from c42e370 to 49a9062 Compare May 16, 2026 00:38
@github-actions github-actions Bot mentioned this pull request May 16, 2026
Open
@bolinfest bolinfest mentioned this pull request May 17, 2026
Merged
@github-actions github-actions Bot mentioned this pull request May 18, 2026
Open
@bolinfest
Copy link
Copy Markdown
Collaborator Author

bolinfest commented May 18, 2026

Addressed both Codex review items in the current stack.

  • The remaining legacy Windows call sites now use common.uses_write_capabilities instead of the removed common.is_workspace_write field. See lib.rs and unified_exec/backends/legacy.rs.
  • Legacy workspace-write resolution now resolves the profile with sandbox_policy_cwd / policy_cwd, while still using command_cwd as the process cwd. See allow.rs and spawn_prep.rs.
  • Regression coverage was added for both the allow-path behavior and legacy session capability roots when the command cwd is a workspace subdirectory.

Current-head CI for #22923 is green, including Windows Bazel shards.

This was referenced May 20, 2026
Merged
Merged
@bolinfest bolinfest requested a review from iceweasel-oai May 20, 2026 17:03
iceweasel-oai
iceweasel-oai reviewed May 20, 2026
View reviewed changes
Comment thread codex-rs/windows-sandbox-rs/src/lib.rs Outdated
Comment thread codex-rs/windows-sandbox-rs/src/spawn_prep.rs
bolinfest added a commit that referenced this pull request May 20, 2026
@bolinfest
windows-sandbox: add resolved permissions helper (#22896)
2b4898c 
## Why

The Windows sandbox migration away from the legacy `SandboxPolicy`
abstraction needs a small local bridge before IPC and core wiring can
move to `PermissionProfile`. Leaf helpers currently branch directly on
`WorkspaceWrite`, which spreads legacy assumptions through path planning
and token setup code.

This PR introduces a Windows-local resolved permissions view so those
helpers can ask Windows-specific questions about runtime
filesystem/network permissions without matching on the legacy policy
enum everywhere.

## What changed

- Added `ResolvedWindowsSandboxPermissions` in
`windows-sandbox-rs/src/resolved_permissions.rs`, with legacy
`SandboxPolicy` constructors for the current call sites.
- Moved `allow.rs` writable-root and read-only-subpath planning onto the
resolved permissions type.
- Preserved Windows `TEMP`/`TMP` writable-root behavior when the
effective policy includes writable tmpdir access.
- Avoided resolving Unix `:slash_tmp` or parent-process `TMPDIR` while
computing Windows writable roots.
- Reused the shared allow-path result for setup write-root gathering and
routed network-block selection through the resolved abstraction.

## Verification

- `cargo test -p codex-windows-sandbox`
- `just fix -p codex-windows-sandbox`
- GitHub CI restarted on the amended commit; Windows Bazel is the
required signal for the Windows-only code paths.












---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/openai/codex/pull/22896).
* #23715
* #23714
* #23167
* #22923
* #22918
* __->__ #22896
Base automatically changed from pr22918 to main May 20, 2026 19:41
bolinfest added a commit that referenced this pull request May 20, 2026
@bolinfest
windows-sandbox: send permission profiles to elevated runner (#22918)
729bdf3 
## Why

This is the next PR in the Windows sandbox migration stack after #22896.
The bottom PR introduces a Windows-local resolved permissions helper
while existing callers still start from legacy `SandboxPolicy`. This PR
moves the elevated runner IPC boundary to `PermissionProfile`, which
makes the direction of the stack visible without changing the public
core call sites yet.

Because that changes the CLI-to-command-runner message shape, the framed
IPC protocol version is bumped in the same PR so the boundary change is
explicit.

## What changed

- Replaced elevated IPC `policy_json_or_preset`/`sandbox_policy_cwd`
fields with `permission_profile`/`permission_profile_cwd`.
- Bumped the elevated command-runner IPC protocol to
`IPC_PROTOCOL_VERSION = 2` and switched parent/runner frames to use the
shared constant.
- Converted the parent elevated paths from the parsed legacy policy into
a materialized `PermissionProfile` before sending the runner request.
- Added `WindowsSandboxTokenMode` resolution for managed
`PermissionProfile` values and made the runner choose read-only vs
writable-root capability tokens from that resolved profile.
- Rejected disabled, external, unrestricted, and full-disk-write
profiles before token selection.
- Added IPC JSON coverage for tagged `PermissionProfile` payloads and
token-mode unit coverage for the resolved permission helper.

## Verification

- `cargo test -p codex-windows-sandbox`
- `just fix -p codex-windows-sandbox`
- `cargo check -p codex-windows-sandbox --target x86_64-pc-windows-msvc
--tests` was attempted locally but blocked before crate type-checking
because the macOS compiler environment lacks Windows C headers such as
`windows.h` and `assert.h`; GitHub Windows CI is the required
verification for the runner path.

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/openai/codex/pull/22918).
* #23715
* #23714
* #23167
* #22923
* __->__ #22918
@bolinfest bolinfest force-pushed the pr22923 branch 2 times, most recently from 412eaba to 2bd530d Compare May 20, 2026 19:53
@bolinfest bolinfest requested a review from iceweasel-oai May 20, 2026 20:15
@bolinfest bolinfest requested a review from a team as a code owner May 20, 2026 20:59
@bolinfest bolinfest merged commit e1ec0ee into main May 20, 2026
47 of 62 checks passed
@bolinfest bolinfest deleted the pr22923 branch May 20, 2026 21:30
@github-actions github-actions Bot locked and limited conversation to collaborators May 20, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@iceweasel-oai iceweasel-oai iceweasel-oai approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@bolinfest @iceweasel-oai