fix(security): replace regex path detection with token-based analysis in exec guard
The old regex `/[^\s\"']+` falsely matched slashes in relative paths (e.g., "tests/cold/file.py" → "/cold/file.py") as absolute paths, blocking legitimate commands. Windows path regex also truncated at the first backslash segment. Replace with strings.Fields + filepath.IsAbs for accurate absolute path detection, and add isExecutable() check to allow system binary invocation (e.g., /usr/bin/python, .exe) outside the workspace while still blocking data file access. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
dj-oyu
parent
c73a229a1e
commit
d0c683aa4a
2 changed files with 249 additions and 5 deletions
|
|
@ -256,21 +256,29 @@ func (t *ExecTool) guardCommand(command, cwd string) string {
|
|||
return ""
|
||||
}
|
||||
|
||||
pathPattern := regexp.MustCompile(`[A-Za-z]:\\[^\\\"']+|/[^\s\"']+`)
|
||||
matches := pathPattern.FindAllString(cmd, -1)
|
||||
// Token-based absolute path detection.
|
||||
// Uses strings.Fields instead of regex to avoid false positives
|
||||
// from slashes in relative paths (e.g., "tests/cold/file.py").
|
||||
// Flags like -I/usr/local/include are naturally skipped because
|
||||
// filepath.IsAbs returns false for tokens starting with "-".
|
||||
for _, token := range strings.Fields(cmd) {
|
||||
token = strings.Trim(token, "\"'")
|
||||
|
||||
for _, raw := range matches {
|
||||
p, err := filepath.Abs(raw)
|
||||
if err != nil {
|
||||
if !filepath.IsAbs(token) {
|
||||
continue
|
||||
}
|
||||
|
||||
p := filepath.Clean(token)
|
||||
rel, err := filepath.Rel(cwdPath, p)
|
||||
if err != nil {
|
||||
continue
|
||||
}
|
||||
|
||||
if strings.HasPrefix(rel, "..") {
|
||||
// Path is outside workspace — allow if it's an executable binary
|
||||
if isExecutable(p) {
|
||||
continue
|
||||
}
|
||||
return "Command blocked by safety guard (path outside working dir)"
|
||||
}
|
||||
}
|
||||
|
|
@ -279,6 +287,28 @@ func (t *ExecTool) guardCommand(command, cwd string) string {
|
|||
return ""
|
||||
}
|
||||
|
||||
// isExecutable checks if a path points to an executable file.
|
||||
// On Unix, checks the execute permission bits.
|
||||
// On Windows, checks for known executable extensions.
|
||||
func isExecutable(path string) bool {
|
||||
info, err := os.Stat(path)
|
||||
if err != nil {
|
||||
return false
|
||||
}
|
||||
if info.IsDir() {
|
||||
return false
|
||||
}
|
||||
if runtime.GOOS == "windows" {
|
||||
ext := strings.ToLower(filepath.Ext(path))
|
||||
switch ext {
|
||||
case ".exe", ".cmd", ".bat", ".ps1", ".com":
|
||||
return true
|
||||
}
|
||||
return false
|
||||
}
|
||||
return info.Mode()&0111 != 0
|
||||
}
|
||||
|
||||
func (t *ExecTool) SetTimeout(timeout time.Duration) {
|
||||
t.timeout = timeout
|
||||
}
|
||||
|
|
|
|||
|
|
@ -4,6 +4,7 @@ import (
|
|||
"context"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"runtime"
|
||||
"strings"
|
||||
"testing"
|
||||
"time"
|
||||
|
|
@ -208,3 +209,216 @@ func TestShellTool_RestrictToWorkspace(t *testing.T) {
|
|||
t.Errorf("Expected 'blocked' message for path traversal, got ForLLM: %s, ForUser: %s", result.ForLLM, result.ForUser)
|
||||
}
|
||||
}
|
||||
|
||||
// --- guardCommand unit tests ---
|
||||
|
||||
// TestGuardCommand_RelativePathWithSlashes verifies that relative paths
|
||||
// containing slashes (e.g., tests/cold/test.py, projects/terra-py-form)
|
||||
// are NOT falsely blocked. This was a regression caused by the old regex
|
||||
// matching "/cold/test.py" from "tests/cold/test.py" as an absolute path.
|
||||
func TestGuardCommand_RelativePathWithSlashes(t *testing.T) {
|
||||
workspace := t.TempDir()
|
||||
tool := NewExecTool(workspace, true)
|
||||
|
||||
cmds := []string{
|
||||
"pytest tests/cold/test_solver.py -v --tb=short",
|
||||
"cd projects/terra-py-form && pytest",
|
||||
"uv run pytest tests/cold/test_solver.py -v --tb=short",
|
||||
"cat src/terra_py_form/cold/parser.py",
|
||||
"python src/main.py --config config/dev.json",
|
||||
}
|
||||
|
||||
for _, cmd := range cmds {
|
||||
result := tool.guardCommand(cmd, workspace)
|
||||
if result != "" {
|
||||
t.Errorf("Relative path should not be blocked: %q → %s", cmd, result)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// TestGuardCommand_VenvBinary verifies that .venv/bin/... paths are allowed
|
||||
// (they are relative paths, not absolute).
|
||||
func TestGuardCommand_VenvBinary(t *testing.T) {
|
||||
workspace := t.TempDir()
|
||||
tool := NewExecTool(workspace, true)
|
||||
|
||||
cmds := []string{
|
||||
".venv/bin/python -m pytest",
|
||||
".venv/bin/pytest tests/ -v",
|
||||
".venv/bin/pip install -e .",
|
||||
}
|
||||
|
||||
for _, cmd := range cmds {
|
||||
result := tool.guardCommand(cmd, workspace)
|
||||
if result != "" {
|
||||
t.Errorf("Venv relative path should not be blocked: %q → %s", cmd, result)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// TestGuardCommand_ExecutableBinaryAllowed verifies that absolute paths
|
||||
// to executable files outside the workspace are allowed (system binaries).
|
||||
func TestGuardCommand_ExecutableBinaryAllowed(t *testing.T) {
|
||||
if runtime.GOOS == "windows" {
|
||||
t.Skip("Unix executable permission test not applicable on Windows")
|
||||
}
|
||||
|
||||
workspace := t.TempDir()
|
||||
externalDir := t.TempDir()
|
||||
|
||||
// Create a fake executable outside the workspace
|
||||
execPath := filepath.Join(externalDir, "mybin")
|
||||
os.WriteFile(execPath, []byte("#!/bin/sh\necho ok"), 0755)
|
||||
|
||||
tool := NewExecTool(workspace, true)
|
||||
|
||||
cmd := execPath + " --help"
|
||||
result := tool.guardCommand(cmd, workspace)
|
||||
if result != "" {
|
||||
t.Errorf("Executable binary outside workspace should be allowed: %q → %s", cmd, result)
|
||||
}
|
||||
}
|
||||
|
||||
// TestGuardCommand_ExecutableBinaryAllowed_Windows verifies that .exe files
|
||||
// outside the workspace are allowed on Windows.
|
||||
func TestGuardCommand_ExecutableBinaryAllowed_Windows(t *testing.T) {
|
||||
if runtime.GOOS != "windows" {
|
||||
t.Skip("Windows-specific test")
|
||||
}
|
||||
|
||||
workspace := t.TempDir()
|
||||
externalDir := t.TempDir()
|
||||
|
||||
// Create a fake .exe outside the workspace
|
||||
execPath := filepath.Join(externalDir, "tool.exe")
|
||||
os.WriteFile(execPath, []byte("MZ"), 0644)
|
||||
|
||||
tool := NewExecTool(workspace, true)
|
||||
|
||||
cmd := execPath + " --version"
|
||||
result := tool.guardCommand(cmd, workspace)
|
||||
if result != "" {
|
||||
t.Errorf("Windows .exe outside workspace should be allowed: %q → %s", cmd, result)
|
||||
}
|
||||
}
|
||||
|
||||
// TestGuardCommand_NonExecutableOutsideBlocked verifies that non-executable
|
||||
// files outside the workspace are blocked (e.g., reading /etc/shadow).
|
||||
func TestGuardCommand_NonExecutableOutsideBlocked(t *testing.T) {
|
||||
if runtime.GOOS == "windows" {
|
||||
t.Skip("Unix permission test not applicable on Windows")
|
||||
}
|
||||
|
||||
workspace := t.TempDir()
|
||||
externalDir := t.TempDir()
|
||||
|
||||
// Create a regular (non-executable) file outside workspace
|
||||
dataFile := filepath.Join(externalDir, "secret.txt")
|
||||
os.WriteFile(dataFile, []byte("secret data"), 0644)
|
||||
|
||||
tool := NewExecTool(workspace, true)
|
||||
|
||||
cmd := "cat " + dataFile
|
||||
result := tool.guardCommand(cmd, workspace)
|
||||
if result == "" {
|
||||
t.Errorf("Non-executable file outside workspace should be blocked: %q", cmd)
|
||||
}
|
||||
if !strings.Contains(result, "path outside working dir") {
|
||||
t.Errorf("Expected 'path outside working dir' message, got: %s", result)
|
||||
}
|
||||
}
|
||||
|
||||
// TestGuardCommand_NonExistentAbsolutePathBlocked verifies that absolute
|
||||
// paths that don't exist are blocked (could be file creation outside workspace).
|
||||
func TestGuardCommand_NonExistentAbsolutePathBlocked(t *testing.T) {
|
||||
workspace := t.TempDir()
|
||||
tool := NewExecTool(workspace, true)
|
||||
|
||||
// Use platform-appropriate absolute path
|
||||
var cmd string
|
||||
if runtime.GOOS == "windows" {
|
||||
cmd = "echo hello > C:\\nonexistent_picoclaw_test_output"
|
||||
} else {
|
||||
cmd = "echo hello > /tmp/nonexistent_picoclaw_test_output"
|
||||
}
|
||||
result := tool.guardCommand(cmd, workspace)
|
||||
if result == "" {
|
||||
t.Errorf("Non-existent absolute path outside workspace should be blocked: %q", cmd)
|
||||
}
|
||||
}
|
||||
|
||||
// TestGuardCommand_FlagEmbeddedPathSkipped verifies that paths embedded in
|
||||
// flags (e.g., -I/usr/local/include) are NOT extracted as absolute paths
|
||||
// because the token starts with "-", not "/".
|
||||
func TestGuardCommand_FlagEmbeddedPathSkipped(t *testing.T) {
|
||||
workspace := t.TempDir()
|
||||
tool := NewExecTool(workspace, true)
|
||||
|
||||
cmds := []string{
|
||||
"gcc -I/usr/local/include -L/usr/lib main.c",
|
||||
"g++ -std=c++17 -I/opt/include file.cpp",
|
||||
"python --prefix=/usr/local script.py",
|
||||
}
|
||||
|
||||
for _, cmd := range cmds {
|
||||
result := tool.guardCommand(cmd, workspace)
|
||||
if result != "" {
|
||||
t.Errorf("Flag-embedded path should not be blocked: %q → %s", cmd, result)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// TestGuardCommand_AbsolutePathInsideWorkspace verifies that absolute paths
|
||||
// within the workspace are always allowed.
|
||||
func TestGuardCommand_AbsolutePathInsideWorkspace(t *testing.T) {
|
||||
workspace := t.TempDir()
|
||||
tool := NewExecTool(workspace, true)
|
||||
|
||||
innerDir := filepath.Join(workspace, "projects", "myapp")
|
||||
os.MkdirAll(innerDir, 0755)
|
||||
|
||||
cmd := "ls " + innerDir
|
||||
result := tool.guardCommand(cmd, workspace)
|
||||
if result != "" {
|
||||
t.Errorf("Absolute path inside workspace should be allowed: %q → %s", cmd, result)
|
||||
}
|
||||
}
|
||||
|
||||
// TestGuardCommand_PathTraversal verifies that various path traversal
|
||||
// patterns are blocked.
|
||||
func TestGuardCommand_PathTraversal(t *testing.T) {
|
||||
workspace := t.TempDir()
|
||||
tool := NewExecTool(workspace, true)
|
||||
|
||||
cmds := []string{
|
||||
"cat ../../etc/passwd",
|
||||
"cat ../../../etc/shadow",
|
||||
"ls projects/../../../../etc",
|
||||
}
|
||||
|
||||
for _, cmd := range cmds {
|
||||
result := tool.guardCommand(cmd, workspace)
|
||||
if result == "" {
|
||||
t.Errorf("Path traversal should be blocked: %q", cmd)
|
||||
}
|
||||
if !strings.Contains(result, "path traversal") {
|
||||
t.Errorf("Expected 'path traversal' message, got: %s", result)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// TestGuardCommand_CdWithAbsoluteWorkspacePath verifies that cd to an
|
||||
// absolute path within the workspace followed by other commands is allowed.
|
||||
func TestGuardCommand_CdWithAbsoluteWorkspacePath(t *testing.T) {
|
||||
workspace := t.TempDir()
|
||||
innerDir := filepath.Join(workspace, "projects", "foo")
|
||||
os.MkdirAll(innerDir, 0755)
|
||||
|
||||
tool := NewExecTool(workspace, true)
|
||||
|
||||
cmd := "cd " + innerDir + " && ls -la"
|
||||
result := tool.guardCommand(cmd, workspace)
|
||||
if result != "" {
|
||||
t.Errorf("cd to workspace subdir should be allowed: %q → %s", cmd, result)
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue