From 7ef1cacbfbfda2acf0b91468bc0bbe784f8e2d77 Mon Sep 17 00:00:00 2001 From: 0x5487 Date: Sun, 1 Mar 2026 02:13:32 +0800 Subject: [PATCH] feat: expand default sandbox tool permissions and sanitize container environment variables --- pkg/agent/sandbox/container.go | 4 ++ pkg/agent/sandbox/container_test.go | 22 ++++++++ pkg/agent/sandbox/host.go | 75 ++++++++++++++++++++------- pkg/agent/sandbox/host_test.go | 28 ++++++++++ pkg/agent/sandbox/manager.go | 2 +- pkg/agent/sandbox/manager_test.go | 28 +++++++++- pkg/agent/sandbox/path.go | 12 ++++- pkg/agent/sandbox/registry.go | 15 ++---- pkg/agent/sandbox/tool_policy.go | 15 ++---- pkg/agent/sandbox/tool_policy_test.go | 22 ++++---- pkg/config/config_test.go | 12 +++++ pkg/config/defaults.go | 2 +- pkg/session/manager.go | 36 +------------ pkg/tools/filesystem_test.go | 21 +++++++- 14 files changed, 201 insertions(+), 93 deletions(-) diff --git a/pkg/agent/sandbox/container.go b/pkg/agent/sandbox/container.go index 7b3105174..fac74d412 100644 --- a/pkg/agent/sandbox/container.go +++ b/pkg/agent/sandbox/container.go @@ -103,6 +103,10 @@ func NewContainerSandbox(cfg ContainerSandboxConfig) *ContainerSandbox { if cfg.Env == nil { cfg.Env = map[string]string{"LANG": "C.UTF-8"} } + cfg.Env = sanitizeEnvVars(cfg.Env) + if len(cfg.Env) == 0 { + cfg.Env = map[string]string{"LANG": "C.UTF-8"} + } cfg.WorkspaceAccess = string(normalizeWorkspaceAccess(config.WorkspaceAccess(cfg.WorkspaceAccess))) cfg.WorkspaceRoot = strings.TrimSpace(cfg.WorkspaceRoot) sb := &ContainerSandbox{cfg: cfg} diff --git a/pkg/agent/sandbox/container_test.go b/pkg/agent/sandbox/container_test.go index decd6bdad..4e149e819 100644 --- a/pkg/agent/sandbox/container_test.go +++ b/pkg/agent/sandbox/container_test.go @@ -536,3 +536,25 @@ func TestContainerSandbox_StopWithoutClient(t *testing.T) { t.Fatalf("Prune() error: %v", err) } } + +func TestNewContainerSandbox_SanitizesEnv(t *testing.T) { + sb := NewContainerSandbox(ContainerSandboxConfig{ + Env: map[string]string{ + "LANG": "C.UTF-8", + "OPENAI_API_KEY": "secret", + "SAFE_NAME": "ok", + }, + }) + + got := sb.containerEnv() + joined := strings.Join(got, "\n") + if strings.Contains(joined, "OPENAI_API_KEY=") { + t.Fatalf("sensitive env key should be filtered, got: %v", got) + } + if !strings.Contains(joined, "SAFE_NAME=ok") { + t.Fatalf("safe env key should be preserved, got: %v", got) + } + if !strings.Contains(joined, "LANG=C.UTF-8") { + t.Fatalf("LANG should be preserved or defaulted, got: %v", got) + } +} diff --git a/pkg/agent/sandbox/host.go b/pkg/agent/sandbox/host.go index 17fd1f4f1..a5e738005 100644 --- a/pkg/agent/sandbox/host.go +++ b/pkg/agent/sandbox/host.go @@ -13,6 +13,8 @@ import ( "strings" "sync" "time" + + "github.com/sipeed/picoclaw/pkg/fileutil" ) type HostSandbox struct { @@ -239,22 +241,19 @@ func (h *hostFS) WriteFile(ctx context.Context, path string, data []byte, mkdir if err != nil { return err } - if mkdir { - if err := os.MkdirAll(filepath.Dir(resolved), 0o755); err != nil { + + parent := filepath.Dir(resolved) + if !mkdir { + parentInfo, err := os.Stat(parent) + if err != nil { return err } + if !parentInfo.IsDir() { + return fmt.Errorf("parent path is not a directory: %s", parent) + } } - // Atomic write: write to temp file then rename to prevent partial writes. - tmpPath := fmt.Sprintf("%s.%d.tmp", resolved, time.Now().UnixNano()) - if err := os.WriteFile(tmpPath, data, 0o644); err != nil { - os.Remove(tmpPath) - return fmt.Errorf("failed to write temp file: %w", err) - } - if err := os.Rename(tmpPath, resolved); err != nil { - os.Remove(tmpPath) - return fmt.Errorf("failed to replace original file: %w", err) - } - return nil + + return fileutil.WriteFileAtomic(resolved, data, 0o644) } relPath, err := h.getSafeRelPath(path) @@ -268,16 +267,54 @@ func (h *hostFS) WriteFile(ctx context.Context, path string, data []byte, mkdir return err } } - // Atomic write within os.Root: write to temp file then rename. - tmpRelPath := fmt.Sprintf("%s.%d.tmp", relPath, time.Now().UnixNano()) - if err := h.root.WriteFile(tmpRelPath, data, 0o644); err != nil { - h.root.Remove(tmpRelPath) + return writeFileAtomicInRoot(h.root, relPath, data) +} + +func writeFileAtomicInRoot(root *os.Root, relPath string, data []byte) error { + dir := filepath.Dir(relPath) + tmpName := fmt.Sprintf(".tmp-%d-%d", os.Getpid(), time.Now().UnixNano()) + tmpRelPath := tmpName + if dir != "." && dir != "/" { + tmpRelPath = filepath.Join(dir, tmpName) + } + + tmpFile, err := root.OpenFile(tmpRelPath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o644) + if err != nil { + _ = root.Remove(tmpRelPath) + return fmt.Errorf("failed to open temp file: %w", err) + } + + if _, err := tmpFile.Write(data); err != nil { + _ = tmpFile.Close() + _ = root.Remove(tmpRelPath) return fmt.Errorf("failed to write temp file: %w", err) } - if err := h.root.Rename(tmpRelPath, relPath); err != nil { - h.root.Remove(tmpRelPath) + + if err := tmpFile.Sync(); err != nil { + _ = tmpFile.Close() + _ = root.Remove(tmpRelPath) + return fmt.Errorf("failed to sync temp file: %w", err) + } + + if err := tmpFile.Close(); err != nil { + _ = root.Remove(tmpRelPath) + return fmt.Errorf("failed to close temp file: %w", err) + } + + if err := root.Rename(tmpRelPath, relPath); err != nil { + _ = root.Remove(tmpRelPath) return fmt.Errorf("failed to rename temp file over target: %w", err) } + + syncDir := "." + if dir != "" && dir != "/" { + syncDir = dir + } + if dirFile, err := root.Open(syncDir); err == nil { + _ = dirFile.Sync() + _ = dirFile.Close() + } + return nil } diff --git a/pkg/agent/sandbox/host_test.go b/pkg/agent/sandbox/host_test.go index b4c1c75bc..43167918e 100644 --- a/pkg/agent/sandbox/host_test.go +++ b/pkg/agent/sandbox/host_test.go @@ -233,6 +233,16 @@ func TestHostFS_ReadFileWriteFile_Unrestricted(t *testing.T) { } } +func TestHostFS_WriteFile_Unrestricted_NoMkdirMissingParent(t *testing.T) { + root := t.TempDir() + sb := NewHostSandbox(root, false) + + err := sb.Fs().WriteFile(context.Background(), "missing/parent/file.txt", []byte("x"), false) + if err == nil { + t.Fatalf("expected WriteFile to fail when parent directory is missing and mkdir=false") + } +} + func TestHostFS_WriteFileMKdir(t *testing.T) { root := t.TempDir() sb := NewHostSandbox(root, true) @@ -352,6 +362,24 @@ func TestValidatePathErrors(t *testing.T) { t.Fatalf("expected err for empty workspace with restrict=true") } + absTarget := filepath.Join(t.TempDir(), "abs.txt") + got, err := ValidatePath(absTarget, "", false) + if err != nil { + t.Fatalf("expected unrestricted empty-workspace absolute path to pass, got: %v", err) + } + if got != absTarget { + t.Fatalf("ValidatePath(abs, empty, false) = %q, want %q", got, absTarget) + } + + relTarget := "rel.txt" + got, err = ValidatePath(relTarget, "", false) + if err != nil { + t.Fatalf("expected unrestricted empty-workspace relative path to pass, got: %v", err) + } + if !filepath.IsAbs(got) { + t.Fatalf("ValidatePath(rel, empty, false) should return abs path, got %q", got) + } + root := t.TempDir() // target parent is file, evalSymlinks should fail diff --git a/pkg/agent/sandbox/manager.go b/pkg/agent/sandbox/manager.go index eef03658a..cf71ed77a 100644 --- a/pkg/agent/sandbox/manager.go +++ b/pkg/agent/sandbox/manager.go @@ -287,7 +287,7 @@ func (m *scopedSandboxManager) pruneOnce(ctx context.Context) error { if m.pruneIdleHours <= 0 && m.pruneMaxAgeDays <= 0 { return nil } - regPath := filepath.Join(infra.ResolveHomeDir(), "sandbox", defaultSandboxRegistryFile) + regPath := filepath.Join(infra.ResolveHomeDir(), "sandboxes", defaultSandboxRegistryFile) registryMu.Lock() data, err := loadRegistry(regPath) registryMu.Unlock() diff --git a/pkg/agent/sandbox/manager_test.go b/pkg/agent/sandbox/manager_test.go index 35b66c703..bd24fdd4d 100644 --- a/pkg/agent/sandbox/manager_test.go +++ b/pkg/agent/sandbox/manager_test.go @@ -8,6 +8,7 @@ import ( "time" "github.com/sipeed/picoclaw/pkg/config" + "github.com/sipeed/picoclaw/pkg/routing" ) func TestNormalizeWorkspaceAccess(t *testing.T) { @@ -94,7 +95,7 @@ func TestScopedSandboxManager_PruneLoopLifecycle(t *testing.T) { func TestScopedSandboxManager_PruneOnceLoadRegistryError(t *testing.T) { home := t.TempDir() t.Setenv("HOME", home) - stateDir := filepath.Join(home, ".picoclaw", "sandbox") + stateDir := filepath.Join(home, ".picoclaw", "sandboxes") if err := os.MkdirAll(stateDir, 0o755); err != nil { t.Fatalf("mkdir state dir: %v", err) } @@ -113,3 +114,28 @@ func TestScopedSandboxManager_PruneOnceLoadRegistryError(t *testing.T) { t.Fatal("expected pruneOnce() to return registry load error") } } + +func TestScopedSandboxManager_ShouldSandbox_NonMain(t *testing.T) { + m := &scopedSandboxManager{ + mode: config.SandboxModeNonMain, + agentID: "default", + } + + if m.shouldSandbox(context.Background()) { + t.Fatal("expected background context to map to main session (host path)") + } + + if m.shouldSandbox(WithSessionKey(context.Background(), "main")) { + t.Fatal("expected explicit main alias to remain host path") + } + + mainKey := routing.BuildAgentMainSessionKey("default") + if m.shouldSandbox(WithSessionKey(context.Background(), mainKey)) { + t.Fatal("expected agent main session key to remain host path") + } + + nonMainKey := "agent:default:direct:user-1" + if !m.shouldSandbox(WithSessionKey(context.Background(), nonMainKey)) { + t.Fatal("expected non-main session to use sandbox path") + } +} diff --git a/pkg/agent/sandbox/path.go b/pkg/agent/sandbox/path.go index 0399f6eac..52e1e95a0 100644 --- a/pkg/agent/sandbox/path.go +++ b/pkg/agent/sandbox/path.go @@ -9,7 +9,17 @@ import ( // ValidatePath ensures the given path is within the workspace if restrict is true. func ValidatePath(path, workspace string, restrict bool) (string, error) { if workspace == "" { - return "", fmt.Errorf("workspace is not defined") + if restrict { + return "", fmt.Errorf("workspace is not defined") + } + if filepath.IsAbs(path) { + return filepath.Clean(path), nil + } + absPath, err := filepath.Abs(path) + if err != nil { + return "", fmt.Errorf("failed to resolve file path: %w", err) + } + return absPath, nil } absWorkspace, err := filepath.Abs(workspace) diff --git a/pkg/agent/sandbox/registry.go b/pkg/agent/sandbox/registry.go index 4bd617cf9..f5b7b5820 100644 --- a/pkg/agent/sandbox/registry.go +++ b/pkg/agent/sandbox/registry.go @@ -10,6 +10,8 @@ import ( "path/filepath" "sync" "time" + + "github.com/sipeed/picoclaw/pkg/fileutil" ) type registryEntry struct { @@ -89,22 +91,11 @@ func loadRegistry(path string) (*registryData, error) { } func saveRegistry(path string, data *registryData) error { - if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { - return err - } raw, err := json.MarshalIndent(data, "", " ") if err != nil { return err } - tmp := fmt.Sprintf("%s.%d.tmp", path, time.Now().UnixNano()) - if err := os.WriteFile(tmp, append(raw, '\n'), 0o644); err != nil { - return err - } - if err := os.Rename(tmp, path); err != nil { - _ = os.Remove(tmp) - return err - } - return nil + return fileutil.WriteFileAtomic(path, append(raw, '\n'), 0o644) } func upsertRegistryEntry(path string, entry registryEntry) error { diff --git a/pkg/agent/sandbox/tool_policy.go b/pkg/agent/sandbox/tool_policy.go index b6a8b354a..4651d9d0a 100644 --- a/pkg/agent/sandbox/tool_policy.go +++ b/pkg/agent/sandbox/tool_policy.go @@ -6,7 +6,7 @@ import ( "github.com/sipeed/picoclaw/pkg/config" ) -var defaultSandboxAllow = []string{"exec", "read_file", "write_file"} +var defaultSandboxAllow = []string{"exec", "read_file", "write_file", "list_dir", "edit_file", "append_file"} func IsToolSandboxEnabled(cfg *config.Config, tool string) bool { name := strings.ToLower(strings.TrimSpace(tool)) @@ -14,26 +14,19 @@ func IsToolSandboxEnabled(cfg *config.Config, tool string) bool { return false } - allow, hasAllow := defaultSandboxAllow, false + allow := defaultSandboxAllow deny := []string{} if cfg != nil { allow = cfg.Tools.Sandbox.Tools.Allow deny = cfg.Tools.Sandbox.Tools.Deny - hasAllow = cfg.Tools.Sandbox.Tools.Allow != nil } if containsTool(deny, name) { return false } - if !hasAllow { - // No allow list configured: use the built-in default set. - return containsTool(defaultSandboxAllow, name) - } if len(allow) == 0 { - // Explicit empty allow list means "deny all" — no tool gets - // sandbox routing. This is the intuitive interpretation: an empty - // allowlist blocks everything (principle of least privilege). - return false + // Empty allow list falls back to built-in defaults. + allow = defaultSandboxAllow } return containsTool(allow, name) } diff --git a/pkg/agent/sandbox/tool_policy_test.go b/pkg/agent/sandbox/tool_policy_test.go index 4822fbf7a..e1dddd916 100644 --- a/pkg/agent/sandbox/tool_policy_test.go +++ b/pkg/agent/sandbox/tool_policy_test.go @@ -10,8 +10,8 @@ func TestIsToolSandboxEnabled_Default(t *testing.T) { if !IsToolSandboxEnabled(nil, "exec") { t.Fatal("expected exec to be sandbox-enabled by default") } - if IsToolSandboxEnabled(nil, "list_dir") { - t.Fatal("expected list_dir to be host by default") + if !IsToolSandboxEnabled(nil, "list_dir") { + t.Fatal("expected list_dir to be sandbox-enabled by default") } } @@ -31,21 +31,19 @@ func TestIsToolSandboxEnabled_AllowDeny(t *testing.T) { } } -// TestIsToolSandboxEnabled_EmptyAllowDeniesAll verifies BOUNDARY-1 fix: -// an explicitly empty allow list now means "deny all tools" (principle of least privilege). -func TestIsToolSandboxEnabled_EmptyAllowDeniesAll(t *testing.T) { +// TestIsToolSandboxEnabled_EmptyAllowUsesDefault verifies that an explicitly +// empty allow list falls back to built-in defaults. +func TestIsToolSandboxEnabled_EmptyAllowUsesDefault(t *testing.T) { cfg := config.DefaultConfig() cfg.Tools.Sandbox.Tools.Allow = []string{} cfg.Tools.Sandbox.Tools.Deny = []string{"cron"} - // Empty explicit allow should now deny everything (including read_file which was previously allowed) - if IsToolSandboxEnabled(cfg, "read_file") { - t.Fatal("expected read_file to be DISABLED when allow list is explicitly empty (deny all)") + for _, tool := range []string{"exec", "read_file", "write_file", "list_dir", "edit_file", "append_file"} { + if !IsToolSandboxEnabled(cfg, tool) { + t.Fatalf("expected %s to use default allow list when allow is empty", tool) + } } - if IsToolSandboxEnabled(cfg, "exec") { - t.Fatal("expected exec to be DISABLED when allow list is explicitly empty (deny all)") - } - // Deny list still applies (as a belt-and-suspenders check) + if IsToolSandboxEnabled(cfg, "cron") { t.Fatal("expected denied tool to be disabled") } diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index f835f022c..76d7d27d9 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -377,6 +377,18 @@ func TestDefaultConfig_SandboxTools(t *testing.T) { if len(cfg.Tools.Sandbox.Tools.Allow) == 0 { t.Fatal("Expected sandbox allow tools to be configured") } + for _, tool := range []string{"exec", "read_file", "write_file", "list_dir", "edit_file", "append_file"} { + found := false + for _, v := range cfg.Tools.Sandbox.Tools.Allow { + if v == tool { + found = true + break + } + } + if !found { + t.Fatalf("Expected sandbox allow tools to include %q, got %v", tool, cfg.Tools.Sandbox.Tools.Allow) + } + } if cfg.Agents.Defaults.Sandbox.Prune.IdleHours == nil || *cfg.Agents.Defaults.Sandbox.Prune.IdleHours <= 0 { t.Fatal("Expected sandbox prune idle hours > 0") } diff --git a/pkg/config/defaults.go b/pkg/config/defaults.go index 37056bbf6..b6b9cbe2b 100644 --- a/pkg/config/defaults.go +++ b/pkg/config/defaults.go @@ -370,7 +370,7 @@ func DefaultConfig() *Config { }, Sandbox: SandboxToolsConfig{ Tools: SandboxToolPolicyConfig{ - Allow: []string{"exec", "read_file", "write_file"}, + Allow: []string{"exec", "read_file", "write_file", "list_dir", "edit_file", "append_file"}, Deny: []string{"cron"}, }, }, diff --git a/pkg/session/manager.go b/pkg/session/manager.go index 08f0b0ad2..3f66fbf85 100644 --- a/pkg/session/manager.go +++ b/pkg/session/manager.go @@ -8,6 +8,7 @@ import ( "sync" "time" + "github.com/sipeed/picoclaw/pkg/fileutil" "github.com/sipeed/picoclaw/pkg/providers" ) @@ -197,40 +198,7 @@ func (sm *SessionManager) Save(key string) error { } sessionPath := filepath.Join(sm.storage, filename+".json") - tmpFile, err := os.CreateTemp(sm.storage, "session-*.tmp") - if err != nil { - return err - } - - tmpPath := tmpFile.Name() - cleanup := true - defer func() { - if cleanup { - _ = os.Remove(tmpPath) - } - }() - - if _, err := tmpFile.Write(data); err != nil { - _ = tmpFile.Close() - return err - } - if err := tmpFile.Chmod(0o644); err != nil { - _ = tmpFile.Close() - return err - } - if err := tmpFile.Sync(); err != nil { - _ = tmpFile.Close() - return err - } - if err := tmpFile.Close(); err != nil { - return err - } - - if err := os.Rename(tmpPath, sessionPath); err != nil { - return err - } - cleanup = false - return nil + return fileutil.WriteFileAtomic(sessionPath, data, 0o644) } func (sm *SessionManager) loadSessions() error { diff --git a/pkg/tools/filesystem_test.go b/pkg/tools/filesystem_test.go index ff6a47580..483250c75 100644 --- a/pkg/tools/filesystem_test.go +++ b/pkg/tools/filesystem_test.go @@ -318,7 +318,7 @@ func TestFilesystemTool_EmptyWorkspace_AccessDenied(t *testing.T) { os.WriteFile(secretFile, []byte("secret data"), 0o600) result := tool.Execute(sandbox.WithSandbox(context.Background(), &stubSandbox{ - err: fmt.Errorf("workspace is not defined"), + fs: sandbox.NewHostSandbox("", true).Fs(), }), map[string]any{ "path": secretFile, }) @@ -330,6 +330,25 @@ func TestFilesystemTool_EmptyWorkspace_AccessDenied(t *testing.T) { assert.Contains(t, result.ForLLM, "workspace is not defined", "Expected 'workspace is not defined' error") } +func TestFilesystemTool_EmptyWorkspace_UnrestrictedAllowed(t *testing.T) { + tool := NewReadFileTool("", false) // restrict=false and workspace="" + + tmpDir := t.TempDir() + secretFile := filepath.Join(tmpDir, "public.txt") + if err := os.WriteFile(secretFile, []byte("public data"), 0o644); err != nil { + t.Fatalf("failed to write test file: %v", err) + } + + result := tool.Execute(sandbox.WithSandbox(context.Background(), &stubSandbox{ + fs: sandbox.NewHostSandbox("", false).Fs(), + }), map[string]any{ + "path": secretFile, + }) + + assert.False(t, result.IsError, "Expected unrestricted empty-workspace read to succeed, got: %s", result.ForLLM) + assert.Contains(t, result.ForLLM, "public data") +} + // 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) {