fix: validate tool access against Docker bind mounts#19055
Closed
StressTestor wants to merge 4 commits intoopenclaw:mainfrom
Closed
fix: validate tool access against Docker bind mounts#19055StressTestor wants to merge 4 commits intoopenclaw:mainfrom
StressTestor wants to merge 4 commits intoopenclaw:mainfrom
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/agents/pi-tools.read.ts
Outdated
| if (opts?.additionalRoots?.length) { | ||
| const isAllowed = opts.additionalRoots.some((allowedRoot) => { | ||
| try { | ||
| resolveSandboxPath({ filePath, cwd: root, root: allowedRoot }); |
Contributor
There was a problem hiding this comment.
resolveSandboxPath doesn't check for symlink escapes (only assertSandboxPath does). This means bind mount paths aren't protected against symlink escape attacks.
Suggested change
| resolveSandboxPath({ filePath, cwd: root, root: allowedRoot }); | |
| await assertSandboxPath({ filePath, cwd: root, root: allowedRoot }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/pi-tools.read.ts
Line: 328:328
Comment:
`resolveSandboxPath` doesn't check for symlink escapes (only `assertSandboxPath` does). This means bind mount paths aren't protected against symlink escape attacks.
```suggestion
await assertSandboxPath({ filePath, cwd: root, root: allowedRoot });
```
How can I resolve this? If you propose a fix, please make it concise.wrapToolWorkspaceRootGuard was only checking paths against the primary workspace root, rejecting valid bind-mounted directories even when the operator explicitly configured them in docker.binds. Now the guard accepts an additionalRoots option populated from parsed bind mount host paths. Paths outside the workspace but inside a bind mount are allowed through.
StressTestor
force-pushed
the
fix/16379-sandbox-bound-folder-check
branch
from
e6e1822 to
6b2c97b
Compare
Member
|
Closing as duplicate of #16509. If this is incorrect, please contact us. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
wrapToolWorkspaceRootGuardonly checked the primary workspace root, blocking legitimate access to Docker bind-mounted directoriesadditionalRootsoption that accepts parsed bind mount host paths and validates them withassertSandboxPath(including symlink escape detection)pi-tools.tsto pass bind mount roots from sandbox configFixes #16379
Test plan
assertSandboxPath)pnpm test:fast,pnpm check)Greptile Summary
Extended
wrapToolWorkspaceRootGuardto accept bind mount host paths viaadditionalRootsoption, allowing workspace-only tools to access Docker bind-mounted directories.Key changes:
additionalRootsparameter towrapToolWorkspaceRootGuardto validate paths against multiple allowed rootspi-tools.tsusingparseSandboxBindMountCritical issue found:
pi-tools.read.tsusesresolveSandboxPathinstead ofassertSandboxPath, which skips symlink escape detection for bind mount paths. This creates a security vulnerability where symlinks in bind mounts could escape the sandbox.Confidence Score: 2/5
resolveSandboxPathinstead ofassertSandboxPath, which means bind mount paths skip symlink escape validation. This directly contradicts the PR's test plan claim that "symlink escape detection applies to additional roots". While the primary workspace root is protected, bind mount roots are vulnerable to symlink escape attacks.src/agents/pi-tools.read.tsline 328 - the symlink escape vulnerability must be fixedLast reviewed commit: e6e1822