Skip to content

fix: #3169 constrain local sandbox artifact sources to base dir#3177

Merged
seratch merged 3 commits intomainfrom
fix/local-artifact-base-dir-boundary
May 8, 2026
Merged

fix: #3169 constrain local sandbox artifact sources to base dir#3177
seratch merged 3 commits intomainfrom
fix/local-artifact-base-dir-boundary

Conversation

@seratch
Copy link
Copy Markdown
Member

@seratch seratch commented May 7, 2026

This pull request fixes #3169 local sandbox artifact source resolution so LocalFile and LocalDir cannot read host files outside the manifest base directory by default.

The implementation treats base_dir as the trust boundary rather than treating every absolute path as unsafe. During normal manifest application, base_dir is the SDK process working directory, so local artifact sources are allowed only when their normalized host path stays within that directory. This means absolute paths are still supported without extra configuration when they point inside the trusted base directory, while both absolute outside paths and relative escapes such as ../outside are rejected by default.

For trusted application code that intentionally needs to materialize a host path outside base_dir, this adds an explicit allow_outside_base_dir=True opt-in. That escape hatch keeps the host-path trust boundary visible at the call site without blocking legitimate cases such as temporary generated skill directories or integration fixtures. The same boundary is applied to lazy local skill metadata and skill loading so discovery and materialization follow the same rules.

The final behavior is:

  • LocalFile.src and LocalDir.src stay constrained to the SDK process base_dir by default.
  • A source outside base_dir is allowed only when its host path is under an explicit manifest.extra_path_grants entry.
  • read_only=True grants are accepted for local source materialization because this path only reads from the host source before copying into the sandbox.
  • Lazy local skill discovery and load_skill use the same grant boundary.
  • Symlink protections remain in place, including for lazy skill metadata discovery.

@seratch
Copy link
Copy Markdown
Member Author

seratch commented May 7, 2026

@codex review

Copy link
Copy Markdown

@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: 78a98ebf88

ℹ️ 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 thread src/agents/sandbox/entries/artifacts.py Outdated
Copy link
Copy Markdown

@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: 78a98ebf88

ℹ️ 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 thread src/agents/sandbox/entries/artifacts.py Outdated
seratch added a commit that referenced this pull request May 7, 2026
@seratch seratch force-pushed the fix/local-artifact-base-dir-boundary branch from 78a98eb to dca1c01 Compare May 7, 2026 11:15
@qiyaoq-oai
Copy link
Copy Markdown
Contributor

I wonder if we can leverage the extra_path_grants in the manifest? That gives a finer grained control rather than allowing arbitrary outside path access, wdyt?

@seratch seratch force-pushed the fix/local-artifact-base-dir-boundary branch from 8705e33 to d21d7b0 Compare May 8, 2026 02:49
@seratch
Copy link
Copy Markdown
Member Author

seratch commented May 8, 2026

After discussing this w/ @qiyaoq-oai, I updated the implementation to use manifest.extra_path_grants instead of adding a per-entry opt-in flag.

The final behavior is:

  • LocalFile.src and LocalDir.src stay constrained to the SDK process base_dir by default.
  • A source outside base_dir is allowed only when its host path is under an explicit manifest.extra_path_grants entry.
  • read_only=True grants are accepted for local source materialization because this path only reads from the host source before copying into the sandbox.
  • Lazy local skill discovery and load_skill use the same grant boundary.
  • Symlink protections remain in place, including for lazy skill metadata discovery.

This keeps the host-path trust decision at the manifest/application configuration boundary and avoids a broader per-artifact escape hatch. For this release, manifests containing extra_path_grants are treated as trusted configuration; applications that load manifests from untrusted user/model payloads should strip or validate grants before passing them to the SDK. If we need a first-class untrusted manifest loading mode later, we can add a separate validation path without changing this API shape.

I will clearly mention this change in release notes and changelog when shipping itt.

@seratch seratch merged commit f47d486 into main May 8, 2026
10 checks passed
@seratch seratch deleted the fix/local-artifact-base-dir-boundary branch May 8, 2026 06:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working feature:sandboxes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LocalFile and LocalDir absolute src paths can read outside base_dir

2 participants