Skip to content

artifacts: honor filesystem policy carveouts in path checks#13442

Open
bolinfest wants to merge 4 commits intomainfrom
pr13442
Open

artifacts: honor filesystem policy carveouts in path checks#13442
bolinfest wants to merge 4 commits intomainfrom
pr13442

Conversation

@bolinfest
Copy link
Collaborator

@bolinfest bolinfest commented Mar 4, 2026

Why

#13434 and #13440 introduced FileSystemSandboxPolicy and its explicit unreadable-path semantics, but the artifact tool handlers were still authorizing reads and writes via legacy SandboxPolicy helpers.

That meant the new none carveouts were invisible to presentation_artifact and spreadsheet_artifact approvals. Before the backend sandbox implementations learn to enforce unreadable entries directly, these tool-level path checks still need to honor them so artifact operations do not bypass the new policy model.

What Changed

  • added a small shared helper in core/src/tools/handlers/artifact_path_access.rs that answers readable and writable path checks from FileSystemSandboxPolicy
  • made that helper treat explicit unreadable roots as higher priority than both broad read access and writable roots
  • switched presentation_artifact and spreadsheet_artifact to authorize paths against turn.file_system_sandbox_policy instead of the legacy turn.sandbox_policy
  • added regression tests covering unreadable subpaths overriding both root read access and writable-root access

Verification

  • cargo test -p codex-core artifact_path_access
  • cargo test -p codex-core --lib

Stack created with Sapling. Best reviewed with ReviewStack.

@chatgpt-codex-connector
Copy link
Contributor

💡 Codex Review

&& cfg.default_permissions.is_none()
{
return Err(std::io::Error::new(
std::io::ErrorKind::InvalidInput,
"config defines `[permissions]` profiles but does not set `default_permissions`",

P1 Badge Preserve legacy permissions.network config loading

This new validation rejects any non-empty [permissions] table unless default_permissions is set, which now breaks previously valid configs that used [permissions.network] for proxy settings. In this commit PermissionsToml was changed to a flattened profile map, so legacy permissions.network data is interpreted as a profile entry (with unknown keys ignored) and then fails here with a default_permissions error. That turns an existing network configuration into a startup failure instead of preserving compatibility or emitting a targeted migration error.

ℹ️ 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 pr13440 branch 8 times, most recently from 20a29b2 to 8a70b18 Compare March 7, 2026 00:25
Base automatically changed from pr13440 to main March 7, 2026 03:49
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