From 7cf0dc5dfc93a4daeaa9b242a9968ad4d2853695 Mon Sep 17 00:00:00 2001 From: 0x5487 Date: Wed, 18 Feb 2026 10:25:14 +0800 Subject: [PATCH 01/10] chore: Update default host bindings from 0.0.0.0 to 127.0.0.1 for various services and examples. --- README.ja.md | 2 +- README.md | 2 +- README.pt-br.md | 2 +- config/config.example.json | 6 +++--- pkg/config/config.go | 6 +++--- pkg/config/config_test.go | 4 ++-- 6 files changed, 11 insertions(+), 11 deletions(-) diff --git a/README.ja.md b/README.ja.md index 0da84571a..9f96dce32 100644 --- a/README.ja.md +++ b/README.ja.md @@ -398,7 +398,7 @@ picoclaw gateway "enabled": true, "channel_secret": "YOUR_CHANNEL_SECRET", "channel_access_token": "YOUR_CHANNEL_ACCESS_TOKEN", - "webhook_host": "0.0.0.0", + "webhook_host": "127.0.0.1", "webhook_port": 18791, "webhook_path": "/webhook/line", "allow_from": [] diff --git a/README.md b/README.md index 59b9bea7c..ab8cecd99 100644 --- a/README.md +++ b/README.md @@ -432,7 +432,7 @@ picoclaw gateway "enabled": true, "channel_secret": "YOUR_CHANNEL_SECRET", "channel_access_token": "YOUR_CHANNEL_ACCESS_TOKEN", - "webhook_host": "0.0.0.0", + "webhook_host": "127.0.0.1", "webhook_port": 18791, "webhook_path": "/webhook/line", "allow_from": [] diff --git a/README.pt-br.md b/README.pt-br.md index d250cc956..0091e919b 100644 --- a/README.pt-br.md +++ b/README.pt-br.md @@ -438,7 +438,7 @@ picoclaw gateway "enabled": true, "channel_secret": "YOUR_CHANNEL_SECRET", "channel_access_token": "YOUR_CHANNEL_ACCESS_TOKEN", - "webhook_host": "0.0.0.0", + "webhook_host": "127.0.0.1", "webhook_port": 18791, "webhook_path": "/webhook/line", "allow_from": [] diff --git a/config/config.example.json b/config/config.example.json index 7cd0ab8c6..fdbce0887 100644 --- a/config/config.example.json +++ b/config/config.example.json @@ -25,7 +25,7 @@ }, "maixcam": { "enabled": false, - "host": "0.0.0.0", + "host": "127.0.0.1", "port": 18790, "allow_from": [] }, @@ -58,7 +58,7 @@ "enabled": false, "channel_secret": "YOUR_LINE_CHANNEL_SECRET", "channel_access_token": "YOUR_LINE_CHANNEL_ACCESS_TOKEN", - "webhook_host": "0.0.0.0", + "webhook_host": "127.0.0.1", "webhook_port": 18791, "webhook_path": "/webhook/line", "allow_from": [] @@ -141,7 +141,7 @@ "monitor_usb": true }, "gateway": { - "host": "0.0.0.0", + "host": "127.0.0.1", "port": 18790 } } \ No newline at end of file diff --git a/pkg/config/config.go b/pkg/config/config.go index 1d34f56f3..076625894 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -266,7 +266,7 @@ func DefaultConfig() *Config { }, MaixCam: MaixCamConfig{ Enabled: false, - Host: "0.0.0.0", + Host: "127.0.0.1", Port: 18790, AllowFrom: FlexibleStringSlice{}, }, @@ -292,7 +292,7 @@ func DefaultConfig() *Config { Enabled: false, ChannelSecret: "", ChannelAccessToken: "", - WebhookHost: "0.0.0.0", + WebhookHost: "127.0.0.1", WebhookPort: 18791, WebhookPath: "/webhook/line", AllowFrom: FlexibleStringSlice{}, @@ -319,7 +319,7 @@ func DefaultConfig() *Config { ShengSuanYun: ProviderConfig{}, }, Gateway: GatewayConfig{ - Host: "0.0.0.0", + Host: "127.0.0.1", Port: 18790, }, Tools: ToolsConfig{ diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index febfd0456..2dcc589c5 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -67,7 +67,7 @@ func TestDefaultConfig_Temperature(t *testing.T) { func TestDefaultConfig_Gateway(t *testing.T) { cfg := DefaultConfig() - if cfg.Gateway.Host != "0.0.0.0" { + if cfg.Gateway.Host != "127.0.0.1" { t.Error("Gateway host should have default value") } if cfg.Gateway.Port == 0 { @@ -194,7 +194,7 @@ func TestConfig_Complete(t *testing.T) { if cfg.Agents.Defaults.MaxToolIterations == 0 { t.Error("MaxToolIterations should not be zero") } - if cfg.Gateway.Host != "0.0.0.0" { + if cfg.Gateway.Host != "127.0.0.1" { t.Error("Gateway host should have default value") } if cfg.Gateway.Port == 0 { From b450faaf6810a14af52ad6da4d63e676073c5b1c Mon Sep 17 00:00:00 2001 From: 0x5487 Date: Thu, 19 Feb 2026 07:23:58 +0800 Subject: [PATCH 02/10] config: Update default host bindings to 0.0.0.0 for improved Docker accessibility and add related documentation. --- README.ja.md | 6 +++++- README.md | 6 +++++- README.pt-br.md | 6 +++++- README.zh.md | 6 +++++- config/config.example.json | 4 ++-- pkg/config/config.go | 4 ++-- 6 files changed, 24 insertions(+), 8 deletions(-) diff --git a/README.ja.md b/README.ja.md index 9bd52d203..70ba458e9 100644 --- a/README.ja.md +++ b/README.ja.md @@ -133,6 +133,10 @@ vim config/config.json # DISCORD_BOT_TOKEN, プロバイダーの API キ # 3. ビルドと起動 docker compose --profile gateway up -d +> [!TIP] +> **Docker ユーザー**: デフォルトでは、Gateway は `127.0.0.1` でリッスンしており、ホストからアクセスできません。ヘルスチェックエンドポイントにアクセスしたり、ポートを公開したりする必要がある場合は、環境変数で `PICOCLAW_GATEWAY_HOST=0.0.0.0` を設定するか、`config.json` を更新してください。 + + # 4. ログ確認 docker compose logs -f picoclaw-gateway @@ -398,7 +402,7 @@ picoclaw gateway "enabled": true, "channel_secret": "YOUR_CHANNEL_SECRET", "channel_access_token": "YOUR_CHANNEL_ACCESS_TOKEN", - "webhook_host": "127.0.0.1", + "webhook_host": "0.0.0.0", "webhook_port": 18791, "webhook_path": "/webhook/line", "allow_from": [] diff --git a/README.md b/README.md index 2b962b0cd..01c81b20b 100644 --- a/README.md +++ b/README.md @@ -165,6 +165,10 @@ vim config/config.json # Set DISCORD_BOT_TOKEN, API keys, etc. # 3. Build & Start docker compose --profile gateway up -d +> [!TIP] +> **Docker Users**: By default, the Gateway listens on `127.0.0.1` which is not accessible from the host. If you need to access the health endpoints or expose ports, set `PICOCLAW_GATEWAY_HOST=0.0.0.0` in your environment or update `config.json`. + + # 4. Check logs docker compose logs -f picoclaw-gateway @@ -432,7 +436,7 @@ picoclaw gateway "enabled": true, "channel_secret": "YOUR_CHANNEL_SECRET", "channel_access_token": "YOUR_CHANNEL_ACCESS_TOKEN", - "webhook_host": "127.0.0.1", + "webhook_host": "0.0.0.0", "webhook_port": 18791, "webhook_path": "/webhook/line", "allow_from": [] diff --git a/README.pt-br.md b/README.pt-br.md index 51e6baf7f..171a6557b 100644 --- a/README.pt-br.md +++ b/README.pt-br.md @@ -172,6 +172,10 @@ vim config/config.json # Configure DISCORD_BOT_TOKEN, API keys, etc. # 3. Build & Iniciar docker compose --profile gateway up -d +> [!TIP] +> **Usuários Docker**: Por padrão, o Gateway ouve em `127.0.0.1`, o que não é acessível a partir do host. Se você precisar acessar os endpoints de integridade ou expor portas, defina `PICOCLAW_GATEWAY_HOST=0.0.0.0` em seu ambiente ou atualize o `config.json`. + + # 4. Ver logs docker compose logs -f picoclaw-gateway @@ -439,7 +443,7 @@ picoclaw gateway "enabled": true, "channel_secret": "YOUR_CHANNEL_SECRET", "channel_access_token": "YOUR_CHANNEL_ACCESS_TOKEN", - "webhook_host": "127.0.0.1", + "webhook_host": "0.0.0.0", "webhook_port": 18791, "webhook_path": "/webhook/line", "allow_from": [] diff --git a/README.zh.md b/README.zh.md index 42bd20be4..434805f93 100644 --- a/README.zh.md +++ b/README.zh.md @@ -167,9 +167,13 @@ cd picoclaw cp config/config.example.json config/config.json vim config/config.json # 设置 DISCORD_BOT_TOKEN, API keys 等 -# 3. 构建并启动 +# 3. 構建並啟動 docker compose --profile gateway up -d +> [!TIP] +> **Docker 用戶**: 預設情況下，Gateway 監聽 `127.0.0.1`，這在容器內無法從外部訪問。如果您需要透過端口映射訪問健康檢查接口，請在環境變量中設置 `PICOCLAW_GATEWAY_HOST=0.0.0.0` 或修改 `config.json`。 + + # 4. 查看日志 docker compose logs -f picoclaw-gateway diff --git a/config/config.example.json b/config/config.example.json index 7943898ab..07f052a98 100644 --- a/config/config.example.json +++ b/config/config.example.json @@ -25,7 +25,7 @@ }, "maixcam": { "enabled": false, - "host": "127.0.0.1", + "host": "0.0.0.0", "port": 18790, "allow_from": [] }, @@ -58,7 +58,7 @@ "enabled": false, "channel_secret": "YOUR_LINE_CHANNEL_SECRET", "channel_access_token": "YOUR_LINE_CHANNEL_ACCESS_TOKEN", - "webhook_host": "127.0.0.1", + "webhook_host": "0.0.0.0", "webhook_port": 18791, "webhook_path": "/webhook/line", "allow_from": [] diff --git a/pkg/config/config.go b/pkg/config/config.go index 93f468cc9..0e68a053f 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -360,7 +360,7 @@ func DefaultConfig() *Config { }, MaixCam: MaixCamConfig{ Enabled: false, - Host: "127.0.0.1", + Host: "0.0.0.0", Port: 18790, AllowFrom: FlexibleStringSlice{}, }, @@ -386,7 +386,7 @@ func DefaultConfig() *Config { Enabled: false, ChannelSecret: "", ChannelAccessToken: "", - WebhookHost: "127.0.0.1", + WebhookHost: "0.0.0.0", WebhookPort: 18791, WebhookPath: "/webhook/line", AllowFrom: FlexibleStringSlice{}, From c7db77bb6f4665d0e45eeb609b6191b3aac684fd Mon Sep 17 00:00:00 2001 From: 0x5487 Date: Thu, 19 Feb 2026 13:31:43 +0800 Subject: [PATCH 03/10] refactor: reimplement filesystem tools with `os.OpenRoot` for enhanced security and simplified path validation. --- pkg/tools/edit.go | 123 ++++++++++++++------ pkg/tools/edit_test.go | 13 ++- pkg/tools/filesystem.go | 215 ++++++++++++++++++++++------------- pkg/tools/filesystem_test.go | 26 ++++- 4 files changed, 257 insertions(+), 120 deletions(-) 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") +} From 59f62f6405f24cfc4fcb343157a21690661ba048 Mon Sep 17 00:00:00 2001 From: 0x5487 Date: Thu, 19 Feb 2026 13:54:06 +0800 Subject: [PATCH 04/10] chore: revert other PR content from this branch --- README.ja.md | 4 ---- README.md | 4 ---- README.pt-br.md | 4 ---- README.zh.md | 4 ---- config/config.example.json | 2 +- pkg/config/config.go | 2 +- pkg/config/config_test.go | 4 ++-- 7 files changed, 4 insertions(+), 20 deletions(-) diff --git a/README.ja.md b/README.ja.md index 70ba458e9..7da16565f 100644 --- a/README.ja.md +++ b/README.ja.md @@ -133,10 +133,6 @@ vim config/config.json # DISCORD_BOT_TOKEN, プロバイダーの API キ # 3. ビルドと起動 docker compose --profile gateway up -d -> [!TIP] -> **Docker ユーザー**: デフォルトでは、Gateway は `127.0.0.1` でリッスンしており、ホストからアクセスできません。ヘルスチェックエンドポイントにアクセスしたり、ポートを公開したりする必要がある場合は、環境変数で `PICOCLAW_GATEWAY_HOST=0.0.0.0` を設定するか、`config.json` を更新してください。 - - # 4. ログ確認 docker compose logs -f picoclaw-gateway diff --git a/README.md b/README.md index 01c81b20b..d6a3d5696 100644 --- a/README.md +++ b/README.md @@ -165,10 +165,6 @@ vim config/config.json # Set DISCORD_BOT_TOKEN, API keys, etc. # 3. Build & Start docker compose --profile gateway up -d -> [!TIP] -> **Docker Users**: By default, the Gateway listens on `127.0.0.1` which is not accessible from the host. If you need to access the health endpoints or expose ports, set `PICOCLAW_GATEWAY_HOST=0.0.0.0` in your environment or update `config.json`. - - # 4. Check logs docker compose logs -f picoclaw-gateway diff --git a/README.pt-br.md b/README.pt-br.md index 171a6557b..fa73465dd 100644 --- a/README.pt-br.md +++ b/README.pt-br.md @@ -172,10 +172,6 @@ vim config/config.json # Configure DISCORD_BOT_TOKEN, API keys, etc. # 3. Build & Iniciar docker compose --profile gateway up -d -> [!TIP] -> **Usuários Docker**: Por padrão, o Gateway ouve em `127.0.0.1`, o que não é acessível a partir do host. Se você precisar acessar os endpoints de integridade ou expor portas, defina `PICOCLAW_GATEWAY_HOST=0.0.0.0` em seu ambiente ou atualize o `config.json`. - - # 4. Ver logs docker compose logs -f picoclaw-gateway diff --git a/README.zh.md b/README.zh.md index 434805f93..f751a5a97 100644 --- a/README.zh.md +++ b/README.zh.md @@ -170,10 +170,6 @@ vim config/config.json # 设置 DISCORD_BOT_TOKEN, API keys 等 # 3. 構建並啟動 docker compose --profile gateway up -d -> [!TIP] -> **Docker 用戶**: 預設情況下，Gateway 監聽 `127.0.0.1`，這在容器內無法從外部訪問。如果您需要透過端口映射訪問健康檢查接口，請在環境變量中設置 `PICOCLAW_GATEWAY_HOST=0.0.0.0` 或修改 `config.json`。 - - # 4. 查看日志 docker compose logs -f picoclaw-gateway diff --git a/config/config.example.json b/config/config.example.json index 07f052a98..37c2bcd81 100644 --- a/config/config.example.json +++ b/config/config.example.json @@ -142,7 +142,7 @@ "monitor_usb": true }, "gateway": { - "host": "127.0.0.1", + "host": "0.0.0.0", "port": 18790 } } diff --git a/pkg/config/config.go b/pkg/config/config.go index 0e68a053f..682996bd6 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -413,7 +413,7 @@ func DefaultConfig() *Config { ShengSuanYun: ProviderConfig{}, }, Gateway: GatewayConfig{ - Host: "127.0.0.1", + Host: "0.0.0.0", Port: 18790, }, Tools: ToolsConfig{ diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index b4a39cc8b..47916d155 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -246,7 +246,7 @@ func TestDefaultConfig_Temperature(t *testing.T) { func TestDefaultConfig_Gateway(t *testing.T) { cfg := DefaultConfig() - if cfg.Gateway.Host != "127.0.0.1" { + if cfg.Gateway.Host != "0.0.0.0" { t.Error("Gateway host should have default value") } if cfg.Gateway.Port == 0 { @@ -343,7 +343,7 @@ func TestConfig_Complete(t *testing.T) { if cfg.Agents.Defaults.MaxToolIterations == 0 { t.Error("MaxToolIterations should not be zero") } - if cfg.Gateway.Host != "127.0.0.1" { + if cfg.Gateway.Host != "0.0.0.0" { t.Error("Gateway host should have default value") } if cfg.Gateway.Port == 0 { From ef0b0f9a574666ffb494e75b9863f6a96b719cc3 Mon Sep 17 00:00:00 2001 From: 0x5487 Date: Thu, 19 Feb 2026 13:55:15 +0800 Subject: [PATCH 05/10] docs: Update Chinese README. --- README.zh.md | 1 - 1 file changed, 1 deletion(-) diff --git a/README.zh.md b/README.zh.md index f751a5a97..26194596f 100644 --- a/README.zh.md +++ b/README.zh.md @@ -167,7 +167,6 @@ cd picoclaw cp config/config.example.json config/config.json vim config/config.json # 设置 DISCORD_BOT_TOKEN, API keys 等 -# 3. 構建並啟動 docker compose --profile gateway up -d # 4. 查看日志 From 74a05c35418db8fca68fe9f5248c20ae58cf301b Mon Sep 17 00:00:00 2001 From: 0x5487 Date: Thu, 19 Feb 2026 13:55:43 +0800 Subject: [PATCH 06/10] docs: Update Chinese README. --- README.zh.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.zh.md b/README.zh.md index 26194596f..60dc0d64e 100644 --- a/README.zh.md +++ b/README.zh.md @@ -167,6 +167,7 @@ cd picoclaw cp config/config.example.json config/config.json vim config/config.json # 设置 DISCORD_BOT_TOKEN, API keys 等 +# 3. 建構並啟動 docker compose --profile gateway up -d # 4. 查看日志 From 239420bf39049f30b7b4b7fffb0b757062c25e72 Mon Sep 17 00:00:00 2001 From: 0x5487 Date: Thu, 19 Feb 2026 13:58:17 +0800 Subject: [PATCH 07/10] docs: Update Chinese README. --- README.zh.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.zh.md b/README.zh.md index 60dc0d64e..42bd20be4 100644 --- a/README.zh.md +++ b/README.zh.md @@ -167,7 +167,7 @@ cd picoclaw cp config/config.example.json config/config.json vim config/config.json # 设置 DISCORD_BOT_TOKEN, API keys 等 -# 3. 建構並啟動 +# 3. 构建并启动 docker compose --profile gateway up -d # 4. 查看日志 From 63e45f5e387d68b60b0066a7d59d7e6f556cf28d Mon Sep 17 00:00:00 2001 From: 0x5487 Date: Thu, 19 Feb 2026 22:05:42 +0800 Subject: [PATCH 08/10] refactor: Reorder filesystem helper functions, extract directory entry formatting logic, and enhance `WriteFileTool`'s result message. --- pkg/tools/filesystem.go | 172 +++++++++++++++++++--------------------- 1 file changed, 82 insertions(+), 90 deletions(-) diff --git a/pkg/tools/filesystem.go b/pkg/tools/filesystem.go index e9f985c86..1d58f7b5c 100644 --- a/pkg/tools/filesystem.go +++ b/pkg/tools/filesystem.go @@ -9,81 +9,6 @@ import ( "strings" ) -// Helper to get a safe relative path for os.Root usage -func getSafeRelPath(workspace, path string) (string, error) { - if workspace == "" { - return "", fmt.Errorf("workspace is not defined") - } - - // Clean the path first - path = filepath.Clean(path) - - // If absolute, make it relative to workspace - if filepath.IsAbs(path) { - rel, err := filepath.Rel(workspace, path) - if err != nil { - return "", fmt.Errorf("failed to calculate relative path: %w", err) - } - path = rel - } - - // Check for escape - if path == ".." || strings.HasPrefix(path, "../") { - return "", fmt.Errorf("path escapes workspace: %s", path) - } - - return path, nil -} - -// 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 -} - -// 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 { workspace string restrict bool @@ -133,7 +58,7 @@ func (t *ReadFileTool) Execute(ctx context.Context, args map[string]interface{}) 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("failed to read file: file not found: %s", path) } return nil, fmt.Errorf("access denied or failed to open: %w", err) } @@ -221,7 +146,7 @@ func (t *WriteFileTool) Execute(ctx context.Context, args map[string]interface{} if err != nil { return nil, fmt.Errorf("failed to write file: %w", err) } - return &ToolResult{Silent: true}, nil + return SilentResult(fmt.Sprintf("File written: %s", path)), nil }) } @@ -281,26 +206,93 @@ func (t *ListDirTool) Execute(ctx context.Context, args map[string]interface{}) 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 + return formatDirEntries(entries), nil }) } func formatDirEntries(entries []os.DirEntry) *ToolResult { - result := "" + var result strings.Builder for _, entry := range entries { if entry.IsDir() { - result += "DIR: " + entry.Name() + "\n" + result.WriteString("DIR: " + entry.Name() + "\n") } else { - result += "FILE: " + entry.Name() + "\n" + result.WriteString("FILE: " + entry.Name() + "\n") } } - return NewToolResult(result) + return NewToolResult(result.String()) +} + +// Helper to get a safe relative path for os.Root usage +func getSafeRelPath(workspace, path string) (string, error) { + if workspace == "" { + return "", fmt.Errorf("workspace is empty and not defined") + } + + path = filepath.Clean(path) + + // If absolute, make it relative to workspace + // os.Root only accepts relative paths + if filepath.IsAbs(path) { + rel, err := filepath.Rel(workspace, path) + if err != nil { + return "", fmt.Errorf("failed to calculate relative path: %w", err) + } + path = rel + } + + // Check for escape + if path == ".." || strings.HasPrefix(path, "../") { + return "", fmt.Errorf("path escapes workspace: %s", path) + } + + return path, nil +} + +// 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 +} + +// 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 } From bdb472b8a4b1c4801f10f5be58d32308593f86d5 Mon Sep 17 00:00:00 2001 From: 0x5487 Date: Thu, 19 Feb 2026 22:20:30 +0800 Subject: [PATCH 09/10] feat: Enhance `mkdirAllInRoot` to prevent creating directories over existing files and add tests for directory creation functionality. --- pkg/tools/filesystem.go | 13 ++++++++- pkg/tools/filesystem_test.go | 53 ++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 1 deletion(-) diff --git a/pkg/tools/filesystem.go b/pkg/tools/filesystem.go index 1d58f7b5c..77b0b91b6 100644 --- a/pkg/tools/filesystem.go +++ b/pkg/tools/filesystem.go @@ -291,7 +291,18 @@ func mkdirAllInRoot(root *os.Root, relPath string) error { } err := root.Mkdir(relPath, 0755) - if err != nil && !os.IsExist(err) { + if err != nil { + if os.IsExist(err) { + // Check if it's a directory + st, statErr := root.Stat(relPath) + if statErr != nil { + return statErr + } + if !st.IsDir() { + return fmt.Errorf("%s: not a directory", relPath) + } + return nil + } return err } return nil diff --git a/pkg/tools/filesystem_test.go b/pkg/tools/filesystem_test.go index a17c3d587..d6f8b747a 100644 --- a/pkg/tools/filesystem_test.go +++ b/pkg/tools/filesystem_test.go @@ -303,3 +303,56 @@ func TestFilesystemTool_EmptyWorkspace_AccessDenied(t *testing.T) { // Verify it failed for the right reason assert.Contains(t, result.ForLLM, "workspace is not defined", "Expected 'workspace is not defined' error") } + +func TestMkdirAllInRoot(t *testing.T) { + workspace := t.TempDir() + root, err := os.OpenRoot(workspace) + if err != nil { + t.Fatalf("failed to open root: %v", err) + } + defer root.Close() + + // Test case 1: Single directory + err = mkdirAllInRoot(root, "dir1") + assert.NoError(t, err) + _, err = os.Stat(filepath.Join(workspace, "dir1")) + assert.NoError(t, err) + + // Test case 2: Deeply nested directory + err = mkdirAllInRoot(root, "a/b/c/d") + assert.NoError(t, err) + _, err = os.Stat(filepath.Join(workspace, "a/b/c/d")) + assert.NoError(t, err) + + // Test case 3: Already exists + err = mkdirAllInRoot(root, "a/b/c/d") + assert.NoError(t, err) + + // Test case 4: File exists with same name (should fail) + err = os.WriteFile(filepath.Join(workspace, "file_exists"), []byte("data"), 0644) + assert.NoError(t, err) + err = mkdirAllInRoot(root, "file_exists") + assert.Error(t, err) +} + +func TestFilesystemTool_WriteFile_Restricted_CreateDir(t *testing.T) { + workspace := t.TempDir() + tool := NewWriteFileTool(workspace, true) + ctx := context.Background() + + testFile := "deep/nested/path/to/file.txt" + content := "deep content" + args := map[string]any{ + "path": testFile, + "content": content, + } + + result := tool.Execute(ctx, args) + assert.False(t, result.IsError, "Expected success, got: %s", result.ForLLM) + + // Verify file content + actualPath := filepath.Join(workspace, testFile) + data, err := os.ReadFile(actualPath) + assert.NoError(t, err) + assert.Equal(t, content, string(data)) +} From e37fde11170c6ee1434d91ef2d78afe445afc14e Mon Sep 17 00:00:00 2001 From: 0x5487 Date: Fri, 20 Feb 2026 11:06:50 +0800 Subject: [PATCH 10/10] Refactor filesystem tools to use a `fileReadWriter` interface for both host and sandboxed I/O, improving atomic writes and error handling. --- pkg/tools/edit.go | 206 ++++++++++++++++++---------------- pkg/tools/edit_test.go | 137 +++++++++++++++++++++++ pkg/tools/filesystem.go | 207 +++++++++++++++++++++-------------- pkg/tools/filesystem_test.go | 149 +++++++++++++++++++++++-- 4 files changed, 508 insertions(+), 191 deletions(-) diff --git a/pkg/tools/edit.go b/pkg/tools/edit.go index 1388690ee..aafa2adaf 100644 --- a/pkg/tools/edit.go +++ b/pkg/tools/edit.go @@ -2,8 +2,10 @@ package tools import ( "context" + "errors" "fmt" "io" + "io/fs" "os" "strings" ) @@ -68,77 +70,19 @@ func (t *EditFileTool) Execute(ctx context.Context, args map[string]interface{}) return ErrorResult("new_text is required") } - // 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)) + if t.restrict { + return executeInRoot(t.allowedDir, path, func(root *os.Root, relPath string) (*ToolResult, error) { + if err := editFileInRoot(root, relPath, oldText, newText); err != nil { + return nil, err } - 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)) + return SilentResult(fmt.Sprintf("File edited: %s", path)), nil + }) } - // 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 := io.ReadAll(f) - f.Close() - - if err != nil { - return nil, fmt.Errorf("failed to read file: %v", err) - } - - contentStr := string(content) - - if !strings.Contains(contentStr, oldText) { - return nil, fmt.Errorf("old_text not found in file. Make sure it matches exactly") - } - - 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) - } - - newContent := strings.Replace(contentStr, oldText, newText, 1) - - 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 - }) + if err := editFile(&hostRW{}, path, oldText, newText); err != nil { + return ErrorResult(err.Error()) + } + return SilentResult(fmt.Sprintf("File edited: %s", path)) } type AppendFileTool struct { @@ -181,38 +125,108 @@ func (t *AppendFileTool) Execute(ctx context.Context, args map[string]interface{ return ErrorResult("path is required") } - content, ok := args["content"].(string) + appendContent, ok := args["content"].(string) if !ok { return ErrorResult("content is required") } - // 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)) + var rw fileReadWriter + if t.restrict { + return executeInRoot(t.workspace, path, func(root *os.Root, relPath string) (*ToolResult, error) { + if err := appendFileWithRW(&rootRW{root: root}, relPath, appendContent); err != nil { + return nil, err + } + return SilentResult(fmt.Sprintf("Appended to %s", path)), nil + }) } - // 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 nil, fmt.Errorf("failed to append to file: %w", err) - } - - return SilentResult(fmt.Sprintf("Appended to %s", path)), nil - }) + rw = &hostRW{} + if err := appendFileWithRW(rw, path, appendContent); err != nil { + return ErrorResult(err.Error()) + } + return SilentResult(fmt.Sprintf("Appended to %s", path)) +} + +// editFile reads the file via rw, performs the replacement, and writes back. +// It uses a fileReadWriter, allowing the same logic for both restricted and unrestricted modes. +func editFile(rw fileReadWriter, path, oldText, newText string) error { + content, err := rw.Read(path) + if err != nil { + return err + } + + newContent, err := replaceEditContent(content, oldText, newText) + if err != nil { + return err + } + + return rw.Write(path, newContent) +} + +// editFileInRoot performs an in-place edit within an os.Root using a single open call. +// By opening with O_RDWR and reusing the same file descriptor for both read and write, +// we narrow the TOCTOU window compared to two separate open calls. +func editFileInRoot(root *os.Root, relPath, oldText, newText string) error { + f, err := root.OpenFile(relPath, os.O_RDWR, 0) + if err != nil { + if os.IsNotExist(err) { + return fmt.Errorf("failed to read file: file not found: %w", err) + } + if os.IsPermission(err) || strings.Contains(err.Error(), "escapes from parent") { + return fmt.Errorf("failed to read file: access denied: %w", err) + } + return fmt.Errorf("failed to open file for editing: %w", err) + } + defer f.Close() + + content, err := io.ReadAll(f) + if err != nil { + return fmt.Errorf("failed to read file content: %w", err) + } + + newContent, err := replaceEditContent(content, oldText, newText) + if err != nil { + return err + } + + // Truncate the file and seek back to the beginning before writing. + if err := f.Truncate(0); err != nil { + return fmt.Errorf("failed to truncate file for in-place edit: %w", err) + } + if _, err := f.Seek(0, io.SeekStart); err != nil { + return fmt.Errorf("failed to seek to beginning of file: %w", err) + } + + if _, err := f.Write(newContent); err != nil { + return fmt.Errorf("failed to write edited content: %w", err) + } + return nil +} + +// appendFileWithRW reads the existing content (if any) via rw, appends new content, and writes back. +func appendFileWithRW(rw fileReadWriter, path, appendContent string) error { + content, err := rw.Read(path) + if err != nil && !errors.Is(err, fs.ErrNotExist) { + return err + } + + newContent := append(content, []byte(appendContent)...) + return rw.Write(path, newContent) +} + +// replaceEditContent handles the core logic of finding and replacing a single occurrence of oldText. +func replaceEditContent(content []byte, oldText, newText string) ([]byte, error) { + contentStr := string(content) + + if !strings.Contains(contentStr, oldText) { + return nil, fmt.Errorf("old_text not found in file. Make sure it matches exactly") + } + + 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) + } + + newContent := strings.Replace(contentStr, oldText, newText, 1) + return []byte(newContent), nil } diff --git a/pkg/tools/edit_test.go b/pkg/tools/edit_test.go index 4855c3f83..8897512ab 100644 --- a/pkg/tools/edit_test.go +++ b/pkg/tools/edit_test.go @@ -288,3 +288,140 @@ func TestEditTool_AppendFile_MissingContent(t *testing.T) { t.Errorf("Expected error when content is missing") } } + +// TestReplaceEditContent verifies the helper function replaceEditContent +func TestReplaceEditContent(t *testing.T) { + tests := []struct { + name string + content []byte + oldText string + newText string + expected []byte + expectError bool + }{ + { + name: "successful replacement", + content: []byte("hello world"), + oldText: "world", + newText: "universe", + expected: []byte("hello universe"), + expectError: false, + }, + { + name: "old text not found", + content: []byte("hello world"), + oldText: "golang", + newText: "rust", + expected: nil, + expectError: true, + }, + { + name: "multiple matches found", + content: []byte("test text test"), + oldText: "test", + newText: "done", + expected: nil, + expectError: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result, err := replaceEditContent(tt.content, tt.oldText, tt.newText) + if tt.expectError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + assert.Equal(t, tt.expected, result) + } + }) + } +} + +// TestAppendFileTool_AppendToNonExistent_Restricted verifies that AppendFileTool in restricted mode +// can append to a file that does not yet exist — it should silently create the file. +// This exercises the errors.Is(err, fs.ErrNotExist) path in appendFileWithRW + rootRW. +func TestAppendFileTool_AppendToNonExistent_Restricted(t *testing.T) { + workspace := t.TempDir() + tool := NewAppendFileTool(workspace, true) + ctx := context.Background() + + args := map[string]interface{}{ + "path": "brand_new_file.txt", + "content": "first content", + } + + result := tool.Execute(ctx, args) + assert.False(t, result.IsError, "Expected success when appending to non-existent file in restricted mode, got: %s", result.ForLLM) + + // Verify the file was created with correct content + data, err := os.ReadFile(filepath.Join(workspace, "brand_new_file.txt")) + assert.NoError(t, err) + assert.Equal(t, "first content", string(data)) +} + +// TestAppendFileTool_Restricted_Success verifies that AppendFileTool in restricted mode +// correctly appends to an existing file within the sandbox. +func TestAppendFileTool_Restricted_Success(t *testing.T) { + workspace := t.TempDir() + testFile := "existing.txt" + err := os.WriteFile(filepath.Join(workspace, testFile), []byte("initial"), 0644) + assert.NoError(t, err) + + tool := NewAppendFileTool(workspace, true) + ctx := context.Background() + args := map[string]interface{}{ + "path": testFile, + "content": " appended", + } + + result := tool.Execute(ctx, args) + assert.False(t, result.IsError, "Expected success, got: %s", result.ForLLM) + assert.True(t, result.Silent) + + data, err := os.ReadFile(filepath.Join(workspace, testFile)) + assert.NoError(t, err) + assert.Equal(t, "initial appended", string(data)) +} + +// TestEditFileTool_Restricted_InPlaceEdit verifies that EditFileTool in restricted mode +// correctly edits a file using the single-open editFileInRoot path. +func TestEditFileTool_Restricted_InPlaceEdit(t *testing.T) { + workspace := t.TempDir() + testFile := "edit_target.txt" + err := os.WriteFile(filepath.Join(workspace, testFile), []byte("Hello World"), 0644) + assert.NoError(t, err) + + tool := NewEditFileTool(workspace, true) + ctx := context.Background() + args := map[string]interface{}{ + "path": testFile, + "old_text": "World", + "new_text": "Go", + } + + result := tool.Execute(ctx, args) + assert.False(t, result.IsError, "Expected success, got: %s", result.ForLLM) + assert.True(t, result.Silent) + + data, err := os.ReadFile(filepath.Join(workspace, testFile)) + assert.NoError(t, err) + assert.Equal(t, "Hello Go", string(data)) +} + +// TestEditFileTool_Restricted_FileNotFound verifies that editFileInRoot returns a proper +// error message when the target file does not exist. +func TestEditFileTool_Restricted_FileNotFound(t *testing.T) { + workspace := t.TempDir() + tool := NewEditFileTool(workspace, true) + ctx := context.Background() + args := map[string]interface{}{ + "path": "no_such_file.txt", + "old_text": "old", + "new_text": "new", + } + + result := tool.Execute(ctx, args) + assert.True(t, result.IsError) + assert.Contains(t, result.ForLLM, "not found") +} diff --git a/pkg/tools/filesystem.go b/pkg/tools/filesystem.go index 77b0b91b6..507ffbd42 100644 --- a/pkg/tools/filesystem.go +++ b/pkg/tools/filesystem.go @@ -3,10 +3,10 @@ package tools import ( "context" "fmt" - "io" "os" "path/filepath" "strings" + "time" ) type ReadFileTool struct { @@ -45,31 +45,21 @@ func (t *ReadFileTool) Execute(ctx context.Context, args map[string]interface{}) return ErrorResult("path is required") } - // 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)) + if t.restrict { + return executeInRoot(t.workspace, path, func(root *os.Root, relPath string) (*ToolResult, error) { + content, err := (&rootRW{root: root}).Read(relPath) + if err != nil { + return nil, err + } + return NewToolResult(string(content)), nil + }) } - 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() - - content, err := io.ReadAll(f) - if err != nil { - return nil, fmt.Errorf("failed to read file: %v", err) - } - return NewToolResult(string(content)), nil - }) + content, err := (&hostRW{}).Read(path) + if err != nil { + return ErrorResult(err.Error()) + } + return NewToolResult(string(content)) } type WriteFileTool struct { @@ -117,37 +107,20 @@ func (t *WriteFileTool) Execute(ctx context.Context, args map[string]interface{} return ErrorResult("content is required") } - 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)) + if t.restrict { + return executeInRoot(t.workspace, path, func(root *os.Root, relPath string) (*ToolResult, error) { + if err := (&rootRW{root: root}).Write(relPath, []byte(content)); err != nil { + return nil, err + } + return SilentResult(fmt.Sprintf("File written: %s", path)), nil + }) } - 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 := (&hostRW{}).Write(path, []byte(content)); err != nil { + return ErrorResult(err.Error()) + } - f, err := root.Create(relPath) - if err != nil { - return nil, fmt.Errorf("failed to create file: %w", err) - } - defer f.Close() - - _, err = f.WriteString(content) - if err != nil { - return nil, fmt.Errorf("failed to write file: %w", err) - } - return SilentResult(fmt.Sprintf("File written: %s", path)), nil - }) + return SilentResult(fmt.Sprintf("File written: %s", path)) } type ListDirTool struct { @@ -222,6 +195,102 @@ func formatDirEntries(entries []os.DirEntry) *ToolResult { return NewToolResult(result.String()) } +// fileReadWriter abstracts reading and writing files, allowing both unrestricted +// (host filesystem) and sandbox (os.Root) implementations to share the same logic. +type fileReadWriter interface { + Read(path string) ([]byte, error) + Write(path string, data []byte) error +} + +// hostRW is an unrestricted fileReadWriter that operates directly on the host filesystem. +type hostRW struct{} + +func (h *hostRW) Read(path string) ([]byte, error) { + content, err := os.ReadFile(path) + if err != nil { + if os.IsNotExist(err) { + return nil, fmt.Errorf("failed to read file: file not found: %w", err) + } + if os.IsPermission(err) { + return nil, fmt.Errorf("failed to read file: access denied: %w", err) + } + return nil, fmt.Errorf("failed to read file: %w", err) + } + return content, nil +} + +func (h *hostRW) Write(path string, data []byte) error { + dir := filepath.Dir(path) + if err := os.MkdirAll(dir, 0755); err != nil { + return fmt.Errorf("failed to create parent directories: %w", err) + } + + tmpPath := fmt.Sprintf("%s.%d.tmp", path, time.Now().UnixNano()) + if err := os.WriteFile(tmpPath, data, 0644); err != nil { + return fmt.Errorf("failed to write temp file: %w", err) + } + + if err := os.Rename(tmpPath, path); err != nil { + os.Remove(tmpPath) + return fmt.Errorf("failed to replace original file: %w", err) + } + return nil +} + +// rootRW is a sandboxed fileReadWriter that operates within an os.Root boundary. +// All paths passed to Read/Write must be relative to the root. +type rootRW struct { + root *os.Root +} + +func (r *rootRW) Read(path string) ([]byte, error) { + content, err := r.root.ReadFile(path) + if err != nil { + if os.IsNotExist(err) { + return nil, fmt.Errorf("failed to read file: file not found: %w", err) + } + // os.Root returns "escapes from parent" for paths outside the root + if os.IsPermission(err) || strings.Contains(err.Error(), "escapes from parent") || strings.Contains(err.Error(), "permission denied") { + return nil, fmt.Errorf("failed to read file: access denied: %w", err) + } + return nil, fmt.Errorf("failed to read file: %w", err) + } + return content, nil +} + +func (r *rootRW) Write(path string, data []byte) error { + dir := filepath.Dir(path) + if dir != "." && dir != "/" { + // Use native root.MkdirAll which handles the "file exists at path" check internally. + if err := r.root.MkdirAll(dir, 0755); err != nil { + return fmt.Errorf("failed to create parent directories: %w", err) + } + } + + tmpRelPath := fmt.Sprintf("%s.%d.tmp", path, time.Now().UnixNano()) + fw, err := r.root.Create(tmpRelPath) + if err != nil { + return fmt.Errorf("failed to create temp file for writing: %w", err) + } + + if _, err := fw.Write(data); err != nil { + fw.Close() + r.root.Remove(tmpRelPath) + return fmt.Errorf("failed to write to temp file: %w", err) + } + + if err := fw.Close(); err != nil { + r.root.Remove(tmpRelPath) + return fmt.Errorf("failed to close temp file: %w", err) + } + + if err := r.root.Rename(tmpRelPath, path); err != nil { + r.root.Remove(tmpRelPath) + return fmt.Errorf("failed to rename temp file over target: %w", err) + } + return nil +} + // Helper to get a safe relative path for os.Root usage func getSafeRelPath(workspace, path string) (string, error) { if workspace == "" { @@ -240,7 +309,7 @@ func getSafeRelPath(workspace, path string) (string, error) { path = rel } - // Check for escape + // Check for escape manually (defense-in-depth, as os.Root also rejects paths that escape the root) if path == ".." || strings.HasPrefix(path, "../") { return "", fmt.Errorf("path escapes workspace: %s", path) } @@ -275,35 +344,3 @@ func executeInRoot(workspace string, path string, fn func(root *os.Root, relPath return result } - -// 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 { - if os.IsExist(err) { - // Check if it's a directory - st, statErr := root.Stat(relPath) - if statErr != nil { - return statErr - } - if !st.IsDir() { - return fmt.Errorf("%s: not a directory", relPath) - } - return nil - } - return err - } - return nil -} diff --git a/pkg/tools/filesystem_test.go b/pkg/tools/filesystem_test.go index d6f8b747a..635bf009f 100644 --- a/pkg/tools/filesystem_test.go +++ b/pkg/tools/filesystem_test.go @@ -2,6 +2,7 @@ package tools import ( "context" + "io" "os" "path/filepath" "strings" @@ -304,7 +305,9 @@ func TestFilesystemTool_EmptyWorkspace_AccessDenied(t *testing.T) { assert.Contains(t, result.ForLLM, "workspace is not defined", "Expected 'workspace is not defined' error") } -func TestMkdirAllInRoot(t *testing.T) { +// TestRootMkdirAll verifies that root.MkdirAll (used by atomicWriteFileInRoot) handles all cases: +// single dir, deeply nested dirs, already-existing dirs, and a file blocking a directory path. +func TestRootMkdirAll(t *testing.T) { workspace := t.TempDir() root, err := os.OpenRoot(workspace) if err != nil { @@ -312,27 +315,27 @@ func TestMkdirAllInRoot(t *testing.T) { } defer root.Close() - // Test case 1: Single directory - err = mkdirAllInRoot(root, "dir1") + // Case 1: Single directory + err = root.MkdirAll("dir1", 0755) assert.NoError(t, err) _, err = os.Stat(filepath.Join(workspace, "dir1")) assert.NoError(t, err) - // Test case 2: Deeply nested directory - err = mkdirAllInRoot(root, "a/b/c/d") + // Case 2: Deeply nested directory + err = root.MkdirAll("a/b/c/d", 0755) assert.NoError(t, err) _, err = os.Stat(filepath.Join(workspace, "a/b/c/d")) assert.NoError(t, err) - // Test case 3: Already exists - err = mkdirAllInRoot(root, "a/b/c/d") + // Case 3: Already exists — must be idempotent + err = root.MkdirAll("a/b/c/d", 0755) assert.NoError(t, err) - // Test case 4: File exists with same name (should fail) + // Case 4: A regular file blocks directory creation — must error err = os.WriteFile(filepath.Join(workspace, "file_exists"), []byte("data"), 0644) assert.NoError(t, err) - err = mkdirAllInRoot(root, "file_exists") - assert.Error(t, err) + err = root.MkdirAll("file_exists", 0755) + assert.Error(t, err, "expected error when a file exists at the directory path") } func TestFilesystemTool_WriteFile_Restricted_CreateDir(t *testing.T) { @@ -356,3 +359,129 @@ func TestFilesystemTool_WriteFile_Restricted_CreateDir(t *testing.T) { assert.NoError(t, err) assert.Equal(t, content, string(data)) } + +// TestHostRW_Read_PermissionDenied verifies that hostRW.Read surfaces access denied errors. +func TestHostRW_Read_PermissionDenied(t *testing.T) { + if os.Getuid() == 0 { + t.Skip("skipping permission test: running as root") + } + tmpDir := t.TempDir() + protected := filepath.Join(tmpDir, "protected.txt") + err := os.WriteFile(protected, []byte("secret"), 0000) + assert.NoError(t, err) + defer os.Chmod(protected, 0644) // ensure cleanup + + _, err = (&hostRW{}).Read(protected) + assert.Error(t, err) + assert.Contains(t, err.Error(), "access denied") +} + +// TestHostRW_Read_Directory verifies that hostRW.Read returns an error when given a directory path. +func TestHostRW_Read_Directory(t *testing.T) { + tmpDir := t.TempDir() + + _, err := (&hostRW{}).Read(tmpDir) + assert.Error(t, err, "expected error when reading a directory as a file") +} + +// TestRootRW_Read_Directory verifies that rootRW.Read returns an error when given a directory. +func TestRootRW_Read_Directory(t *testing.T) { + workspace := t.TempDir() + root, err := os.OpenRoot(workspace) + assert.NoError(t, err) + defer root.Close() + + // Create a subdirectory + err = root.Mkdir("subdir", 0755) + assert.NoError(t, err) + + _, err = (&rootRW{root: root}).Read("subdir") + assert.Error(t, err, "expected error when reading a directory as a file") +} + +// TestHostRW_Write_ParentDirMissing verifies that hostRW.Write creates parent dirs automatically. +func TestHostRW_Write_ParentDirMissing(t *testing.T) { + tmpDir := t.TempDir() + target := filepath.Join(tmpDir, "a", "b", "c", "file.txt") + + err := (&hostRW{}).Write(target, []byte("hello")) + assert.NoError(t, err) + + data, err := os.ReadFile(target) + assert.NoError(t, err) + assert.Equal(t, "hello", string(data)) +} + +// TestRootRW_Write_ParentDirMissing verifies that rootRW.Write creates +// nested parent directories automatically within the sandbox. +func TestRootRW_Write_ParentDirMissing(t *testing.T) { + workspace := t.TempDir() + root, err := os.OpenRoot(workspace) + assert.NoError(t, err) + defer root.Close() + + relPath := "x/y/z/file.txt" + err = (&rootRW{root: root}).Write(relPath, []byte("nested")) + assert.NoError(t, err) + + data, err := os.ReadFile(filepath.Join(workspace, relPath)) + assert.NoError(t, err) + assert.Equal(t, "nested", string(data)) +} + +// TestHostRW_Write verifies the hostRW.Write helper function +func TestHostRW_Write(t *testing.T) { + tmpDir := t.TempDir() + testFile := filepath.Join(tmpDir, "atomic_test.txt") + testData := []byte("atomic test content") + + err := (&hostRW{}).Write(testFile, testData) + assert.NoError(t, err) + + content, err := os.ReadFile(testFile) + assert.NoError(t, err) + assert.Equal(t, testData, content) + + // Verify it overwrites correctly + newData := []byte("new atomic content") + err = (&hostRW{}).Write(testFile, newData) + assert.NoError(t, err) + + content, err = os.ReadFile(testFile) + assert.NoError(t, err) + assert.Equal(t, newData, content) +} + +// TestRootRW_Write verifies the rootRW.Write helper function +func TestRootRW_Write(t *testing.T) { + tmpDir := t.TempDir() + root, err := os.OpenRoot(tmpDir) + assert.NoError(t, err) + defer root.Close() + + relPath := "atomic_root_test.txt" + testData := []byte("atomic root test content") + + erw := &rootRW{root: root} + err = erw.Write(relPath, testData) + assert.NoError(t, err) + + f, err := root.Open(relPath) + assert.NoError(t, err) + content, err := io.ReadAll(f) + assert.NoError(t, err) + f.Close() + assert.Equal(t, testData, content) + + // Verify it overwrites correctly + newData := []byte("new root atomic content") + err = erw.Write(relPath, newData) + assert.NoError(t, err) + + f, err = root.Open(relPath) + assert.NoError(t, err) + content, err = io.ReadAll(f) + assert.NoError(t, err) + f.Close() + assert.Equal(t, newData, content) +}