From 6c28061d67d02acd9efb605f84da9aa34b7ac76c Mon Sep 17 00:00:00 2001 From: Boris Bliznioukov Date: Thu, 5 Mar 2026 23:10:09 +0100 Subject: [PATCH] feat(exec): enable execution in config and update async callback handling Signed-off-by: Boris Bliznioukov --- config/config.example.json | 3 +- docs/design/DDR-shell-tool-hardening.md | 43 +++++++++++++------------ docs/tools_configuration.md | 17 +++------- pkg/tools/shell/sandbox.go | 9 +++--- pkg/tools/shell_tool.go | 11 +++++-- pkg/tools/shell_tool_test.go | 24 ++++++++++++-- 6 files changed, 62 insertions(+), 45 deletions(-) diff --git a/config/config.example.json b/config/config.example.json index bab6eb2d1..c43ac6497 100644 --- a/config/config.example.json +++ b/config/config.example.json @@ -340,6 +340,7 @@ } }, "exec": { + "enabled": true, "risk_threshold": "medium", "risk_overrides": {}, "arg_modifiers": {}, @@ -424,4 +425,4 @@ "host": "127.0.0.1", "port": 18790 } -} +} \ No newline at end of file diff --git a/docs/design/DDR-shell-tool-hardening.md b/docs/design/DDR-shell-tool-hardening.md index 92fdfffe4..b0098eac4 100644 --- a/docs/design/DDR-shell-tool-hardening.md +++ b/docs/design/DDR-shell-tool-hardening.md @@ -114,9 +114,10 @@ On Windows (`runtime.GOOS == "windows"`), the allowlist MUST additionally include: `PATHEXT`, `SYSTEMROOT`, `SYSTEMDRIVE`, `COMSPEC`, `APPDATA`, `USERPROFILE`, `HOMEDRIVE`, `HOMEPATH`. Without `SYSTEMROOT`, many Windows system calls fail. Without `PATHEXT`, executable lookup cannot probe extensions. 7a. The `pathAwareExecHandler` MUST resolve commands using the sanitized environment's PATH (not `os.Getenv`). The `lookPath` implementation MUST: - - Detect path-containing commands via `filepath.Base` (handles both `/` and `\`), not `strings.Contains(cmd, "/")`. - - On Windows: probe PATHEXT extensions (`.com`, `.exe`, `.bat`, `.cmd` by default) from the sanitized environment for each PATH directory. Accept any non-directory file (the executable bit is meaningless on Windows). - - On Unix: require the executable permission bit (`mode & 0o111 != 0`). + +- Detect path-containing commands via `filepath.Base` (handles both `/` and `\`), not `strings.Contains(cmd, "/")`. +- On Windows: probe PATHEXT extensions (`.com`, `.exe`, `.bat`, `.cmd` by default) from the sanitized environment for each PATH directory. Accept any non-directory file (the executable bit is meaningless on Windows). +- On Unix: require the executable permission bit (`mode & 0o111 != 0`). 7b. The `baseCommand` function MUST extract the basename via `filepath.Base`. On Windows only, it MUST additionally lowercase the result and strip known executable extensions (`.exe`, `.cmd`, `.bat`, `.com`) so that `C:\Windows\System32\cmd.exe` resolves to `cmd` in the risk table. On Unix, these transformations MUST NOT be applied: commands are case-sensitive and extensions are part of the filename. Stripping them on Unix would let an attacker disguise a binary as a known-safe command (e.g., a malicious `ls.exe` classified as low-risk `ls`). @@ -140,7 +141,7 @@ 12. The cron tool (`pkg/tools/cron.go`) MUST use the same `ExecTool` with the same guard system. -13. The `ExecTool` MUST implement the `AsyncExecutor` interface (`ExecuteAsync(ctx, args, cb)`). When the LLM passes `background=true` and a `bus.MessageBus` was injected at construction, the command MUST be launched in a goroutine; the result is delivered via `bus.PublishInbound` with `Channel: "system"`, `SenderID: "exec:"`, `ChatID: ":"`. When `background=true` but no bus is available (e.g. cron-created instances), execution falls through to synchronous mode. Compile-time interface check: `var _ AsyncExecutor = (*ExecTool)(nil)`. +13. The `ExecTool` MUST implement the `AsyncExecutor` interface (`ExecuteAsync(ctx, args, cb)`). When the LLM passes `background=true` and a non-nil `AsyncCallback` is provided by the tool registry, the command MUST be launched in a goroutine; the result is delivered via the callback as a completed (non-async) `ToolResult` with `ForLLM`, `ForUser`, and `IsError` populated. When `background=true` but no callback is available (e.g. cron-created instances), execution falls through to synchronous mode. Compile-time interface check: `var _ AsyncExecutor = (*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` + `AsyncExecutor` interfaces. Tests live alongside their source in both locations. @@ -148,7 +149,7 @@ - **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. `ExecTool` construction moved from `NewAgentInstance` to `registerSharedTools` (in `loop.go`) where the message bus is available for constructor injection. +- **Backward compatibility**: The `Tool` interface (`Name`, `Description`, `Parameters`, `Execute`) is unchanged. `ExecTool` construction moved from `NewAgentInstance` to the tool registration block in `loop.go` for consistency with other shared tools. --- @@ -203,19 +204,19 @@ ### Immediate impacts -| Who / what | Impact | -| ------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------- | -| `pkg/tools/shell_tool.go` | Thin adapter: `ExecTool` struct implementing `Tool` + `AsyncExecutor`. Delegates to `pkg/tools/shell/` subpackage. Bus-injected for background result delivery. | -| `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. | +| Who / what | Impact | +| ------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `pkg/tools/shell_tool.go` | Thin adapter: `ExecTool` struct implementing `Tool` + `AsyncExecutor`. Delegates to `pkg/tools/shell/` subpackage. Background results delivered via registry `AsyncCallback`. | +| `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 @@ -229,9 +230,9 @@ | `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(workDir, restrict, cfg, bus)`, `AsyncExecutor` impl, arg modifier wiring. | +| `pkg/tools/shell_tool.go` | Adapter: `ExecTool` struct, `NewExecToolWithConfig(workDir, restrict, cfg)`, `AsyncExecutor` 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. | +| `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 @@ -243,7 +244,7 @@ | 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 `AsyncExecutor` on `ExecTool` (bus-based background delivery) | Maintainer | — | **Done** | +| 7 | Implement `AsyncExecutor` on `ExecTool` (callback-based background delivery) | 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** | diff --git a/docs/tools_configuration.md b/docs/tools_configuration.md index 06f65bc7f..6e66d40c4 100644 --- a/docs/tools_configuration.md +++ b/docs/tools_configuration.md @@ -138,20 +138,11 @@ parameter. When the LLM passes `background: true`, the exec tool launches the command in a goroutine and immediately returns a confirmation to the agent. The result is -delivered asynchronously via `bus.PublishInbound` as a system-channel inbound -message: +delivered asynchronously via the `AsyncCallback` provided by the tool registry. -| Field | Value | -| ---------- | -------------------------------------- | -| `Channel` | `"system"` | -| `SenderID` | `"exec:"` | -| `ChatID` | `":"` | -| `Content` | stdout/stderr output (or error string) | - -This is the same delivery mechanism used by `SpawnTool` for subagent results. - -If no message bus is available (e.g. cron-created instances), `background: true` -falls through to synchronous execution. +If no callback is available (e.g. cron-created instances using +`NewExecToolWithConfig`), `background: true` falls through to synchronous +execution. ### Configuration Example diff --git a/pkg/tools/shell/sandbox.go b/pkg/tools/shell/sandbox.go index a26c8170c..301869b67 100644 --- a/pkg/tools/shell/sandbox.go +++ b/pkg/tools/shell/sandbox.go @@ -49,11 +49,12 @@ func SandboxedOpenHandler(workspaceDir string) interp.OpenHandlerFunc { 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. + if resolved, err := filepath.EvalSymlinks(absWorkspace); err == nil { + absWorkspace = resolved } + // Non-fatal: on EvalSymlinks failure, keep the original absolute + // path so sandbox checks still run. Per-file realpath failures + // are caught when the sandbox is actually used. return func(ctx context.Context, path string, flag int, perm os.FileMode) (io.ReadWriteCloser, error) { if isSafePath(path) { diff --git a/pkg/tools/shell_tool.go b/pkg/tools/shell_tool.go index 03bcd1e2b..2f97a042e 100644 --- a/pkg/tools/shell_tool.go +++ b/pkg/tools/shell_tool.go @@ -158,15 +158,20 @@ func (t *ExecTool) ExecuteAsync(ctx context.Context, args map[string]any, cb Asy go func() { result := shell.Run(ctx, cfg) - content := fmt.Sprintf("Background command completed: `%s`\n\n%s", - utils.Truncate(cfg.Command, 100), result.Output) + var content string if result.IsError { content = fmt.Sprintf("Background command failed: `%s`\n\n%s", utils.Truncate(cfg.Command, 100), result.Output) + } else { + content = fmt.Sprintf("Background command completed: `%s`\n\n%s", + utils.Truncate(cfg.Command, 100), result.Output) } if cb != nil { - cb(context.Background(), AsyncResult(content)) + cb(context.Background(), &ToolResult{ + ForLLM: content, + IsError: result.IsError, + }) } }() return AsyncResult(fmt.Sprintf("Running `%s` in background", utils.Truncate(cfg.Command, 100))) diff --git a/pkg/tools/shell_tool_test.go b/pkg/tools/shell_tool_test.go index 8b743f9d6..e4536442c 100644 --- a/pkg/tools/shell_tool_test.go +++ b/pkg/tools/shell_tool_test.go @@ -78,11 +78,20 @@ func TestExecTool_BackgroundWithCallback(t *testing.T) { if cbResult == nil { t.Fatal("callback was not invoked") } + if cbResult.Async { + t.Error("callback result should not be async (it is a completion)") + } + if cbResult.IsError { + t.Errorf("callback result should not be an error: %s", cbResult.ForLLM) + } if !strings.Contains(cbResult.ForLLM, "bg_output") { - t.Errorf("expected output in callback result: %s", cbResult.ForLLM) + t.Errorf("expected output in callback ForLLM: %s", cbResult.ForLLM) } if !strings.Contains(cbResult.ForLLM, "completed") { - t.Errorf("expected 'completed' in callback result: %s", cbResult.ForLLM) + t.Errorf("expected 'completed' in callback ForLLM: %s", cbResult.ForLLM) + } + if cbResult.ForUser == "" { + t.Error("callback ForUser should be populated for user notification") } } @@ -115,8 +124,17 @@ func TestExecTool_BackgroundBlockedCommand(t *testing.T) { if cbResult == nil { t.Fatal("callback was not invoked") } + if cbResult.Async { + t.Error("callback result should not be async (it is a completion)") + } + if !cbResult.IsError { + t.Error("callback result should be an error for blocked commands") + } if !strings.Contains(cbResult.ForLLM, "failed") { - t.Errorf("expected 'failed' in callback result: %s", cbResult.ForLLM) + t.Errorf("expected 'failed' in callback ForLLM: %s", cbResult.ForLLM) + } + if cbResult.ForUser == "" { + t.Error("callback ForUser should be populated for user notification") } }