Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions pkg/tools/builtin/filesystem.go
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,10 @@ func (t *FilesystemTool) isPathAllowed(path string) error {
continue
}

if strings.HasPrefix(absPath, allowedAbs) {
// Check if path matches exactly or is a subdirectory (with separator check)
// This prevents sibling directories with similar names from bypassing the check
// (e.g., /project-secrets when /project is allowed)
if absPath == allowedAbs || strings.HasPrefix(absPath, allowedAbs+string(filepath.Separator)) {
return nil
}
}
Expand Down Expand Up @@ -504,7 +507,8 @@ func (t *FilesystemTool) handleAddAllowedDirectory(_ context.Context, args AddAl
return &tools.ToolCallResult{Output: fmt.Sprintf("Directory %s is already in allowed directories list", absPath)}, nil
}
// Check if the requested path is already covered by an existing allowed directory
if strings.HasPrefix(absPath, allowedAbs) {
// Use proper separator check to prevent sibling directory bypass
if absPath == allowedAbs || strings.HasPrefix(absPath, allowedAbs+string(filepath.Separator)) {
return &tools.ToolCallResult{Output: fmt.Sprintf("Directory %s is already accessible (covered by %s)", absPath, allowedAbs)}, nil
}
}
Expand Down
56 changes: 56 additions & 0 deletions pkg/tools/builtin/filesystem_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,62 @@ func TestFilesystemTool_IsPathAllowed(t *testing.T) {
assert.Contains(t, err.Error(), "not within allowed directories")
}

// TestFilesystemTool_IsPathAllowed_SiblingDirectories tests the fix for issue #1076
// It verifies that sibling directories with similar names don't bypass the allow-list
func TestFilesystemTool_IsPathAllowed_SiblingDirectories(t *testing.T) {
t.Parallel()

// Create a temporary directory structure:
// - tmpRoot/project (allowed)
// - tmpRoot/project-secrets (should NOT be allowed)
// - tmpRoot/project2 (should NOT be allowed)
// - tmpRoot/projectx (should NOT be allowed)
tmpRoot := t.TempDir()

projectDir := filepath.Join(tmpRoot, "project")
projectSecretsDir := filepath.Join(tmpRoot, "project-secrets")
project2Dir := filepath.Join(tmpRoot, "project2")
projectXDir := filepath.Join(tmpRoot, "projectx")

require.NoError(t, os.Mkdir(projectDir, 0o755))
require.NoError(t, os.Mkdir(projectSecretsDir, 0o755))
require.NoError(t, os.Mkdir(project2Dir, 0o755))
require.NoError(t, os.Mkdir(projectXDir, 0o755))

// Only allow the "project" directory
tool := NewFilesystemTool([]string{projectDir})

// Test that subdirectories of allowed directory are accessible
allowedSubdir := filepath.Join(projectDir, "src", "main.go")
err := tool.isPathAllowed(allowedSubdir)
require.NoError(t, err, "Subdirectories of allowed directory should be accessible")

// Test that the allowed directory itself is accessible
err = tool.isPathAllowed(projectDir)
require.NoError(t, err, "Allowed directory itself should be accessible")

// Test that sibling directories with similar names are NOT accessible
siblingTests := []struct {
name string
path string
}{
{"project-secrets", filepath.Join(projectSecretsDir, "confidential.txt")},
{"project2", filepath.Join(project2Dir, "file.txt")},
{"projectx", filepath.Join(projectXDir, "file.txt")},
{"project-secrets dir", projectSecretsDir},
{"project2 dir", project2Dir},
{"projectx dir", projectXDir},
}

for _, tc := range siblingTests {
t.Run(tc.name, func(t *testing.T) {
err := tool.isPathAllowed(tc.path)
require.Error(t, err, "Sibling directory %s should NOT be accessible", tc.name)
assert.Contains(t, err.Error(), "not within allowed directories")
})
}
}

func TestFilesystemTool_WriteFile(t *testing.T) {
t.Parallel()
tmpDir := t.TempDir()
Expand Down