feat(security): fix reviewer comments enhance risk classification and sandboxing for shell commands

Signed-off-by: Boris Bliznioukov <blib@mail.com>

docs/design/DDR-shell-tool-hardening.md

@@ -90,7 +90,11 @@
    | `high`     | Block          | Destructive, system-modifying (rm, chmod, git push)     |
    | `critical` | Block          | Privilege escalation, always dangerous (sudo, dd, eval) |
 
-5. The risk classifier MUST apply argument-aware modifiers (e.g., `curl` is `medium`, but `curl -X POST` is `high`; `git` is `medium`, but `git push` is `high`).
+   Shell interpreters (`sh`, `bash`, `zsh`, `dash`, `fish`, `ksh`, `csh`, `tcsh`, `powershell`, `pwsh`, `cmd`) MUST be classified as `critical` because they can execute arbitrary nested commands that bypass the risk classifier entirely (e.g., `sh -c 'rm -rf /'`).
+
+5. The risk classifier MUST apply argument-aware modifiers (e.g., `curl` is `medium`, but `curl -X POST` is `high`; `git` is `medium`, but `git push` is `high`). All matching modifiers MUST be scanned and the highest level that exceeds the base level is applied (highest-match-wins, not first-match-wins).
+
+5a. `risk_overrides` sets the **base level** for a command (replacing the built-in table entry). Argument modifiers MUST still be applied on top of the overridden base level and can elevate it further. This means `risk_overrides: {"rm": "medium"}` allows plain `rm` but `rm -rf` is still elevated to `critical` by the built-in modifier.
 
 6. When a command is blocked, the `ToolResult` MUST include:
    - Risk level of the command.

docs/tools_configuration.md

@@ -51,9 +51,9 @@ AST-based risk classification, environment sanitization, and file-access sandbox
 ### Configuration
 
 | Config           | Type   | Default    | Description                                                                     |
-| ---------------- | ------ | ---------- | ----------------------------------------------------------------------- |
+| ---------------- | ------ | ---------- | ------------------------------------------------------------------------------- |
 | `risk_threshold` | string | `"medium"` | Maximum allowed risk level: `"low"`, `"medium"`, `"high"`, `"critical"`         |
-| `risk_overrides` | object | `{}`       | Per-command risk level overrides (command name → level)                 |
+| `risk_overrides` | object | `{}`       | Per-command base risk level (command name → level); modifiers can still elevate |
 | `arg_modifiers`  | object | `{}`       | Per-command argument patterns that adjust risk level                            |
 | `env_allowlist`  | array  | `[]`       | Extra environment variables to expose (extends built-in defaults)               |
 | `env_set`        | object | `{}`       | Explicit `VAR=value` pairs injected into every command                          |
@@ -91,6 +91,24 @@ must all be present (order-independent) and the resulting level:
 
 The **highest matching** modifier wins (built-in and custom are merged).
 
+#### Precedence
+
+The final risk level is computed as:
+
+1. **Base level**: `risk_overrides` entry if present, else built-in table, else `medium`.
+2. **Modifiers**: All matching argument modifiers (built-in + custom) are scanned.
+   The highest level that exceeds the base is applied. Modifiers can only elevate,
+   never lower.
+
+This means `"risk_overrides": {"rm": "medium"}` allows plain `rm` at the `medium`
+threshold, but `rm -rf` is still elevated to `critical` by the built-in modifier.
+
+#### Shell wrappers
+
+Shell interpreters (`sh`, `bash`, `zsh`, `dash`, `fish`, `ksh`, `csh`, `tcsh`,
+`powershell`, `pwsh`, `cmd`) are classified as `critical` because they can execute
+arbitrary nested commands that bypass the risk classifier (e.g., `sh -c 'rm -rf /'`).
+
 ### Environment Sanitization
 
 The shell interpreter runs with a sanitized environment. Only a safe allowlist

pkg/tools/shell/risk.go

@@ -1,6 +1,9 @@
 package shell
 
-import "fmt"
+import (
+	"fmt"
+	"path/filepath"
+)
 
 // RiskLevel represents the potential danger of a shell command.
 type RiskLevel int
@@ -203,6 +206,21 @@ var commandRiskTable = map[string]RiskLevel{
 	".":        RiskCritical,
 	"format":   RiskCritical,
 	"diskpart": RiskCritical,
+
+	// Critical — shell wrappers can execute arbitrary nested commands,
+	// bypassing the risk classifier entirely (e.g. sh -c 'rm -rf /').
+	"sh":         RiskCritical,
+	"bash":       RiskCritical,
+	"zsh":        RiskCritical,
+	"dash":       RiskCritical,
+	"fish":       RiskCritical,
+	"csh":        RiskCritical,
+	"tcsh":       RiskCritical,
+	"ksh":        RiskCritical,
+	"powershell": RiskCritical,
+	"pwsh":       RiskCritical,
+	"cmd":        RiskCritical,
+	"cmd.exe":    RiskCritical,
 }
 
 // ArgModifier describes a condition that elevates a command's risk level.
@@ -214,7 +232,7 @@ type ArgModifier struct {
 }
 
 // argumentModifiers maps command names to their argument-aware risk adjustments.
-// Checked in order; first match wins.
+// All matching modifiers are scanned; the highest level wins.
 //
 // Patterns use individual flags (e.g., "-r", "-f") rather than combined forms
 // ("-rf") because normalizeFlags splits combined flags before matching. This
@@ -291,9 +309,17 @@ var argumentModifiers = map[string][]ArgModifier{
 
 // ClassifyCommand determines the risk level of a resolved command.
 // args[0] is the command name (basename), args[1:] are the arguments.
-// overrides allows per-command level overrides from config.
-// extraModifiers are checked after built-in argumentModifiers.
-// The highest matching level across all sources wins.
+//
+// Precedence (highest wins):
+//  1. Argument modifiers (built-in, then user-supplied extraModifiers) —
+//     all matching modifiers are scanned; the maximum level is kept.
+//  2. risk_overrides from config — sets the base level for the command,
+//     replacing the built-in table entry. Modifiers can still elevate above it.
+//  3. Built-in commandRiskTable — default base level per command.
+//  4. Commands not in any table default to RiskMedium.
+//
+// This means risk_overrides: {"rm": "medium"} allows plain `rm` but
+// `rm -rf` is still elevated to critical by the built-in modifier.
 func ClassifyCommand(args []string, overrides map[string]string, extraModifiers ...map[string][]ArgModifier) RiskLevel {
 	if len(args) == 0 {
 		return RiskMedium
@@ -301,60 +327,59 @@ func ClassifyCommand(args []string, overrides map[string]string, extraModifiers
 
 	cmdName := baseCommand(args[0])
 
-	if overrides != nil {
-		if levelStr, ok := overrides[cmdName]; ok {
-			level, err := ParseRiskLevel(levelStr)
-			if err == nil {
-				return level
-			}
-			// Invalid risk level in override: fall through to default classification.
-			// The parse error is descriptive, but we can't log from here without
-			// injecting a logger. Config-time validation catches user errors.
-		}
-	}
-
+	// Determine base level: override > table > medium default.
 	level, known := commandRiskTable[cmdName]
 	if !known {
 		level = RiskMedium
 	}
+	if overrides != nil {
+		if levelStr, ok := overrides[cmdName]; ok {
+			parsed, err := ParseRiskLevel(levelStr)
+			if err == nil {
+				level = parsed
+			}
+			// Invalid risk level in override: keep table/default level.
+			// Config-time validation catches user errors.
+		}
+	}
 
 	// Normalize args: expand combined short flags like -rf → -r, -f
 	// so that modifiers match regardless of how flags were grouped or ordered.
 	normalizedArgs := normalizeFlags(args[1:])
 
-	// Check built-in modifiers, then user-supplied. Keep the highest match.
-	if elevated, ok := applyModifiers(normalizedArgs, cmdName, level, argumentModifiers); ok {
-		level = elevated
-	}
+	// Apply built-in modifiers, then user-supplied. Keep the highest match
+	// across all sources. Modifiers can only elevate, never lower.
+	level = applyModifiers(normalizedArgs, cmdName, level, argumentModifiers)
 	for _, extra := range extraModifiers {
 		if extra == nil {
 			continue
 		}
-		if elevated, ok := applyModifiers(normalizedArgs, cmdName, level, extra); ok {
-			level = elevated
-		}
+		level = applyModifiers(normalizedArgs, cmdName, level, extra)
 	}
 
 	return level
 }
 
-// applyModifiers checks whether any modifier for cmdName matches the
-// normalised args and would elevate the risk. Returns (newLevel, true)
-// on first match, or (0, false) if nothing matched.
+// applyModifiers scans all modifiers for cmdName, matches them against
+// normalizedArgs, and returns the highest level that exceeds baseLevel.
+// If no modifier elevates, returns baseLevel unchanged.
 func applyModifiers(
 	normalizedArgs []string,
 	cmdName string,
 	baseLevel RiskLevel,
 	mods map[string][]ArgModifier,
-) (RiskLevel, bool) {
-	if entries, ok := mods[cmdName]; ok {
+) RiskLevel {
+	entries, ok := mods[cmdName]
+	if !ok {
+		return baseLevel
+	}
+	result := baseLevel
 	for _, mod := range entries {
-			if matchArgs(normalizedArgs, mod.Args) && mod.Level > baseLevel {
-				return mod.Level, true
+		if matchArgs(normalizedArgs, mod.Args) && mod.Level > result {
+			result = mod.Level
 		}
 	}
-	}
-	return 0, false
+	return result
 }
 
 // IsAllowed returns true if the given risk level is at or below the threshold.
@@ -388,13 +413,10 @@ func BlockedCommandError(args []string, level, threshold RiskLevel, reason strin
 }
 
 // baseCommand extracts the basename from a command path.
+// Uses filepath.Base so both forward slashes and Windows backslashes
+// are handled correctly.
 func baseCommand(cmd string) string {
-	for i := len(cmd) - 1; i >= 0; i-- {
-		if cmd[i] == '/' {
-			return cmd[i+1:]
-		}
-	}
-	return cmd
+	return filepath.Base(cmd)
 }
 
 // normalizeFlags expands combined short flags (e.g., "-rf" → "-r", "-f")

pkg/tools/shell/risk_test.go

@@ -97,26 +97,53 @@ func TestClassifyCommand_ArgumentModifiers(t *testing.T) {
 
 func TestClassifyCommand_Overrides(t *testing.T) {
 	overrides := map[string]string{
-		"rm":   "low",
+		"rm":   "medium",
 		"curl": "critical",
 	}
 
+	// Override sets the BASE level, but modifiers still elevate.
+	// rm is overridden to medium, but rm -rf triggers the built-in
+	// modifier that elevates to critical.
 	got := ClassifyCommand([]string{"rm", "-rf", "/"}, overrides)
-	if got != RiskLow {
-		t.Errorf("override rm to low: got %s", got)
+	if got != RiskCritical {
+		t.Errorf("override rm to medium + rm -rf modifier should be critical: got %s", got)
 	}
 
+	// Plain rm (no -rf) stays at the overridden level.
+	got = ClassifyCommand([]string{"rm", "file.txt"}, overrides)
+	if got != RiskMedium {
+		t.Errorf("override rm to medium (no modifier match): got %s", got)
+	}
+
+	// Override elevates curl to critical unconditionally.
 	got = ClassifyCommand([]string{"curl", "https://example.com"}, overrides)
 	if got != RiskCritical {
 		t.Errorf("override curl to critical: got %s", got)
 	}
 
+	// No override for ls — uses table as before.
 	got = ClassifyCommand([]string{"ls"}, overrides)
 	if got != RiskLow {
 		t.Errorf("ls (no override) should be low: got %s", got)
 	}
 }
 
+func TestClassifyCommand_OverrideLowers_ModifierStillElevates(t *testing.T) {
+	// Scenario: user sets rm to low ("I want rm allowed"), but rm -rf
+	// still hits the built-in modifier → critical.
+	overrides := map[string]string{"rm": "low"}
+
+	got := ClassifyCommand([]string{"rm", "file.txt"}, overrides)
+	if got != RiskLow {
+		t.Errorf("plain rm with override=low should be low: got %s", got)
+	}
+
+	got = ClassifyCommand([]string{"rm", "-rf", "/"}, overrides)
+	if got != RiskCritical {
+		t.Errorf("rm -rf should still be critical despite override=low: got %s", got)
+	}
+}
+
 func TestClassifyCommand_UnknownCommand(t *testing.T) {
 	got := ClassifyCommand([]string{"some_unknown_tool", "--flag"}, nil)
 	if got != RiskMedium {
@@ -136,6 +163,25 @@ func TestClassifyCommand_FullPath(t *testing.T) {
 	}
 }
 
+func TestClassifyCommand_BackslashPath(t *testing.T) {
+	// Forward-slash paths at various depths.
+	got := ClassifyCommand([]string{"/usr/sbin/shutdown", "-h"}, nil)
+	if got != RiskCritical {
+		t.Errorf("/usr/sbin/shutdown should be critical, got %s", got)
+	}
+
+	got = ClassifyCommand([]string{"/usr/local/bin/sudo", "ls"}, nil)
+	if got != RiskCritical {
+		t.Errorf("/usr/local/bin/sudo should be critical, got %s", got)
+	}
+
+	// Bare command still works after the filepath.Base change.
+	got = ClassifyCommand([]string{"dd", "if=/dev/zero"}, nil)
+	if got != RiskCritical {
+		t.Errorf("bare dd should be critical, got %s", got)
+	}
+}
+
 func TestIsAllowed(t *testing.T) {
 	tests := []struct {
 		level     RiskLevel
@@ -283,7 +329,8 @@ func TestClassifyCommand_ExtraArgModifiers(t *testing.T) {
 
 func TestClassifyCommand_ExtraArgModifiers_NoOverrideBuiltIn(t *testing.T) {
 	// Extra modifier tries to set "rm -rf" to medium, but built-in already
-	// elevates to critical and built-in is checked first.
+	// elevates to critical. Since we take the max across all matching
+	// modifiers, the built-in critical wins.
 	extra := map[string][]ArgModifier{
 		"rm": {
 			{Args: []string{"-r", "-f"}, Level: RiskMedium},
@@ -295,3 +342,53 @@ func TestClassifyCommand_ExtraArgModifiers_NoOverrideBuiltIn(t *testing.T) {
 		t.Errorf("built-in should win over extra for rm -rf: got %s", got)
 	}
 }
+
+func TestClassifyCommand_ShellWrappers(t *testing.T) {
+	// Shell wrappers must be critical to prevent classifier bypass.
+	shells := []string{"sh", "bash", "zsh", "dash", "fish", "ksh", "csh", "tcsh", "powershell", "pwsh", "cmd", "cmd.exe"}
+	for _, sh := range shells {
+		t.Run(sh, func(t *testing.T) {
+			got := ClassifyCommand([]string{sh, "-c", "echo hi"}, nil)
+			if got != RiskCritical {
+				t.Errorf("%s should be critical, got %s", sh, got)
+			}
+		})
+	}
+}
+
+func TestClassifyCommand_ShellWrapperFullPath(t *testing.T) {
+	// /bin/sh, /usr/bin/bash etc. should also be caught via baseCommand.
+	got := ClassifyCommand([]string{"/bin/sh", "-c", "rm -rf /"}, nil)
+	if got != RiskCritical {
+		t.Errorf("/bin/sh should be critical, got %s", got)
+	}
+
+	got = ClassifyCommand([]string{"/usr/bin/bash", "-c", "sudo rm -rf /"}, nil)
+	if got != RiskCritical {
+		t.Errorf("/usr/bin/bash should be critical, got %s", got)
+	}
+}
+
+func TestApplyModifiers_HighestMatchWins(t *testing.T) {
+	// When multiple modifiers match, the highest level should win.
+	// Scenario: git push matches both ["push"] → High and ["push", "-f"] → Critical
+	args := normalizeFlags([]string{"push", "-f", "origin"})
+	result := applyModifiers(args, "git", RiskMedium, argumentModifiers)
+	if result != RiskCritical {
+		t.Errorf("git push -f should resolve to critical (highest match), got %s", result)
+	}
+
+	// Only ["push"] matches → High
+	args2 := normalizeFlags([]string{"push", "origin"})
+	result2 := applyModifiers(args2, "git", RiskMedium, argumentModifiers)
+	if result2 != RiskHigh {
+		t.Errorf("git push (no -f) should resolve to high, got %s", result2)
+	}
+
+	// No modifier matches → base level unchanged
+	args3 := normalizeFlags([]string{"status"})
+	result3 := applyModifiers(args3, "git", RiskMedium, argumentModifiers)
+	if result3 != RiskMedium {
+		t.Errorf("git status should stay medium, got %s", result3)
+	}
+}

pkg/tools/shell/sandbox.go

@@ -45,10 +45,17 @@ func SandboxedOpenHandler(workspaceDir string) interp.OpenHandlerFunc {
 			return os.OpenFile(path, flag, perm)
 		}
 
-		absPath, err := filepath.Abs(path)
-		if err != nil {
-			return nil, fmt.Errorf("sandbox: cannot resolve path %q: %w", path, err)
+		// Resolve relative paths against the interpreter's working directory,
+		// not the process CWD. The interpreter tracks its own CWD via
+		// interp.Dir() and internal cd commands without calling os.Chdir().
+		var absPath string
+		if filepath.IsAbs(path) {
+			absPath = path
+		} else {
+			hctx := interp.HandlerCtx(ctx)
+			absPath = filepath.Join(hctx.Dir, path)
 		}
+		// filepath.Join already returns a clean path; no extra Abs needed.
 
 		// Resolve symlinks to prevent escape.
 		// If the file doesn't exist yet, resolve the parent.

pkg/tools/shell/sandbox_test.go

@@ -4,7 +4,9 @@ import (
 	"context"
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
+	"time"
 )
 
 func TestSandboxedOpenHandler_AllowsInsideWorkspace(t *testing.T) {
@@ -80,6 +82,71 @@ func TestSandboxedOpenHandler_AllowsNewFileInWorkspace(t *testing.T) {
 	os.Remove(newFile)
 }
 
+// TestSandboxedOpenHandler_RelativePathUsesInterpreterCwd verifies that
+// relative paths in shell redirections resolve against the interpreter's
+// working directory (from interp.HandlerCtx), not the process CWD.
+func TestSandboxedOpenHandler_RelativePathUsesInterpreterCwd(t *testing.T) {
+	workspace := t.TempDir()
+
+	// Write to a relative path inside the workspace via the interpreter.
+	// The interpreter's Dir is set to workspace, so "output.txt" should
+	// resolve to workspace/output.txt regardless of the process CWD.
+	result := Run(context.Background(), RunConfig{
+		Command:       "echo sandbox_relative > output.txt",
+		Dir:           workspace,
+		Timeout:       5 * time.Second,
+		Restrict:      true,
+		WorkspaceDir:  workspace,
+		RiskThreshold: RiskMedium,
+	})
+
+	if result.IsError {
+		t.Fatalf("relative redirect inside workspace should succeed: %s", result.Output)
+	}
+
+	content, err := os.ReadFile(filepath.Join(workspace, "output.txt"))
+	if err != nil {
+		t.Fatalf("output.txt should exist in workspace: %v", err)
+	}
+	if !strings.Contains(string(content), "sandbox_relative") {
+		t.Errorf("expected 'sandbox_relative' in file, got: %s", content)
+	}
+}
+
+// TestSandboxedOpenHandler_RelativePathBlocksEscapeViaCd verifies that
+// if a script uses cd to move outside the workspace, a subsequent relative
+// redirect is blocked by the sandbox.
+func TestSandboxedOpenHandler_RelativePathBlocksEscapeViaCd(t *testing.T) {
+	root := t.TempDir()
+	workspace := filepath.Join(root, "workspace")
+	outside := filepath.Join(root, "outside")
+	os.MkdirAll(workspace, 0o755)
+	os.MkdirAll(outside, 0o755)
+
+	// cd to outside dir, then try to write a relative path.
+	// The sandbox should block because the resolved path is outside workspace.
+	result := Run(context.Background(), RunConfig{
+		Command:       "cd " + outside + " && echo escaped > leak.txt",
+		Dir:           workspace,
+		Timeout:       5 * time.Second,
+		Restrict:      true,
+		WorkspaceDir:  workspace,
+		RiskThreshold: RiskHigh, // allow cd
+	})
+
+	if !result.IsError {
+		// If it didn't error, check that the file was NOT written outside
+		if _, err := os.Stat(filepath.Join(outside, "leak.txt")); err == nil {
+			t.Fatal("sandbox should have blocked write outside workspace")
+		}
+	}
+
+	// Verify nothing leaked
+	if _, err := os.Stat(filepath.Join(outside, "leak.txt")); err == nil {
+		t.Error("leak.txt should not exist outside workspace")
+	}
+}
+
 // TestSandboxedOpenHandler_AllowsDottedFiles verifies that files with
 // names starting with ".." (like ".../file", "....txt", "..something")
 // are NOT incorrectly blocked by the escape check.

pkg/tools/shell_tool.go

@@ -71,11 +71,6 @@ func NewExecToolWithConfig(workingDir string, restrict bool, cfg *config.Config)
 }
 
 func warnDeprecatedExecConfig(cfg config.ExecConfig) {
-	if !cfg.EnableDenyPatterns {
-		fmt.Println("Warning: 'enable_deny_patterns' is deprecated and ignored. " +
-			"The new shell tool uses AST-based risk classification. " +
-			"Use 'risk_threshold' to control command blocking.")
-	}
 	if len(cfg.CustomDenyPatterns) > 0 {
 		fmt.Println("Warning: 'custom_deny_patterns' is deprecated and ignored. " +
 			"Use 'risk_overrides' to adjust per-command risk levels.")