From 78e3508b796b1a9bedc2fe74ddd8e34485a72611 Mon Sep 17 00:00:00 2001 From: Boris Bliznioukov Date: Wed, 4 Mar 2026 15:18:14 +0100 Subject: [PATCH] feat(tools): replace shell exec with AST-based risk classifier Replace regex deny-patterns with in-process POSIX interpreter (mvdan.cc/sh/v3) and four-tier risk classification. Add env sanitization, file-access sandboxing, and argument-aware risk modifiers. Cron commands now share the same security path. Signed-off-by: Boris Bliznioukov --- config/config.example.json | 9 +- docs/design/DDR-shell-tool-hardening.md | 277 +++++++++++++++ docs/tools_configuration.md | 94 +++-- go.mod | 1 + go.sum | 12 +- pkg/config/config.go | 21 +- pkg/config/defaults.go | 6 +- pkg/tools/cron_exec_test.go | 153 ++++++++ pkg/tools/shell.go | 383 -------------------- pkg/tools/shell/env.go | 108 ++++++ pkg/tools/shell/env_test.go | 112 ++++++ pkg/tools/shell/risk.go | 435 +++++++++++++++++++++++ pkg/tools/shell/risk_test.go | 297 ++++++++++++++++ pkg/tools/shell/runner.go | 210 +++++++++++ pkg/tools/shell/runner_test.go | 243 +++++++++++++ pkg/tools/shell/sandbox.go | 79 +++++ pkg/tools/shell/sandbox_test.go | 123 +++++++ pkg/tools/shell_process_unix.go | 32 -- pkg/tools/shell_process_windows.go | 27 -- pkg/tools/shell_test.go | 445 ------------------------ pkg/tools/shell_timeout_unix_test.go | 65 ---- pkg/tools/shell_tool.go | 228 ++++++++++++ pkg/tools/shell_tool_test.go | 145 ++++++++ 23 files changed, 2513 insertions(+), 992 deletions(-) create mode 100644 docs/design/DDR-shell-tool-hardening.md create mode 100644 pkg/tools/cron_exec_test.go delete mode 100644 pkg/tools/shell.go create mode 100644 pkg/tools/shell/env.go create mode 100644 pkg/tools/shell/env_test.go create mode 100644 pkg/tools/shell/risk.go create mode 100644 pkg/tools/shell/risk_test.go create mode 100644 pkg/tools/shell/runner.go create mode 100644 pkg/tools/shell/runner_test.go create mode 100644 pkg/tools/shell/sandbox.go create mode 100644 pkg/tools/shell/sandbox_test.go delete mode 100644 pkg/tools/shell_process_unix.go delete mode 100644 pkg/tools/shell_process_windows.go delete mode 100644 pkg/tools/shell_test.go delete mode 100644 pkg/tools/shell_timeout_unix_test.go create mode 100644 pkg/tools/shell_tool.go create mode 100644 pkg/tools/shell_tool_test.go diff --git a/config/config.example.json b/config/config.example.json index 2f643d41b..bab6eb2d1 100644 --- a/config/config.example.json +++ b/config/config.example.json @@ -340,10 +340,11 @@ } }, "exec": { - "enabled": true, - "enable_deny_patterns": true, - "custom_deny_patterns": null, - "custom_allow_patterns": null + "risk_threshold": "medium", + "risk_overrides": {}, + "arg_modifiers": {}, + "env_allowlist": [], + "env_set": {} }, "skills": { "enabled": true, diff --git a/docs/design/DDR-shell-tool-hardening.md b/docs/design/DDR-shell-tool-hardening.md new file mode 100644 index 000000000..3d5ed80cc --- /dev/null +++ b/docs/design/DDR-shell-tool-hardening.md @@ -0,0 +1,277 @@ +# DDR: Shell Tool Hardening — AST-Based Guard + Interpreter Execution + +| Field | Value | +| --------------- | --------------------- | +| **Status** | IMPLEMENTED | +| **Date** | 2026-03-04 | +| **Author** | — | +| **Review date** | 2026-06-04 (3 months) | + +--- + +## 1. Problem Frame + +- **System**: PicoClaw `ExecTool` (`pkg/tools/shell.go`) — the shell command execution tool exposed to the LLM agent. +- **Audience**: Maintainers, security reviewers, contributors. +- **Baseline rule**: Commands are guarded by ~40 regex patterns matched against the raw command string (`guardCommand()`). Execution is via `sh -c ` (Unix) or `powershell` (Windows). Environment is fully inherited (`cmd.Env` is never set). Directory restriction uses regex-based absolute-path detection. +- **Baseline config** (`ExecConfig`): + ```go + EnableDenyPatterns bool // default: true + CustomDenyPatterns []string // default: [] + CustomAllowPatterns []string // default: [] + ``` +- **The problem**: + 1. **Regex guards are trivially bypassable.** Variable indirection (`x=rm; $x -rf /`), command substitution (`` `echo rm` -rf / ``), quoting tricks, IFS manipulation, hex/octal encoding, and Unicode escapes all evade string-level pattern matching. The guard operates on surface syntax, not semantics. + 2. **Full environment inheritance.** Child processes receive every env var from the parent — `OPENAI_API_KEY`, `ANTHROPIC_API_KEY`, provider tokens, database credentials. An LLM-generated `curl` command can exfiltrate these. + 3. **No structured risk classification.** The system is binary (blocked / allowed) with no granularity, no feedback to the LLM about _why_ a command was blocked, and no user-configurable risk threshold. + 4. **Path validation is regex-based.** Absolute path detection via regex is incomplete and bypassable via symlinks, relative path tricks, and shell expansion. + +- **Proposed change**: Replace the regex guard and `sh -c` execution with (a) a proper shell parser (`mvdan.cc/sh/v3/syntax`), (b) an in-process shell interpreter (`mvdan.cc/sh/v3/interp`) with `ExecHandlers` middleware for runtime command interception, (c) a 4-level risk classifier, (d) env-var allowlisting, and (e) `OpenHandler`-based file-access sandboxing. Hard switch — no fallback to the old system. + +- **Constraints**: + - MUST NOT introduce a new external runtime dependency (no Docker, chroot, or OS-level sandboxing required). + - MUST cover every command currently blocked by regex deny patterns. + - MUST preserve the `Tool` interface contract (`Name`, `Description`, `Parameters`, `Execute`). + - MUST work cross-platform (interpreter replaces both `sh -c` and `powershell` paths). + - Config changes MUST be additive where possible; removed fields MUST produce a logged migration warning. + +- **Non-goals**: + - OS-level sandboxing (namespaces, seccomp, chroot). + - Full bash compatibility (job control, complete `shopt`/`set` support). + - Web-UI or approval workflows for blocked commands. + +- **⚠ Hard limitation — external program file access**: + + > **Executed programs can read and write any file the OS process has access to.** The `OpenHandler` only intercepts file opens from _shell redirections_ (`>`, `<`, `>>`). When the interpreter execs an external binary (e.g., `cat /etc/shadow`, `python script.py`, `tar czf /tmp/x.tar /`), that binary runs as a normal OS process with the full filesystem permissions of the picoclaw user. **This design does NOT sandbox external program I/O.** Mitigating this requires OS-level isolation (namespaces, seccomp, chroot) which is explicitly out of scope. The risk classifier and env sanitization reduce the _likelihood_ of damage but do not eliminate filesystem access. + +- **Stakeholders**: Maintainers (implementation), self-hosters (config migration), LLM agent loop (structured error contract), channel integrations (no change). + +- **Evidence**: + - Current regex bypass is demonstrable: `x=rm; $x -rf /` passes all 40 deny patterns. + - `cmd.Env` is never set — confirmed by code audit of `shell.go` L198–L212. + - `mvdan.cc/sh/v3`: 244 importers, BSD-3-Clause, v3.12.0, actively maintained (powers `shfmt`). + - `go-safe-cmd-runner` risk model reviewed: 4-level classification (low/medium/high/critical), env allowlisting, path validation. Concept adopted; no code dependency (0-star repo, 3/4 contributors are bots). + +- **Acceptance criteria**: + - [x] AC-1: All 40+ existing regex deny-pattern test cases pass under the new system (no security regression). + - [x] AC-2: Variable indirection (`x=rm; $x -rf /`), command substitution, and quoting tricks are blocked at runtime via `ExecHandlers`. + - [x] AC-3: Child processes receive only allowlisted env vars; `*_KEY`, `*_TOKEN`, `*_SECRET` vars are absent. + - [x] AC-4: Commands classified above the configured `risk_threshold` return a structured `ToolResult` containing risk level, command name, and reason. + - [x] AC-5: Shell redirections (`> /etc/passwd`) to paths outside the workspace are blocked by `OpenHandler`. + - [x] AC-6: Common LLM-generated commands (`grep`, `find`, `cat`, `python -c`, `wc`, `jq`) execute successfully. + - [x] AC-7: Deprecated config fields (`enable_deny_patterns`, `custom_deny_patterns`, `custom_allow_patterns`) produce a logged warning with migration guidance. + - [x] AC-8: Cron tool uses the same guard system. + +--- + +## 2. Decision + +### 2.1 Definitions + +- **Risk level**: One of `low`, `medium`, `high`, `critical` — a classification of the potential damage a resolved shell command can cause. +- **Risk threshold**: The maximum risk level the system will allow to execute. Commands above this level are blocked. +- **Env allowlist**: The set of environment variable names permitted to propagate to child processes. Everything else is stripped. +- **Resolved command**: The actual binary path and arguments after all shell expansion (variables, globs, substitution) has been performed by the interpreter. + +### 2.2 Rule Changes + +1. The `ExecTool` MUST parse all command strings using `mvdan.cc/sh/v3/syntax` with `Variant(syntax.LangBash)`. Unparseable input MUST be rejected with a structured error. + +2. The `ExecTool` MUST execute commands using `mvdan.cc/sh/v3/interp.Runner` instead of `os/exec` with `sh -c` or `powershell`. The Windows-specific PowerShell code path MUST be removed. + +3. The `interp.Runner` MUST be configured with an `ExecHandlers` middleware that intercepts every resolved external command and classifies it against a risk table before execution. + +4. The risk classifier MUST implement four levels: + + | Level | Default action | Characterization | + | ---------- | -------------- | ------------------------------------------------------- | + | `low` | Allow | Read-only, informational (ls, cat, grep, find, wc) | + | `medium` | Allow (logged) | File modification, network read (cp, mv, curl GET) | + | `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`). + +6. When a command is blocked, the `ToolResult` MUST include: + - Risk level of the command. + - The blocked command name and arguments. + - The configured threshold. + - A human-readable reason string. + +7. The `ExecTool` MUST construct a sanitized environment for the `interp.Runner` using a strict allowlist. The default allowlist MUST be: + + ``` + PATH, HOME, USER, LANG, SHELL, TERM, PWD, OLDPWD, + HOSTNAME, LOGNAME, TZ, DISPLAY, TMPDIR, EDITOR, PAGER + ``` + + Plus all variables matching the `LC_*` prefix. + +8. The `interp.Runner` MUST be configured with an `OpenHandler` that validates all file-open paths (from shell redirections) resolve within the configured workspace directory. Paths to safe pseudo-devices (`/dev/null`, `/dev/zero`, `/dev/urandom`, `/dev/stdin`, `/dev/stdout`, `/dev/stderr`) MUST be exempted. + +9. The existing regex-based guard (`defaultDenyPatterns`, `guardCommand()`) MUST be removed entirely. The `ExecConfig` fields `EnableDenyPatterns`, `CustomDenyPatterns`, and `CustomAllowPatterns` MUST be removed from the struct. + +10. The `ExecConfig` struct MUST be extended with: + + ```go + RiskThreshold string `json:"risk_threshold"` // "low"|"medium"|"high"|"critical"; default "medium" + RiskOverrides map[string]string `json:"risk_overrides"` // command → level override + EnvAllowlist []string `json:"env_allowlist"` // extra vars to pass (extends defaults) + EnvSet map[string]string `json:"env_set"` // explicit var=value pairs + ArgModifiers map[string][]ArgModifierConfig `json:"arg_modifiers"` // command → argument-aware risk adjustments (extends built-ins) + ``` + + `ArgModifierConfig` is `struct { Args []string; Level string }`. User-defined modifiers are checked alongside built-ins using highest-match-wins semantics (the maximum level across all matching modifiers is applied). + +11. If deprecated config fields (`enable_deny_patterns`, `custom_deny_patterns`, `custom_allow_patterns`) are present in user config, the system SHOULD log a warning with migration instructions. The system MUST NOT fail to start. + +12. The cron tool (`pkg/tools/cron.go`) MUST use the same `ExecTool` with the same guard system. + +13. The `ExecTool` MUST implement the `AsyncTool` interface (`SetCallback(AsyncCallback)`). When the LLM passes `background=true` and a callback is registered, the command MUST be launched in a goroutine; the result is delivered via the callback. When `background=true` but no callback is registered, execution falls through to synchronous mode. Compile-time interface check: `var _ AsyncTool = (*ExecTool)(nil)`. + +14. The implementation MUST use a subpackage structure: `pkg/tools/shell/` contains the core logic (risk classifier, env sanitizer, sandbox, runner) and `pkg/tools/shell_tool.go` is the thin adapter implementing the `Tool` + `AsyncTool` interfaces. Tests live alongside their source in both locations. + +### 2.3 Migration + +- **Config**: Old fields are silently ignored with a logged warning. No config version bump required. Add a migration note to `docs/tools_configuration.md`. +- **Behavioral**: Commands that previously passed regex checks but are genuinely dangerous (variable indirection bypasses) will now be blocked. This is intentional and constitutes the security fix. +- **Backward compatibility**: The `Tool` interface (`Name`, `Description`, `Parameters`, `Execute`) is unchanged. Callers (`ToolRegistry`, `RunToolLoop`, agent instance) require no changes. + +--- + +## 3. Rationale + +### Core reasons + +- Regex pattern matching against raw shell strings is a fundamentally wrong abstraction level. Shell is a programming language; security analysis requires parsing it as one. +- `mvdan.cc/sh/v3` provides a battle-tested parser (powers `shfmt`, 244+ importers) and an interpreter with middleware hooks (`ExecHandlers`, `OpenHandler`, `Env`) designed exactly for sandboxed execution. +- Runtime interception via `ExecHandlers` catches dynamic command construction (variable expansion, command substitution, arithmetic evaluation) that static analysis cannot. +- Env inheritance is a silent data-exfiltration vector. The LLM generates arbitrary commands; any `curl`/`wget`/`nc` call can leak every secret in the parent process environment. +- A 4-level risk classifier with structured LLM feedback is strictly better than binary block/allow — it lets the agent retry with safer alternatives. + +### Alternatives considered + +| # | Alternative | Disposition | Why | +| --- | ------------------------------------------------------------------------------------------ | ----------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| 1 | **Do nothing** | Rejected | Regex guards are demonstrably bypassable. Env leak is an active vulnerability. | +| 2 | **Improve regexes** (add more patterns, normalize input before matching) | Rejected | Arms race against shell syntax. Every new evasion requires a new regex. Fundamentally wrong abstraction. Input normalization is itself a parsing problem. | +| 3 | **Static AST analysis only** (parse with `mvdan.cc/sh`, walk tree, keep `sh -c` execution) | Rejected | Cannot resolve dynamic constructs (`$x`, `$(...)`) at parse time. Catches structural patterns but misses the primary bypass vector (variable indirection). | +| 4 | **Hybrid: static AST + `sh -c` with improved guards** | Rejected | Two systems to maintain, still bypassable at exec time. Adds complexity without solving the core problem. | +| 5 | **OS-level sandboxing** (namespaces, seccomp-bpf, chroot) | Rejected as primary mechanism | Requires elevated privileges, platform-specific, heavy. Out of scope. Could be layered on top later. | +| 6 | **Import `go-safe-cmd-runner` as dependency** | Rejected | Overkill framework (TOML config, SUID privilege management, ULID audit trails). 0 stars, 3/4 contributors are bots. The useful concept (risk levels + env allowlist) is simple enough to own. | + +### Trade-offs + +| Dimension | Cost | Benefit | +| ----------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| **Complexity** | New dependency (`mvdan.cc/sh/v3`), new risk table to maintain | Eliminates regex arms race, single enforcement point | +| **Compatibility** | Interpreter is not 100% bash (no job control, limited shopt) | Covers >95% of LLM-generated commands; bash-isms parse and execute | +| **Performance** | Parse step adds <1ms per command; interpreter overhead comparable to `sh -c` | No meaningful regression for typical agent commands | +| **Binary size** | `mvdan.cc/sh/v3` adds ~450 KB to the compiled binary (parser + interpreter + syntax tables) | Acceptable for a security-critical component; no runtime memory cost beyond parse/exec | +| **Safety** | **`OpenHandler` only catches shell redirections.** External programs (`cat`, `python`, `tar`, etc.) can still read/write any file the process user has access to — this is an OS-level limitation, not solvable without namespace/seccomp isolation. | Significant improvement over regex path detection for shell-level I/O. Env sanitization prevents secret exfiltration via env. Risk classifier blocks known-dangerous commands. But **filesystem access by executed binaries remains unsandboxed.** | +| **Usability** | Some previously-allowed commands may be blocked by risk classifier | Configurable threshold + per-command overrides; structured feedback enables LLM retry | +| **Migration** | Config fields removed; users with custom patterns lose them | Clear migration path; new system is strictly more expressive | + +### Risks and mitigations + +| Risk | Likelihood | Impact | Mitigation | +| --------------------------------------------------- | ---------- | --------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Interpreter fails on valid bash edge case | Medium | Medium (command fails) | Hard switch makes failures visible immediately. LangBash covers most LLM output. Users can file bugs against `mvdan/sh`. | +| Risk table misclassifies a command (false positive) | Medium | Low (command blocked, LLM retries) | Per-command `risk_overrides` config. Structured error tells LLM what happened. | +| Risk table misclassifies a command (false negative) | Low | High (dangerous command runs) | Defense in depth: env sanitization limits damage. OpenHandler blocks shell-level file access. Risk table starts conservative. | +| **External programs read/write arbitrary files** | **High** | **High** (data exfil, data destruction) | **Not mitigated by this design.** `cat`, `python`, `curl --upload-file`, `tar` etc. run as normal OS processes with full FS access. Risk classifier blocks _known_ dangerous commands but cannot prevent a novel `python -c "open('/etc/passwd').read()"`. OS-level sandboxing (future work) is the only real fix. Env sanitization reduces secret exposure but not file exposure. | +| `mvdan.cc/sh/v3` dependency becomes unmaintained | Low | Medium | Widely used (shfmt), BSD-licensed, vendorable. Could fork if needed. | +| Binary size increase (~450 KB) | Certain | Low | One-time cost. Acceptable for security infrastructure. No runtime memory overhead beyond parse/exec. | +| Performance regression on command-heavy agent loops | Low | Low | Parser is <1ms. Benchmark before/after in CI. | + +--- + +## 4. Consequences + +### Immediate impacts + +| Who / what | Impact | +| ------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------- | +| `pkg/tools/shell_tool.go` | Thin adapter: `ExecTool` struct implementing `Tool` + `AsyncTool`. Delegates to `pkg/tools/shell/` subpackage. | +| `pkg/tools/shell_tool_test.go` | Tests for `ExecTool` sync/async behavior and interface compliance. | +| `pkg/tools/shell_process_unix.go`, `shell_process_windows.go` | Removed. Interpreter manages process lifecycle. | +| `pkg/config/config.go` (`ExecConfig`) | Three fields removed, four fields added. | +| `pkg/config/defaults.go` | Default `RiskThreshold` set to `"medium"`. | +| `pkg/tools/cron.go` | Updated to use new `ExecTool` constructor. | +| `docs/tools_configuration.md` | Rewritten for new config fields and risk model. | +| `config/config.example.json` | Updated. | +| `go.mod` | `mvdan.cc/sh/v3` added. | +| Self-hosters with `custom_deny_patterns` | Logged warning on startup. Patterns no longer functional. Must migrate to `risk_overrides`. | +| LLM agent loop | No code change. Receives richer error messages on blocked commands. | + +### New files + +| File | Purpose | +| --------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------ | +| `pkg/tools/shell/risk.go` | Risk classifier: command→level mapping, argument modifiers (built-in + configurable), `ClassifyCommand` with highest-match-wins semantics. | +| `pkg/tools/shell/risk_test.go` | Table-driven tests for all 4 levels, argument modifiers, overrides, extra modifiers. | +| `pkg/tools/shell/env.go` | Env sanitization: allowlist builder, default list, `LC_*` prefix matching, `env_set` application. | +| `pkg/tools/shell/env_test.go` | Verify secrets stripped, allowlist passed, `env_set` applied. | +| `pkg/tools/shell/sandbox.go` | `OpenHandler` implementation, path-within-workspace validation, pseudo-device exemptions. | +| `pkg/tools/shell/sandbox_test.go` | Redirect inside/outside workspace, symlink escape, safe-path exemption. | +| `pkg/tools/shell/runner.go` | `Run` function: parser + interpreter + `ExecHandlers` middleware integration. | +| `pkg/tools/shell/runner_test.go` | End-to-end runner tests: timeout, working dir, env sanitization, pipelines. | +| `pkg/tools/shell_tool.go` | Adapter: `ExecTool` struct, `NewExecToolWithConfig`, `AsyncTool` impl, arg modifier wiring. | +| `pkg/tools/shell_tool_test.go` | `ExecTool` sync/async tests, interface compliance checks. | +| `pkg/tools/cron_exec_test.go` | AC-8: cron-originated `ExecTool` blocks dangerous commands identically to agent-created one; safe commands pass. | + +### Follow-up tasks + +| # | Task | Owner (role) | Blocks | Status | +| --- | ------------------------------------------------------------------------------------------- | --------------- | ------ | -------- | +| 1 | Implement risk classifier (`risk.go`) with command table | Maintainer | 3, 4 | **Done** | +| 2 | Implement env sanitization (`env.go`) | Maintainer | 4 | **Done** | +| 3 | Implement sandbox OpenHandler (`sandbox.go`) | Maintainer | 4 | **Done** | +| 4 | Rewrite shell tool: parser + interpreter + middleware (`shell/runner.go` + `shell_tool.go`) | Maintainer | 5, 6 | **Done** | +| 5 | Port all existing test cases + add bypass tests | Maintainer | 7 | **Done** | +| 6 | Update `ExecConfig`, defaults, migration warning | Maintainer | 7 | **Done** | +| 7 | Implement `AsyncTool` on `ExecTool` | Maintainer | — | **Done** | +| 8 | Implement configurable `ArgModifiers` (user-defined, highest-match-wins) | Maintainer | — | **Done** | +| 9 | Fix runner_test PATH resolution for external binaries in sandboxed interpreter | Maintainer | — | **Done** | +| 10 | Update cron tool, docs, config example | Maintainer | — | **Done** | +| 11 | CI: add benchmark comparison (old vs new exec latency) | Maintainer | — | Open | +| 12 | Run against corpus of real LLM-generated commands (compatibility) | QA / Maintainer | — | Open | + +### Test / verification plan + +| AC | Test | Method | +| ---- | ------------------------------------------------------ | --------------------------------------------------------------------------------------------------------------------------------------------------------- | +| AC-1 | Port all 40+ regex deny-pattern test cases | Unit tests in `shell_test.go` — same inputs, same block expectations | +| AC-2 | Variable indirection, cmd substitution, quoting bypass | New unit tests: `x=rm; $x -rf /`, `` `echo rm` -rf / ``, `'r'"m"' -rf /` | +| AC-3 | Env sanitization | Unit test: set `OPENAI_API_KEY=secret` in parent, verify absent in child env. Verify `PATH`, `HOME` present. | +| AC-4 | Structured error on blocked command | Unit test: execute `sudo ls`, verify `ToolResult.ForLLM` contains risk level, command, threshold, reason. Verify `IsError == true`. | +| AC-5 | OpenHandler path validation | Unit test: `echo test > /etc/shadow` — blocked. `echo test > ./output.txt` — allowed. Symlink to outside — blocked. `/dev/null` — allowed. | +| AC-6 | Compatibility with common commands | Integration test: `grep -r pattern .`, `find . -name '*.go'`, `cat file.txt`, `python3 -c "print(1)"`, `wc -l file`, `jq .field file.json` — all succeed. | +| AC-7 | Config migration warning | Unit test: load config with `enable_deny_patterns: false` — verify logged warning, no startup failure. | +| AC-8 | Cron uses same system | Unit test: cron-created `ExecTool` blocks `sudo ls` identically to agent-created one. | + +### Rollback plan + +- The change is contained within `pkg/tools/` and `pkg/config/`. Git revert of the implementation commits restores the regex-based system. +- Assumption: no other packages depend on `ExecTool` internals (only the `Tool` interface is public contract). Verified: only `pkg/agent/instance.go` and `pkg/tools/cron.go` call `NewExecToolWithConfig`. +- If the interpreter causes widespread command failures post-deploy, revert and reconsider the hybrid approach (Alternative #4). + +### Decision review date + +**2026-06-04** — Review: + +- False-positive rate (commands incorrectly blocked). +- False-negative rate (dangerous commands that slipped through). +- Interpreter compatibility issues reported. +- Whether `OpenHandler` limitations warrant OS-level sandboxing investment. + +--- + +## Assumptions + +- **Assumption**: >95% of LLM-generated shell commands are compatible with `mvdan.cc/sh/v3/interp` in LangBash mode. Based on: LLMs generate simple pipelines, not job-control or advanced bash internals. +- **Assumption**: The `ExecHandlers` middleware sees the fully-resolved command (after variable expansion, globbing, etc.) before execution. Based on: `mvdan.cc/sh/v3/interp` documentation for `ExecHandlerFunc`. +- **Assumption**: No downstream consumers depend on the `ExecConfig` fields being removed. Based on: config is JSON-deserialized; unknown fields are ignored by default in Go's `encoding/json`. +- **Assumption**: `mvdan.cc/sh/v3` will remain maintained for the foreseeable future. Based on: BSD-3-Clause, powers `shfmt`, 244+ importers, latest release v3.12.0 (Jul 2025). +- **Assumption**: `mvdan.cc/sh/v3` adds ~450 KB to compiled binary size. Based on: typical Go module size for parser+interpreter; to be verified with `go build` size comparison before/after. +- **Assumption**: Users accept that executed external programs have full filesystem access. This is inherent to exec-based execution and is not a regression — the current `sh -c` system has the same property. The new system makes this limitation _explicit_ rather than hiding it behind regex theater. diff --git a/docs/tools_configuration.md b/docs/tools_configuration.md index e64a3a107..09470fdeb 100644 --- a/docs/tools_configuration.md +++ b/docs/tools_configuration.md @@ -45,34 +45,70 @@ Web tools are used for web search and fetching. ## Exec Tool -The exec tool is used to execute shell commands. +The exec tool executes shell commands using an in-process POSIX interpreter with +AST-based risk classification, environment sanitization, and file-access sandboxing. -| Config | Type | Default | Description | -| ---------------------- | ----- | ------- | ------------------------------------------ | -| `enable_deny_patterns` | bool | true | Enable default dangerous command blocking | -| `custom_deny_patterns` | array | [] | Custom deny patterns (regular expressions) | +### Configuration -### Functionality +| 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) | +| `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 | -- **`enable_deny_patterns`**: Set to `false` to completely disable the default dangerous command blocking patterns -- **`custom_deny_patterns`**: Add custom deny regex patterns; commands matching these will be blocked +### Risk Classification -### Default Blocked Command Patterns +Every command is parsed into an AST before execution. Each resolved binary is +looked up in a built-in risk table with four levels: -By default, PicoClaw blocks the following dangerous commands: +| Level | Meaning | Examples | +| ---------- | ------------------------------------- | ---------------------------------- | +| `low` | Read-only / informational | `echo`, `cat`, `ls`, `date` | +| `medium` | Writes files but limited blast radius | `cp`, `mv`, `mkdir`, `tee` | +| `high` | System-wide side effects | `apt`, `brew`, `docker`, `mount` | +| `critical` | Destructive / privilege escalation | `sudo`, `rm -rf`, `shutdown`, `dd` | -- Delete commands: `rm -rf`, `del /f/q`, `rmdir /s` -- Disk operations: `format`, `mkfs`, `diskpart`, `dd if=`, writing to `/dev/sd*` -- System operations: `shutdown`, `reboot`, `poweroff` -- Command substitution: `$()`, `${}`, backticks -- Pipe to shell: `| sh`, `| bash` -- Privilege escalation: `sudo`, `chmod`, `chown` -- Process control: `pkill`, `killall`, `kill -9` -- Remote operations: `curl | sh`, `wget | sh`, `ssh` -- Package management: `apt`, `yum`, `dnf`, `npm install -g`, `pip install --user` -- Containers: `docker run`, `docker exec` -- Git: `git push`, `git force` -- Other: `eval`, `source *.sh` +Commands with a risk level **above** `risk_threshold` are blocked before execution. + +#### Argument modifiers + +Some commands change risk depending on their arguments. For example, `rm` is +`medium` by default but becomes `critical` when called with `-rf`. + +You can add custom argument modifiers via config. Each entry lists tokens that +must all be present (order-independent) and the resulting level: + +```json +{ + "arg_modifiers": { + "curl": [{ "args": ["--upload-file"], "level": "high" }], + "git": [{ "args": ["push", "--force"], "level": "critical" }] + } +} +``` + +The **highest matching** modifier wins (built-in and custom are merged). + +### Environment Sanitization + +The shell interpreter runs with a sanitized environment. Only a safe allowlist +of variables is exposed (e.g., `PATH`, `HOME`, `LANG`, `TERM`). + +- **`env_allowlist`**: Extend the defaults with additional variable names. +- **`env_set`**: Inject fixed `VAR=value` pairs (overrides real env). + +### File-Access Sandboxing + +When `restrict_to_workspace` is enabled (the default), the interpreter's +`OpenHandler` blocks reads and writes outside the configured workspace directory. + +### Cron Integration + +The cron tool creates its own `ExecTool` via `NewExecToolWithConfig`, so +scheduled commands go through the same risk classifier, env sanitization, and +sandbox as agent-originated commands. ### Configuration Example @@ -80,8 +116,18 @@ By default, PicoClaw blocks the following dangerous commands: { "tools": { "exec": { - "enable_deny_patterns": true, - "custom_deny_patterns": ["\\brm\\s+-r\\b", "\\bkillall\\s+python"] + "risk_threshold": "medium", + "risk_overrides": { + "ffmpeg": "low", + "terraform": "critical" + }, + "arg_modifiers": { + "curl": [{ "args": ["--upload-file"], "level": "high" }] + }, + "env_allowlist": ["GOPATH", "JAVA_HOME"], + "env_set": { + "NODE_ENV": "production" + } } } } diff --git a/go.mod b/go.mod index 6fa3a900c..d35d6ffb0 100644 --- a/go.mod +++ b/go.mod @@ -28,6 +28,7 @@ require ( golang.org/x/time v0.14.0 google.golang.org/protobuf v1.36.11 modernc.org/sqlite v1.46.1 + mvdan.cc/sh/v3 v3.12.0 ) require ( diff --git a/go.sum b/go.sum index 060594d06..43ce95f57 100644 --- a/go.sum +++ b/go.sum @@ -40,6 +40,8 @@ github.com/coder/websocket v1.8.14/go.mod h1:NX3SzP+inril6yawo5CQXx8+fk145lPDC6p github.com/coreos/go-systemd/v22 v22.5.0/go.mod h1:Y58oyj3AT4RCenI/lSvhwexgC+NSVTIJ3seZv2GcEnc= github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g= github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E= +github.com/creack/pty v1.1.24 h1:bJrF4RRfyJnbTJqzRLHzcGaZK1NeM5kTC9jGgovnR1s= +github.com/creack/pty v1.1.24/go.mod h1:08sCNb52WyoAwi2QDyzUCTgcvVFhUzewun7wtTfvcwE= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= @@ -56,6 +58,8 @@ github.com/gdamore/tcell/v2 v2.13.8 h1:Mys/Kl5wfC/GcC5Cx4C2BIQH9dbnhnkPgS9/wF3Rl github.com/gdamore/tcell/v2 v2.13.8/go.mod h1:+Wfe208WDdB7INEtCsNrAN6O2m+wsTPk1RAovjaILlo= github.com/github/copilot-sdk/go v0.1.23 h1:uExtO/inZQndCZMiSAA1hvXINiz9tqo/MZgQzFzurxw= github.com/github/copilot-sdk/go v0.1.23/go.mod h1:GdwwBfMbm9AABLEM3x5IZKw4ZfwCYxZ1BgyytmZenQ0= +github.com/go-quicktest/qt v1.101.0 h1:O1K29Txy5P2OK0dGo59b7b0LR6wKfIhttaAhHUyn7eI= +github.com/go-quicktest/qt v1.101.0/go.mod h1:14Bz/f7NwaXPtdYEgzsx46kqSxVwTbzVZsDC26tQJow= github.com/go-redis/redis/v8 v8.11.4/go.mod h1:2Z2wHZXdQpCDXEGzqMockDpNyYvi2l4Pxt6RJr792+w= github.com/go-resty/resty/v2 v2.6.0/go.mod h1:PwvJS6hvaPkjtjNg9ph+VrSD92bi5Zq73w/BIH7cC3Q= github.com/go-resty/resty/v2 v2.17.1 h1:x3aMpHK1YM9e4va/TMDRlusDDoZiQ+ViDu/WpA6xTM4= @@ -113,8 +117,9 @@ github.com/klauspost/cpuid/v2 v2.3.0 h1:S4CRMLnYUhGeDFDqkGriYKdfoFlDnMtqTiI/sFzh github.com/klauspost/cpuid/v2 v2.3.0/go.mod h1:hqwkgyIinND0mEev00jJYCxPNVRVXFQeu1XKlok6oO0= github.com/kr/pretty v0.1.0/go.mod h1:dAy3ld7l9f0ibDNOQOHHMYYIIbhfbHSm3C4ZsoJORNo= github.com/kr/pretty v0.2.1/go.mod h1:ipq/a2n7PKx3OHsz4KJII5eveXtPO4qwEXGdVfWzfnI= -github.com/kr/pretty v0.3.0 h1:WgNl7dwNpEZ6jJ9k1snq4pZsg7DOEN8hP9Xw0Tsjwk0= github.com/kr/pretty v0.3.0/go.mod h1:640gp4NfQd8pI5XOwp5fnNeVWj67G7CFk/SaSQn7NBk= +github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE= +github.com/kr/pretty v0.3.1/go.mod h1:hoEshYVHaxMs3cyo3Yncou5ZscifuDolrwPKZanG3xk= github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ= github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= @@ -165,8 +170,9 @@ github.com/rivo/tview v0.42.0/go.mod h1:cSfIYfhpSGCjp3r/ECJb+GKS7cGJnqV8vfjQPwoX github.com/rivo/uniseg v0.4.7 h1:WUdvkW8uEhrYfLC4ZzdpI2ztxP1I582+49Oc5Mq64VQ= github.com/rivo/uniseg v0.4.7/go.mod h1:FN3SvrM+Zdj16jyLfmOkMNblXMcoc8DfTHruCPUcx88= github.com/rogpeppe/go-internal v1.6.1/go.mod h1:xXDCJY+GAPziupqXw64V24skbSoqbTEfhy4qGm1nDQc= -github.com/rogpeppe/go-internal v1.9.0 h1:73kH8U+JUqXU8lRuOHeVHaa/SZPifC7BkcraZVejAe8= github.com/rogpeppe/go-internal v1.9.0/go.mod h1:WtVeX8xhTBvf0smdhujwtBcq4Qrzq/fJaraNFVN+nFs= +github.com/rogpeppe/go-internal v1.14.1 h1:UQB4HGPB6osV0SQTLymcB4TgvyWu6ZyliaW0tI/otEQ= +github.com/rogpeppe/go-internal v1.14.1/go.mod h1:MaRKkUm5W0goXpeCfT7UZI6fk/L7L7so1lCWt35ZSgc= github.com/rs/xid v1.6.0/go.mod h1:7XoLgs4eV+QndskICGsho+ADou8ySMSjJKDIan90Nz0= github.com/rs/zerolog v1.34.0 h1:k43nTLIwcTVQAncfCw4KZ2VY6ukYoZaBPNOE8txlOeY= github.com/rs/zerolog v1.34.0/go.mod h1:bJsvje4Z08ROH4Nhs5iH600c3IkWhwp44iRc54W6wYQ= @@ -383,5 +389,7 @@ modernc.org/strutil v1.2.1 h1:UneZBkQA+DX2Rp35KcM69cSsNES9ly8mQWD71HKlOA0= modernc.org/strutil v1.2.1/go.mod h1:EHkiggD70koQxjVdSBM3JKM7k6L0FbGE5eymy9i3B9A= modernc.org/token v1.1.0 h1:Xl7Ap9dKaEs5kLoOQeQmPWevfnk/DM5qcLcYlA8ys6Y= modernc.org/token v1.1.0/go.mod h1:UGzOrNV1mAFSEB63lOFHIpNRUVMvYTc6yu1SMY/XTDM= +mvdan.cc/sh/v3 v3.12.0 h1:ejKUR7ONP5bb+UGHGEG/k9V5+pRVIyD+LsZz7o8KHrI= +mvdan.cc/sh/v3 v3.12.0/go.mod h1:Se6Cj17eYSn+sNooLZiEUnNNmNxg0imoYlTu4CyaGyg= rsc.io/qr v0.2.0 h1:6vBLea5/NRMVTz8V66gipeLycZMl/+UlFmk8DvqQ6WY= rsc.io/qr v0.2.0/go.mod h1:IF+uZjkb9fqyeF/4tlBoynqmQxUoPfWEKh921coOuXs= diff --git a/pkg/config/config.go b/pkg/config/config.go index 72af3e2fb..3584f5d3f 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -602,12 +602,23 @@ type CronToolsConfig struct { ExecTimeoutMinutes int ` env:"PICOCLAW_TOOLS_CRON_EXEC_TIMEOUT_MINUTES" json:"exec_timeout_minutes"` // 0 means no timeout } +// ArgModifierConfig describes an argument pattern that elevates a command's risk. +type ArgModifierConfig struct { + Args []string `json:"args"` // tokens that must all be present (order-independent) + Level string `json:"level"` // target risk level: "low"|"medium"|"high"|"critical" +} + type ExecConfig struct { - ToolConfig ` envPrefix:"PICOCLAW_TOOLS_EXEC_"` - EnableDenyPatterns bool ` env:"PICOCLAW_TOOLS_EXEC_ENABLE_DENY_PATTERNS" json:"enable_deny_patterns"` - CustomDenyPatterns []string ` env:"PICOCLAW_TOOLS_EXEC_CUSTOM_DENY_PATTERNS" json:"custom_deny_patterns"` - CustomAllowPatterns []string ` env:"PICOCLAW_TOOLS_EXEC_CUSTOM_ALLOW_PATTERNS" json:"custom_allow_patterns"` - TimeoutSeconds int ` env:"PICOCLAW_TOOLS_EXEC_TIMEOUT_SECONDS" json:"timeout_seconds"` // 0 means use default (60s) + RiskThreshold string `json:"risk_threshold" env:"PICOCLAW_TOOLS_EXEC_RISK_THRESHOLD"` // "low"|"medium"|"high"|"critical"; default "medium" + RiskOverrides map[string]string `json:"risk_overrides" env:"PICOCLAW_TOOLS_EXEC_RISK_OVERRIDES"` // command → level override + ArgModifiers map[string][]ArgModifierConfig `json:"arg_modifiers" env:"PICOCLAW_TOOLS_EXEC_ARG_MODIFIERS"` // command → argument-aware risk adjustments (extends built-ins) + EnvAllowlist []string `json:"env_allowlist" env:"PICOCLAW_TOOLS_EXEC_ENV_ALLOWLIST"` // extra env vars to pass (extends defaults) + EnvSet map[string]string `json:"env_set" env:"PICOCLAW_TOOLS_EXEC_ENV_SET"` // explicit var=value pairs + + // Deprecated: these fields are ignored. See risk_threshold and risk_overrides. + EnableDenyPatterns bool `json:"enable_deny_patterns,omitempty" env:"PICOCLAW_TOOLS_EXEC_ENABLE_DENY_PATTERNS"` + CustomDenyPatterns []string `json:"custom_deny_patterns,omitempty" env:"PICOCLAW_TOOLS_EXEC_CUSTOM_DENY_PATTERNS"` + CustomAllowPatterns []string `json:"custom_allow_patterns,omitempty" env:"PICOCLAW_TOOLS_EXEC_CUSTOM_ALLOW_PATTERNS"` } type SkillsToolsConfig struct { diff --git a/pkg/config/defaults.go b/pkg/config/defaults.go index 1902480c5..c73d9ea16 100644 --- a/pkg/config/defaults.go +++ b/pkg/config/defaults.go @@ -382,11 +382,7 @@ func DefaultConfig() *Config { ExecTimeoutMinutes: 5, }, Exec: ExecConfig{ - ToolConfig: ToolConfig{ - Enabled: true, - }, - EnableDenyPatterns: true, - TimeoutSeconds: 60, + RiskThreshold: "medium", }, Skills: SkillsToolsConfig{ ToolConfig: ToolConfig{ diff --git a/pkg/tools/cron_exec_test.go b/pkg/tools/cron_exec_test.go new file mode 100644 index 000000000..0c72ae8bb --- /dev/null +++ b/pkg/tools/cron_exec_test.go @@ -0,0 +1,153 @@ +package tools + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/sipeed/picoclaw/pkg/bus" + "github.com/sipeed/picoclaw/pkg/config" + "github.com/sipeed/picoclaw/pkg/cron" +) + +// readOutbound reads the next outbound message with a timeout. +func readOutbound(t *testing.T, msgBus *bus.MessageBus, timeout time.Duration) (bus.OutboundMessage, bool) { + t.Helper() + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + msg, ok := msgBus.SubscribeOutbound(ctx) + if !ok { + t.Fatal("timed out or bus closed waiting for outbound message") + } + return msg, ok +} + +// stubJobExecutor satisfies JobExecutor for testing without a real agent. +type stubJobExecutor struct{} + +func (s *stubJobExecutor) ProcessDirectWithChannel(_ context.Context, _, _, _, _ string) (string, error) { + return "ok", nil +} + +// TestCronTool_ExecuteJob_BlocksDangerousCommands verifies that commands +// executed via the cron scheduler go through the same risk classifier +// as agent-originated ExecTool calls (acceptance criterion AC-8). +func TestCronTool_ExecuteJob_BlocksDangerousCommands(t *testing.T) { + tmpDir := t.TempDir() + storePath := filepath.Join(tmpDir, "cron.json") + + cronSvc := cron.NewCronService(storePath, func(_ *cron.CronJob) (string, error) { + return "ok", nil + }) + + msgBus := bus.NewMessageBus() + defer msgBus.Close() + + workspace := t.TempDir() + + ct, err := NewCronTool( + cronSvc, + &stubJobExecutor{}, + msgBus, + workspace, + true, + 5*time.Second, + &config.Config{}, // default config → risk threshold = medium + ) + if err != nil { + t.Fatalf("NewCronTool: %v", err) + } + + tests := []struct { + name string + command string + }{ + {"sudo", "sudo ls"}, + {"rm -rf", "rm -rf /"}, + {"shutdown", "shutdown -h now"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + job := &cron.CronJob{ + ID: "test-" + tt.name, + Name: tt.name, + Payload: cron.CronPayload{ + Command: tt.command, + Channel: "test", + To: "test-chat", + }, + } + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + ct.ExecuteJob(ctx, job) + + msg, _ := readOutbound(t, msgBus, 3*time.Second) + if !strings.Contains(msg.Content, "Error executing scheduled command") { + t.Errorf("expected blocked error, got: %s", msg.Content) + } + if !strings.Contains(msg.Content, "Command blocked by risk classifier") { + t.Errorf("expected risk classifier message, got: %s", msg.Content) + } + }) + } +} + +// TestCronTool_ExecuteJob_AllowsSafeCommands verifies harmless commands +// still execute normally through the cron path. +func TestCronTool_ExecuteJob_AllowsSafeCommands(t *testing.T) { + tmpDir := t.TempDir() + storePath := filepath.Join(tmpDir, "cron.json") + + cronSvc := cron.NewCronService(storePath, func(_ *cron.CronJob) (string, error) { + return "ok", nil + }) + + msgBus := bus.NewMessageBus() + defer msgBus.Close() + + workspace := t.TempDir() + // Create a file to read so the command has visible output. + os.WriteFile(filepath.Join(workspace, "hello.txt"), []byte("cron-test"), 0o644) + + ct, err := NewCronTool( + cronSvc, + &stubJobExecutor{}, + msgBus, + workspace, + true, + 5*time.Second, + &config.Config{}, + ) + if err != nil { + t.Fatalf("NewCronTool: %v", err) + } + + job := &cron.CronJob{ + ID: "safe-echo", + Name: "safe echo", + Payload: cron.CronPayload{ + Command: "echo hello", + Channel: "test", + To: "test-chat", + }, + } + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + ct.ExecuteJob(ctx, job) + + msg, _ := readOutbound(t, msgBus, 3*time.Second) + if strings.Contains(msg.Content, "Error executing scheduled command") { + t.Errorf("safe command should not be blocked: %s", msg.Content) + } + if !strings.Contains(msg.Content, "hello") { + t.Errorf("expected 'hello' in output, got: %s", msg.Content) + } +} diff --git a/pkg/tools/shell.go b/pkg/tools/shell.go deleted file mode 100644 index b8a811d03..000000000 --- a/pkg/tools/shell.go +++ /dev/null @@ -1,383 +0,0 @@ -package tools - -import ( - "bytes" - "context" - "errors" - "fmt" - "os" - "os/exec" - "path/filepath" - "regexp" - "runtime" - "strings" - "time" - - "github.com/sipeed/picoclaw/pkg/config" -) - -type ExecTool struct { - workingDir string - timeout time.Duration - denyPatterns []*regexp.Regexp - allowPatterns []*regexp.Regexp - customAllowPatterns []*regexp.Regexp - restrictToWorkspace bool -} - -var ( - defaultDenyPatterns = []*regexp.Regexp{ - regexp.MustCompile(`\brm\s+-[rf]{1,2}\b`), - regexp.MustCompile(`\bdel\s+/[fq]\b`), - regexp.MustCompile(`\brmdir\s+/s\b`), - // Match disk wiping commands (must be followed by space/args) - regexp.MustCompile( - `\b(format|mkfs|diskpart)\b\s`, - ), - regexp.MustCompile(`\bdd\s+if=`), - // Block writes to block devices (all common naming schemes). - regexp.MustCompile( - `>\s*/dev/(sd[a-z]|hd[a-z]|vd[a-z]|xvd[a-z]|nvme\d|mmcblk\d|loop\d|dm-\d|md\d|sr\d|nbd\d)`, - ), - regexp.MustCompile(`\b(shutdown|reboot|poweroff)\b`), - regexp.MustCompile(`:\(\)\s*\{.*\};\s*:`), - regexp.MustCompile(`\$\([^)]+\)`), - regexp.MustCompile(`\$\{[^}]+\}`), - regexp.MustCompile("`[^`]+`"), - regexp.MustCompile(`\|\s*sh\b`), - regexp.MustCompile(`\|\s*bash\b`), - regexp.MustCompile(`;\s*rm\s+-[rf]`), - regexp.MustCompile(`&&\s*rm\s+-[rf]`), - regexp.MustCompile(`\|\|\s*rm\s+-[rf]`), - regexp.MustCompile(`<<\s*EOF`), - regexp.MustCompile(`\$\(\s*cat\s+`), - regexp.MustCompile(`\$\(\s*curl\s+`), - regexp.MustCompile(`\$\(\s*wget\s+`), - regexp.MustCompile(`\$\(\s*which\s+`), - regexp.MustCompile(`\bsudo\b`), - regexp.MustCompile(`\bchmod\s+[0-7]{3,4}\b`), - regexp.MustCompile(`\bchown\b`), - regexp.MustCompile(`\bpkill\b`), - regexp.MustCompile(`\bkillall\b`), - regexp.MustCompile(`\bkill\b`), - regexp.MustCompile(`\bcurl\b.*\|\s*(sh|bash)`), - regexp.MustCompile(`\bwget\b.*\|\s*(sh|bash)`), - regexp.MustCompile(`\bnpm\s+install\s+-g\b`), - regexp.MustCompile(`\bpip\s+install\s+--user\b`), - regexp.MustCompile(`\bapt\s+(install|remove|purge)\b`), - regexp.MustCompile(`\byum\s+(install|remove)\b`), - regexp.MustCompile(`\bdnf\s+(install|remove)\b`), - regexp.MustCompile(`\bdocker\s+run\b`), - regexp.MustCompile(`\bdocker\s+exec\b`), - regexp.MustCompile(`\bgit\s+push\b`), - regexp.MustCompile(`\bgit\s+force\b`), - regexp.MustCompile(`\bssh\b.*@`), - regexp.MustCompile(`\beval\b`), - regexp.MustCompile(`\bsource\s+.*\.sh\b`), - } - - // absolutePathPattern matches absolute file paths in commands (Unix and Windows). - absolutePathPattern = regexp.MustCompile(`[A-Za-z]:\\[^\\\"']+|/[^\s\"']+`) - - // safePaths are kernel pseudo-devices that are always safe to reference in - // commands, regardless of workspace restriction. They contain no user data - // and cannot cause destructive writes. - safePaths = map[string]bool{ - "/dev/null": true, - "/dev/zero": true, - "/dev/random": true, - "/dev/urandom": true, - "/dev/stdin": true, - "/dev/stdout": true, - "/dev/stderr": true, - } -) - -func NewExecTool(workingDir string, restrict bool) (*ExecTool, error) { - return NewExecToolWithConfig(workingDir, restrict, nil) -} - -func NewExecToolWithConfig(workingDir string, restrict bool, config *config.Config) (*ExecTool, error) { - denyPatterns := make([]*regexp.Regexp, 0) - customAllowPatterns := make([]*regexp.Regexp, 0) - - if config != nil { - execConfig := config.Tools.Exec - enableDenyPatterns := execConfig.EnableDenyPatterns - if enableDenyPatterns { - denyPatterns = append(denyPatterns, defaultDenyPatterns...) - if len(execConfig.CustomDenyPatterns) > 0 { - fmt.Printf("Using custom deny patterns: %v\n", execConfig.CustomDenyPatterns) - for _, pattern := range execConfig.CustomDenyPatterns { - re, err := regexp.Compile(pattern) - if err != nil { - return nil, fmt.Errorf("invalid custom deny pattern %q: %w", pattern, err) - } - denyPatterns = append(denyPatterns, re) - } - } - } else { - // If deny patterns are disabled, we won't add any patterns, allowing all commands. - fmt.Println("Warning: deny patterns are disabled. All commands will be allowed.") - } - for _, pattern := range execConfig.CustomAllowPatterns { - re, err := regexp.Compile(pattern) - if err != nil { - return nil, fmt.Errorf("invalid custom allow pattern %q: %w", pattern, err) - } - customAllowPatterns = append(customAllowPatterns, re) - } - } else { - denyPatterns = append(denyPatterns, defaultDenyPatterns...) - } - - timeout := 60 * time.Second - if config != nil && config.Tools.Exec.TimeoutSeconds > 0 { - timeout = time.Duration(config.Tools.Exec.TimeoutSeconds) * time.Second - } - - return &ExecTool{ - workingDir: workingDir, - timeout: timeout, - denyPatterns: denyPatterns, - allowPatterns: nil, - customAllowPatterns: customAllowPatterns, - restrictToWorkspace: restrict, - }, nil -} - -func (t *ExecTool) Name() string { - return "exec" -} - -func (t *ExecTool) Description() string { - return "Execute a shell command and return its output. Use with caution." -} - -func (t *ExecTool) Parameters() map[string]any { - return map[string]any{ - "type": "object", - "properties": map[string]any{ - "command": map[string]any{ - "type": "string", - "description": "The shell command to execute", - }, - "working_dir": map[string]any{ - "type": "string", - "description": "Optional working directory for the command", - }, - }, - "required": []string{"command"}, - } -} - -func (t *ExecTool) Execute(ctx context.Context, args map[string]any) *ToolResult { - command, ok := args["command"].(string) - if !ok { - return ErrorResult("command is required") - } - - cwd := t.workingDir - if wd, ok := args["working_dir"].(string); ok && wd != "" { - if t.restrictToWorkspace && t.workingDir != "" { - resolvedWD, err := validatePath(wd, t.workingDir, true) - if err != nil { - return ErrorResult("Command blocked by safety guard (" + err.Error() + ")") - } - cwd = resolvedWD - } else { - cwd = wd - } - } - - if cwd == "" { - wd, err := os.Getwd() - if err == nil { - cwd = wd - } - } - - if guardError := t.guardCommand(command, cwd); guardError != "" { - return ErrorResult(guardError) - } - - // timeout == 0 means no timeout - var cmdCtx context.Context - var cancel context.CancelFunc - if t.timeout > 0 { - cmdCtx, cancel = context.WithTimeout(ctx, t.timeout) - } else { - cmdCtx, cancel = context.WithCancel(ctx) - } - defer cancel() - - var cmd *exec.Cmd - if runtime.GOOS == "windows" { - cmd = exec.CommandContext(cmdCtx, "powershell", "-NoProfile", "-NonInteractive", "-Command", command) - } else { - cmd = exec.CommandContext(cmdCtx, "sh", "-c", command) - } - if cwd != "" { - cmd.Dir = cwd - } - - prepareCommandForTermination(cmd) - - var stdout, stderr bytes.Buffer - cmd.Stdout = &stdout - cmd.Stderr = &stderr - - if err := cmd.Start(); err != nil { - return ErrorResult(fmt.Sprintf("failed to start command: %v", err)) - } - - done := make(chan error, 1) - go func() { - done <- cmd.Wait() - }() - - var err error - select { - case err = <-done: - case <-cmdCtx.Done(): - _ = terminateProcessTree(cmd) - select { - case err = <-done: - case <-time.After(2 * time.Second): - if cmd.Process != nil { - _ = cmd.Process.Kill() - } - err = <-done - } - } - - output := stdout.String() - if stderr.Len() > 0 { - output += "\nSTDERR:\n" + stderr.String() - } - - if err != nil { - if errors.Is(cmdCtx.Err(), context.DeadlineExceeded) { - msg := fmt.Sprintf("Command timed out after %v", t.timeout) - return &ToolResult{ - ForLLM: msg, - ForUser: msg, - IsError: true, - } - } - output += fmt.Sprintf("\nExit code: %v", err) - } - - if output == "" { - output = "(no output)" - } - - maxLen := 10000 - if len(output) > maxLen { - output = output[:maxLen] + fmt.Sprintf("\n... (truncated, %d more chars)", len(output)-maxLen) - } - - if err != nil { - return &ToolResult{ - ForLLM: output, - ForUser: output, - IsError: true, - } - } - - return &ToolResult{ - ForLLM: output, - ForUser: output, - IsError: false, - } -} - -func (t *ExecTool) guardCommand(command, cwd string) string { - cmd := strings.TrimSpace(command) - lower := strings.ToLower(cmd) - - // Custom allow patterns exempt a command from deny checks. - explicitlyAllowed := false - for _, pattern := range t.customAllowPatterns { - if pattern.MatchString(lower) { - explicitlyAllowed = true - break - } - } - - if !explicitlyAllowed { - for _, pattern := range t.denyPatterns { - if pattern.MatchString(lower) { - return "Command blocked by safety guard (dangerous pattern detected)" - } - } - } - - if len(t.allowPatterns) > 0 { - allowed := false - for _, pattern := range t.allowPatterns { - if pattern.MatchString(lower) { - allowed = true - break - } - } - if !allowed { - return "Command blocked by safety guard (not in allowlist)" - } - } - - if t.restrictToWorkspace { - if strings.Contains(cmd, "..\\") || strings.Contains(cmd, "../") { - return "Command blocked by safety guard (path traversal detected)" - } - - cwdPath, err := filepath.Abs(cwd) - if err != nil { - return "" - } - - matches := absolutePathPattern.FindAllString(cmd, -1) - - for _, raw := range matches { - p, err := filepath.Abs(raw) - if err != nil { - continue - } - - if safePaths[p] { - continue - } - - rel, err := filepath.Rel(cwdPath, p) - if err != nil { - continue - } - - if strings.HasPrefix(rel, "..") { - return "Command blocked by safety guard (path outside working dir)" - } - } - } - - return "" -} - -func (t *ExecTool) SetTimeout(timeout time.Duration) { - t.timeout = timeout -} - -func (t *ExecTool) SetRestrictToWorkspace(restrict bool) { - t.restrictToWorkspace = restrict -} - -func (t *ExecTool) SetAllowPatterns(patterns []string) error { - t.allowPatterns = make([]*regexp.Regexp, 0, len(patterns)) - for _, p := range patterns { - re, err := regexp.Compile(p) - if err != nil { - return fmt.Errorf("invalid allow pattern %q: %w", p, err) - } - t.allowPatterns = append(t.allowPatterns, re) - } - return nil -} diff --git a/pkg/tools/shell/env.go b/pkg/tools/shell/env.go new file mode 100644 index 000000000..a3bc54682 --- /dev/null +++ b/pkg/tools/shell/env.go @@ -0,0 +1,108 @@ +package shell + +import ( + "os" + "strings" + + "mvdan.cc/sh/v3/expand" +) + +// DefaultEnvAllowlist is the set of environment variable names that are safe +// to propagate to child processes. Everything else is stripped to prevent +// accidental credential leakage. +var DefaultEnvAllowlist = map[string]bool{ + "PATH": true, + "HOME": true, + "USER": true, + "LANG": true, + "SHELL": true, + "TERM": true, + "PWD": true, + "OLDPWD": true, + "HOSTNAME": true, + "LOGNAME": true, + "TZ": true, + "DISPLAY": true, + "TMPDIR": true, + "EDITOR": true, + "PAGER": true, +} + +// defaultEnvAllowPrefixes are env var prefixes that are always allowed. +var defaultEnvAllowPrefixes = []string{ + "LC_", +} + +// BuildSanitizedEnv constructs an expand.Environ from the current process +// environment, filtering to only allowlisted variables. +// +// extraAllowlist adds additional variable names to the default allowlist. +// envSet provides explicit key=value pairs that override any inherited value. +func BuildSanitizedEnv(extraAllowlist []string, envSet map[string]string) expand.Environ { + allowed := make(map[string]bool, len(DefaultEnvAllowlist)+len(extraAllowlist)) + for k := range DefaultEnvAllowlist { + allowed[k] = true + } + for _, k := range extraAllowlist { + allowed[k] = true + } + + vars := make(map[string]string, len(allowed)+len(envSet)) + + for _, entry := range os.Environ() { + k, v, ok := strings.Cut(entry, "=") + if !ok { + continue + } + if allowed[k] || isAllowedPrefix(k) { + vars[k] = v + } + } + + for k, v := range envSet { + vars[k] = v + } + + return &sanitizedEnv{vars: vars} +} + +func isAllowedPrefix(name string) bool { + for _, prefix := range defaultEnvAllowPrefixes { + if strings.HasPrefix(name, prefix) { + return true + } + } + return false +} + +// sanitizedEnv implements expand.Environ backed by a simple map. +type sanitizedEnv struct { + vars map[string]string +} + +func (e *sanitizedEnv) Get(name string) expand.Variable { + val, ok := e.vars[name] + if !ok { + return expand.Variable{} + } + return expand.Variable{ + Set: true, + Exported: true, + Kind: expand.String, + Str: val, + } +} + +func (e *sanitizedEnv) Each(fn func(name string, vr expand.Variable) bool) { + for k, v := range e.vars { + vr := expand.Variable{ + Set: true, + Exported: true, + Kind: expand.String, + Str: v, + } + if !fn(k, vr) { + return + } + } +} diff --git a/pkg/tools/shell/env_test.go b/pkg/tools/shell/env_test.go new file mode 100644 index 000000000..a819ee4c8 --- /dev/null +++ b/pkg/tools/shell/env_test.go @@ -0,0 +1,112 @@ +package shell + +import ( + "testing" + + "mvdan.cc/sh/v3/expand" +) + +func TestBuildSanitizedEnv_FiltersSecrets(t *testing.T) { + // Set some secret env vars. + t.Setenv("OPENAI_API_KEY", "sk-secret") + t.Setenv("ANTHROPIC_API_KEY", "anthro-secret") + t.Setenv("AWS_SECRET_ACCESS_KEY", "aws-secret") + t.Setenv("DATABASE_URL", "postgres://secret") + // Set something that should pass. + t.Setenv("PATH", "/usr/bin") + t.Setenv("HOME", "/home/test") + t.Setenv("LC_ALL", "en_US.UTF-8") + + env := BuildSanitizedEnv(nil, nil) + + assertEnvPresent(t, env, "PATH") + assertEnvPresent(t, env, "HOME") + assertEnvPresent(t, env, "LC_ALL") + + assertEnvAbsent(t, env, "OPENAI_API_KEY") + assertEnvAbsent(t, env, "ANTHROPIC_API_KEY") + assertEnvAbsent(t, env, "AWS_SECRET_ACCESS_KEY") + assertEnvAbsent(t, env, "DATABASE_URL") +} + +func TestBuildSanitizedEnv_ExtraAllowlist(t *testing.T) { + t.Setenv("MY_CUSTOM_VAR", "hello") + + env := BuildSanitizedEnv([]string{"MY_CUSTOM_VAR"}, nil) + assertEnvPresent(t, env, "MY_CUSTOM_VAR") +} + +func TestBuildSanitizedEnv_EnvSet(t *testing.T) { + env := BuildSanitizedEnv(nil, map[string]string{ + "INJECTED": "value123", + }) + + v := env.Get("INJECTED") + if !v.IsSet() { + t.Fatal("expected INJECTED to be present") + } + if v.Str != "value123" { + t.Errorf("INJECTED = %q, want %q", v.Str, "value123") + } +} + +func TestBuildSanitizedEnv_EnvSetOverridesInherited(t *testing.T) { + t.Setenv("PATH", "/original") + + env := BuildSanitizedEnv(nil, map[string]string{ + "PATH": "/overridden", + }) + + v := env.Get("PATH") + if v.Str != "/overridden" { + t.Errorf("PATH = %q, want %q", v.Str, "/overridden") + } +} + +func TestBuildSanitizedEnv_DefaultAllowlist(t *testing.T) { + for name := range DefaultEnvAllowlist { + t.Setenv(name, "test-"+name) + } + + env := BuildSanitizedEnv(nil, nil) + + for name := range DefaultEnvAllowlist { + v := env.Get(name) + if !v.IsSet() { + t.Errorf("expected %s to be in sanitized env", name) + } + } +} + +func TestBuildSanitizedEnv_Each(t *testing.T) { + env := BuildSanitizedEnv(nil, map[string]string{ + "TEST_A": "a", + "TEST_B": "b", + }) + + found := make(map[string]bool) + env.Each(func(name string, vr expand.Variable) bool { + found[name] = true + return true + }) + + if !found["TEST_A"] || !found["TEST_B"] { + t.Errorf("Each did not iterate over all vars: %v", found) + } +} + +func assertEnvPresent(t *testing.T, env expand.Environ, name string) { + t.Helper() + v := env.Get(name) + if !v.IsSet() { + t.Errorf("expected %s to be present in sanitized env", name) + } +} + +func assertEnvAbsent(t *testing.T, env expand.Environ, name string) { + t.Helper() + v := env.Get(name) + if v.IsSet() { + t.Errorf("expected %s to be absent from sanitized env, got %q", name, v.Str) + } +} diff --git a/pkg/tools/shell/risk.go b/pkg/tools/shell/risk.go new file mode 100644 index 000000000..f8dc11edc --- /dev/null +++ b/pkg/tools/shell/risk.go @@ -0,0 +1,435 @@ +package shell + +import "fmt" + +// RiskLevel represents the potential danger of a shell command. +type RiskLevel int + +const ( + RiskLow RiskLevel = iota // Read-only, informational + RiskMedium // File modification, network read + RiskHigh // Destructive, system-modifying + RiskCritical // Privilege escalation, always dangerous +) + +func (r RiskLevel) String() string { + switch r { + case RiskLow: + return "low" + case RiskMedium: + return "medium" + case RiskHigh: + return "high" + case RiskCritical: + return "critical" + default: + return "unknown" + } +} + +// ParseRiskLevel converts a string to a RiskLevel. +// Returns an error if the string is unrecognized. +func ParseRiskLevel(s string) (RiskLevel, error) { + switch s { + case "low": + return RiskLow, nil + case "medium": + return RiskMedium, nil + case "high": + return RiskHigh, nil + case "critical": + return RiskCritical, nil + default: + return RiskMedium, fmt.Errorf("unknown risk level %q, must be one of: low, medium, high, critical", s) + } +} + +// commandRiskTable maps base command names to their default risk level. +// Commands not in this table default to RiskMedium. +var commandRiskTable = map[string]RiskLevel{ + // Low — read-only, informational + "ls": RiskLow, + "cat": RiskLow, + "head": RiskLow, + "tail": RiskLow, + "grep": RiskLow, + "egrep": RiskLow, + "fgrep": RiskLow, + "find": RiskLow, + "wc": RiskLow, + "echo": RiskLow, + "printf": RiskLow, + "pwd": RiskLow, + "whoami": RiskLow, + "id": RiskLow, + "date": RiskLow, + "uname": RiskLow, + "hostname": RiskLow, + "env": RiskLow, + "printenv": RiskLow, + "which": RiskLow, + "type": RiskLow, + "file": RiskLow, + "stat": RiskLow, + "readlink": RiskLow, + "realpath": RiskLow, + "basename": RiskLow, + "dirname": RiskLow, + "sort": RiskLow, + "uniq": RiskLow, + "cut": RiskLow, + "tr": RiskLow, + "awk": RiskLow, + "sed": RiskLow, + "diff": RiskLow, + "md5sum": RiskLow, + "sha256sum": RiskLow, + "sha1sum": RiskLow, + "xxd": RiskLow, + "od": RiskLow, + "hexdump": RiskLow, + "strings": RiskLow, + "tee": RiskLow, + "xargs": RiskLow, + "true": RiskLow, + "false": RiskLow, + "test": RiskLow, + "[": RiskLow, + "seq": RiskLow, + "yes": RiskLow, + "sleep": RiskLow, + "du": RiskLow, + "df": RiskLow, + "free": RiskLow, + "top": RiskLow, + "ps": RiskLow, + "uptime": RiskLow, + "lsof": RiskLow, + "tree": RiskLow, + "less": RiskLow, + "more": RiskLow, + "jq": RiskLow, + "yq": RiskLow, + "column": RiskLow, + "fold": RiskLow, + "fmt": RiskLow, + "rev": RiskLow, + "tac": RiskLow, + "nl": RiskLow, + "comm": RiskLow, + "join": RiskLow, + "paste": RiskLow, + "expand": RiskLow, + "unexpand": RiskLow, + + // Medium — file modification, network reads, build tools + "cp": RiskMedium, + "mv": RiskMedium, + "mkdir": RiskMedium, + "touch": RiskMedium, + "ln": RiskMedium, + "tar": RiskMedium, + "zip": RiskMedium, + "unzip": RiskMedium, + "gzip": RiskMedium, + "gunzip": RiskMedium, + "bzip2": RiskMedium, + "xz": RiskMedium, + "curl": RiskMedium, + "wget": RiskMedium, + "git": RiskMedium, + "make": RiskMedium, + "go": RiskMedium, + "python": RiskMedium, + "python3": RiskMedium, + "node": RiskMedium, + "npm": RiskMedium, + "npx": RiskMedium, + "yarn": RiskMedium, + "pnpm": RiskMedium, + "pip": RiskMedium, + "pip3": RiskMedium, + "cargo": RiskMedium, + "rustc": RiskMedium, + "gcc": RiskMedium, + "g++": RiskMedium, + "clang": RiskMedium, + "javac": RiskMedium, + "java": RiskMedium, + "ruby": RiskMedium, + "perl": RiskMedium, + "php": RiskMedium, + "patch": RiskMedium, + + // High — destructive, system-modifying + "rm": RiskHigh, + "rmdir": RiskHigh, + "chmod": RiskHigh, + "chown": RiskHigh, + "chgrp": RiskHigh, + "kill": RiskHigh, + "pkill": RiskHigh, + "killall": RiskHigh, + "ssh": RiskHigh, + "scp": RiskHigh, + "rsync": RiskHigh, + "docker": RiskHigh, + "kubectl": RiskHigh, + "systemctl": RiskHigh, + "service": RiskHigh, + + // Critical — privilege escalation, always dangerous + "sudo": RiskCritical, + "su": RiskCritical, + "dd": RiskCritical, + "mkfs": RiskCritical, + "fdisk": RiskCritical, + "parted": RiskCritical, + "mount": RiskCritical, + "umount": RiskCritical, + "shutdown": RiskCritical, + "reboot": RiskCritical, + "poweroff": RiskCritical, + "halt": RiskCritical, + "init": RiskCritical, + "insmod": RiskCritical, + "rmmod": RiskCritical, + "modprobe": RiskCritical, + "iptables": RiskCritical, + "nft": RiskCritical, + "eval": RiskCritical, + "exec": RiskCritical, + "source": RiskCritical, + ".": RiskCritical, + "format": RiskCritical, + "diskpart": RiskCritical, +} + +// ArgModifier describes a condition that elevates a command's risk level. +// All tokens in Args must be present in the command (order-independent, after +// flag normalization). +type ArgModifier struct { + Args []string + Level RiskLevel +} + +// argumentModifiers maps command names to their argument-aware risk adjustments. +// Checked in order; first match wins. +// +// Patterns use individual flags (e.g., "-r", "-f") rather than combined forms +// ("-rf") because normalizeFlags splits combined flags before matching. This +// means "rm -rf", "rm -fr", "rm -r -f", and "rm -f -r" all match correctly. +var argumentModifiers = map[string][]ArgModifier{ + "git": { + {Args: []string{"push", "--force"}, Level: RiskCritical}, + {Args: []string{"push", "-f"}, Level: RiskCritical}, + {Args: []string{"push"}, Level: RiskHigh}, + {Args: []string{"reset", "--hard"}, Level: RiskHigh}, + {Args: []string{"clean", "-f", "-d"}, Level: RiskHigh}, + {Args: []string{"clean", "-f"}, Level: RiskHigh}, + }, + "curl": { + {Args: []string{"-X", "POST"}, Level: RiskHigh}, + {Args: []string{"-X", "PUT"}, Level: RiskHigh}, + {Args: []string{"-X", "DELETE"}, Level: RiskHigh}, + {Args: []string{"--request", "POST"}, Level: RiskHigh}, + {Args: []string{"--request", "PUT"}, Level: RiskHigh}, + {Args: []string{"--request", "DELETE"}, Level: RiskHigh}, + {Args: []string{"--data"}, Level: RiskHigh}, + {Args: []string{"-d"}, Level: RiskHigh}, + {Args: []string{"--upload-file"}, Level: RiskHigh}, + {Args: []string{"-T"}, Level: RiskHigh}, + }, + "wget": { + {Args: []string{"--post-data"}, Level: RiskHigh}, + {Args: []string{"--post-file"}, Level: RiskHigh}, + }, + "npm": { + {Args: []string{"install", "-g"}, Level: RiskHigh}, + {Args: []string{"install", "--global"}, Level: RiskHigh}, + {Args: []string{"publish"}, Level: RiskHigh}, + }, + "pip": { + {Args: []string{"install", "--user"}, Level: RiskHigh}, + }, + "pip3": { + {Args: []string{"install", "--user"}, Level: RiskHigh}, + }, + "docker": { + {Args: []string{"run"}, Level: RiskHigh}, + {Args: []string{"exec"}, Level: RiskHigh}, + {Args: []string{"rm"}, Level: RiskHigh}, + {Args: []string{"rmi"}, Level: RiskHigh}, + }, + "apt": { + {Args: []string{"install"}, Level: RiskHigh}, + {Args: []string{"remove"}, Level: RiskHigh}, + {Args: []string{"purge"}, Level: RiskCritical}, + }, + "apt-get": { + {Args: []string{"install"}, Level: RiskHigh}, + {Args: []string{"remove"}, Level: RiskHigh}, + {Args: []string{"purge"}, Level: RiskCritical}, + }, + "yum": { + {Args: []string{"install"}, Level: RiskHigh}, + {Args: []string{"remove"}, Level: RiskHigh}, + }, + "dnf": { + {Args: []string{"install"}, Level: RiskHigh}, + {Args: []string{"remove"}, Level: RiskHigh}, + }, + "rm": { + {Args: []string{"-r", "-f"}, Level: RiskCritical}, + }, + "kill": { + {Args: []string{"-9"}, Level: RiskCritical}, + {Args: []string{"-KILL"}, Level: RiskCritical}, + {Args: []string{"-SIGKILL"}, Level: RiskCritical}, + }, +} + +// 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. +func ClassifyCommand(args []string, overrides map[string]string, extraModifiers ...map[string][]ArgModifier) RiskLevel { + if len(args) == 0 { + return RiskMedium + } + + 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. + } + } + + level, known := commandRiskTable[cmdName] + if !known { + level = RiskMedium + } + + // 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 + } + for _, extra := range extraModifiers { + if extra == nil { + continue + } + if elevated, ok := applyModifiers(normalizedArgs, cmdName, level, extra); ok { + level = elevated + } + } + + 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. +func applyModifiers( + normalizedArgs []string, + cmdName string, + baseLevel RiskLevel, + mods map[string][]ArgModifier, +) (RiskLevel, bool) { + if entries, ok := mods[cmdName]; ok { + for _, mod := range entries { + if matchArgs(normalizedArgs, mod.Args) && mod.Level > baseLevel { + return mod.Level, true + } + } + } + return 0, false +} + +// IsAllowed returns true if the given risk level is at or below the threshold. +func IsAllowed(level, threshold RiskLevel) bool { + return level <= threshold +} + +// BlockedCommandError formats a structured error message for the LLM. +func BlockedCommandError(args []string, level, threshold RiskLevel, reason string) string { + cmd := "" + if len(args) > 0 { + cmd = args[0] + if len(args) > 1 { + end := len(args) + if end > 5 { + end = 5 + } + for _, a := range args[1:end] { + cmd += " " + a + } + if len(args) > 5 { + cmd += " ..." + } + } + } + + return fmt.Sprintf( + "Command blocked by risk classifier: command=%q risk_level=%s threshold=%s reason=%s", + cmd, level, threshold, reason, + ) +} + +// baseCommand extracts the basename from a command path. +func baseCommand(cmd string) string { + for i := len(cmd) - 1; i >= 0; i-- { + if cmd[i] == '/' { + return cmd[i+1:] + } + } + return cmd +} + +// normalizeFlags expands combined short flags (e.g., "-rf" → "-r", "-f") +// so that modifier matching works regardless of how flags are grouped. +// Long flags (--flag) and non-flag arguments are passed through unchanged. +func normalizeFlags(args []string) []string { + result := make([]string, 0, len(args)*2) + for _, a := range args { + if len(a) > 2 && a[0] == '-' && a[1] != '-' { + for _, ch := range a[1:] { + result = append(result, "-"+string(ch)) + } + } else { + result = append(result, a) + } + } + return result +} + +// matchArgs checks if ALL pattern tokens are present in args (order-independent). +// "git push -x -f" matches pattern ["push", "-f"] because both tokens exist. +// "git -f push" also matches ["push", "-f"]. Order does not matter. +func matchArgs(args, pattern []string) bool { + if len(pattern) == 0 { + return true + } + argSet := make(map[string]int, len(args)) + for _, a := range args { + argSet[a]++ + } + for _, p := range pattern { + if argSet[p] <= 0 { + return false + } + argSet[p]-- + } + return true +} diff --git a/pkg/tools/shell/risk_test.go b/pkg/tools/shell/risk_test.go new file mode 100644 index 000000000..383b14439 --- /dev/null +++ b/pkg/tools/shell/risk_test.go @@ -0,0 +1,297 @@ +package shell + +import "testing" + +func TestClassifyCommand_BaseTable(t *testing.T) { + tests := []struct { + args []string + level RiskLevel + }{ + {[]string{"ls", "-la"}, RiskLow}, + {[]string{"cat", "file.txt"}, RiskLow}, + {[]string{"grep", "-r", "pattern", "."}, RiskLow}, + {[]string{"find", ".", "-name", "*.go"}, RiskLow}, + {[]string{"wc", "-l"}, RiskLow}, + {[]string{"echo", "hello"}, RiskLow}, + {[]string{"jq", ".field", "data.json"}, RiskLow}, + + {[]string{"cp", "a", "b"}, RiskMedium}, + {[]string{"mv", "a", "b"}, RiskMedium}, + {[]string{"python3", "-c", "print(1)"}, RiskMedium}, + {[]string{"git", "status"}, RiskMedium}, + {[]string{"curl", "https://example.com"}, RiskMedium}, + + {[]string{"rm", "file.txt"}, RiskHigh}, + {[]string{"chmod", "755", "script.sh"}, RiskHigh}, + {[]string{"docker", "ps"}, RiskHigh}, + {[]string{"ssh", "user@host"}, RiskHigh}, + + {[]string{"sudo", "ls"}, RiskCritical}, + {[]string{"dd", "if=/dev/zero", "of=/dev/sda"}, RiskCritical}, + {[]string{"shutdown", "-h", "now"}, RiskCritical}, + {[]string{"eval", "echo hi"}, RiskCritical}, + } + + for _, tt := range tests { + got := ClassifyCommand(tt.args, nil) + if got != tt.level { + t.Errorf("ClassifyCommand(%v) = %s, want %s", tt.args, got, tt.level) + } + } +} + +func TestClassifyCommand_ArgumentModifiers(t *testing.T) { + tests := []struct { + name string + args []string + level RiskLevel + }{ + {"git push", []string{"git", "push", "origin", "main"}, RiskHigh}, + {"git push --force", []string{"git", "push", "--force", "origin"}, RiskCritical}, + {"git push -f", []string{"git", "push", "-f"}, RiskCritical}, + {"git -f push (reordered)", []string{"git", "-f", "push"}, RiskCritical}, + {"git push -x -f (extra flags)", []string{"git", "push", "-x", "-f"}, RiskCritical}, + {"git reset --hard", []string{"git", "reset", "--hard", "HEAD~1"}, RiskHigh}, + {"git clean -f", []string{"git", "clean", "-f"}, RiskHigh}, + {"git clean -fd", []string{"git", "clean", "-fd"}, RiskHigh}, + {"git clean -d -f (reordered)", []string{"git", "clean", "-d", "-f"}, RiskHigh}, + + {"curl GET (default)", []string{"curl", "https://example.com"}, RiskMedium}, + {"curl POST", []string{"curl", "-X", "POST", "https://example.com"}, RiskHigh}, + {"curl -d data", []string{"curl", "-d", "data", "https://example.com"}, RiskHigh}, + {"curl --data data", []string{"curl", "--data", "data", "url"}, RiskHigh}, + {"curl -X DELETE", []string{"curl", "-X", "DELETE", "url"}, RiskHigh}, + {"curl --request POST", []string{"curl", "--request", "POST", "url"}, RiskHigh}, + + {"rm file (no flags)", []string{"rm", "file.txt"}, RiskHigh}, + {"rm -rf", []string{"rm", "-rf", "/"}, RiskCritical}, + {"rm -fr", []string{"rm", "-fr", "/"}, RiskCritical}, + {"rm -r -f (separate)", []string{"rm", "-r", "-f", "dir"}, RiskCritical}, + {"rm -f -r (reordered)", []string{"rm", "-f", "-r", "dir"}, RiskCritical}, + {"rm -f -x -r (extra flags between)", []string{"rm", "-f", "-x", "-r", "dir"}, RiskCritical}, + + {"kill (no signal)", []string{"kill", "1234"}, RiskHigh}, + {"kill -9", []string{"kill", "-9", "1234"}, RiskCritical}, + + {"npm install (local)", []string{"npm", "install", "lodash"}, RiskMedium}, + {"npm install -g", []string{"npm", "install", "-g", "lodash"}, RiskHigh}, + {"npm publish", []string{"npm", "publish"}, RiskHigh}, + + {"docker ps (no modifier)", []string{"docker", "ps"}, RiskHigh}, + {"docker run", []string{"docker", "run", "ubuntu"}, RiskHigh}, + {"docker exec", []string{"docker", "exec", "-it", "cnt", "bash"}, RiskHigh}, + + {"apt install", []string{"apt", "install", "vim"}, RiskHigh}, + {"apt purge", []string{"apt", "purge", "vim"}, RiskCritical}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := ClassifyCommand(tt.args, nil) + if got != tt.level { + t.Errorf("ClassifyCommand(%v) = %s, want %s", tt.args, got, tt.level) + } + }) + } +} + +func TestClassifyCommand_Overrides(t *testing.T) { + overrides := map[string]string{ + "rm": "low", + "curl": "critical", + } + + got := ClassifyCommand([]string{"rm", "-rf", "/"}, overrides) + if got != RiskLow { + t.Errorf("override rm to low: got %s", got) + } + + got = ClassifyCommand([]string{"curl", "https://example.com"}, overrides) + if got != RiskCritical { + t.Errorf("override curl to critical: got %s", got) + } + + got = ClassifyCommand([]string{"ls"}, overrides) + if got != RiskLow { + t.Errorf("ls (no override) should be low: got %s", got) + } +} + +func TestClassifyCommand_UnknownCommand(t *testing.T) { + got := ClassifyCommand([]string{"some_unknown_tool", "--flag"}, nil) + if got != RiskMedium { + t.Errorf("unknown command should default to medium, got %s", got) + } +} + +func TestClassifyCommand_FullPath(t *testing.T) { + got := ClassifyCommand([]string{"/usr/bin/rm", "-rf", "/"}, nil) + if got != RiskCritical { + t.Errorf("/usr/bin/rm -rf should be critical, got %s", got) + } + + got = ClassifyCommand([]string{"/bin/ls", "-la"}, nil) + if got != RiskLow { + t.Errorf("/bin/ls should be low, got %s", got) + } +} + +func TestIsAllowed(t *testing.T) { + tests := []struct { + level RiskLevel + threshold RiskLevel + allowed bool + }{ + {RiskLow, RiskMedium, true}, + {RiskMedium, RiskMedium, true}, + {RiskHigh, RiskMedium, false}, + {RiskCritical, RiskMedium, false}, + {RiskCritical, RiskCritical, true}, + {RiskLow, RiskLow, true}, + {RiskMedium, RiskLow, false}, + } + + for _, tt := range tests { + got := IsAllowed(tt.level, tt.threshold) + if got != tt.allowed { + t.Errorf("IsAllowed(%s, %s) = %v, want %v", tt.level, tt.threshold, got, tt.allowed) + } + } +} + +func TestParseRiskLevel(t *testing.T) { + tests := []struct { + input string + want RiskLevel + wantErr bool + }{ + {"low", RiskLow, false}, + {"medium", RiskMedium, false}, + {"high", RiskHigh, false}, + {"critical", RiskCritical, false}, + {"bogus", RiskMedium, true}, + {"", RiskMedium, true}, + {"invalid", RiskMedium, true}, + } + + for _, tt := range tests { + got, err := ParseRiskLevel(tt.input) + if (err != nil) != tt.wantErr { + t.Errorf("ParseRiskLevel(%q) error = %v, wantErr %v", tt.input, err, tt.wantErr) + continue + } + if !tt.wantErr && got != tt.want { + t.Errorf("ParseRiskLevel(%q) = %s, want %s", tt.input, got, tt.want) + } + // For error cases, verify the default is RiskMedium + if tt.wantErr && got != RiskMedium { + t.Errorf("ParseRiskLevel(%q) = %s on error, should default to RiskMedium", tt.input, got) + } + } +} + +func TestNormalizeFlags(t *testing.T) { + tests := []struct { + input []string + want []string + }{ + {[]string{"-rf"}, []string{"-r", "-f"}}, + {[]string{"-fr"}, []string{"-f", "-r"}}, + {[]string{"-r", "-f"}, []string{"-r", "-f"}}, + {[]string{"--force"}, []string{"--force"}}, + {[]string{"-f"}, []string{"-f"}}, + {[]string{"push"}, []string{"push"}}, + {[]string{"-rf", "dir", "--verbose"}, []string{"-r", "-f", "dir", "--verbose"}}, + } + + for _, tt := range tests { + got := normalizeFlags(tt.input) + if len(got) != len(tt.want) { + t.Errorf("normalizeFlags(%v) = %v, want %v", tt.input, got, tt.want) + continue + } + for i := range got { + if got[i] != tt.want[i] { + t.Errorf("normalizeFlags(%v) = %v, want %v", tt.input, got, tt.want) + break + } + } + } +} + +func TestMatchArgs_OrderIndependent(t *testing.T) { + tests := []struct { + name string + args []string + pattern []string + match bool + }{ + {"exact match", []string{"push", "-f"}, []string{"push", "-f"}, true}, + {"reversed order", []string{"-f", "push"}, []string{"push", "-f"}, true}, + {"extra flags between", []string{"push", "-x", "-f"}, []string{"push", "-f"}, true}, + {"missing token", []string{"push", "-x"}, []string{"push", "-f"}, false}, + {"empty pattern", []string{"push"}, []string{}, true}, + {"empty args", []string{}, []string{"push"}, false}, + {"superset ok", []string{"push", "-f", "--verbose", "origin"}, []string{"push", "-f"}, true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := matchArgs(tt.args, tt.pattern) + if got != tt.match { + t.Errorf("matchArgs(%v, %v) = %v, want %v", tt.args, tt.pattern, got, tt.match) + } + }) + } +} + +func TestClassifyCommand_ExtraArgModifiers(t *testing.T) { + extra := map[string][]ArgModifier{ + // Custom: "make deploy" should be critical + "make": { + {Args: []string{"deploy"}, Level: RiskCritical}, + }, + // Custom: "git push --mirror" should be critical (not in built-ins) + "git": { + {Args: []string{"push", "--mirror"}, Level: RiskCritical}, + }, + } + + tests := []struct { + name string + args []string + level RiskLevel + }{ + {"make deploy (extra)", []string{"make", "deploy"}, RiskCritical}, + {"make build (no extra)", []string{"make", "build"}, RiskMedium}, + {"git push --mirror (extra)", []string{"git", "push", "--mirror"}, RiskCritical}, + // Built-in still works: git push -f is critical even without extra + {"git push -f (built-in)", []string{"git", "push", "-f"}, RiskCritical}, + // Extra doesn't override a higher built-in: git push --force is already critical + {"git push --force (built-in wins)", []string{"git", "push", "--force"}, RiskCritical}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := ClassifyCommand(tt.args, nil, extra) + if got != tt.level { + t.Errorf("ClassifyCommand(%v, nil, extra) = %s, want %s", tt.args, got, tt.level) + } + }) + } +} + +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. + extra := map[string][]ArgModifier{ + "rm": { + {Args: []string{"-r", "-f"}, Level: RiskMedium}, + }, + } + + got := ClassifyCommand([]string{"rm", "-rf", "/"}, nil, extra) + if got != RiskCritical { + t.Errorf("built-in should win over extra for rm -rf: got %s", got) + } +} diff --git a/pkg/tools/shell/runner.go b/pkg/tools/shell/runner.go new file mode 100644 index 000000000..55f8a4d79 --- /dev/null +++ b/pkg/tools/shell/runner.go @@ -0,0 +1,210 @@ +package shell + +import ( + "bytes" + "context" + "fmt" + "os" + "path/filepath" + "strings" + "time" + + "mvdan.cc/sh/v3/expand" + "mvdan.cc/sh/v3/interp" + "mvdan.cc/sh/v3/syntax" +) + +// RunConfig holds all parameters for a single command execution. +type RunConfig struct { + Command string + Dir string + Timeout time.Duration + Restrict bool + WorkspaceDir string + + RiskThreshold RiskLevel + RiskOverrides map[string]string + ExtraArgModifiers map[string][]ArgModifier // user-defined, appended after built-ins + EnvAllowlist []string + EnvSet map[string]string +} + +// RunResult contains the output of a command execution. +type RunResult struct { + Output string + IsError bool +} + +// Run parses and executes a shell command using the in-process interpreter. +func Run(ctx context.Context, cfg RunConfig) RunResult { + parser := syntax.NewParser(syntax.Variant(syntax.LangBash)) + prog, err := parser.Parse(strings.NewReader(cfg.Command), "") + if err != nil { + return RunResult{ + Output: fmt.Sprintf("Failed to parse command: %v", err), + IsError: true, + } + } + + var runCtx context.Context + var cancel context.CancelFunc + if cfg.Timeout > 0 { + runCtx, cancel = context.WithTimeout(ctx, cfg.Timeout) + } else { + runCtx, cancel = context.WithCancel(ctx) + } + defer cancel() + + env := BuildSanitizedEnv(cfg.EnvAllowlist, cfg.EnvSet) + + var stdout, stderr bytes.Buffer + + opts := []interp.RunnerOption{ + interp.Env(env), + interp.StdIO(nil, &stdout, &stderr), + } + + if cfg.Dir != "" { + opts = append(opts, interp.Dir(cfg.Dir)) + } + + opts = append( + opts, + interp.ExecHandlers( + pathAwareExecHandler(env), + riskExecHandler(cfg.RiskThreshold, cfg.RiskOverrides, cfg.ExtraArgModifiers), + ), + ) + + if cfg.Restrict && cfg.WorkspaceDir != "" { + opts = append(opts, interp.OpenHandler(SandboxedOpenHandler(cfg.WorkspaceDir))) + } + + runner, err := interp.New(opts...) + if err != nil { + return RunResult{ + Output: fmt.Sprintf("Failed to create interpreter: %v", err), + IsError: true, + } + } + + err = runner.Run(runCtx, prog) + + output := stdout.String() + if stderr.Len() > 0 { + output += "\nSTDERR:\n" + stderr.String() + } + + if err != nil { + if runCtx.Err() == context.DeadlineExceeded { + msg := fmt.Sprintf("Command timed out after %v", cfg.Timeout) + return RunResult{Output: msg, IsError: true} + } + + errStr := err.Error() + if strings.Contains(errStr, "Command blocked by risk classifier") { + return RunResult{Output: errStr, IsError: true} + } + + output += fmt.Sprintf("\nExit code: %v", err) + } + + if output == "" { + output = "(no output)" + } + + maxLen := 10000 + if len(output) > maxLen { + output = output[:maxLen] + fmt.Sprintf("\n... (truncated, %d more chars)", len(output)-maxLen) + } + + return RunResult{ + Output: output, + IsError: err != nil, + } +} + +// riskExecHandler returns an ExecHandlers middleware that classifies each resolved +// command against the risk table before delegating to the default handler. +func riskExecHandler( + threshold RiskLevel, + overrides map[string]string, + extraMods map[string][]ArgModifier, +) func(next interp.ExecHandlerFunc) interp.ExecHandlerFunc { + return func(next interp.ExecHandlerFunc) interp.ExecHandlerFunc { + return func(ctx context.Context, args []string) error { + if len(args) == 0 { + return next(ctx, args) + } + + level := ClassifyCommand(args, overrides, extraMods) + if !IsAllowed(level, threshold) { + reason := "command risk exceeds configured threshold" + return fmt.Errorf("%s", BlockedCommandError(args, level, threshold, reason)) + } + + return next(ctx, args) + } + } +} + +// pathAwareExecHandler returns an ExecHandlerFunc that looks up commands +// using the interpreter's environment (not os.Getenv). This ensures that +// PATH from the sanitized environment is used for command resolution. +func pathAwareExecHandler(env expand.Environ) func(next interp.ExecHandlerFunc) interp.ExecHandlerFunc { + return func(next interp.ExecHandlerFunc) interp.ExecHandlerFunc { + return func(ctx context.Context, args []string) error { + if len(args) == 0 { + return next(ctx, args) + } + + // Look up command using interpreter's PATH + path, err := lookPath(env, args[0]) + if err != nil { + // Command not found in PATH, try the default handler + return next(ctx, args) + } + + // Replace command with full path and continue + fullArgs := append([]string{path}, args[1:]...) + return next(ctx, fullArgs) + } + } +} + +// lookPath searches for an executable named cmd in the directories +// listed in the PATH variable from the given environment. +func lookPath(env expand.Environ, cmd string) (string, error) { + // If command contains a slash, it's a path - return as-is + if strings.Contains(cmd, "/") { + return cmd, nil + } + + // Get PATH from interpreter environment + pathVar := env.Get("PATH") + if !pathVar.Set { + // PATH not set in environment - let default handler try + return "", fmt.Errorf("PATH not set") + } + if pathVar.Str == "" { + return "", fmt.Errorf("PATH is empty") + } + + // Search each directory in PATH + for _, dir := range filepath.SplitList(pathVar.Str) { + if dir == "" { + dir = "." + } + fullPath := filepath.Join(dir, cmd) + // Check if file exists and is executable + if stat, err := os.Stat(fullPath); err == nil && !stat.IsDir() { + // On Unix, check executable bit + if stat.Mode()&0o111 != 0 { + return fullPath, nil + } + } + } + + // Not found - let the default handler handle it + return "", fmt.Errorf("command %q not found in PATH", cmd) +} diff --git a/pkg/tools/shell/runner_test.go b/pkg/tools/shell/runner_test.go new file mode 100644 index 000000000..f6bf37e13 --- /dev/null +++ b/pkg/tools/shell/runner_test.go @@ -0,0 +1,243 @@ +package shell + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +func TestRun_Success(t *testing.T) { + result := Run(context.Background(), RunConfig{ + Command: "echo 'hello world'", + Dir: t.TempDir(), + Timeout: 5 * time.Second, + RiskThreshold: RiskMedium, + }) + + if result.IsError { + t.Fatalf("expected success, got error: %s", result.Output) + } + if !strings.Contains(result.Output, "hello world") { + t.Errorf("expected 'hello world' in output, got: %s", result.Output) + } +} + +func TestRun_BlocksDangerousCommand(t *testing.T) { + result := Run(context.Background(), RunConfig{ + Command: "rm -rf /", + Dir: t.TempDir(), + Timeout: 5 * time.Second, + RiskThreshold: RiskMedium, + }) + + if !result.IsError { + t.Fatal("expected rm -rf to be blocked") + } + if !strings.Contains(result.Output, "blocked") { + t.Errorf("expected 'blocked' in output: %s", result.Output) + } +} + +func TestRun_BlocksSudo(t *testing.T) { + result := Run(context.Background(), RunConfig{ + Command: "sudo ls", + Dir: t.TempDir(), + Timeout: 5 * time.Second, + RiskThreshold: RiskMedium, + }) + + if !result.IsError { + t.Fatal("expected sudo to be blocked") + } + if !strings.Contains(result.Output, "risk_level=critical") { + t.Errorf("expected risk_level=critical in output: %s", result.Output) + } +} + +func TestRun_BlocksVariableIndirection(t *testing.T) { + // x=rm; $x -rf / — the old regex system couldn't catch this. + result := Run(context.Background(), RunConfig{ + Command: `x=rm; $x -rf /`, + Dir: t.TempDir(), + Timeout: 5 * time.Second, + RiskThreshold: RiskMedium, + }) + + if !result.IsError { + t.Fatal("expected variable indirection bypass to be blocked") + } + if !strings.Contains(result.Output, "blocked") { + t.Errorf("expected 'blocked' in output: %s", result.Output) + } +} + +func TestRun_BlocksCommandSubstitution(t *testing.T) { + result := Run(context.Background(), RunConfig{ + Command: "$(echo rm) -rf /tmp/safe_test_dir", + Dir: t.TempDir(), + Timeout: 5 * time.Second, + RiskThreshold: RiskMedium, + }) + + if !result.IsError { + t.Fatal("expected command substitution bypass to be blocked") + } +} + +func TestRun_Timeout(t *testing.T) { + result := Run(context.Background(), RunConfig{ + Command: "sleep 60", + Dir: t.TempDir(), + Timeout: 200 * time.Millisecond, + RiskThreshold: RiskMedium, + }) + + if !result.IsError { + t.Fatal("expected timeout error") + } + if !strings.Contains(result.Output, "timed out") { + t.Errorf("expected 'timed out' in output: %s", result.Output) + } +} + +func TestRun_WorkingDir(t *testing.T) { + tmpDir := t.TempDir() + testFile := filepath.Join(tmpDir, "test.txt") + os.WriteFile(testFile, []byte("test content"), 0o644) + + result := Run(context.Background(), RunConfig{ + Command: "cat test.txt", + Dir: tmpDir, + Timeout: 5 * time.Second, + RiskThreshold: RiskMedium, + }) + + if result.IsError { + t.Fatalf("expected success: %s", result.Output) + } + if !strings.Contains(result.Output, "test content") { + t.Errorf("expected 'test content' in output: %s", result.Output) + } +} + +func TestRun_ParseError(t *testing.T) { + result := Run(context.Background(), RunConfig{ + Command: "echo 'unclosed", + Dir: t.TempDir(), + Timeout: 5 * time.Second, + RiskThreshold: RiskMedium, + }) + + if !result.IsError { + t.Fatal("expected parse error") + } + if !strings.Contains(result.Output, "parse") { + t.Errorf("expected 'parse' in error: %s", result.Output) + } +} + +func TestRun_StderrCapture(t *testing.T) { + result := Run(context.Background(), RunConfig{ + Command: "echo stdout_msg; echo stderr_msg >&2", + Dir: t.TempDir(), + Timeout: 5 * time.Second, + RiskThreshold: RiskMedium, + }) + + if !strings.Contains(result.Output, "stdout_msg") { + t.Errorf("expected stdout in output: %s", result.Output) + } + if !strings.Contains(result.Output, "stderr_msg") { + t.Errorf("expected stderr in output: %s", result.Output) + } +} + +func TestRun_HighThresholdAllowsRm(t *testing.T) { + tmpDir := t.TempDir() + testFile := filepath.Join(tmpDir, "delete_me.txt") + os.WriteFile(testFile, []byte("bye"), 0o644) + + result := Run(context.Background(), RunConfig{ + Command: "rm delete_me.txt", + Dir: tmpDir, + Timeout: 5 * time.Second, + RiskThreshold: RiskHigh, + }) + + if result.IsError { + t.Fatalf("with threshold=high, rm should be allowed: %s", result.Output) + } + if _, err := os.Stat(testFile); err == nil { + t.Error("file should have been deleted") + } +} + +func TestRun_EnvSanitization(t *testing.T) { + t.Setenv("OPENAI_API_KEY", "sk-secret-test") + t.Setenv("PATH", os.Getenv("PATH")) + + result := Run(context.Background(), RunConfig{ + Command: "env", + Dir: t.TempDir(), + Timeout: 5 * time.Second, + RiskThreshold: RiskMedium, + }) + + if result.IsError { + t.Fatalf("expected env command to succeed: %s", result.Output) + } + if strings.Contains(result.Output, "OPENAI_API_KEY") { + t.Error("OPENAI_API_KEY should not be in child environment") + } + if !strings.Contains(result.Output, "PATH=") { + t.Error("PATH should be in child environment") + } +} + +func TestRun_PipelineCommand(t *testing.T) { + result := Run(context.Background(), RunConfig{ + Command: "echo 'line1\nline2\nline3' | wc -l", + Dir: t.TempDir(), + Timeout: 5 * time.Second, + RiskThreshold: RiskMedium, + }) + + if result.IsError { + t.Fatalf("expected pipeline to succeed: %s", result.Output) + } +} + +func TestRun_DevNullRedirection(t *testing.T) { + result := Run(context.Background(), RunConfig{ + Command: "echo hello 2>/dev/null", + Dir: t.TempDir(), + Timeout: 5 * time.Second, + Restrict: true, + WorkspaceDir: t.TempDir(), + RiskThreshold: RiskMedium, + }) + + if result.IsError && strings.Contains(result.Output, "sandbox") { + t.Errorf("/dev/null should not be blocked: %s", result.Output) + } +} + +func TestRun_RiskOverrides(t *testing.T) { + // Override rm to low so it passes with threshold=medium. + result := Run(context.Background(), RunConfig{ + Command: "rm nonexistent_file_xyz 2>/dev/null; echo done", + Dir: t.TempDir(), + Timeout: 5 * time.Second, + RiskThreshold: RiskMedium, + RiskOverrides: map[string]string{"rm": "low"}, + }) + + // rm should be allowed because of override. The command may fail + // (file doesn't exist) but it should not be _blocked_. + if result.IsError && strings.Contains(result.Output, "blocked") { + t.Errorf("rm should be allowed with override: %s", result.Output) + } +} diff --git a/pkg/tools/shell/sandbox.go b/pkg/tools/shell/sandbox.go new file mode 100644 index 000000000..43729de81 --- /dev/null +++ b/pkg/tools/shell/sandbox.go @@ -0,0 +1,79 @@ +package shell + +import ( + "context" + "fmt" + "io" + "os" + "path/filepath" + + "mvdan.cc/sh/v3/interp" +) + +// SafePaths are kernel pseudo-devices that are always safe to open, +// regardless of workspace restriction. +var SafePaths = map[string]bool{ + "/dev/null": true, + "/dev/zero": true, + "/dev/random": true, + "/dev/urandom": true, + "/dev/stdin": true, + "/dev/stdout": true, + "/dev/stderr": true, +} + +// SandboxedOpenHandler returns an interp.OpenHandlerFunc that restricts +// shell redirections (>, <, >>) to files within the workspace directory. +// +// NOTE: This only intercepts opens from the shell interpreter for +// redirections. External programs open files via their own syscalls +// and are NOT restricted by this handler. +func SandboxedOpenHandler(workspaceDir string) interp.OpenHandlerFunc { + absWorkspace, err := filepath.Abs(workspaceDir) + if err != nil { + absWorkspace = workspaceDir + } + // Resolve workspace symlinks for accurate escape detection. + absWorkspace, err = filepath.EvalSymlinks(absWorkspace) + if err != nil { + // Non-fatal: continue with absolute path. Realpath failures + // are caught per-file when the sandbox is actually used. + } + + return func(ctx context.Context, path string, flag int, perm os.FileMode) (io.ReadWriteCloser, error) { + if SafePaths[path] { + 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 symlinks to prevent escape. + // If the file doesn't exist yet, resolve the parent. + var resolved string + if _, err := os.Lstat(absPath); err == nil { + resolved, err = filepath.EvalSymlinks(absPath) + if err != nil { + return nil, fmt.Errorf("sandbox: cannot resolve symlink %q: %w", path, err) + } + } else { + parentDir := filepath.Dir(absPath) + resolvedParent, err := filepath.EvalSymlinks(parentDir) + if err != nil { + return nil, fmt.Errorf("sandbox: cannot resolve parent dir %q: %w", parentDir, err) + } + resolved = filepath.Join(resolvedParent, filepath.Base(absPath)) + } + + if absWorkspace != "" { + rel, err := filepath.Rel(absWorkspace, resolved) + if err != nil || !filepath.IsLocal(rel) { + return nil, fmt.Errorf("sandbox: path %q resolves outside workspace", path) + } + } + + return os.OpenFile(resolved, flag, perm) + } +} diff --git a/pkg/tools/shell/sandbox_test.go b/pkg/tools/shell/sandbox_test.go new file mode 100644 index 000000000..2b008ed29 --- /dev/null +++ b/pkg/tools/shell/sandbox_test.go @@ -0,0 +1,123 @@ +package shell + +import ( + "context" + "os" + "path/filepath" + "testing" +) + +func TestSandboxedOpenHandler_AllowsInsideWorkspace(t *testing.T) { + workspace := t.TempDir() + handler := SandboxedOpenHandler(workspace) + + testFile := filepath.Join(workspace, "test.txt") + os.WriteFile(testFile, []byte("hello"), 0o644) + + f, err := handler(context.Background(), testFile, os.O_RDONLY, 0) + if err != nil { + t.Fatalf("expected open inside workspace to succeed: %v", err) + } + f.Close() +} + +func TestSandboxedOpenHandler_BlocksOutsideWorkspace(t *testing.T) { + workspace := t.TempDir() + handler := SandboxedOpenHandler(workspace) + + outsideFile := filepath.Join(t.TempDir(), "secret.txt") + os.WriteFile(outsideFile, []byte("secret"), 0o644) + + _, err := handler(context.Background(), outsideFile, os.O_RDONLY, 0) + if err == nil { + t.Fatal("expected open outside workspace to be blocked") + } +} + +func TestSandboxedOpenHandler_AllowsSafePaths(t *testing.T) { + workspace := t.TempDir() + handler := SandboxedOpenHandler(workspace) + + f, err := handler(context.Background(), "/dev/null", os.O_WRONLY, 0) + if err != nil { + t.Fatalf("expected /dev/null to be allowed: %v", err) + } + f.Close() +} + +func TestSandboxedOpenHandler_BlocksSymlinkEscape(t *testing.T) { + root := t.TempDir() + workspace := filepath.Join(root, "workspace") + secretDir := filepath.Join(root, "secret") + os.MkdirAll(workspace, 0o755) + os.MkdirAll(secretDir, 0o755) + os.WriteFile(filepath.Join(secretDir, "data.txt"), []byte("secret"), 0o644) + + link := filepath.Join(workspace, "escape") + if err := os.Symlink(secretDir, link); err != nil { + t.Skipf("symlinks not supported: %v", err) + } + + handler := SandboxedOpenHandler(workspace) + + target := filepath.Join(link, "data.txt") + _, err := handler(context.Background(), target, os.O_RDONLY, 0) + if err == nil { + t.Fatal("expected symlink escape to be blocked") + } +} + +func TestSandboxedOpenHandler_AllowsNewFileInWorkspace(t *testing.T) { + workspace := t.TempDir() + handler := SandboxedOpenHandler(workspace) + + newFile := filepath.Join(workspace, "new_output.txt") + f, err := handler(context.Background(), newFile, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o644) + if err != nil { + t.Fatalf("expected new file creation inside workspace to succeed: %v", err) + } + f.Close() + os.Remove(newFile) +} + +// TestSandboxedOpenHandler_AllowsDottedFiles verifies that files with +// names starting with ".." (like ".../file", "....txt", "..something") +// are NOT incorrectly blocked by the escape check. +// Regression test for bug where rel[:2] == ".." would match these. +func TestSandboxedOpenHandler_AllowsDottedFiles(t *testing.T) { + workspace := t.TempDir() + handler := SandboxedOpenHandler(workspace) + + // Create files with names that start with ".." but don't escape + testCases := []string{ + ".../test.txt", + "....txt", + "..something.txt", + } + + for _, tc := range testCases { + t.Run(tc, func(t *testing.T) { + fullPath := filepath.Join(workspace, tc) + + // Create parent directories if needed + if err := os.MkdirAll(filepath.Dir(fullPath), 0o755); err != nil { + t.Fatal(err) + } + + // Create the file + if err := os.WriteFile(fullPath, []byte("test"), 0o644); err != nil { + t.Fatal(err) + } + defer os.Remove(fullPath) + + // Try to open via sandbox handler + f, err := handler(context.Background(), fullPath, os.O_RDONLY, 0) + if err != nil { + t.Errorf("file %q should be allowed within workspace, got error: %v", tc, err) + } + if f != nil { + f.Close() + } + }) + } +} diff --git a/pkg/tools/shell_process_unix.go b/pkg/tools/shell_process_unix.go deleted file mode 100644 index 7b29a81bf..000000000 --- a/pkg/tools/shell_process_unix.go +++ /dev/null @@ -1,32 +0,0 @@ -//go:build !windows - -package tools - -import ( - "os/exec" - "syscall" -) - -func prepareCommandForTermination(cmd *exec.Cmd) { - if cmd == nil { - return - } - cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} -} - -func terminateProcessTree(cmd *exec.Cmd) error { - if cmd == nil || cmd.Process == nil { - return nil - } - - pid := cmd.Process.Pid - if pid <= 0 { - return nil - } - - // Kill the entire process group spawned by the shell command. - _ = syscall.Kill(-pid, syscall.SIGKILL) - // Fallback kill on the shell process itself. - _ = cmd.Process.Kill() - return nil -} diff --git a/pkg/tools/shell_process_windows.go b/pkg/tools/shell_process_windows.go deleted file mode 100644 index fe23b5c96..000000000 --- a/pkg/tools/shell_process_windows.go +++ /dev/null @@ -1,27 +0,0 @@ -//go:build windows - -package tools - -import ( - "os/exec" - "strconv" -) - -func prepareCommandForTermination(cmd *exec.Cmd) { - // no-op on Windows -} - -func terminateProcessTree(cmd *exec.Cmd) error { - if cmd == nil || cmd.Process == nil { - return nil - } - - pid := cmd.Process.Pid - if pid <= 0 { - return nil - } - - _ = exec.Command("taskkill", "/T", "/F", "/PID", strconv.Itoa(pid)).Run() - _ = cmd.Process.Kill() - return nil -} diff --git a/pkg/tools/shell_test.go b/pkg/tools/shell_test.go deleted file mode 100644 index ff9ea4a15..000000000 --- a/pkg/tools/shell_test.go +++ /dev/null @@ -1,445 +0,0 @@ -package tools - -import ( - "context" - "os" - "path/filepath" - "strings" - "testing" - "time" - - "github.com/sipeed/picoclaw/pkg/config" -) - -// TestShellTool_Success verifies successful command execution -func TestShellTool_Success(t *testing.T) { - tool, err := NewExecTool("", false) - if err != nil { - t.Errorf("unable to configure exec tool: %s", err) - } - - ctx := context.Background() - args := map[string]any{ - "command": "echo 'hello world'", - } - - result := tool.Execute(ctx, args) - - // Success should not be an error - if result.IsError { - t.Errorf("Expected success, got IsError=true: %s", result.ForLLM) - } - - // ForUser should contain command output - if !strings.Contains(result.ForUser, "hello world") { - t.Errorf("Expected ForUser to contain 'hello world', got: %s", result.ForUser) - } - - // ForLLM should contain full output - if !strings.Contains(result.ForLLM, "hello world") { - t.Errorf("Expected ForLLM to contain 'hello world', got: %s", result.ForLLM) - } -} - -// TestShellTool_Failure verifies failed command execution -func TestShellTool_Failure(t *testing.T) { - tool, err := NewExecTool("", false) - if err != nil { - t.Errorf("unable to configure exec tool: %s", err) - } - - ctx := context.Background() - args := map[string]any{ - "command": "ls /nonexistent_directory_12345", - } - - result := tool.Execute(ctx, args) - - // Failure should be marked as error - if !result.IsError { - t.Errorf("Expected error for failed command, got IsError=false") - } - - // ForUser should contain error information - if result.ForUser == "" { - t.Errorf("Expected ForUser to contain error info, got empty string") - } - - // ForLLM should contain exit code or error - if !strings.Contains(result.ForLLM, "Exit code") && result.ForUser == "" { - t.Errorf("Expected ForLLM to contain exit code or error, got: %s", result.ForLLM) - } -} - -// TestShellTool_Timeout verifies command timeout handling -func TestShellTool_Timeout(t *testing.T) { - tool, err := NewExecTool("", false) - if err != nil { - t.Errorf("unable to configure exec tool: %s", err) - } - - tool.SetTimeout(100 * time.Millisecond) - - ctx := context.Background() - args := map[string]any{ - "command": "sleep 10", - } - - result := tool.Execute(ctx, args) - - // Timeout should be marked as error - if !result.IsError { - t.Errorf("Expected error for timeout, got IsError=false") - } - - // Should mention timeout - if !strings.Contains(result.ForLLM, "timed out") && !strings.Contains(result.ForUser, "timed out") { - t.Errorf("Expected timeout message, got ForLLM: %s, ForUser: %s", result.ForLLM, result.ForUser) - } -} - -// TestShellTool_WorkingDir verifies custom working directory -func TestShellTool_WorkingDir(t *testing.T) { - // Create temp directory - tmpDir := t.TempDir() - testFile := filepath.Join(tmpDir, "test.txt") - os.WriteFile(testFile, []byte("test content"), 0o644) - - tool, err := NewExecTool("", false) - if err != nil { - t.Errorf("unable to configure exec tool: %s", err) - } - - ctx := context.Background() - args := map[string]any{ - "command": "cat test.txt", - "working_dir": tmpDir, - } - - result := tool.Execute(ctx, args) - - if result.IsError { - t.Errorf("Expected success in custom working dir, got error: %s", result.ForLLM) - } - - if !strings.Contains(result.ForUser, "test content") { - t.Errorf("Expected output from custom dir, got: %s", result.ForUser) - } -} - -// TestShellTool_DangerousCommand verifies safety guard blocks dangerous commands -func TestShellTool_DangerousCommand(t *testing.T) { - tool, err := NewExecTool("", false) - if err != nil { - t.Errorf("unable to configure exec tool: %s", err) - } - - ctx := context.Background() - args := map[string]any{ - "command": "rm -rf /", - } - - result := tool.Execute(ctx, args) - - // Dangerous command should be blocked - if !result.IsError { - t.Errorf("Expected dangerous command to be blocked (IsError=true)") - } - - if !strings.Contains(result.ForLLM, "blocked") && !strings.Contains(result.ForUser, "blocked") { - t.Errorf("Expected 'blocked' message, got ForLLM: %s, ForUser: %s", result.ForLLM, result.ForUser) - } -} - -func TestShellTool_DangerousCommand_KillBlocked(t *testing.T) { - tool, err := NewExecTool("", false) - if err != nil { - t.Errorf("unable to configure exec tool: %s", err) - } - - ctx := context.Background() - args := map[string]any{ - "command": "kill 12345", - } - - result := tool.Execute(ctx, args) - if !result.IsError { - t.Errorf("Expected kill command to be blocked") - } - if !strings.Contains(result.ForLLM, "blocked") && !strings.Contains(result.ForUser, "blocked") { - t.Errorf("Expected blocked message, got ForLLM: %s, ForUser: %s", result.ForLLM, result.ForUser) - } -} - -// TestShellTool_MissingCommand verifies error handling for missing command -func TestShellTool_MissingCommand(t *testing.T) { - tool, err := NewExecTool("", false) - if err != nil { - t.Errorf("unable to configure exec tool: %s", err) - } - - ctx := context.Background() - args := map[string]any{} - - result := tool.Execute(ctx, args) - - // Should return error result - if !result.IsError { - t.Errorf("Expected error when command is missing") - } -} - -// TestShellTool_StderrCapture verifies stderr is captured and included -func TestShellTool_StderrCapture(t *testing.T) { - tool, err := NewExecTool("", false) - if err != nil { - t.Errorf("unable to configure exec tool: %s", err) - } - - ctx := context.Background() - args := map[string]any{ - "command": "sh -c 'echo stdout; echo stderr >&2'", - } - - result := tool.Execute(ctx, args) - - // Both stdout and stderr should be in output - if !strings.Contains(result.ForLLM, "stdout") { - t.Errorf("Expected stdout in output, got: %s", result.ForLLM) - } - if !strings.Contains(result.ForLLM, "stderr") { - t.Errorf("Expected stderr in output, got: %s", result.ForLLM) - } -} - -// TestShellTool_OutputTruncation verifies long output is truncated -func TestShellTool_OutputTruncation(t *testing.T) { - tool, err := NewExecTool("", false) - if err != nil { - t.Errorf("unable to configure exec tool: %s", err) - } - - ctx := context.Background() - // Generate long output (>10000 chars) - args := map[string]any{ - "command": "python3 -c \"print('x' * 20000)\" || echo " + strings.Repeat("x", 20000), - } - - result := tool.Execute(ctx, args) - - // Should have truncation message or be truncated - if len(result.ForLLM) > 15000 { - t.Errorf("Expected output to be truncated, got length: %d", len(result.ForLLM)) - } -} - -// TestShellTool_WorkingDir_OutsideWorkspace verifies that working_dir cannot escape the workspace directly -func TestShellTool_WorkingDir_OutsideWorkspace(t *testing.T) { - root := t.TempDir() - workspace := filepath.Join(root, "workspace") - outsideDir := filepath.Join(root, "outside") - if err := os.MkdirAll(workspace, 0o755); err != nil { - t.Fatalf("failed to create workspace: %v", err) - } - if err := os.MkdirAll(outsideDir, 0o755); err != nil { - t.Fatalf("failed to create outside dir: %v", err) - } - - tool, err := NewExecTool(workspace, true) - if err != nil { - t.Errorf("unable to configure exec tool: %s", err) - } - - result := tool.Execute(context.Background(), map[string]any{ - "command": "pwd", - "working_dir": outsideDir, - }) - - if !result.IsError { - t.Fatalf("expected working_dir outside workspace to be blocked, got output: %s", result.ForLLM) - } - if !strings.Contains(result.ForLLM, "blocked") { - t.Errorf("expected 'blocked' in error, got: %s", result.ForLLM) - } -} - -// TestShellTool_WorkingDir_SymlinkEscape verifies that a symlink inside the workspace -// pointing outside cannot be used as working_dir to escape the sandbox. -func TestShellTool_WorkingDir_SymlinkEscape(t *testing.T) { - root := t.TempDir() - workspace := filepath.Join(root, "workspace") - secretDir := filepath.Join(root, "secret") - if err := os.MkdirAll(workspace, 0o755); err != nil { - t.Fatalf("failed to create workspace: %v", err) - } - if err := os.MkdirAll(secretDir, 0o755); err != nil { - t.Fatalf("failed to create secret dir: %v", err) - } - os.WriteFile(filepath.Join(secretDir, "secret.txt"), []byte("top secret"), 0o644) - - // symlink lives inside the workspace but resolves to secretDir outside it - link := filepath.Join(workspace, "escape") - if err := os.Symlink(secretDir, link); err != nil { - t.Skipf("symlinks not supported in this environment: %v", err) - } - - tool, err := NewExecTool(workspace, true) - if err != nil { - t.Errorf("unable to configure exec tool: %s", err) - } - - result := tool.Execute(context.Background(), map[string]any{ - "command": "cat secret.txt", - "working_dir": link, - }) - - if !result.IsError { - t.Fatalf("expected symlink working_dir escape to be blocked, got output: %s", result.ForLLM) - } - if !strings.Contains(result.ForLLM, "blocked") { - t.Errorf("expected 'blocked' in error, got: %s", result.ForLLM) - } -} - -// TestShellTool_RestrictToWorkspace verifies workspace restriction -func TestShellTool_RestrictToWorkspace(t *testing.T) { - tmpDir := t.TempDir() - tool, err := NewExecTool(tmpDir, false) - if err != nil { - t.Errorf("unable to configure exec tool: %s", err) - } - - tool.SetRestrictToWorkspace(true) - - ctx := context.Background() - args := map[string]any{ - "command": "cat ../../etc/passwd", - } - - result := tool.Execute(ctx, args) - - // Path traversal should be blocked - if !result.IsError { - t.Errorf("Expected path traversal to be blocked with restrictToWorkspace=true") - } - - if !strings.Contains(result.ForLLM, "blocked") && !strings.Contains(result.ForUser, "blocked") { - t.Errorf( - "Expected 'blocked' message for path traversal, got ForLLM: %s, ForUser: %s", - result.ForLLM, - result.ForUser, - ) - } -} - -// 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") - } -} diff --git a/pkg/tools/shell_timeout_unix_test.go b/pkg/tools/shell_timeout_unix_test.go deleted file mode 100644 index 357e1276e..000000000 --- a/pkg/tools/shell_timeout_unix_test.go +++ /dev/null @@ -1,65 +0,0 @@ -//go:build !windows - -package tools - -import ( - "context" - "os" - "path/filepath" - "strconv" - "strings" - "syscall" - "testing" - "time" -) - -func processExists(pid int) bool { - if pid <= 0 { - return false - } - err := syscall.Kill(pid, 0) - return err == nil || err == syscall.EPERM -} - -func TestShellTool_TimeoutKillsChildProcess(t *testing.T) { - tool, err := NewExecTool(t.TempDir(), false) - if err != nil { - t.Errorf("unable to configure exec tool: %s", err) - } - - tool.SetTimeout(500 * time.Millisecond) - - args := map[string]any{ - // Spawn a child process that would outlive the shell unless process-group kill is used. - "command": "sleep 60 & echo $! > child.pid; wait", - } - - result := tool.Execute(context.Background(), args) - if !result.IsError { - t.Fatalf("expected timeout error, got success: %s", result.ForLLM) - } - if !strings.Contains(result.ForLLM, "timed out") { - t.Fatalf("expected timeout message, got: %s", result.ForLLM) - } - - childPIDPath := filepath.Join(tool.workingDir, "child.pid") - data, err := os.ReadFile(childPIDPath) - if err != nil { - t.Fatalf("failed to read child pid file: %v", err) - } - - childPID, err := strconv.Atoi(strings.TrimSpace(string(data))) - if err != nil { - t.Fatalf("failed to parse child pid: %v", err) - } - - deadline := time.Now().Add(2 * time.Second) - for time.Now().Before(deadline) { - if !processExists(childPID) { - return - } - time.Sleep(50 * time.Millisecond) - } - - t.Fatalf("child process %d is still running after timeout", childPID) -} diff --git a/pkg/tools/shell_tool.go b/pkg/tools/shell_tool.go new file mode 100644 index 000000000..4276fd819 --- /dev/null +++ b/pkg/tools/shell_tool.go @@ -0,0 +1,228 @@ +package tools + +import ( + "context" + "fmt" + "os" + "time" + + "github.com/sipeed/picoclaw/pkg/config" + "github.com/sipeed/picoclaw/pkg/tools/shell" +) + +// Compile-time interface checks. +var ( + _ Tool = (*ExecTool)(nil) + _ AsyncTool = (*ExecTool)(nil) +) + +// ExecTool executes shell commands using an in-process interpreter +// with AST-based risk classification, env sanitization, and file-access sandboxing. +// +// ExecTool implements AsyncTool. When the LLM passes background=true the +// command runs in a goroutine and the result is delivered via the callback +// injected by the tool registry. +type ExecTool struct { + workingDir string + timeout time.Duration + restrictToWorkspace bool + + riskThreshold shell.RiskLevel + riskOverrides map[string]string + argModifiers map[string][]shell.ArgModifier + envAllowlist []string + envSet map[string]string + + callback AsyncCallback +} + +func NewExecTool(workingDir string, restrict bool) (*ExecTool, error) { + return NewExecToolWithConfig(workingDir, restrict, nil) +} + +func NewExecToolWithConfig(workingDir string, restrict bool, cfg *config.Config) (*ExecTool, error) { + t := &ExecTool{ + workingDir: workingDir, + timeout: 60 * time.Second, + restrictToWorkspace: restrict, + riskThreshold: shell.RiskMedium, + } + + if cfg != nil { + execCfg := cfg.Tools.Exec + + warnDeprecatedExecConfig(execCfg) + + if execCfg.RiskThreshold != "" { + level, err := shell.ParseRiskLevel(execCfg.RiskThreshold) + if err != nil { + fmt.Printf("Warning: invalid risk_threshold %q: %v. Using medium.\n", execCfg.RiskThreshold, err) + } else { + t.riskThreshold = level + } + } + t.riskOverrides = execCfg.RiskOverrides + t.argModifiers = parseArgModifiers(execCfg.ArgModifiers) + t.envAllowlist = execCfg.EnvAllowlist + t.envSet = execCfg.EnvSet + } + + return t, nil +} + +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.") + } + if len(cfg.CustomAllowPatterns) > 0 { + fmt.Println("Warning: 'custom_allow_patterns' is deprecated and ignored. " + + "Use 'risk_overrides' to lower the risk level of specific commands.") + } +} + +func (t *ExecTool) Name() string { + return "exec" +} + +func (t *ExecTool) Description() string { + return "Execute a shell command and return its output. Use with caution." +} + +// SetCallback implements AsyncTool. +func (t *ExecTool) SetCallback(cb AsyncCallback) { + t.callback = cb +} + +func (t *ExecTool) Parameters() map[string]any { + return map[string]any{ + "type": "object", + "properties": map[string]any{ + "command": map[string]any{ + "type": "string", + "description": "The shell command to execute", + }, + "working_dir": map[string]any{ + "type": "string", + "description": "Optional working directory for the command", + }, + "background": map[string]any{ + "type": "boolean", + "description": "Run the command in the background. Returns immediately; result is delivered asynchronously.", + }, + }, + "required": []string{"command"}, + } +} + +func (t *ExecTool) Execute(ctx context.Context, args map[string]any) *ToolResult { + command, ok := args["command"].(string) + if !ok { + return ErrorResult("command is required") + } + + cwd := t.workingDir + if wd, ok := args["working_dir"].(string); ok && wd != "" { + if t.restrictToWorkspace && t.workingDir != "" { + resolvedWD, err := validatePath(wd, t.workingDir, true) + if err != nil { + return ErrorResult("Command blocked by safety guard (" + err.Error() + ")") + } + cwd = resolvedWD + } else { + cwd = wd + } + } + + if cwd == "" { + wd, err := os.Getwd() + if err == nil { + cwd = wd + } + } + + cfg := shell.RunConfig{ + Command: command, + Dir: cwd, + Timeout: t.timeout, + Restrict: t.restrictToWorkspace, + WorkspaceDir: t.workingDir, + RiskThreshold: t.riskThreshold, + RiskOverrides: t.riskOverrides, + ExtraArgModifiers: t.argModifiers, + EnvAllowlist: t.envAllowlist, + EnvSet: t.envSet, + } + + background, _ := args["background"].(bool) + if background && t.callback != nil { + return t.executeAsync(ctx, cfg) + } + + result := shell.Run(ctx, cfg) + return &ToolResult{ + ForLLM: result.Output, + ForUser: result.Output, + IsError: result.IsError, + } +} + +// executeAsync launches the command in a goroutine and delivers the result +// through the AsyncCallback. The parent ctx is used for cancellation so the +// goroutine respects agent shutdown. +func (t *ExecTool) executeAsync(ctx context.Context, cfg shell.RunConfig) *ToolResult { + cb := t.callback // capture before goroutine + go func() { + result := shell.Run(ctx, cfg) + cb(ctx, &ToolResult{ + ForLLM: result.Output, + ForUser: result.Output, + IsError: result.IsError, + }) + }() + return AsyncResult(fmt.Sprintf("Running `%s` in background", cfg.Command)) +} + +func (t *ExecTool) SetTimeout(timeout time.Duration) { + t.timeout = timeout +} + +func (t *ExecTool) SetRestrictToWorkspace(restrict bool) { + t.restrictToWorkspace = restrict +} + +func (t *ExecTool) SetRiskThreshold(level shell.RiskLevel) { + t.riskThreshold = level +} + +// parseArgModifiers converts config.ArgModifierConfig entries into shell.ArgModifier. +func parseArgModifiers(raw map[string][]config.ArgModifierConfig) map[string][]shell.ArgModifier { + if len(raw) == 0 { + return nil + } + out := make(map[string][]shell.ArgModifier, len(raw)) + for cmd, entries := range raw { + for _, e := range entries { + level, err := shell.ParseRiskLevel(e.Level) + if err != nil { + fmt.Printf( + "Warning: invalid risk level %q for command %q: %v. Skipping this modifier.\n", + e.Level, + cmd, + err, + ) + continue + } + out[cmd] = append(out[cmd], shell.ArgModifier{ + Args: e.Args, + Level: level, + }) + } + } + return out +} diff --git a/pkg/tools/shell_tool_test.go b/pkg/tools/shell_tool_test.go new file mode 100644 index 000000000..863e19868 --- /dev/null +++ b/pkg/tools/shell_tool_test.go @@ -0,0 +1,145 @@ +package tools + +import ( + "context" + "sync" + "testing" + "time" +) + +func TestExecTool_SyncExecution(t *testing.T) { + tool, err := NewExecTool(t.TempDir(), false) + if err != nil { + t.Fatal(err) + } + + result := tool.Execute(context.Background(), map[string]any{ + "command": "echo sync_output", + }) + + if result.IsError { + t.Fatalf("expected success: %s", result.ForLLM) + } + if result.Async { + t.Error("sync execution should not be async") + } +} + +func TestExecTool_BackgroundWithoutCallback(t *testing.T) { + tool, err := NewExecTool(t.TempDir(), false) + if err != nil { + t.Fatal(err) + } + + // background=true but no callback set → falls through to sync + result := tool.Execute(context.Background(), map[string]any{ + "command": "echo fallback", + "background": true, + }) + + if result.Async { + t.Error("should fall back to sync when no callback is set") + } + if result.IsError { + t.Fatalf("expected success: %s", result.ForLLM) + } +} + +func TestExecTool_BackgroundWithCallback(t *testing.T) { + tool, err := NewExecTool(t.TempDir(), false) + if err != nil { + t.Fatal(err) + } + + var ( + mu sync.Mutex + received *ToolResult + ) + done := make(chan struct{}) + + tool.SetCallback(func(_ context.Context, r *ToolResult) { + mu.Lock() + received = r + mu.Unlock() + close(done) + }) + + result := tool.Execute(context.Background(), map[string]any{ + "command": "echo async_output", + "background": true, + }) + + if !result.Async { + t.Fatal("expected async result") + } + + select { + case <-done: + case <-time.After(10 * time.Second): + t.Fatal("timed out waiting for async callback") + } + + mu.Lock() + defer mu.Unlock() + + if received == nil { + t.Fatal("callback was never invoked") + } + if received.IsError { + t.Fatalf("async command failed: %s", received.ForLLM) + } +} + +func TestExecTool_BackgroundBlockedCommand(t *testing.T) { + tool, err := NewExecTool(t.TempDir(), false) + if err != nil { + t.Fatal(err) + } + + var ( + mu sync.Mutex + received *ToolResult + ) + done := make(chan struct{}) + + tool.SetCallback(func(_ context.Context, r *ToolResult) { + mu.Lock() + received = r + mu.Unlock() + close(done) + }) + + result := tool.Execute(context.Background(), map[string]any{ + "command": "sudo rm -rf /", + "background": true, + }) + + if !result.Async { + t.Fatal("expected async result even for blocked commands") + } + + select { + case <-done: + case <-time.After(10 * time.Second): + t.Fatal("timed out waiting for async callback") + } + + mu.Lock() + defer mu.Unlock() + + if received == nil { + t.Fatal("callback was never invoked") + } + if !received.IsError { + t.Error("blocked command should report error via callback") + } +} + +func TestExecTool_ImplementsAsyncTool(t *testing.T) { + tool, err := NewExecTool(t.TempDir(), false) + if err != nil { + t.Fatal(err) + } + + var _ AsyncTool = tool // compile-time check +}