Skip to content

protocol: preserve glob scan depth in permission profiles#18713

Merged
bolinfest merged 1 commit intomainfrom
pr18713
Apr 21, 2026
Merged

protocol: preserve glob scan depth in permission profiles#18713
bolinfest merged 1 commit intomainfrom
pr18713

Conversation

@bolinfest
Copy link
Copy Markdown
Collaborator

@bolinfest bolinfest commented Apr 20, 2026

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

Stack created with Sapling. Best reviewed with ReviewStack.

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

Here are some automated review suggestions for this pull request.

Reviewed commit: 4ba4370b70

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

FileSystemPermissions { entries }
FileSystemPermissions {
entries,
glob_scan_max_depth: requested_file_system.glob_scan_max_depth,
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 Intersect glob scan depth with granted profile

intersect_permission_profiles filters entries against granted, but it always keeps requested_file_system.glob_scan_max_depth. When an approver returns a stricter depth (for example a larger depth, or None for unbounded scanning) the intersection result can become less restrictive than what was granted, so request_permissions_response_from_client_result may persist broader filesystem access than the client approved.

Useful? React with 👍 / 👎.

Comment thread codex-rs/sandboxing/src/policy_transforms.rs Outdated
@bolinfest bolinfest force-pushed the pr18713 branch 4 times, most recently from b612649 to f6b0616 Compare April 20, 2026 22:21
@bolinfest bolinfest merged commit 3d2f123 into main Apr 21, 2026
48 of 61 checks passed
@bolinfest bolinfest deleted the pr18713 branch April 21, 2026 02:42
@github-actions github-actions Bot locked and limited conversation to collaborators Apr 21, 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