chore(audit): polish remaining audit findings (M2, H1, H3)
- 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).
This commit is contained in:
+22
-17
@@ -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
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user