Conversation
![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: 627b9d3f0f
ℹ️ 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".
| if !granted_entry.access.can_read() { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The new intersection logic unconditionally drops granted filesystem entries with access: none (can_read() is false), which means deny carve-outs are never preserved even when both requested and granted profiles include them. In request_permissions_response_from_client_result, this can widen the stored grant: a request like "write cwd except cwd/secret" intersected with the same client response becomes plain write cwd after this filter, re-enabling paths that were explicitly denied.
Useful? React with 👍 / 👎.
bolinfest
force-pushed
the
pr18275
branch
2 times, most recently
from
17f71a2 to
bf132de
Compare
bolinfest
force-pushed
the
pr18274
branch
6 times, most recently
from
f3ff42a to
07ac71b
Compare
bolinfest
force-pushed
the
pr18713
branch
2 times, most recently
from
cb3ad47 to
6b4eb40
Compare
bolinfest
force-pushed
the
pr18713
branch
2 times, most recently
from
b612649 to
f6b0616
Compare
bolinfest
force-pushed
the
pr18275
branch
2 times, most recently
from
e6518e0 to
f9163e8
Compare
## Why #18274 made `PermissionProfile` the canonical file-system permissions shape, but the round-trip from `FileSystemSandboxPolicy` to `PermissionProfile` still dropped one piece of policy metadata: `glob_scan_max_depth`. That field is security-relevant for deny-read globs such as `**/*.env`. On Linux, bubblewrap sandbox construction uses it to bound unreadable glob expansion. If a profile copied from active runtime permissions loses this value and is submitted back as an override, the resulting `FileSystemSandboxPolicy` can behave differently even though the visible permission entries look equivalent. ## What changed - Add `glob_scan_max_depth` to protocol `FileSystemPermissions` and preserve it when converting to/from `FileSystemSandboxPolicy`. - Keep legacy `read`/`write` JSON for simple path-only permissions, but force canonical JSON when glob scan depth is present so the metadata is not silently dropped. - Carry `globScanMaxDepth` through app-server `AdditionalFileSystemPermissions`, generated JSON/TypeScript schemas, and app-server/TUI conversion call sites. - Preserve the metadata through sandboxing permission normalization, merging, and intersection. - Carry the merged scan depth into the effective `FileSystemSandboxPolicy` used for command execution, so bounded deny-read globs reach Linux bubblewrap materialization. ## Verification - `cargo test -p codex-sandboxing glob_scan -- --nocapture` - `cargo test -p codex-sandboxing policy_transforms -- --nocapture` - `just fix -p codex-sandboxing` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/18713). * #18288 * #18287 * #18286 * #18285 * #18284 * #18283 * #18282 * #18281 * #18280 * #18279 * #18278 * #18277 * #18276 * #18275 * __->__ #18713
bolinfest
force-pushed
the
pr18275
branch
4 times, most recently
from
9a0c246 to
f26a648
Compare
bolinfest
force-pushed
the
pr18275
branch
4 times, most recently
from
5bc0ac8 to
cf9f1ea
Compare
| { | ||
| let entry = materialize_cwd_dependent_entry(entry, cwd); | ||
| if !entries.contains(&entry) { | ||
| entries.push(entry); |
There was a problem hiding this comment.
[P2] Do not persist standalone deny entries from partial grants
The intersection now appends every FileSystemAccessMode::None entry from both the request and the response after filtering accepted read/write grants. That preserves carveouts for broad filesystem grants, but it also records denies when no filesystem grant survived. For example, if the model requests network plus :cwd write with /repo/secrets denied, and the client approves only network, the returned session grant still includes the /repo/secrets deny. Later commands then run with network enabled but unexpectedly lose read access to /repo/secrets. Deny entries should only be retained when attached to an accepted filesystem grant they constrain.
| } | ||
|
|
||
| if let Some(path) = resolve_permission_path(&granted_entry.path, cwd) { | ||
| return access_covers( |
There was a problem hiding this comment.
[P2] Honor deny globs when accepting concrete grants
For concrete granted paths, this check asks resolve_access_with_cwd() whether the requested policy covers the path, but that resolver only considers resolved path/special entries and ignores GlobPattern deny entries. A request like :cwd write plus **/*.env deny can therefore accept a client response granting /repo/.env write because the broad :cwd entry covers it during intersection. The returned profile also carries the deny glob, so enforcement either later rejects the approved path or, on paths that consult can_write_path_with_cwd(), treats it as writable despite the requested carveout. The semantic subset check should reject concrete grants matched by requested deny globs before accepting the broad grant.
| let request_cwd = match request.cwd.clone() { | ||
| Some(cwd) => cwd, | ||
| None => conversation.config_snapshot().await.cwd, | ||
| }; |
There was a problem hiding this comment.
[P2] Send request cwd to permission reviewers
The server computes request_cwd and uses it later to normalize the response, but the approval request sent to app-server clients still omits that cwd. Permission profiles can contain :cwd, :project_roots, and cwd-relative deny globs; in delegated flows the request cwd can differ from the visible thread cwd. A client can therefore approve write :cwd without knowing which concrete directory the server will apply. Include the request cwd in the approval params, or materialize cwd-dependent entries before sending the request to clients.
viyatb-oai
left a comment
viyatb-oai
left a comment
There was a problem hiding this comment.
non blocking - there are some edge cases codex found that are not covered by subsequent prs in the stack
bolinfest
force-pushed
the
pr18275
branch
2 times, most recently
from
87b4c7d to
9a98ccb
Compare
2 participants
Why
Permission approval responses must not be able to grant more access than the tool requested. Moving this flow to
PermissionProfilemeans the comparison must be profile-shaped instead ofSandboxPolicy-shaped, and cwd-relative special paths such as:cwdand:project_rootsmust stay anchored to the turn that produced the request.What changed
This implements semantic
PermissionProfileintersection incodex-sandboxingfor file-system and network permissions. The intersection accepts narrower path grants, rejects broader grants, preserves deny-read carve-outs and glob scan depth, and materializes cwd-dependent special-path grants to absolute paths before they can be recorded for reuse.The request-permissions response paths now use that intersection consistently. App-server captures the request turn cwd before waiting for the client response, includes that cwd in the v2 approval params, and core stores the requested profile plus cwd for direct TUI/client responses and Guardian decisions before recording turn- or session-scoped grants. The TUI app-server bridge now preserves the app-server request cwd when converting permission approval params into core events.
Verification
cargo test -p codex-sandboxing intersect_permission_profiles -- --nocapturecargo test -p codex-app-server request_permissions_response -- --nocapturecargo test -p codex-core request_permissions_response_materializes_session_cwd_grants_before_recording -- --nocapturecargo check -p codex-tui --testscargo check --testscargo test -p codex-tui app_server_request_permissions_preserves_file_system_permissions