fix(sandbox): allow write mkdir step on existing workspace directories#28853
fix(sandbox): allow write mkdir step on existing workspace directories#28853rylena wants to merge 1 commit intoopenclaw:mainfrom
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 452b32225b
ℹ️ 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".
| guarded.reason === "validation" && | ||
| options.allowExistingDirectoryTarget === true && | ||
| this.isExistingDirectoryPath(target.hostPath); |
There was a problem hiding this comment.
Keep symlink-escape validation for existing mkdirp directories
openBoundaryFile reports symlink-boundary violations as reason === "validation", but this new allowance treats any validation failure as safe when lstatSync(target.hostPath).isDirectory() is true. That means mkdirp now bypasses the host-side symlink escape guard for paths like workspace/link/existing when link is an intermediate symlink outside the mount and existing already exists, because lstatSync follows intermediate links and returns directory; this is a regression from the previous strict boundary behavior and can let escaped paths proceed to container-side execution.
Useful? React with 👍 / 👎.
Greptile SummaryFixed sandbox regression where The fix adds a targeted exemption in
The implementation uses Confidence Score: 5/5
Last reviewed commit: 452b322 |
nikolasdehor
left a comment
There was a problem hiding this comment.
Duplicate of #28863, which has a cleaner implementation. If this one is preferred, the test should use a relative filePath and add try/finally cleanup. The lstatSync check here is more defensive but introduces TOCTOU and the nested control flow is harder to follow.
|
This pull request has been automatically marked as stale due to inactivity. |
Summary
Fixes a sandbox regression where
writefailed on existing in-workspace paths because the internal mkdir boundary guard treated existing directories as invalid non-file targets.Closes #28745
Root cause
SandboxFsBridge.assertPathSafety()usesopenBoundaryFile(), which validates files only. Duringmkdirp, when the target directory already existed, this returned avalidationfailure and was surfaced as a sandbox boundary error even though the path was inside an allowed writable mount.Changes
create directoriessafety checks to accept an already-existing directory target.mkdirpon an existing in-workspace directory.Validation
pnpm vitest src/agents/sandbox/fs-bridge.test.tspnpm vitest src/agents/pi-tools.sandbox-mounted-paths.workspace-only.test.tsRisk
Low: behavior change is narrowly scoped to directory-creation safety checks and still enforces mount/boundary constraints.