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 |
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".
|
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".
|
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".
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 👍 / 👎.
|
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ebc93ca86
ℹ️ 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".
| parent_path = await self._validate_path_access(requested.parent, for_write=True) | ||
| workspace_path = parent_path / requested.name | ||
| path_arg = sandbox_path_str(workspace_path) | ||
| staging_path = parent_path / f".{requested.name}.create-{uuid.uuid4().hex}" |
There was a problem hiding this comment.
On a filesystem with the common 255-byte component limit, a valid destination leaf such as a 220-byte filename becomes a 261-byte staging name after adding the dot, .create-, and UUID. The staging write therefore fails with ENAMETOOLONG even though the requested file could be created directly, regressing Add File for valid paths in both this shared implementation and the Unix-local override. Use a fixed-size staging basename that does not embed the entire destination name.
Useful? React with 👍 / 👎.
| # Create the parent as the bound user so a fresh nested path is owned the same | ||
| # way the ordinary write path owned it. | ||
| await self.mkdir(parent_path, parents=True, user=user) | ||
| await self.write(staging_path, data, user=user) |
There was a problem hiding this comment.
When the target already exists inside an executable but non-writable parent directory, such as a workspace directory changed to mode 0555, this staging write fails with a permission error before the link script can classify the existing destination. Add File consequently surfaces WorkspaceArchiveWriteError rather than the promised ApplyPatchDiffError directing the caller to update_file; the Unix-local override has the same ordering. Short-circuit an already-observable leaf collision before staging while retaining the atomic link check for intervening creators.
Useful? React with 👍 / 👎.
|
Rumbo follow-up: PR #4930 hardens this implementation for two current P2 findings — fixed-length staging basenames and early observable-target collision classification, while retaining the atomic claim for races. #4930 is intentionally based on this PR rather than presenting the work as unrelated. |
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.
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.
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.
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.
4ebc93c to
9820c3f
Compare
|
Thanks for flagging #4930, and for basing it on this branch openly instead of presenting the work as unrelated. On the two findings: Fixed-length staging basename is real, and it is my regression. The staging name is Early collision classification I read as an improvement rather than a correctness bug. Today the payload is staged before the collision is detected, so Add File onto an existing name uploads bytes that are then discarded, which is wasteful on remote backends. That seems right to do, as long as the preflight stays additive and the atomic claim is still what decides races, which is what your description says. One practical note: #4930 shows as closed, opened 11:49:38Z and closed 11:54:17Z, so as things stand neither fix has a home. I would rather not absorb your work. If you reopen it or push a replacement, I will leave both to you and keep this branch as the base. If you would rather not carry it, say so and I will fold the staging-name fix in here with credit to you. For whoever picks this up: there is an open question further up this thread about whether the shared shell implementation should exist at all, because hard links are not available on writable object-storage mounts. Both of these findings apply to that shared path, so the answer to that one probably decides where they land. The branch is rebased onto current |
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.