diff --git a/pkg/tools/edit.go b/pkg/tools/edit.go index 1e7c33b45..1388690ee 100644 --- a/pkg/tools/edit.go +++ b/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 + }) } diff --git a/pkg/tools/edit_test.go b/pkg/tools/edit_test.go index c4c02772d..4855c3f83 100644 --- a/pkg/tools/edit_test.go +++ b/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 diff --git a/pkg/tools/filesystem.go b/pkg/tools/filesystem.go index 09063ea0a..e9f985c86 100644 --- a/pkg/tools/filesystem.go +++ b/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) } diff --git a/pkg/tools/filesystem_test.go b/pkg/tools/filesystem_test.go index 958036419..a17c3d587 100644 --- a/pkg/tools/filesystem_test.go +++ b/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") +}