Skip to content

core: route view_image through a sandbox-backed fs helper#15213

Open
bolinfest wants to merge 1 commit intopr14989from
pr15213
Open

core: route view_image through a sandbox-backed fs helper#15213
bolinfest wants to merge 1 commit intopr14989from
pr15213

Conversation

@bolinfest
Copy link
Copy Markdown
Collaborator

@bolinfest bolinfest commented Mar 19, 2026 •
edited
Loading

Why

#14989 adds the sandbox-backed filesystem helper, but view_image still read local files through the host-side filesystem path. That meant the tool was not yet benefiting from the new boundary, even though reading the image bytes is the security-sensitive part of the operation.

This PR makes view_image the first consumer of that helper so image reads honor the same filesystem sandbox rules as other sandboxed child processes.

What changed

  • replaced the handler's direct metadata/read_file calls with sandboxed_fs::read_file()
  • kept image decoding and data-URL generation in view_image, but moved the actual file read behind the sandbox-backed helper
  • surfaced helper read failures as tool errors, so sandbox denials and missing-file cases are reported through the existing view_image error path
  • added focused integration coverage in core/tests/suite/view_image.rs that a view_image call outside the allowed filesystem sandbox does not attach an image and instead returns a read error

Testing

  • added view_image_tool_respects_filesystem_sandbox in core/tests/suite/view_image.rs

Stack created with Sapling. Best reviewed with ReviewStack.

@bolinfest bolinfest changed the base branch from main to pr14989 March 19, 2026 19:36
@bolinfest bolinfest mentioned this pull request Mar 19, 2026
Open
@chatgpt-codex-connector
Copy link
Copy Markdown
Contributor

chatgpt-codex-connector bot commented Mar 19, 2026

💡 Codex Review

codex/codex-rs/core/src/sandboxed_fs.rs

Lines 87 to 89 in 294e1da

let output = execute_env_bytes(exec_request, /*stdout_stream*/ None)
.await
.map_err(|error| map_exec_error(path, error))?;

P1 Badge Avoid truncating image bytes via exec output capture

When the selected image is larger than 1 MiB, this path now breaks view_image: sandboxed_fs::read_file fetches the file by spawning a helper and returning its stdout, but codex-rs/core/src/exec.rs hard-caps child output at DEFAULT_OUTPUT_BYTES_CAP in read_capped. That means the helper silently truncates any larger PNG/JPEG before load_for_prompt_bytes sees it, so many ordinary screenshots that previously worked will now decode as corrupt or fail to attach.

codex/codex-rs/fs-ops/src/runner.rs

Lines 39 to 40 in 294e1da

let mut file = std::fs::File::open(path).map_err(FsError::from)?;
std::io::copy(&mut file, stdout)

P2 Badge Reject non-regular files before streaming them

On Unix, if view_image is pointed at a FIFO, device node, or procfs entry, this helper now opens it and copies until EOF instead of rejecting it up front. The previous handler checked metadata and returned "not a file" immediately; with the new implementation, a workspace FIFO with no writer or something like /dev/zero will block the turn until the 30s helper timeout expires.

ℹ️ 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 mentioned this pull request Mar 19, 2026
Open
@bolinfest bolinfest force-pushed the pr14989 branch 8 times, most recently from 7bda055 to 9e9ec89 Compare March 19, 2026 23:07
@bolinfest bolinfest force-pushed the pr14989 branch 2 times, most recently from 81ed2df to 923f673 Compare March 20, 2026 03:17
@bolinfest bolinfest force-pushed the pr14989 branch 2 times, most recently from 98ea221 to c3f1765 Compare March 20, 2026 05:12
@bolinfest bolinfest force-pushed the pr14989 branch 2 times, most recently from 76ecd98 to b6af173 Compare March 20, 2026 06:05
@bolinfest bolinfest force-pushed the pr15213 branch 2 times, most recently from 512f7ed to 7aca0d8 Compare March 20, 2026 06:38
@bolinfest bolinfest requested a review from pakrym-oai March 20, 2026 06:57
This was referenced Mar 27, 2026
Draft
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pakrym-oai pakrym-oai Awaiting requested review from pakrym-oai

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@bolinfest