Skip to content

windows-sandbox: send permission profiles to elevated runner#22918

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

windows-sandbox: send permission profiles to elevated runner#22918
bolinfest merged 1 commit into
mainfrom
pr22918

Conversation

@bolinfest
Copy link
Copy Markdown
Collaborator

@bolinfest bolinfest commented May 15, 2026

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.

Stack created with Sapling. Best reviewed with ReviewStack.

@bolinfest bolinfest changed the base branch from main to pr22896 May 15, 2026 22:43
@chatgpt-codex-connector
Copy link
Copy Markdown
Contributor

💡 Codex Review

let mut roots = self
.file_system
.get_writable_roots_with_cwd(cwd)

P2 Badge Filter SlashTmp out of Windows writable roots

In the Windows sandbox path this now feeds the generic runtime filesystem policy directly into get_writable_roots_with_cwd, but the default workspace-write profile still contains the Unix-only :slash_tmp write entry (exclude_slash_tmp defaults to false). On Windows, that special path resolves as a rooted \tmp/current-drive path and, when C:\tmp exists, compute_allow_paths/setup will grant the write capability there even though the previous Windows-specific code only added the command cwd, configured roots, and TEMP/TMP. This broadens every default workspace-write Windows sandbox on machines with that directory, so the Windows resolver should ignore FileSystemSpecialPath::SlashTmp (or otherwise preserve the old Windows behavior).


let mut roots = self
.file_system
.get_writable_roots_with_cwd(cwd)

P2 Badge Avoid resolving Unix TMPDIR for Windows tmp grants

For profiles containing :tmpdir (including the default workspace-write profile), this call resolves the generic FileSystemSpecialPath::Tmpdir before the Windows-specific TEMP/TMP handling below. That generic resolver reads the parent process TMPDIR, not the command env, so a Windows run where TMPDIR is set to an existing absolute path will grant write access to that unrelated directory in addition to TEMP/TMP; the old Windows-specific code only considered TEMP and TMP from env_map/process env. Filter the symbolic tmpdir entry out before calling get_writable_roots_with_cwd or make the Windows path resolution use the TEMP/TMP logic exclusively.

ℹ️ 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
Copy link
Copy Markdown
Collaborator Author

Addressed the two Windows temp-root review items in the current stack.

  • :slash_tmp and generic :tmpdir entries are filtered before the Windows writable-root resolver calls get_writable_roots_with_cwd, so Windows no longer grants C:\tmp/\tmp or parent-process TMPDIR as a generic path.
  • :tmpdir write support now goes through the Windows-specific TEMP/TMP handling only, using the command env map first and then process env.
  • Coverage is in permission_profile_workspace_write_uses_windows_temp_env_vars in codex-rs/windows-sandbox-rs/src/resolved_permissions.rs.

Relevant code is in ResolvedWindowsSandboxPermissions::writable_roots_for_cwd and windows_temp_env_roots.

Copy link
Copy Markdown
Collaborator

@iceweasel-oai iceweasel-oai left a comment

Choose a reason for hiding this comment

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

approved - couple of nit/comments

Comment thread codex-rs/windows-sandbox-rs/src/resolved_permissions.rs Outdated
Comment thread codex-rs/windows-sandbox-rs/src/unified_exec/backends/elevated.rs
Base automatically changed from pr22896 to main May 20, 2026 17:30
bolinfest added a commit that referenced this pull request May 20, 2026
## 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
@bolinfest bolinfest merged commit 729bdf3 into main May 20, 2026
46 checks passed
@bolinfest bolinfest deleted the pr22918 branch May 20, 2026 19:41
@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.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants