refactor: reimplement filesystem tools with os.OpenRoot for enhanced security and simplified path validation.

pkg/tools/edit.go

@@ -3,6 +3,7 @@ package tools
 import (
 	"context"
 	"fmt"
+	"io"
 	"os"
 	"strings"
 )
@@ -67,38 +68,77 @@ func (t *EditFileTool) Execute(ctx context.Context, args map[string]interface{})
 		return ErrorResult("new_text is required")
 	}
 
-	resolvedPath, err := validatePath(path, t.allowedDir, t.restrict)
-	if err != nil {
-		return ErrorResult(err.Error())
+	// If not restricted, perform operations directly
+	if !t.restrict {
+		content, err := os.ReadFile(path)
+		if err != nil {
+			if os.IsNotExist(err) {
+				return ErrorResult(fmt.Sprintf("file not found: %s", path))
+			}
+			return ErrorResult(fmt.Sprintf("failed to read file: %v", err))
+		}
+
+		contentStr := string(content)
+		if !strings.Contains(contentStr, oldText) {
+			return ErrorResult("old_text not found in file. Make sure it matches exactly")
+		}
+
+		count := strings.Count(contentStr, oldText)
+		if count > 1 {
+			return ErrorResult(fmt.Sprintf("old_text appears %d times. Please provide more context to make it unique", count))
+		}
+
+		newContent := strings.Replace(contentStr, oldText, newText, 1)
+
+		if err := os.WriteFile(path, []byte(newContent), 0644); err != nil {
+			return ErrorResult(fmt.Sprintf("failed to write file: %v", err))
+		}
+
+		return SilentResult(fmt.Sprintf("File edited: %s", path))
 	}
 
-	if _, err := os.Stat(resolvedPath); os.IsNotExist(err) {
-		return ErrorResult(fmt.Sprintf("file not found: %s", path))
-	}
+	// Use executeInRoot to safely access the file
+	return executeInRoot(t.allowedDir, path, func(root *os.Root, relPath string) (*ToolResult, error) {
+		f, err := root.Open(relPath)
+		if err != nil {
+			if os.IsNotExist(err) {
+				return nil, fmt.Errorf("file not found: %s", path)
+			}
+			return nil, fmt.Errorf("failed to open file: %w", err)
+		}
 
-	content, err := os.ReadFile(resolvedPath)
-	if err != nil {
-		return ErrorResult(fmt.Sprintf("failed to read file: %v", err))
-	}
+		content, err := io.ReadAll(f)
+		f.Close()
 
-	contentStr := string(content)
+		if err != nil {
+			return nil, fmt.Errorf("failed to read file: %v", err)
+		}
 
-	if !strings.Contains(contentStr, oldText) {
-		return ErrorResult("old_text not found in file. Make sure it matches exactly")
-	}
+		contentStr := string(content)
 
-	count := strings.Count(contentStr, oldText)
-	if count > 1 {
-		return ErrorResult(fmt.Sprintf("old_text appears %d times. Please provide more context to make it unique", count))
-	}
+		if !strings.Contains(contentStr, oldText) {
+			return nil, fmt.Errorf("old_text not found in file. Make sure it matches exactly")
+		}
 
-	newContent := strings.Replace(contentStr, oldText, newText, 1)
+		count := strings.Count(contentStr, oldText)
+		if count > 1 {
+			return nil, fmt.Errorf("old_text appears %d times. Please provide more context to make it unique", count)
+		}
 
-	if err := os.WriteFile(resolvedPath, []byte(newContent), 0644); err != nil {
-		return ErrorResult(fmt.Sprintf("failed to write file: %v", err))
-	}
+		newContent := strings.Replace(contentStr, oldText, newText, 1)
 
-	return SilentResult(fmt.Sprintf("File edited: %s", path))
+		fw, err := root.Create(relPath)
+		if err != nil {
+			return nil, fmt.Errorf("failed to create file for writing: %w", err)
+		}
+		defer fw.Close()
+
+		if _, err := fw.Write([]byte(newContent)); err != nil {
+			return nil, fmt.Errorf("failed to write file: %v", err)
+		}
+
+		return SilentResult(fmt.Sprintf("File edited: %s", path)), nil
+	})
 }
 
 type AppendFileTool struct {
@@ -146,20 +186,33 @@ func (t *AppendFileTool) Execute(ctx context.Context, args map[string]interface{
 		return ErrorResult("content is required")
 	}
 
-	resolvedPath, err := validatePath(path, t.workspace, t.restrict)
-	if err != nil {
-		return ErrorResult(err.Error())
+	// If not restricted, perform operations directly
+	if !t.restrict {
+		f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644)
+		if err != nil {
+			return ErrorResult(fmt.Sprintf("failed to open file: %v", err))
+		}
+		defer f.Close()
+
+		if _, err := f.WriteString(content); err != nil {
+			return ErrorResult(fmt.Sprintf("failed to append to file: %v", err))
+		}
+
+		return SilentResult(fmt.Sprintf("Appended to %s", path))
 	}
 
-	f, err := os.OpenFile(resolvedPath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644)
-	if err != nil {
-		return ErrorResult(fmt.Sprintf("failed to open file: %v", err))
-	}
-	defer f.Close()
+	// Use executeInRoot to safely access the file
+	return executeInRoot(t.workspace, path, func(root *os.Root, relPath string) (*ToolResult, error) {
+		f, err := root.OpenFile(relPath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644)
+		if err != nil {
+			return nil, fmt.Errorf("failed to open file: %w", err)
+		}
+		defer f.Close()
 
-	if _, err := f.WriteString(content); err != nil {
-		return ErrorResult(fmt.Sprintf("failed to append to file: %v", err))
-	}
+		if _, err := f.WriteString(content); err != nil {
+			return nil, fmt.Errorf("failed to append to file: %w", err)
+		}
 
-	return SilentResult(fmt.Sprintf("Appended to %s", path))
+		return SilentResult(fmt.Sprintf("Appended to %s", path)), nil
+	})
 }

pkg/tools/edit_test.go

@@ -6,6 +6,8 @@ import (
 	"path/filepath"
 	"strings"
 	"testing"
+
+	"github.com/stretchr/testify/assert"
 )
 
 // TestEditTool_EditFile_Success verifies successful file editing
@@ -151,14 +153,13 @@ func TestEditTool_EditFile_OutsideAllowedDir(t *testing.T) {
 	result := tool.Execute(ctx, args)
 
 	// Should return error result
-	if !result.IsError {
-		t.Errorf("Expected error when path is outside allowed directory")
-	}
+	assert.True(t, result.IsError, "Expected error when path is outside allowed directory")
 
 	// Should mention outside allowed directory
-	if !strings.Contains(result.ForLLM, "outside") && !strings.Contains(result.ForUser, "outside") {
-		t.Errorf("Expected 'outside allowed' message, got ForLLM: %s", result.ForLLM)
-	}
+	// Note: ErrorResult only sets ForLLM by default, so ForUser might be empty.
+	// We check ForLLM as it's the primary error channel.
+	assert.True(t, strings.Contains(result.ForLLM, "outside") || strings.Contains(result.ForLLM, "access denied") || strings.Contains(result.ForLLM, "escapes"),
+		"Expected 'outside allowed' or 'access denied' message, got ForLLM: %s", result.ForLLM)
 }
 
 // TestEditTool_EditFile_MissingPath verifies error handling for missing path

pkg/tools/filesystem.go

@@ -3,78 +3,85 @@ package tools
 import (
 	"context"
 	"fmt"
+	"io"
 	"os"
 	"path/filepath"
 	"strings"
 )
 
-// validatePath ensures the given path is within the workspace if restrict is true.
-func validatePath(path, workspace string, restrict bool) (string, error) {
+// Helper to get a safe relative path for os.Root usage
+func getSafeRelPath(workspace, path string) (string, error) {
 	if workspace == "" {
-		return path, nil
+		return "", fmt.Errorf("workspace is not defined")
 	}
 
-	absWorkspace, err := filepath.Abs(workspace)
-	if err != nil {
-		return "", fmt.Errorf("failed to resolve workspace path: %w", err)
-	}
+	// Clean the path first
+	path = filepath.Clean(path)
 
-	var absPath string
+	// If absolute, make it relative to workspace
 	if filepath.IsAbs(path) {
-		absPath = filepath.Clean(path)
-	} else {
-		absPath, err = filepath.Abs(filepath.Join(absWorkspace, path))
+		rel, err := filepath.Rel(workspace, path)
 		if err != nil {
-			return "", fmt.Errorf("failed to resolve file path: %w", err)
+			return "", fmt.Errorf("failed to calculate relative path: %w", err)
 		}
+		path = rel
 	}
 
-	if restrict {
-		if !isWithinWorkspace(absPath, absWorkspace) {
-			return "", fmt.Errorf("access denied: path is outside the workspace")
-		}
-
-		workspaceReal := absWorkspace
-		if resolved, err := filepath.EvalSymlinks(absWorkspace); err == nil {
-			workspaceReal = resolved
-		}
-
-		if resolved, err := filepath.EvalSymlinks(absPath); err == nil {
-			if !isWithinWorkspace(resolved, workspaceReal) {
-				return "", fmt.Errorf("access denied: symlink resolves outside workspace")
-			}
-		} else if os.IsNotExist(err) {
-			if parentResolved, err := resolveExistingAncestor(filepath.Dir(absPath)); err == nil {
-				if !isWithinWorkspace(parentResolved, workspaceReal) {
-					return "", fmt.Errorf("access denied: symlink resolves outside workspace")
-				}
-			} else if !os.IsNotExist(err) {
-				return "", fmt.Errorf("failed to resolve path: %w", err)
-			}
-		} else {
-			return "", fmt.Errorf("failed to resolve path: %w", err)
-		}
+	// Check for escape
+	if path == ".." || strings.HasPrefix(path, "../") {
+		return "", fmt.Errorf("path escapes workspace: %s", path)
 	}
 
-	return absPath, nil
+	return path, nil
 }
 
-func resolveExistingAncestor(path string) (string, error) {
-	for current := filepath.Clean(path); ; current = filepath.Dir(current) {
-		if resolved, err := filepath.EvalSymlinks(current); err == nil {
-			return resolved, nil
-		} else if !os.IsNotExist(err) {
-			return "", err
-		}
-		if filepath.Dir(current) == current {
-			return "", os.ErrNotExist
-		}
+// executeInRoot executes a function within the safety of os.Root
+func executeInRoot(workspace string, path string, fn func(root *os.Root, relPath string) (*ToolResult, error)) *ToolResult {
+	if workspace == "" {
+		return ErrorResult("workspace is not defined")
 	}
+
+	// 1. Open the Root
+	root, err := os.OpenRoot(workspace)
+	if err != nil {
+		return ErrorResult(fmt.Sprintf("failed to open workspace root: %v", err))
+	}
+	defer root.Close()
+
+	// 2. Calculate relative path
+	relPath, err := getSafeRelPath(workspace, path)
+	if err != nil {
+		return ErrorResult(err.Error())
+	}
+
+	// 3. Execute the operation
+	result, err := fn(root, relPath)
+	if err != nil {
+		return ErrorResult(err.Error())
+	}
+
+	return result
 }
 
-func isWithinWorkspace(candidate, workspace string) bool {
-	rel, err := filepath.Rel(filepath.Clean(workspace), filepath.Clean(candidate))
-	return err == nil && rel != ".." && !strings.HasPrefix(rel, ".."+string(os.PathSeparator))
+// mkdirAllInRoot mimics os.MkdirAll but within os.Root
+func mkdirAllInRoot(root *os.Root, relPath string) error {
+	relPath = filepath.Clean(relPath)
+	if relPath == "." || relPath == "/" {
+		return nil
+	}
+
+	dir := filepath.Dir(relPath)
+	if dir != "." && dir != "/" {
+		if err := mkdirAllInRoot(root, dir); err != nil {
+			return err
+		}
+	}
+
+	err := root.Mkdir(relPath, 0755)
+	if err != nil && !os.IsExist(err) {
+		return err
+	}
+	return nil
 }
 
 type ReadFileTool struct {
@@ -113,17 +120,31 @@ func (t *ReadFileTool) Execute(ctx context.Context, args map[string]interface{})
 		return ErrorResult("path is required")
 	}
 
-	resolvedPath, err := validatePath(path, t.workspace, t.restrict)
-	if err != nil {
-		return ErrorResult(err.Error())
+	// If restriction is disabled, fall back to standard os interactions (insecure but intended)
+	if !t.restrict {
+		content, err := os.ReadFile(path)
+		if err != nil {
+			return ErrorResult(fmt.Sprintf("failed to read file: %v", err))
+		}
+		return NewToolResult(string(content))
 	}
 
-	content, err := os.ReadFile(resolvedPath)
-	if err != nil {
-		return ErrorResult(fmt.Sprintf("failed to read file: %v", err))
-	}
+	return executeInRoot(t.workspace, path, func(root *os.Root, relPath string) (*ToolResult, error) {
+		f, err := root.Open(relPath)
+		if err != nil {
+			if os.IsNotExist(err) {
+				return nil, fmt.Errorf("failed to read file:file not found: %s", path)
+			}
+			return nil, fmt.Errorf("access denied or failed to open: %w", err)
+		}
+		defer f.Close()
 
-	return NewToolResult(string(content))
+		content, err := io.ReadAll(f)
+		if err != nil {
+			return nil, fmt.Errorf("failed to read file: %v", err)
+		}
+		return NewToolResult(string(content)), nil
+	})
 }
 
 type WriteFileTool struct {
@@ -171,21 +192,37 @@ func (t *WriteFileTool) Execute(ctx context.Context, args map[string]interface{}
 		return ErrorResult("content is required")
 	}
 
-	resolvedPath, err := validatePath(path, t.workspace, t.restrict)
-	if err != nil {
-		return ErrorResult(err.Error())
+	if !t.restrict {
+		if err := os.MkdirAll(filepath.Dir(path), 0755); err != nil {
+			return ErrorResult(fmt.Sprintf("failed to create directory: %v", err))
+		}
+		if err := os.WriteFile(path, []byte(content), 0644); err != nil {
+			return ErrorResult(fmt.Sprintf("failed to write file: %v", err))
+		}
+		return SilentResult(fmt.Sprintf("File written: %s", path))
 	}
 
-	dir := filepath.Dir(resolvedPath)
-	if err := os.MkdirAll(dir, 0755); err != nil {
-		return ErrorResult(fmt.Sprintf("failed to create directory: %v", err))
-	}
+	return executeInRoot(t.workspace, path, func(root *os.Root, relPath string) (*ToolResult, error) {
+		// Ensure parent directory exists within root using recursive creation
+		dir := filepath.Dir(relPath)
+		if dir != "." && dir != "/" {
+			if err := mkdirAllInRoot(root, dir); err != nil {
+				return nil, fmt.Errorf("failed to create parent directories: %w", err)
+			}
+		}
 
-	if err := os.WriteFile(resolvedPath, []byte(content), 0644); err != nil {
-		return ErrorResult(fmt.Sprintf("failed to write file: %v", err))
-	}
+		f, err := root.Create(relPath)
+		if err != nil {
+			return nil, fmt.Errorf("failed to create file: %w", err)
+		}
+		defer f.Close()
 
-	return SilentResult(fmt.Sprintf("File written: %s", path))
+		_, err = f.WriteString(content)
+		if err != nil {
+			return nil, fmt.Errorf("failed to write file: %w", err)
+		}
+		return &ToolResult{Silent: true}, nil
+	})
 }
 
 type ListDirTool struct {
@@ -224,16 +261,39 @@ func (t *ListDirTool) Execute(ctx context.Context, args map[string]interface{})
 		path = "."
 	}
 
-	resolvedPath, err := validatePath(path, t.workspace, t.restrict)
-	if err != nil {
-		return ErrorResult(err.Error())
+	if !t.restrict {
+		entries, err := os.ReadDir(path)
+		if err != nil {
+			return ErrorResult(fmt.Sprintf("failed to read directory: %v", err))
+		}
+		return formatDirEntries(entries)
 	}
 
-	entries, err := os.ReadDir(resolvedPath)
-	if err != nil {
-		return ErrorResult(fmt.Sprintf("failed to read directory: %v", err))
-	}
+	return executeInRoot(t.workspace, path, func(root *os.Root, relPath string) (*ToolResult, error) {
+		f, err := root.Open(relPath)
+		if err != nil {
+			return nil, fmt.Errorf("failed to open directory: %w", err)
+		}
+		defer f.Close()
 
+		entries, err := f.ReadDir(-1)
+		if err != nil {
+			return nil, fmt.Errorf("failed to read directory: %w", err)
+		}
+
+		var result strings.Builder
+		for _, entry := range entries {
+			if entry.IsDir() {
+				result.WriteString("DIR:  " + entry.Name() + "\n")
+			} else {
+				result.WriteString("FILE: " + entry.Name() + "\n")
+			}
+		}
+		return NewToolResult(result.String()), nil
+	})
+}
+
+func formatDirEntries(entries []os.DirEntry) *ToolResult {
 	result := ""
 	for _, entry := range entries {
 		if entry.IsDir() {
@@ -242,6 +302,5 @@ func (t *ListDirTool) Execute(ctx context.Context, args map[string]interface{})
 			result += "FILE: " + entry.Name() + "\n"
 		}
 	}
-
 	return NewToolResult(result)
 }

pkg/tools/filesystem_test.go

@@ -6,6 +6,8 @@ import (
 	"path/filepath"
 	"strings"
 	"testing"
+
+	"github.com/stretchr/testify/assert"
 )
 
 // TestFilesystemTool_ReadFile_Success verifies successful file reading
@@ -275,7 +277,29 @@ func TestFilesystemTool_ReadFile_RejectsSymlinkEscape(t *testing.T) {
 	if !result.IsError {
 		t.Fatalf("expected symlink escape to be blocked")
 	}
-	if !strings.Contains(result.ForLLM, "symlink resolves outside workspace") {
+	// os.Root might return different errors depending on platform/implementation
+	// but it definitely should error.
+	// Our wrapper returns "access denied or file not found"
+	if !strings.Contains(result.ForLLM, "access denied") && !strings.Contains(result.ForLLM, "file not found") && !strings.Contains(result.ForLLM, "no such file") {
 		t.Fatalf("expected symlink escape error, got: %s", result.ForLLM)
 	}
 }
+
+func TestFilesystemTool_EmptyWorkspace_AccessDenied(t *testing.T) {
+	tool := NewReadFileTool("", true) // restrict=true but workspace=""
+
+	// Try to read a sensitive file (simulated by a temp file outside workspace)
+	tmpDir := t.TempDir()
+	secretFile := filepath.Join(tmpDir, "shadow")
+	os.WriteFile(secretFile, []byte("secret data"), 0600)
+
+	result := tool.Execute(context.Background(), map[string]any{
+		"path": secretFile,
+	})
+
+	// We EXPECT IsError=true (access blocked due to empty workspace)
+	assert.True(t, result.IsError, "Security Regression: Empty workspace allowed access! content: %s", result.ForLLM)
+
+	// Verify it failed for the right reason
+	assert.Contains(t, result.ForLLM, "workspace is not defined", "Expected 'workspace is not defined' error")
+}