From 79ae77306ab64cfca14d8f1d8b75928fca8deb55 Mon Sep 17 00:00:00 2001 From: vikingowl Date: Tue, 19 May 2026 17:05:39 +0200 Subject: [PATCH] chore(audit): polish remaining audit findings (M2, H1, H3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - M2: stop echoing the matched pattern name in the user-visible [BLOCKED: ...] message returned by the firewall. The pattern (and the matched secret class) still appear in the operator log, but the string sent back into the prompt is now generic. - H1: document Rule.Pattern semantics on the Rule type and pin them with a regression test. Pattern is a case-sensitive, exact substring match against the JSON-serialised tool arguments — not a glob, regex, or whitespace-insensitive match. The new test exercises both matches and the documented gotchas (double-space, case drift, tab). - H3: every code path in CommandExecutor.Execute that converts a hook failure into Allow via FailOpen now emits a WARN naming the hook and the failure mode (timeout / launch_error / parse_error), so chronic hook failure or abuse is visible in operator logs. Also tightens errcheck on permission/rule.go (Printer.Print on a strings.Builder cannot error in practice; make the intent explicit). --- internal/hook/command.go | 39 +++++++++++++---------- internal/permission/permission_test.go | 44 ++++++++++++++++++++++++++ internal/permission/rule.go | 27 ++++++++++++++-- internal/security/firewall.go | 2 +- 4 files changed, 91 insertions(+), 21 deletions(-) diff --git a/internal/hook/command.go b/internal/hook/command.go index 3ee02e5..8255a00 100644 --- a/internal/hook/command.go +++ b/internal/hook/command.go @@ -4,10 +4,26 @@ import ( "bytes" "context" "fmt" + "log/slog" "os/exec" "time" ) +// failOpenAction returns Allow when FailOpen is set, Deny otherwise, and logs +// a WARN naming the hook and the underlying reason so that abuse or chronic +// hook failure is visible in operator logs (audit H3). +func (c *CommandExecutor) failOpenAction(reason string, cause error) Action { + if c.def.FailOpen { + slog.Warn("hook fail-open: allowing tool call despite hook failure", + "hook", c.def.Name, + "reason", reason, + "error", cause, + ) + return Allow + } + return Deny +} + // CommandExecutor runs a shell command and interprets its stdin/stdout. type CommandExecutor struct { def HookDef @@ -40,32 +56,21 @@ func (c *CommandExecutor) Execute(ctx context.Context, payload []byte) (HookResu exitCode := 0 if runErr != nil { if ctx.Err() != nil { - // Context deadline exceeded — apply fail_open policy. - action := Deny - if c.def.FailOpen { - action = Allow - } - return HookResult{Action: action, Duration: duration}, fmt.Errorf("hook %q: timed out after %v", c.def.Name, timeout) + timeoutErr := fmt.Errorf("hook %q: timed out after %v", c.def.Name, timeout) + return HookResult{Action: c.failOpenAction("timeout", timeoutErr), Duration: duration}, timeoutErr } if exitErr, ok := runErr.(*exec.ExitError); ok { exitCode = exitErr.ExitCode() } else { - // Unexpected error launching the process. - action := Deny - if c.def.FailOpen { - action = Allow - } - return HookResult{Action: action, Duration: duration}, fmt.Errorf("hook %q: %w", c.def.Name, runErr) + launchErr := fmt.Errorf("hook %q: %w", c.def.Name, runErr) + return HookResult{Action: c.failOpenAction("launch_error", launchErr), Duration: duration}, launchErr } } action, transformed, err := ParseHookOutput(stdout.Bytes(), exitCode) if err != nil { - failAction := Deny - if c.def.FailOpen { - failAction = Allow - } - return HookResult{Action: failAction, Duration: duration}, fmt.Errorf("hook %q: %w", c.def.Name, err) + parseErr := fmt.Errorf("hook %q: %w", c.def.Name, err) + return HookResult{Action: c.failOpenAction("parse_error", parseErr), Duration: duration}, parseErr } return HookResult{Action: action, Output: transformed, Duration: duration}, nil diff --git a/internal/permission/permission_test.go b/internal/permission/permission_test.go index 7c174d4..bb45781 100644 --- a/internal/permission/permission_test.go +++ b/internal/permission/permission_test.go @@ -354,3 +354,47 @@ func TestChecker_ConcurrentSetModeAndCheck(t *testing.T) { } <-done } + +// Pins the documented Rule.Pattern semantics so any future refactor that +// changes substring/case/whitespace behaviour will fail loudly. Audit H1. +// +// Uses ModeBypass so the only deny vector is the rule itself (no mode-policy +// or no-prompt fallback can mask the result). Pattern uses a synthetic token +// to avoid overlap with safetyDenyPatterns. +func TestRule_PatternSemantics_SubstringExactCaseSensitive(t *testing.T) { + const pattern = "deploy --prod" + rules := []Rule{{Tool: "bash", Pattern: pattern, Action: ActionDeny}} + c := NewChecker(ModeBypass, rules, nil) + ctx := context.Background() + info := ToolInfo{Name: "bash"} + + matches := []string{ + `{"command":"deploy --prod"}`, + `{"command":"sudo deploy --prod"}`, + `{"prefix":"x","command":"deploy --prod -y"}`, + } + for _, args := range matches { + t.Run("matches: "+args, func(t *testing.T) { + err := c.Check(ctx, info, json.RawMessage(args)) + if !errors.Is(err, ErrDenied) { + t.Errorf("expected deny for %s, got %v", args, err) + } + }) + } + + // Documented gotchas — these intentionally do NOT match. The Pattern is a + // case-sensitive, exact substring; any whitespace or case drift skips it. + misses := []string{ + `{"command":"deploy --prod"}`, // double space inside the literal + `{"command":"DEPLOY --PROD"}`, // case differs + `{"command":"deploy\t--prod"}`, // tab instead of space + } + for _, args := range misses { + t.Run("does NOT match: "+args, func(t *testing.T) { + err := c.Check(ctx, info, json.RawMessage(args)) + if errors.Is(err, ErrDenied) { + t.Errorf("unexpectedly denied %s — Pattern semantics may have changed", args) + } + }) + } +} diff --git a/internal/permission/rule.go b/internal/permission/rule.go index 5a71854..4fca4a8 100644 --- a/internal/permission/rule.go +++ b/internal/permission/rule.go @@ -16,9 +16,29 @@ const ( ) // Rule defines a single permission rule. +// +// Pattern matching semantics — important caveats: +// +// - Tool is matched as a filepath.Match glob ("bash", "fs.*", "*"). +// - Pattern, if set, is a case-sensitive substring match against the +// JSON-serialized tool arguments. It is NOT a glob, regex, or +// structured-field match. +// +// Concretely, given `Pattern = "rm -rf"`, these match: +// - `{"command":"rm -rf /tmp"}` +// - `{"prefix":"echo rm -rf","other":"x"}` (substring appears anywhere) +// +// And these DO NOT match: +// - `{"command":"rm -rf /"}` (double space — substring is exact) +// - `{"command":"RM -RF /"}` (case differs) +// - `{"command":"rm\t-rf /"}` (tab vs space) +// +// Rule authors: write patterns that are unambiguous when serialized as JSON +// (no leading/trailing whitespace, no surrounding quotes) and remember that +// deny rules are bypass-immune — they fire before any mode check. type Rule struct { Tool string `toml:"tool"` // glob pattern: "bash", "fs.*", "*" - Pattern string `toml:"pattern"` // optional: argument pattern + Pattern string `toml:"pattern"` // optional: case-sensitive substring on JSON args (see godoc) Action Action `toml:"action"` } @@ -69,9 +89,10 @@ func extractCommands(node syntax.Command, printer *syntax.Printer, out *[]string return } } - // Everything else (simple command, pipe, subshell) — print as one unit + // Everything else (simple command, pipe, subshell) — print as one unit. + // Printer errors on a strings.Builder are unreachable (Builder.Write never errors). var b strings.Builder - printer.Print(&b, node) + _ = printer.Print(&b, node) if s := strings.TrimSpace(b.String()); s != "" { *out = append(*out, s) } diff --git a/internal/security/firewall.go b/internal/security/firewall.go index abbf89c..49643d0 100644 --- a/internal/security/firewall.go +++ b/internal/security/firewall.go @@ -132,7 +132,7 @@ func (f *Firewall) scanAndRedact(content, source string) string { "pattern", m.Pattern, "source", source, ) - return "[BLOCKED: content contained " + m.Pattern + "]" + return "[BLOCKED: content contained a secret]" default: f.logger.Debug("secret redacted", "pattern", m.Pattern,