feat(filesystem): add allow_list / deny_list to sandbox the toolset#2601
feat(filesystem): add allow_list / deny_list to sandbox the toolset#2601dgageot wants to merge 2 commits intodocker:mainfrom
Conversation
There was a problem hiding this comment.
Assessment: 🔴 CRITICAL
This PR introduces allow-list / deny-list path sandboxing for the filesystem toolset. The overall design is solid and well-tested, but two high-severity bypasses were found in the recursive operations (search_files_content and directory_tree) that allow the LLM to escape the sandbox via symlinks inside an allowed directory. These must be fixed before the sandboxing is trustworthy. Two medium-severity issues (fail-open on misconfiguration and *os.Root FD leak) are also flagged.
| Severity | Finding |
|---|---|
| 🔴 HIGH | search_files_content WalkDir skips per-file allow/deny check — symlink-to-file bypass |
| 🔴 HIGH | directory_tree passes allowAllPaths (no-op) — symlink-to-directory bypass |
| 🟡 MEDIUM | Allow-list construction failure silently fails open (unrestricted mode) |
| 🟡 MEDIUM | FilesystemTool leaks *os.Root FDs — no Close() method |
| filesWithMatches := make(map[string]struct{}) | ||
|
|
||
| err := filepath.WalkDir(resolvedPath, func(path string, d fs.DirEntry, err error) error { | ||
| err = filepath.WalkDir(resolvedPath, func(path string, d fs.DirEntry, err error) error { |
There was a problem hiding this comment.
[HIGH] search_files_content does not check per-file paths against the allow/deny lists during WalkDir
handleSearchFilesContent validates only the search root via resolveAndCheckPath, then calls filepath.WalkDir without any per-file allow/deny check:
resolvedPath, err := t.resolveAndCheckPath(args.Path) // root checked ✓
// ...
err = filepath.WalkDir(resolvedPath, func(path string, d fs.DirEntry, err error) error {
// ← no resolveAndCheckPath(path) here; every file is read regardlessfilepath.WalkDir uses os.Lstat for directory entries and does not follow symlinks to directories — but it does visit symlinks to regular files. If an allowed directory contains a symlink pointing to a file outside (or inside a denied subtree), WalkDir visits it and reads its content without any containment check on the resolved target.
Example: allow-list = ["/workspace"], /workspace/link is a symlink to /etc/passwd →
search_files_content({path: "."}) returns the contents of /etc/passwd.
Fix: call t.resolveAndCheckPath(path) (or a lightweight equivalent) inside the walk callback before reading each file.
| return tools.ResultError(err.Error()), nil | ||
| } | ||
|
|
||
| tree, err := fsx.DirectoryTree(ctx, resolvedPath, allowAllPaths, t.shouldIgnorePath, maxFiles) |
There was a problem hiding this comment.
[HIGH] directory_tree passes allowAllPaths (no-op) to fsx.DirectoryTree — symlinked subtrees outside the sandbox may be listed
handleDirectoryTree checks only the root path, then passes the allowAllPaths no-op to fsx.DirectoryTree:
resolvedPath, err := t.resolveAndCheckPath(args.Path) // root checked ✓
tree, err := fsx.DirectoryTree(ctx, resolvedPath, allowAllPaths, t.shouldIgnorePath, maxFiles)
// ^^^^^^^^^^^^^ never rejects any childInside fsx.DirectoryTree, each child is gated by isPathAllowed(childPath):
if err := isPathAllowed(childPath); err != nil {
continue // Skip disallowed paths
}Because allowAllPaths always returns nil, no child is skipped. fsx.DirectoryTree uses os.Stat (which follows symlinks) to decide whether a child is a directory and then recurses into it. A symlink inside the allowed root pointing to a directory outside it will be followed, listing all files under the forbidden location.
Example: allow-list = ["/workspace"], /workspace/secrets is a symlink to /etc →
directory_tree({path: "."}) lists all files under /etc.
Fix: wire a path-checker function that calls t.resolveAndCheckPath (or t.allowList/t.denyList directly) into the isPathAllowed argument instead of allowAllPaths.
| return func(t *FilesystemTool) { | ||
| set, err := newPathRootSet(t.workingDir, roots) | ||
| if err != nil { | ||
| slog.Warn("filesystem allow-list: invalid entry; ignoring allow-list", "error", err) |
There was a problem hiding this comment.
[MEDIUM] Allow-list construction failure silently fails open — sandbox is dropped instead of erroring
When newPathRootSet returns an error (e.g. an undefined $ENV_VAR in an allow-list entry), the error is logged as a warning and the entire allow-list is silently discarded:
func WithAllowList(roots []string) FileSystemOpt {
return func(t *FilesystemTool) {
set, err := newPathRootSet(t.workingDir, roots)
if err != nil {
slog.Warn("filesystem allow-list: invalid entry; ignoring allow-list", "error", err)
return // ← t.allowList stays nil → unrestricted mode
}
t.allowList = set
}
}A single misconfigured entry (typo in a variable name, path that resolves to an error) causes the sandbox to be silently removed rather than refusing to operate. An operator who thinks they've locked down the toolset may have no restriction at all.
The same pattern applies to WithDenyList.
Consider: returning the error from the option function so NewFilesystemTool can surface it to the caller, or at minimum erroring out the tool's operations entirely when list construction fails (fail-closed).
| // allowList, when non-nil, restricts every filesystem operation to paths | ||
| // that resolve under one of the listed roots. nil means "no allow-list"; | ||
| // the toolset accepts any path the OS will let it touch. | ||
| allowList *pathRootSet |
There was a problem hiding this comment.
[MEDIUM] FilesystemTool leaks *os.Root file descriptors — no Close() method
FilesystemTool holds two *pathRootSet fields that contain *os.Root handles (open file descriptors):
allowList *pathRootSet
denyList *pathRootSetpathRootSet.close() exists and correctly closes all handles, but it is never called. FilesystemTool does not implement io.Closer and there is no teardown path. The current interface compliance checks only cover tools.ToolSet and tools.Instructable.
Go does not set finalizers on *os.Root by default, so these file descriptors are only released when the GC collects the struct — if ever. In test suites that instantiate many FilesystemTool values (19+ new tests here), or in long-running agent processes that reload configuration, this will accumulate open FDs.
Fix: add a Close() error method to FilesystemTool that calls t.allowList.close() and t.denyList.close(), and register io.Closer compliance in the interface check block.
What
Adds an opt-in
allow_list/deny_listto thefilesystemtoolset.By default the toolset is unrestricted today: relative paths resolve from the working dir, but absolute paths and
..traversals can reach anywhere the agent process can. That is fine for local dev but a footgun when running an agent on someone else's machine.Behaviour is unchanged by default — both lists empty preserves the current "anything reachable" semantics. Set either or both to sandbox the toolset.
Tokens
.~/~/...$VAR/${VAR}Semantics
allow_listnon-empty → operations are rejected unless the resolved path is inside one of the listed roots.deny_listnon-empty → operations on paths inside any listed root are rejected.write_filecreating a new file) are checked against the nearest existing ancestor, so they're allowed when the parent is allowed.Bonus:
os.RootEach allow/deny entry opens an
*os.Root(Go 1.24+). The containment check usesRoot.Lstat(rel)so the kernel's rooted-lookup semantics also reject..and symlink escapes at I/O time, not just lexically. Falls back to a lexical prefix check whenos.OpenRootfails (root doesn't exist yet, restricted permissions, etc.).Why
Closes a real footgun: the filesystem toolset's own
Instructions()advertise that "absolute paths and..work as expected" — i.e. the LLM is encouraged to escape the working dir. Operators who run an agent on someone else's behalf had no way to prevent that short of running in a container or sandbox.Tests
.,~,~/sub, multiple roots, absolute paths, deny-only, allow+deny precedence, symlink escape rejection, symlink-into-deny rejection, new-file paths, empty list = no constraint, every handler honouring the allow-list, deny applies toedit_filewithout modifying file, instructions text, dedup,*os.Rootopened./var↔/private/var).Documentation
docs/tools/filesystem/index.md: added a "Path access control" section.agent-schema.json: schema entries with examples.examples/filesystem_allow_deny.yaml: three variants (strict allow+deny, workspace-only, deny-only).