Add sandboxDirectory support to FileSystemTools#46
Closed
kezhenxu94 wants to merge 2 commits into
Closed
Conversation
…ations When a sandboxDirectory is configured via the builder, all read, write, and edit operations are restricted to paths within that directory. Path traversal via '..' and symlink escapes (including dangling symlinks) are both blocked. Closes spring-ai-community#45
There was a problem hiding this comment.
Pull request overview
Adds an optional sandbox directory constraint to FileSystemTools so read, write, and edit are restricted to a configured directory (default remains unrestricted for backward compatibility), with accompanying tests.
Changes:
- Introduces
sandboxDirectoryconfiguration onFileSystemTools.Builder(String and Path overloads). - Adds
validateSandboxAccess(...)checks toread,write, andedit. - Adds a new nested test suite covering sandbox allow/deny behavior including traversal and symlink cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| spring-ai-agent-utils/src/main/java/org/springaicommunity/agent/tools/FileSystemTools.java | Adds sandbox directory support and enforces it in read/write/edit. |
| spring-ai-agent-utils/src/test/java/org/springaicommunity/agent/tools/FileSystemToolsTest.java | Adds unit tests validating sandbox restrictions, traversal blocking, and symlink escape blocking. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+55
to
+62
| private String validateSandboxAccess(String filePath) { | ||
| if (this.sandboxDirectory == null) { | ||
| return null; | ||
| } | ||
| try { | ||
| Path sandbox = this.sandboxDirectory.toAbsolutePath().normalize(); | ||
| Path target = Paths.get(filePath).toAbsolutePath().normalize(); | ||
|
|
Comment on lines
+68
to
+74
| // Check 2: resolve symlinks in all existing path components. | ||
| // Walk up from target using NOFOLLOW_LINKS so dangling symlinks are detected. | ||
| Path realSandbox = Files.exists(sandbox) ? sandbox.toRealPath() : sandbox; | ||
| Path existing = target; | ||
| while (existing != null && !Files.exists(existing, LinkOption.NOFOLLOW_LINKS)) { | ||
| existing = existing.getParent(); | ||
| } |
Comment on lines
+60
to
+66
| Path sandbox = this.sandboxDirectory.toAbsolutePath().normalize(); | ||
| Path target = Paths.get(filePath).toAbsolutePath().normalize(); | ||
|
|
||
| // Check 1: normalized path must be within sandbox (blocks .. traversal) | ||
| if (!target.startsWith(sandbox)) { | ||
| return "Error: Access denied. Path is outside the allowed sandbox directory: " + filePath; | ||
| } |
Comment on lines
+681
to
+685
| @Test | ||
| @DisplayName("Should deny symlink pointing outside sandbox") | ||
| @DisabledOnOs(OS.WINDOWS) | ||
| void shouldDenySymlinkPointingOutsideSandbox() throws IOException { | ||
| FileSystemTools sandboxedTools = FileSystemTools.builder().sandboxDirectory(sandboxDir).build(); |
- Reject '..' components in the raw absolute path to prevent symlink+'..' escapes (e.g. /sandbox/link/../outside normalizes to /sandbox/outside but the OS resolves 'link' as a symlink first, landing outside the sandbox) - Skip symlink real-path check when sandbox directory does not yet exist, so writes into a not-yet-created sandbox directory work correctly (checks 1+2 are sufficient) - Catch RuntimeException (e.g. InvalidPathException) in validateSandboxAccess so invalid paths return a consistent error string instead of escaping as exceptions - Add private Builder() constructor consistent with GlobTool/ListDirectoryTool convention - Add regression tests for symlink+'..' bypass and non-existent sandbox directory
Contributor
|
Thank you @kezhenxu94 , |
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
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
sandboxDirectoryoption toFileSystemTools.Builderthat restricts allread,write, andeditoperations to paths within a configured directory..is blocked by normalizing the absolute path before checkingUsage
Test plan
..for all three operationsPathandStringformsnullString sandbox = no restrictionCloses #45