Skip to content

feat(sandbox): add Windows deny-read parity#18202

Open
viyatb-oai wants to merge 2 commits intomainfrom
codex/viyatb/windows-deny-read-parity
Open

feat(sandbox): add Windows deny-read parity#18202
viyatb-oai wants to merge 2 commits intomainfrom
codex/viyatb/windows-deny-read-parity

Conversation

@viyatb-oai
Copy link
Copy Markdown
Collaborator

@viyatb-oai viyatb-oai commented Apr 16, 2026

Summary

Adds Windows subprocess enforcement for split filesystem access = none read restrictions, now that the exact/glob deny-read policy stack has landed on main.

This PR keeps the scope to Windows parity:

  • Resolves Windows deny-read filesystem policy entries into concrete ACL targets.
  • Preserves exact missing paths so they can be materialized and denied before the sandboxed process starts.
  • Snapshot-expands existing glob matches into ACL targets for Windows subprocess enforcement.
  • Plans both lexical and canonical targets for existing paths so reparse-point aliases do not bypass the deny.
  • Threads deny-read overrides through both Windows sandbox backends:
    • restricted-token backend capability SID ACLs
    • elevated/logon-user backend sandbox group ACL setup
  • Records persistent deny-read ACLs separately for restricted-token and elevated principals so stale cleanup does not cross backend ownership.
  • Adds a Windows-only integration test that exercises exact and glob deny-read behavior through shell execution.

Landed Prerequisites

These base stack PRs are already on main:

  1. feat(permissions): add glob deny-read policy support #15979 feat(permissions): add glob deny-read policy support
  2. feat(sandbox): add glob deny-read platform enforcement #18096 feat(sandbox): add glob deny-read platform enforcement
  3. feat(config): support managed deny-read requirements #17740 feat(config): support managed deny-read requirements

This PR now targets main directly and contains only the Windows deny-read parity layer.

Implementation Notes

  • Exact deny-read paths are kept even when missing, then materialized as directories before applying the deny ACE. This is fail-closed: a sandboxed command cannot create that denied path and read it during the same run.
  • Existing deny-read paths are planned as both the configured lexical path and the canonical target. This covers path aliases caused by existing reparse points while still preserving the user-configured path.
  • Glob deny-read entries are expanded to existing matches before process launch. Windows ACLs receive concrete paths only.
  • Deny-read ACEs are applied after broad read grants so explicit deny entries take precedence.
  • Stale persistent deny-read ACL cleanup is keyed by backend principal kind to avoid revoking ACLs owned by the other Windows backend.

Copy link
Copy Markdown
Contributor

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

for path in additional_deny_write_paths {
if path.exists() {
deny.insert(path.clone());
}

P1 Badge Apply deny-write ACLs to missing None carveouts

The restricted-token path only adds additional_deny_write_paths when the path already exists. New deny-read logic later materializes missing deny paths (apply_deny_read_acls), so a FileSystemAccessMode::None path that starts missing gets read-denied but not write-denied. A sandboxed process can still create/write descendants under that directory, violating None semantics.


for path in additional_deny_write_paths {
if path.exists() {
deny.insert(path.clone());
}

P1 Badge Apply deny-write ACLs to missing None carveouts

The restricted-token path only adds additional_deny_write_paths when the path already exists. New deny-read logic then materializes missing deny paths (apply_deny_read_acls), so a missing FileSystemAccessMode::None path becomes read-denied but not write-denied. A sandboxed command can still create/write under that directory, violating None semantics.

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

Comment on lines +852 to +855
.map(|path| {
if path.exists() {
dunce::canonicalize(&path).unwrap_or(path)
} else {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Badge Keep lexical deny-read paths in elevated payloads

build_payload_deny_read_paths canonicalizes every existing deny-read path before sending it to the helper. This strips the lexical path variant that plan_deny_read_acl_paths is designed to preserve (lexical + canonical) for reparse-point alias coverage, so elevated setup no longer matches the stated alias-hardening intent.

Useful? React with 👍 / 👎.

Comment thread codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs Outdated
)?;
} else {
match read_acl_mutex_exists() {
Ok(true) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Badge Apply current deny-read payload even when helper is busy

When the read-ACL helper mutex exists, setup logs and skips spawning a helper, but deny-read ACL updates are now performed by that helper path. If policy changes while another helper run is active, the new deny-read payload is dropped for this command, so execution can proceed with stale deny rules.

Useful? React with 👍 / 👎.

Comment on lines +88 to +90
let scan_key = dunce::canonicalize(path).unwrap_or_else(|_| path.to_path_buf());
if !seen_scan_dirs.insert(scan_key) {
return Ok(());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Badge Avoid cross-pattern scan dedupe by canonical directory only

Global seen_scan_dirs keys scans by canonical path. If two unreadable globs use different lexical roots that resolve to the same target (junction/symlink aliases), the second root is skipped entirely. Alias-specific matches from later patterns are then never added to deny ACL targets.

Useful? React with 👍 / 👎.

@viyatb-oai viyatb-oai force-pushed the codex/viyatb/deny-read-requirements branch 2 times, most recently from 4db0235 to e665898 Compare April 17, 2026 00:44
Base automatically changed from codex/viyatb/deny-read-requirements to main April 17, 2026 15:40
viyatb-oai and others added 2 commits April 17, 2026 14:07
Add Windows deny-read enforcement for split filesystem policies by resolving exact and glob unreadable entries into ACL targets, threading those paths through the restricted-token and elevated Windows sandbox backends, and applying deny-read ACE overlays with stale cleanup records.

Exact missing paths are materialized before ACE application so sandboxed subprocesses cannot create and read them during the same run. Existing paths are planned with both lexical and canonical targets to cover reparse-point aliases.

Co-authored-by: Codex <noreply@openai.com>
Move Windows deny-read path resolution into the Windows sandbox crate, bound non-recursive glob expansion, and avoid unnecessary deny-read setup work when no read roots or persistent records are present.

Co-authored-by: Codex <noreply@openai.com>
@viyatb-oai viyatb-oai force-pushed the codex/viyatb/windows-deny-read-parity branch from ec765ef to 1013499 Compare April 17, 2026 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant