fix(sandbox): reject apply_patch create_file on an existing file - #4893
fix(sandbox): reject apply_patch create_file on an existing file#4893ayaangazali wants to merge 7 commits into
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: f36d84ba6b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
The data-loss case is worth fixing, and the expected-error tracing change is correct. The remaining problem is that the absence check and write are separate operations. A concurrent sandbox command can create the file after the read, and the unconditional write then destroys that content. Reading also does not establish that a dangling symlink entry is absent. Please enforce create-if-absent at the backend write boundary, preserving the bound user and path policy, and cover an intervening creator whose contents must survive. Existing files should continue to use update_file.
|
The two Root cause: the span assertion test I added needs The unix-only imports stay inside the test body on purpose. At module scope the Nothing in One thing I cannot confirm from my side: the workflow run for d68c465 is sitting at |
Add File is documented to the model as creating a new file, and the delete and update operations both enforce their existing-file precondition. create_file enforced nothing, so an Add File operation aimed at a path that already existed overwrote it and reported "Created <path>", losing the previous contents with no error. Check that the destination is absent before writing, mirroring the existing _ensure_exists precondition used by delete_file.
…pans The absence check reached SandboxSession.read(), which records a failed sandbox.read child span when the file is missing. Missing is the success case for a create, so every successful Add File looked like it contained a failed sandbox operation. Use the existing _read_with_expected_span_errors helper, the same path the skills capability already uses for an existence probe.
The span assertion needs FilesystemTestSandboxSession, which is typed to UnixLocalSandboxSessionState, and importing agents.sandbox.sandboxes.unix_local raises ImportError on Windows by design. tests/conftest.py already collect-ignores the other files that depend on it, but test_apply_patch.py must stay collectible because its remaining tests are platform independent. The unix-only imports stay inside the test body so collection does not touch unix_local on Windows.
The previous read-then-write check left a window: a concurrent sandbox command could create the file after the probe, and the unconditional write then destroyed that content. Reading also did not establish that a dangling symlink entry was absent, because the unix_local path policy resolves symlinks and the write landed on the link target. Add BaseSandboxSession.write_new_file(), which claims the target name before the payload is written and raises FileExistsError when the name is already taken. The shared implementation uses a shell noclobber redirection, so the redirect itself is the O_EXCL attempt; UnixLocal overrides it with os.open(O_CREAT|O_EXCL) for its direct path. Both validate the parent through the normal policy, preserving grants and the bound user, and leave the final component unresolved so a symlink at that name is rejected rather than followed. apply_patch create_file now uses it and drops the probe, so the tracing workaround for the probe read is no longer needed. Existing files still have to go through update_file.
d68c465 to
6ecc682
Compare
|
Done in 6ecc682. You were right that the probe was not good enough, and the dangling symlink half turned out to be sharper than I gave it credit for. Where the enforcement moved. New Why a method rather than a flag on The symlink case was worse than a missing check. Both paths now validate the parent through the normal policy, so grants, symlinked parents and the bound user are unchanged, and leave the final component unresolved. Coverage. In I checked the shell recipe directly rather than assuming it: Existing files still have to go through |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ecc68255e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "stderr": result.stderr.decode("utf-8", errors="replace"), | ||
| }, | ||
| ) | ||
| await self.write(workspace_path, data, user=user) |
There was a problem hiding this comment.
When a session comes from SandboxClient, it is always wrapped in SandboxSession, which does not forward write_new_file; consequently even Unix-local sessions use this base implementation rather than the atomic os.open override. The shell command first creates an empty placeholder, then this separate awaited backend write overwrites it, so a concurrent exec_command that writes or replaces the path during an upload is overwritten, while cancellation or upload failure leaves an empty file that blocks retries. Fresh evidence relative to the earlier atomicity comment is that this commit's new specialized override is bypassed by the normal wrapper path; make the payload part of the exclusive operation or delegate through the wrapper to an equivalent backend primitive.
AGENTS.md reference: AGENTS.md:L104-L104
Useful? React with 👍 / 👎.
| _EXCLUSIVE_CREATE_SCRIPT = ( | ||
| 'target="$1"\nmkdir -p "$(dirname "$target")" || exit 12\nset -C\n: > "$target" || exit 13\n' | ||
| ) |
There was a problem hiding this comment.
On /bin/sh implementations such as dash, : is a POSIX special builtin, so failure to open its redirected target terminates the shell before || exit 13 executes. With an existing file this script returns 2 rather than _EXCLUSIVE_CREATE_EXISTS_CODE; the base path therefore raises WorkspaceArchiveWriteError, and WorkspaceEditor cannot convert the expected collision into its actionable ApplyPatchDiffError. Wrap the redirection in a subshell or use a non-special command whose failure reaches the explicit exit mapping.
Useful? React with 👍 / 👎.
| parent_path.mkdir(parents=True, exist_ok=True) | ||
| descriptor = os.open(workspace_path, flags, 0o644) |
There was a problem hiding this comment.
For an unwrapped UnixLocalSandboxSession, this changes the creation mode from the previous Path.open("wb") behavior (0666 & ~umask) to 0644 & ~umask. With a collaborative umask such as 0002, Add File therefore creates 0644 instead of 0664, so a later operation running as another member of the workspace group can no longer update the file. Pass 0o666 to os.open and let the configured umask determine the final permissions, matching the existing write contract.
Useful? React with 👍 / 👎.
Three problems with the previous commit.
SandboxSession did not forward write_new_file, so a session built by
SandboxClient fell back to the shared implementation and never reached
the UnixLocal os.open override. Forward it.
The shared implementation created an empty file and then wrote the
payload in a separate step, so a concurrent writer could be overwritten
and a failed upload left an empty file holding the name. Write the
payload under a staging name first, then claim the target with ln, which
fails when the name is taken. The content is complete before the name
exists, and a failed create leaves only the staging entry, which is
removed.
The script used ':' for the noclobber redirection. ':' is a POSIX special
builtin, so on dash a redirection failure ended the shell before the exit
mapping ran and a collision surfaced as a generic write error instead of
FileExistsError. ln is a regular command, and a test now runs the script
through sh, dash and bash so this cannot regress silently.
Also create local files with 0o666 so the process umask decides the final
mode, matching Path.open("wb") on the ordinary write path.
|
All three are fixed in 72ede53, and two of them were real. The wrapper bypass was the important one. The split write is gone. Instead of creating an empty file and then writing the payload, the payload goes to a staging name first and The dash point was correct, and I had actually mis-tested it. I validated the recipe with So a collision on dash surfaced as The umask point is right too. Full stack is clean locally. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72ede535aa
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| 'target="$1"\n' | ||
| 'source="$2"\n' | ||
| 'mkdir -p "$(dirname "$target")" || exit 12\n' | ||
| 'ln "$source" "$target" 2>/dev/null && exit 0\n' |
There was a problem hiding this comment.
When Add File targets an existing directory, or a symlink to one, on any session using this base implementation, ln treats its second operand as a target directory and creates the staging file's basename inside it. The command therefore exits successfully, apply_patch reports the directory as created, and a hidden payload file remains inside the pre-existing directory. ln --help identifies -T, --no-target-directory as the option that treats LINK_NAME as a normal file; use an equivalent supported primitive so the existing directory is classified as a collision.
AGENTS.md reference: AGENTS.md:L102-L103
Useful? React with 👍 / 👎.
| cause=exc, | ||
| ) from exc | ||
| await self._write_text(destination, created_text) | ||
| await self._write_new_text(destination, created_text, display_path=display_path) |
There was a problem hiding this comment.
On UnixLocalSandboxSession, this call receives the already-normalized destination from line 74, and that backend's normalize_path() resolves leaf symlinks. Thus Add File on a dangling link.txt -> missing.txt passes missing.txt to write_new_file, creates the missing target, and reports success even though the requested name already existed. Fresh evidence relative to the earlier dangling-symlink comment is that the new primitive preserves the leaf only after WorkspaceEditor has already resolved it; the added symlink test calls write_new_file directly and bypasses this caller path.
AGENTS.md reference: AGENTS.md:L102-L103
Useful? React with 👍 / 👎.
| with os.fdopen(descriptor, "wb") as f: | ||
| shutil.copyfileobj(payload.stream, f) | ||
| except OSError as e: | ||
| raise WorkspaceArchiveWriteError(path=workspace_path, cause=e) from e |
There was a problem hiding this comment.
If the local filesystem write fails after os.open() has exclusively created the target—for example, an OSError from a full filesystem—this block wraps the error but leaves the partial target in place. Add File consequently fails while permanently claiming the name, and a retry is rejected with FileExistsError. Fresh evidence after the shared implementation switched to staging is that this Unix-local override still claims the final name before copying the payload; stage the completed payload before linking it into place, or clean up using an ownership-safe inode check.
AGENTS.md reference: AGENTS.md:L104-L105
Useful? React with 👍 / 👎.
| staging_path = parent_path / f".{requested.name}.create-{uuid.uuid4().hex}" | ||
| staging_arg = sandbox_path_str(staging_path) | ||
|
|
||
| await self.write(staging_path, data, user=user) |
There was a problem hiding this comment.
When write() fails or is cancelled after creating its destination, cleanup never runs because the try/finally begins only after this await. This is reachable on remote implementations that stream into the requested staging path, such as Modal's cat > file write path, and leaves .name.create-<uuid> payloads in the user workspace and subsequent persisted snapshots. Move the initial staging write inside the cleanup scope so failure cannot leak the implementation-detail file.
AGENTS.md reference: AGENTS.md:L105-L105
Useful? React with 👍 / 👎.
| result = await self.exec( | ||
| "sh", | ||
| "-lc", | ||
| _EXCLUSIVE_CREATE_SCRIPT, | ||
| "sh", | ||
| path_arg, | ||
| staging_arg, | ||
| shell=False, | ||
| user=user, |
There was a problem hiding this comment.
When SandboxAgent.run_as creates a nested file through a backend whose write() only permission-probes the requested user and then uploads as the provider identity, such as Runloop or Modal, the staging write creates a missing parent as that provider/default user, but this later ln runs as run_as. With an ordinary 0755 parent, the bound user cannot create the target entry, so Add File on a fresh newdir/file.txt fails even though the previous _write_text path created the parent with user=self._user. Create the parent under the bound user before staging, or keep the entire exclusive-create operation within one user identity.
AGENTS.md reference: AGENTS.md:L102-L103
Useful? React with 👍 / 👎.
| 'target="$1"\n' | ||
| 'source="$2"\n' | ||
| 'mkdir -p "$(dirname "$target")" || exit 12\n' | ||
| 'ln "$source" "$target" 2>/dev/null && exit 0\n' |
There was a problem hiding this comment.
When the destination is under a supported writable rclone, Mountpoint, or Blobfuse object-storage mount, the staging upload can succeed but this ln cannot create the target because those object-backed filesystems do not provide POSIX hard-link semantics. The script then exits 14, cleanup removes the staged payload, and Add File on a fresh mounted path fails with WorkspaceArchiveWriteError, whereas the previous direct write() path supported it. Use a backend-specific exclusive-create primitive or another no-clobber flow that does not require hard links.
AGENTS.md reference: AGENTS.md:L102-L103
Useful? React with 👍 / 👎.
WorkspaceEditor normalizes the destination before dispatching, and UnixLocal resolves leaf symlinks, so create_file handed the primitive the link target. Add File on a dangling link.txt created missing.txt and reported success. Pass the unresolved path for create. Stage the payload locally too, then os.link it into place. os.link fails with EEXIST for a file, a directory or a dangling symlink, and a write that fails partway now leaves only the staging entry instead of a file holding the name. Reject a name held by a directory in the shared script. Bare ln treats an existing directory as a target directory and would have linked the staging file inside it while reporting the directory as created. Move the staging write inside the cleanup scope so a failed upload cannot leak the staging entry, and create the parent as the bound user so a fresh nested path is owned the way the previous write path owned it. The new tests drive session.apply_patch() rather than the primitive, which is the path that was actually broken.
|
Fixed in 3356606. The leaf-resolution one was the real defect and it was live through the actual tool path, not just theoretical. Preserving the leaf name. The create path now passes the unresolved path. My earlier symlink test called The directory case. Bare Local staging. The Unix-local override claimed the name with Staging cleanup and parent ownership. The staging write moved inside the cleanup scope so a failed or cancelled upload cannot leak The Full stack clean locally. I am conscious this diff has grown well past where it started. If you would rather have the primitive landed on its own, or scoped to UnixLocal only with the remote backends following separately, say the word and I will split it. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3356606ee8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| except FileExistsError: | ||
| raise |
There was a problem hiding this comment.
When a local Add File uses a nested path whose parent is a regular file, such as parent/child.txt, parent_path.mkdir(parents=True, exist_ok=True) raises FileExistsError. This broad handler propagates that exception to WorkspaceEditor, which incorrectly reports that parent/child.txt already exists and tells the model to use update_file, although the target is absent and an update cannot succeed. The previous write path wrapped this parent failure as WorkspaceArchiveWriteError; restrict the unwrapped collision handling to the os.link call and normalize parent or staging failures normally.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: 3356606ee8
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
| result = await self.exec( | ||
| "sh", | ||
| "-lc", | ||
| _EXCLUSIVE_CREATE_SCRIPT, |
There was a problem hiding this comment.
When an application exposes Filesystem() without Shell(), on a backend inheriting this method whose $HOME is workspace-writable (Runloop defaults the workspace to its effective home), an attacker-steered patch can Add a seed, Update/Move it to .profile, then Add another file. This new sh -lc loads the profile before _EXCLUSIVE_CREATE_SCRIPT, turning patch-only authority into arbitrary command execution as the sandbox user and exposing whatever environment secrets or network access that identity has. needs_approval defaults false. Use a non-login, sanitized primitive that cannot load workspace startup files.
Dismiss this finding: Reply with @codex security dismiss <reason> [context]. Codex will resolve this conversation automatically; GitHub may require a page refresh to show the result.
Valid reasons: false-positive, duplicate, out-of-scope, compensating-control, risk-accepted, or other. Example: @codex security dismiss duplicate Already flagged by another review
What each reason means
false-positive— Not a vulnerabilityduplicate— Already tracked elsewhereout-of-scope— Outside this review's scopecompensating-control— Mitigated by another controlrisk-accepted— Risk intentionally acceptedother— Another reason; context required
Useful? React with 👍 / 👎.
Use sh -c instead of sh -lc for the exclusive create. This path runs for a filesystem-only capability set, so it must not source shell startup files that live in the workspace it is editing. A parent that is a regular file makes mkdir raise FileExistsError, and the broad handler reported that as a collision on the requested name, telling the model to use update_file for a target that does not exist. Only the os.link call can report a collision now; parent and staging failures are wrapped as write errors.
|
Two of these were live and are fixed in 4ebc93c. Three are reposts of findings already addressed in 3356606. One is a blocker I do not think I should decide alone. Fixed: the login shell. I had used Fixed: collision classification. A parent that is a regular file makes Already addressed in the previous commit: the The blocker: hard links. The object-storage mount point is correct and it breaks the shared implementation rather than needing another condition. A writable rclone, Mountpoint or Blobfuse destination does not provide POSIX hard links, so I do not think I can fix that generically. So the honest position is that the shared shell implementation should go, and enforcement should be backend-native. UnixLocal has one that works. The other seven backends need their own, and I cannot write or meaningfully test those. How would you like to proceed?
I am happy either way, and I would rather close it than keep adding conditions to a shape that cannot hold. Four review rounds is past the point where I should be choosing the scope myself. |
Summary
*** Add File:is described to the model as creating a new file:The other two operations enforce the precondition that sentence implies.
delete_filecalls_ensure_exists, andupdate_filefails through_read_textwithApplyPatchFileNotFoundError.create_fileenforced nothing. It went straight to_write_text, which writes unconditionally.So an Add File aimed at a path that already exists silently destroys that file. The grammar cannot prevent it, since
filenameis/(.+)/and no CFG can express a filesystem precondition, and a model re-creating a file it wrote in an earlier turn is an ordinary thing to see.What made me want to fix it rather than leave it is the reported output. Driving a real patch through the tool boundary on current
main:The model is told a new file was created. Nothing raises, nothing warns, and the previous contents are unrecoverable, so neither the model nor the caller has any signal that would prompt recovery.
The fix checks that the destination is absent before writing, mirroring the
_ensure_existsprecondition already used bydelete_file. It reusesApplyPatchDiffError, which is the general apply_patch validation error in this module already (it covers unknown operation types and unsupported patch formats, not only diff content), so no new error class,ErrorCode, or exported symbol is added. The message names the operation that does work on an existing file:This is independent of #4890, which fixes the case-only rename in the
update_filebranch. Different operation, no textual overlap.One existing test changed.
test_editor_runs_file_operations_as_bound_userasserts that every file operation runs as the bound user by recording the user of each call. The new absence probe is a thirdread, and it correctly runs as the bound user, so the expected list goes from two entries to three. The property under test is unchanged.No
docs/change: the model-facing description already says Add File creates a new file, and this makes the runtime agree with it.Test plan
tests/sandbox/test_apply_patch.py::test_apply_patch_create_rejects_an_existing_fileasserts both halves of the outcome: the operation raises, and the original bytes are still there afterwards. Reverting onlysrc/agents/sandbox/apply_patch.pyand rerunning givesFailed: DID NOT RAISE <class 'agents.sandbox.errors.ApplyPatchDiffError'>, so it fails on the unfixed code..agents/skills/code-change-verification/scripts/run.shpasses: format clean, lint clean, typecheck clean, tests pass.tests/sandbox/is 1448 passed, 2 skipped.Issue number
None. Found while reading the apply_patch precondition handling.
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PRI should be upfront that I work on these with heavy AI assistance. I read the code, reproduced the behavior and checked the reasoning myself before sending it, but if I have misread the intent here I would rather you close it than spend review time on it.