fix(tools): address copilot review feedback

- Fix absolutePathPattern to support single quotes and short flags
- Add comment about file:// and CustomAllowPatterns interaction
- Add prefix constraint to /proc patterns to avoid false positives from URLs
- Fix path traversal check to strip URLs first
- Replace real network commands with echo in tests
- Add TestShellTool_QuotedAndFlagPathsBlocked for new coverage
- Remove TestShellTool_RemotePathInSSHAllowed (no assertions)

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

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)
+		}
 	}
 }