From 2df95aa2b7d3984ca663614a1d4f5bdaec1872f8 Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso Date: Sat, 18 Jul 2026 16:10:46 +0200 Subject: [PATCH] fix: validate non_interactive and reject invalid values as deny (L-1) --- AGENTS.md | 2 +- docs/SECURITY.md | 2 +- internal/config/loader.go | 23 +++++++++++----- internal/config/loader_test.go | 42 ++++++++++++++++++++++++++++++ internal/danger/classifier.go | 22 +++++++++++++++- internal/danger/classifier_test.go | 40 ++++++++++++++++++++++++++++ 6 files changed, 122 insertions(+), 9 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 31ff3c3..3007661 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -158,7 +158,7 @@ Layered prompt-injection / approval-fatigue defenses. Full reference: [docs/SECU - **Schedule atomic-write hardening** (`internal/schedule/store.go` + `internal/fsatomic`) — schedule file writes now use `fsatomic.WriteFile`, which creates a random temp file with `O_EXCL`, fsyncs data and directory, and renames over the target. A swapped-in symlink is replaced rather than followed, closing the symlink-override attack on `schedules.json` / `schedule-state.json`. - **Schedule cross-process lock hard error** (`internal/schedule/store.go`) — `fileLock` now returns an error instead of silently falling back to a no-op releaser when `~/.odek/schedules.lock` cannot be opened or locked. Mutating schedule operations abort rather than risk two concurrent processes loading the same baseline and overwriting each other. - **Schedule JSON file-size cap** (`internal/schedule/store.go`) — `schedules.json` and `schedule-state.json` are rejected if larger than 10 MiB before being read into memory, preventing a tampered multi-gigabyte file from OOMing the scheduler. -- **Default non-interactive policy is deny** (`internal/danger/classifier.go`, `cmd/odek/main.go`) — headless/CI/piped runs no longer auto-approve dangerous operations. Operators must explicitly set `non_interactive: "allow"` for unattended execution. +- **Default non-interactive policy is deny** (`internal/danger/classifier.go`, `cmd/odek/main.go`) — headless/CI/piped runs no longer auto-approve dangerous operations. Operators must explicitly set `non_interactive: "allow"` for unattended execution. Invalid values such as `"prompt"` are rejected at load time with a warning and floored to `"deny"`, because a non-interactive environment cannot prompt. - **`~/.odek` trust-anchor protection** (`internal/danger/classifier.go`, `cmd/odek/file_tool.go`) — generic file tools reject writes to `~/.odek/config.json`, `secrets.env`, `IDENTITY.md`, `skills/`, `sessions/`, `audit/`, `plans/`, `schedules.json`, `schedule-state.json`, `mcp_approvals.json`, `mcp_tool_approvals.json`, `restart.json`, `telegram.lock`, and related state files. These paths classify as `system_write` and must be modified through their dedicated subsystems. Matching is case-insensitive so variants such as `CONFIG.JSON` or `SECRETS.ENV` are also blocked on case-insensitive filesystems (e.g. macOS APFS). Shell reads of these trust anchors are also escalated to `system_write`. - **Nonce'd tool-result delimiter** (`internal/loop/loop.go`) — the static `┌── TOOL RESULT: ... └── END TOOL RESULT: ...` delimiter is now unique per tool call via a random hex nonce embedded in both the opening and closing lines. A tool or MCP server can no longer forge the closing delimiter to break out of the data framing and inject instructions. - **`parallel_shell` context + process-group kill** (`cmd/odek/perf_tools.go`) — commands now run via `exec.CommandContext` bound to the agent context, in their own process group. Cancellation or timeout kills the whole group (negative PID), so `sh -c 'sleep 3600 &'` cannot leave orphaned children. Per-command timeouts are also capped at 30 minutes. diff --git a/docs/SECURITY.md b/docs/SECURITY.md index 50acbf1..33f39cd 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -721,7 +721,7 @@ Defaults: `FrictionThreshold=3`, `FrictionWindow=60s`. To opt out (TTYApprover o When odek cannot open a TTY (headless/CI/piped input), prompted operations used to fall back to the `non_interactive` action. The built-in default was `"allow"`, so a prompt-injected task such as `echo "task" | odek run "download and run attacker.sh"` could silently execute `curl … | sh`. -The default is now `"deny"`. Unattended runs must explicitly opt in to auto-approval by setting `"non_interactive": "allow"` in `~/.odek/config.json`, `ODEK_DANGEROUS_NON_INTERACTIVE=allow`, or the CLI. This makes the safe behaviour the default and closes the headless prompt-injection auto-execution vector. +The default is now `"deny"`. Unattended runs must explicitly opt in to auto-approval by setting `"non_interactive": "allow"` in `~/.odek/config.json` or the CLI. Any other value — including the previously accepted `"prompt"` — is rejected at load time with a warning and treated as `"deny"`, because a non-interactive environment cannot prompt. This makes the safe behaviour the default and closes the headless prompt-injection auto-execution vector. ### 26. Generic file tools cannot write `~/.odek` trust anchors diff --git a/internal/config/loader.go b/internal/config/loader.go index 6412afb..8d36327 100644 --- a/internal/config/loader.go +++ b/internal/config/loader.go @@ -1427,7 +1427,7 @@ func LoadConfig(cli CLIFlags) ResolvedConfig { SandboxEnv: cfg.SandboxEnv, SandboxVolumes: cfg.SandboxVolumes, Skills: resolveSkills(cfg.Skills), - Dangerous: resolveDangerous(cfg.Dangerous), + Dangerous: resolveDangerous(cfg.Dangerous, true), Memory: resolveMemory(cfg.Memory), Guard: resolveGuard(cfg.Guard), Embedding: cfg.Embedding, @@ -1632,11 +1632,22 @@ func resolveSkills(cfg *SkillsConfig) skills.SkillsConfig { // resolveDangerous merges file-level and potential env-level dangerous config. // If no config is provided, returns an empty DangerousConfig (safe defaults). -func resolveDangerous(cfg *danger.DangerousConfig) danger.DangerousConfig { - if cfg != nil { - return *cfg +// When validate is true, invalid non_interactive values are rejected with a +// warning and forced to "deny" so headless runs cannot accidentally +// auto-approve dangerous ops. +func resolveDangerous(cfg *danger.DangerousConfig, validate bool) danger.DangerousConfig { + if cfg == nil { + return danger.DangerousConfig{} } - return danger.DangerousConfig{} + resolved := *cfg + if validate && resolved.NonInteractive != nil { + if _, ok := danger.ParseNonInteractiveAction(*resolved.NonInteractive); !ok { + fmt.Fprintf(os.Stderr, "odek: warning: invalid non_interactive value %q — must be 'allow' or 'deny'; using 'deny'\n", *resolved.NonInteractive) + deny := "deny" + resolved.NonInteractive = &deny + } + } + return resolved } // resolveMemory merges file-level memory config with defaults. @@ -1955,7 +1966,7 @@ func resolveSchedules(cfg *SchedulesConfig) ScheduleConfig { } out.TelegramAdminChats = cfg.TelegramAdminChats out.TelegramAdminUsers = cfg.TelegramAdminUsers - out.Dangerous = resolveDangerous(cfg.Dangerous) + out.Dangerous = resolveDangerous(cfg.Dangerous, false) return out } diff --git a/internal/config/loader_test.go b/internal/config/loader_test.go index 0c68511..a382f99 100644 --- a/internal/config/loader_test.go +++ b/internal/config/loader_test.go @@ -223,6 +223,48 @@ func TestLoadConfig_GlobalFile(t *testing.T) { } } +func TestLoadConfig_InvalidNonInteractiveFlooredToDeny(t *testing.T) { + dir := t.TempDir() + t.Setenv("HOME", dir) + + cfgDir := filepath.Join(dir, ".odek") + os.MkdirAll(cfgDir, 0755) + cfgPath := filepath.Join(cfgDir, "config.json") + if err := os.WriteFile(cfgPath, []byte(`{ + "dangerous": { + "non_interactive": "prompt" + } + }`), 0644); err != nil { + t.Fatal(err) + } + + cfg := LoadConfig(CLIFlags{}) + if cfg.Dangerous.NonInteractive == nil || *cfg.Dangerous.NonInteractive != "deny" { + t.Errorf("invalid non_interactive should be floored to 'deny', got %v", cfg.Dangerous.NonInteractive) + } +} + +func TestLoadConfig_ValidNonInteractivePreserved(t *testing.T) { + dir := t.TempDir() + t.Setenv("HOME", dir) + + cfgDir := filepath.Join(dir, ".odek") + os.MkdirAll(cfgDir, 0755) + cfgPath := filepath.Join(cfgDir, "config.json") + if err := os.WriteFile(cfgPath, []byte(`{ + "dangerous": { + "non_interactive": "allow" + } + }`), 0644); err != nil { + t.Fatal(err) + } + + cfg := LoadConfig(CLIFlags{}) + if cfg.Dangerous.NonInteractive == nil || *cfg.Dangerous.NonInteractive != "allow" { + t.Errorf("valid non_interactive='allow' should be preserved, got %v", cfg.Dangerous.NonInteractive) + } +} + func TestLoadConfig_ProjectOverridesGlobal(t *testing.T) { dir := t.TempDir() diff --git a/internal/danger/classifier.go b/internal/danger/classifier.go index 5f667ca..6dd4536 100644 --- a/internal/danger/classifier.go +++ b/internal/danger/classifier.go @@ -589,13 +589,33 @@ func (c *DangerousConfig) ActionForCommand(cmd string) Action { // NonInteractiveAction returns the action to use when no TTY is available. // Defaults to Deny so unattended/headless runs fail closed rather than // auto-approving dangerous operations. +// +// Only "allow" and "deny" are accepted; any other value (including "prompt") +// is treated as "deny" because a non-interactive environment cannot prompt. func (c *DangerousConfig) NonInteractiveAction() Action { if c.NonInteractive != nil { - return parseAction(*c.NonInteractive) + action, ok := ParseNonInteractiveAction(*c.NonInteractive) + if ok { + return action + } } return Deny } +// ParseNonInteractiveAction parses the non_interactive config value. It accepts +// only "allow" and "deny"; "prompt" and any other value are rejected because +// prompting is impossible without a TTY. +func ParseNonInteractiveAction(s string) (Action, bool) { + switch strings.ToLower(strings.TrimSpace(s)) { + case "allow": + return Allow, true + case "deny": + return Deny, true + default: + return Deny, false + } +} + // CheckOperation checks whether a tool operation is allowed, denied, // or needs approval. Returns nil on allow, error on deny, and prompts // the user on prompt. Uses the configured Approver when set; falls back diff --git a/internal/danger/classifier_test.go b/internal/danger/classifier_test.go index dca0cfe..df801f7 100644 --- a/internal/danger/classifier_test.go +++ b/internal/danger/classifier_test.go @@ -961,6 +961,46 @@ func TestNonInteractiveAction_Deny(t *testing.T) { } } +func TestNonInteractiveAction_InvalidFailsClosed(t *testing.T) { + // Any value other than "allow" or "deny" (including the previously + // accepted "prompt") must fail closed to Deny, because a non-interactive + // environment cannot prompt. + for _, s := range []string{"prompt", "maybe", "yes", "", " prompt "} { + s := s + t.Run(s, func(t *testing.T) { + cfg := &DangerousConfig{NonInteractive: &s} + if got := cfg.NonInteractiveAction(); got != Deny { + t.Errorf("non-interactive %q = %s, want deny", s, got) + } + }) + } +} + +func TestParseNonInteractiveAction(t *testing.T) { + cases := []struct { + input string + want Action + wantOK bool + }{ + {"allow", Allow, true}, + {"deny", Deny, true}, + {"ALLOW", Allow, true}, + {"Deny", Deny, true}, + {"prompt", Deny, false}, + {"", Deny, false}, + {"maybe", Deny, false}, + } + for _, tc := range cases { + got, ok := ParseNonInteractiveAction(tc.input) + if ok != tc.wantOK { + t.Errorf("ParseNonInteractiveAction(%q) ok=%v, want %v", tc.input, ok, tc.wantOK) + } + if got != tc.want { + t.Errorf("ParseNonInteractiveAction(%q) = %s, want %s", tc.input, got, tc.want) + } + } +} + func TestActionFor_UnknownClass(t *testing.T) { cfg := &DangerousConfig{} action := cfg.ActionFor(RiskClass("nonexistent"))