fix(tools): allow /dev/null redirection and add read/write sandbox split (#967)

* fix(tools): allow /dev/null redirection and add read/write sandbox split - Remove deny pattern that incorrectly blocked redirects to /dev/null - Expand block device write pattern to cover nvme, mmcblk, vd, xvd, hd, loop, dm-, md, sr and nbd in addition to sd - Add safe path whitelist for kernel pseudo-devices so workspace path check does not reject /dev/null, /dev/zero, /dev/random, /dev/urandom, /dev/stdin, /dev/stdout and /dev/stderr - Add allow_read_outside_workspace config option (default true) so file read and list tools are unrestricted while write tools stay sandboxed Closes https://github.com/sipeed/picoclaw/issues/964 Closes https://github.com/sipeed/picoclaw/issues/965 Signed-off-by: Huang Rui <vowstar@gmail.com> * feat(tools): add configurable allow patterns and path whitelists - Add custom_allow_patterns to exec config so users can exempt specific commands from deny pattern checks - Add allow_read_paths and allow_write_paths regex lists to tools config for whitelisting specific paths outside the workspace - Introduce whitelistFs that wraps sandboxFs and falls through to hostFs for paths matching whitelist patterns - Use variadic constructor signatures to keep backward compatibility Suggested-by: lxowalle Signed-off-by: Huang Rui <vowstar@gmail.com> --------- Signed-off-by: Huang Rui <vowstar@gmail.com>
2026-06-12 18:08:54 +00:00 · 2026-03-02 12:22:02 +08:00
parent b26337501c
commit d5370c9605
7 changed files with 318 additions and 62 deletions
@@ -7,6 +7,8 @@ import (
 	"strings"
 	"testing"
 	"time"
+
+	"github.com/sipeed/picoclaw/pkg/config"
 )

 // TestShellTool_Success verifies successful command execution
@@ -309,3 +311,115 @@ func TestShellTool_RestrictToWorkspace(t *testing.T) {
 		)
 	}
 }
+
+// TestShellTool_DevNullAllowed verifies that /dev/null redirections are not blocked (issue #964).
+func TestShellTool_DevNullAllowed(t *testing.T) {
+	tmpDir := t.TempDir()
+	tool, err := NewExecTool(tmpDir, true)
+	if err != nil {
+		t.Fatalf("unable to configure exec tool: %s", err)
+	}
+
+	commands := []string{
+		"echo hello 2>/dev/null",
+		"echo hello >/dev/null",
+		"echo hello > /dev/null",
+		"echo hello 2> /dev/null",
+		"echo hello >/dev/null 2>&1",
+		"find " + tmpDir + " -name '*.go' 2>/dev/null",
+	}
+
+	for _, cmd := range commands {
+		result := tool.Execute(context.Background(), map[string]any{"command": cmd})
+		if result.IsError && strings.Contains(result.ForLLM, "blocked") {
+			t.Errorf("command should not be blocked: %s\n  error: %s", cmd, result.ForLLM)
+		}
+	}
+}
+
+// TestShellTool_BlockDevices verifies that writes to block devices are blocked (issue #965).
+func TestShellTool_BlockDevices(t *testing.T) {
+	tool, err := NewExecTool("", false)
+	if err != nil {
+		t.Fatalf("unable to configure exec tool: %s", err)
+	}
+
+	blocked := []string{
+		"echo x > /dev/sda",
+		"echo x > /dev/hda",
+		"echo x > /dev/vda",
+		"echo x > /dev/xvda",
+		"echo x > /dev/nvme0n1",
+		"echo x > /dev/mmcblk0",
+		"echo x > /dev/loop0",
+		"echo x > /dev/dm-0",
+		"echo x > /dev/md0",
+		"echo x > /dev/sr0",
+		"echo x > /dev/nbd0",
+	}
+
+	for _, cmd := range blocked {
+		result := tool.Execute(context.Background(), map[string]any{"command": cmd})
+		if !result.IsError {
+			t.Errorf("expected block device write to be blocked: %s", cmd)
+		}
+	}
+}
+
+// TestShellTool_SafePathsInWorkspaceRestriction verifies that safe kernel pseudo-devices
+// are allowed even when workspace restriction is active.
+func TestShellTool_SafePathsInWorkspaceRestriction(t *testing.T) {
+	tmpDir := t.TempDir()
+	tool, err := NewExecTool(tmpDir, true)
+	if err != nil {
+		t.Fatalf("unable to configure exec tool: %s", err)
+	}
+
+	// These reference paths outside workspace but should be allowed via safePaths.
+	commands := []string{
+		"cat /dev/urandom | head -c 16 | od",
+		"echo test > /dev/null",
+		"dd if=/dev/zero bs=1 count=1",
+	}
+
+	for _, cmd := range commands {
+		result := tool.Execute(context.Background(), map[string]any{"command": cmd})
+		if result.IsError && strings.Contains(result.ForLLM, "path outside working dir") {
+			t.Errorf("safe path should not be blocked by workspace check: %s\n  error: %s", cmd, result.ForLLM)
+		}
+	}
+}
+
+// TestShellTool_CustomAllowPatterns verifies that custom allow patterns exempt
+// commands from deny pattern checks.
+func TestShellTool_CustomAllowPatterns(t *testing.T) {
+	cfg := &config.Config{
+		Tools: config.ToolsConfig{
+			Exec: config.ExecConfig{
+				EnableDenyPatterns:  true,
+				CustomAllowPatterns: []string{`\bgit\s+push\s+origin\b`},
+			},
+		},
+	}
+
+	tool, err := NewExecToolWithConfig("", false, cfg)
+	if err != nil {
+		t.Fatalf("unable to configure exec tool: %s", err)
+	}
+
+	// "git push origin main" should be allowed by custom allow pattern.
+	result := tool.Execute(context.Background(), map[string]any{
+		"command": "git push origin main",
+	})
+	if result.IsError && strings.Contains(result.ForLLM, "blocked") {
+		t.Errorf("custom allow pattern should exempt 'git push origin main', got: %s", result.ForLLM)
+	}
+
+	// "git push upstream main" should still be blocked (does not match allow pattern).
+	result = tool.Execute(context.Background(), map[string]any{
+		"command": "git push upstream main",
+	})
+	if !result.IsError {
+		t.Errorf("'git push upstream main' should still be blocked by deny pattern")
+	}
+}