diff --git a/pkg/tools/shell.go b/pkg/tools/shell.go index 9a46626ce..75029e41f 100644 --- a/pkg/tools/shell.go +++ b/pkg/tools/shell.go @@ -74,21 +74,25 @@ var ( regexp.MustCompile(`\bssh\b.*@`), regexp.MustCompile(`\beval\b`), regexp.MustCompile(`\bsource\s+.*\.sh\b`), - // file:// protocol allows reading local files outside workspace + // file:// protocol allows reading local files outside workspace. + // Note: deny patterns are skipped when CustomAllowPatterns match, + // so configuring custom allow patterns may disable this protection. regexp.MustCompile(`\bfile://`), - // Sensitive /proc paths that may expose credentials or process info - regexp.MustCompile(`/proc/self/environ\b`), - regexp.MustCompile(`/proc/self/cmdline\b`), - regexp.MustCompile(`/proc/self/fd/\d+`), - regexp.MustCompile(`/proc/self/mem\b`), - regexp.MustCompile(`/proc/self/maps\b`), - regexp.MustCompile(`/proc/\d+/environ\b`), + // Sensitive /proc paths that may expose credentials or process info. + // Prefix constraint prevents false positives from URLs containing /proc. + regexp.MustCompile(`(?:^|[\s=\"])/proc/self/environ\b`), + regexp.MustCompile(`(?:^|[\s=\"])/proc/self/cmdline\b`), + regexp.MustCompile(`(?:^|[\s=\"])/proc/self/fd/\d+`), + regexp.MustCompile(`(?:^|[\s=\"])/proc/self/mem\b`), + regexp.MustCompile(`(?:^|[\s=\"])/proc/self/maps\b`), + regexp.MustCompile(`(?:^|[\s=\"])/proc/\d+/environ\b`), } // absolutePathPattern matches absolute file paths (Unix and Windows). - // Unix paths must be preceded by whitespace, '=', '"' or start of string - // to avoid matching paths inside relative paths like "./download/file". - absolutePathPattern = regexp.MustCompile(`(?:^|[\s="])(/[^\s\"']+)|([A-Za-z]:\\[^\\\"']+)`) + // Unix paths must be preceded by whitespace, '=', '"', "'", a short flag + // (e.g., "-o"), or start of string to avoid matching paths inside URLs + // or relative paths like "./download/file". + absolutePathPattern = regexp.MustCompile(`(?:^|[\s=\"']|-[A-Za-z]*)(/[^\s\"']+)|([A-Za-z]:\\[^\\\"']+)`) // urlPattern matches URLs (http://, https://, ftp://, sftp://, git+https://, etc.) urlPattern = regexp.MustCompile(`(?i)[a-z][a-z0-9+.-]*://[^\s\"']+`) @@ -346,7 +350,10 @@ func (t *ExecTool) guardCommand(command, cwd string) string { } if t.restrictToWorkspace { - if strings.Contains(cmd, "..\\") || strings.Contains(cmd, "../") { + // Remove URLs before checking path traversal to avoid false positives + // from URLs like https://example.com/a/../b + cmdWithoutURLs := urlPattern.ReplaceAllString(cmd, "") + if strings.Contains(cmdWithoutURLs, "..\\") || strings.Contains(cmdWithoutURLs, "../") { return "Command blocked by safety guard (path traversal detected)" } @@ -355,9 +362,6 @@ func (t *ExecTool) guardCommand(command, cwd string) string { return "" } - // Remove URLs before matching paths to avoid treating https://github.com as a file path - cmdWithoutURLs := urlPattern.ReplaceAllString(cmd, "") - submatches := absolutePathPattern.FindAllStringSubmatch(cmdWithoutURLs, -1) for _, match := range submatches { raw := match[1] // Unix path diff --git a/pkg/tools/shell_test.go b/pkg/tools/shell_test.go index c20a0cd92..9642819f7 100644 --- a/pkg/tools/shell_test.go +++ b/pkg/tools/shell_test.go @@ -453,48 +453,54 @@ func TestShellTool_RemoteURLsAllowed(t *testing.T) { t.Fatalf("unable to configure exec tool: %s", err) } - // Commands with remote URLs should not be blocked by path check. + // Use harmless echo commands that still contain the same URL patterns, + // so we exercise the guard logic without performing network operations. allowed := []string{ - "curl https://github.com/user/repo", - "wget http://example.com/file.tar.gz", - "git clone https://github.com/user/repo.git", - "agent-browser open https://github.com", - "pip install git+https://github.com/user/pkg", - "curl ftp://ftp.example.com/file", - "curl sftp://server.com/path/file", - "curl HTTPS://github.com/user/repo", // case insensitive - "curl HTTP://example.com/file", // case insensitive + "echo https://github.com/user/repo", + "echo http://example.com/file.tar.gz", + "echo https://github.com/user/repo.git", + "echo https://github.com", + "echo git+https://github.com/user/pkg", + "echo ftp://ftp.example.com/file", + "echo sftp://server.com/path/file", + "echo HTTPS://github.com/user/repo", // case insensitive + "echo HTTP://example.com/file", // case insensitive + // URL with path traversal should not be blocked (it's in the URL) + "echo https://example.com/a/../b", + "echo https://example.com/proc/self/environ", } for _, cmd := range allowed { result := tool.Execute(context.Background(), map[string]any{"command": cmd}) - if result.IsError && strings.Contains(result.ForLLM, "path outside working dir") { - t.Errorf("URL command should not be blocked by path check: %s\n error: %s", cmd, result.ForLLM) + // URL commands should not be blocked by the workspace/path guard. + if result.IsError { + t.Errorf("URL command should not fail or be blocked: %s\n error: %s", cmd, result.ForLLM) } } } -// TestShellTool_RemotePathInSSHAllowed verifies that remote paths in SSH/SCP commands -// are not blocked (they refer to remote systems, not local files). -func TestShellTool_RemotePathInSSHAllowed(t *testing.T) { +// TestShellTool_QuotedAndFlagPathsBlocked verifies that paths with quotes +// or after short flags are properly detected and blocked. +func TestShellTool_QuotedAndFlagPathsBlocked(t *testing.T) { tmpDir := t.TempDir() tool, err := NewExecTool(tmpDir, true) if err != nil { t.Fatalf("unable to configure exec tool: %s", err) } - // SSH/SCP with remote paths - the /path/file is on the remote host. - // Note: "ssh user@host" is blocked by deny pattern, so we test the path logic - // only by checking that /path/file in scp/rsync syntax is not blocked. - allowed := []string{ - "rsync /local/file ./dest", // local absolute path in workspace check - "rsync ./src/ /tmp/", // /tmp/ should be blocked + // Paths with single quotes or after short flags should be blocked. + blocked := []string{ + "cat '/etc/passwd'", + "cat \"/etc/passwd\"", + "curl -o/tmp/file https://example.com", + "tar -C/etc -xf archive.tar", } - for _, cmd := range allowed { + for _, cmd := range blocked { result := tool.Execute(context.Background(), map[string]any{"command": cmd}) - // We just verify no crash and proper handling - _ = result + if !result.IsError { + t.Errorf("command with path outside workspace should be blocked: %s", cmd) + } } }