refactor(sandbox): replace regex exec allowlist with command prefix dictionary
Replace the regex-based presetExecPatterns with presetAllowRules, a dictionary of command prefix strings (e.g. "go test", "pnpm run lint"). Benefits: - Readable at a glance without decoding regex alternations - No regex compilation; matching is a simple word-prefix check - Easy to extend with new commands per preset - Each preset's permissions are an explicit list ExecPolicy.AllowPattern (string) → ExecPolicy.AllowRules ([]string). ExecTool.SetAllowPatterns → ExecTool.SetAllowRules. Tests rewritten to use guardCommand directly instead of regex matching. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
73560dbf66
commit
d2801e3651
4 changed files with 202 additions and 60 deletions
|
|
@ -22,8 +22,8 @@ func IsValidPreset(p Preset) bool {
|
||||||
|
|
||||||
// ExecPolicy defines which commands are allowed for execution.
|
// ExecPolicy defines which commands are allowed for execution.
|
||||||
type ExecPolicy struct {
|
type ExecPolicy struct {
|
||||||
AllowPattern string // Prefix-match regex; matched commands are allowed
|
AllowRules []string // Command prefix allowlist (e.g., "go test", "pnpm run test")
|
||||||
LocalNetOnly bool // Restrict curl/wget to localhost and RFC 1918 private addresses
|
LocalNetOnly bool // Restrict curl/wget to localhost and RFC 1918 private addresses
|
||||||
}
|
}
|
||||||
|
|
||||||
// SandboxConfig describes the sandbox isolation policy for a preset.
|
// SandboxConfig describes the sandbox isolation policy for a preset.
|
||||||
|
|
@ -44,15 +44,43 @@ type SubagentEnvironment struct {
|
||||||
ContextFiles []string // Files to provide as context
|
ContextFiles []string // Files to provide as context
|
||||||
}
|
}
|
||||||
|
|
||||||
// presetExecPatterns maps presets to command allowlist regexes.
|
// presetAllowRules maps presets to command prefix allowlists.
|
||||||
|
// Each entry is a command prefix: the first N words of the executed command
|
||||||
|
// must match exactly. e.g. "go test" allows "go test ./..." but not "go build".
|
||||||
|
// A single word like "curl" allows any arguments.
|
||||||
// curl/wget are included where exec is allowed; LocalNetOnly in ExecPolicy
|
// curl/wget are included where exec is allowed; LocalNetOnly in ExecPolicy
|
||||||
// ensures all curl/wget requests are restricted to localhost and RFC 1918 addresses.
|
// ensures all curl/wget requests are restricted to localhost and RFC 1918 addresses.
|
||||||
var presetExecPatterns = map[Preset]string{
|
var presetAllowRules = map[Preset][]string{
|
||||||
PresetScout: ``, // No exec allowed
|
PresetScout: nil, // No exec allowed
|
||||||
PresetAnalyst: `^(go\s+(test|vet)|git\s+(log|diff|status)|curl|wget|grep|find)\b`,
|
PresetAnalyst: {
|
||||||
PresetCoder: `^(go\s+(test|vet|fmt)|gofmt|goimports|golangci-lint|prettier|eslint|black|ruff|cargo\s+(test|fmt|clippy)|pnpm\s+(test|run\s+(test|lint|format))|bun\s+(test|run\s+(test|lint|format))|uv\s+run\s+|curl|wget)\b`,
|
"go test", "go vet",
|
||||||
PresetWorker: `^(go\s+|pnpm\s+(install|add|run|test|build)|bun\s+(install|add|run|test|build)|uv\s+(run|sync|add|pip\s+install)|pip\s+install|cargo\s+|curl|wget)\b`,
|
"git log", "git diff", "git status",
|
||||||
PresetCoordinator: `^(go\s+|pnpm\s+|bun\s+|curl|wget)\b`,
|
"curl", "wget", "grep", "find",
|
||||||
|
},
|
||||||
|
PresetCoder: {
|
||||||
|
"go test", "go vet", "go fmt",
|
||||||
|
"gofmt", "goimports", "golangci-lint",
|
||||||
|
"prettier", "eslint", "black", "ruff",
|
||||||
|
"cargo test", "cargo fmt", "cargo clippy",
|
||||||
|
"pnpm test", "pnpm run test", "pnpm run lint", "pnpm run format",
|
||||||
|
"bun test", "bun run test", "bun run lint", "bun run format",
|
||||||
|
"uv run",
|
||||||
|
"curl", "wget",
|
||||||
|
},
|
||||||
|
PresetWorker: {
|
||||||
|
"go",
|
||||||
|
"pnpm install", "pnpm add", "pnpm run", "pnpm test", "pnpm build",
|
||||||
|
"bun install", "bun add", "bun run", "bun test", "bun build",
|
||||||
|
"uv run", "uv sync", "uv add", "uv pip install",
|
||||||
|
"pip install",
|
||||||
|
"cargo",
|
||||||
|
"curl", "wget",
|
||||||
|
},
|
||||||
|
PresetCoordinator: {
|
||||||
|
"go",
|
||||||
|
"pnpm", "bun",
|
||||||
|
"curl", "wget",
|
||||||
|
},
|
||||||
}
|
}
|
||||||
|
|
||||||
// presetSpawnablePresets maps presets to which presets they can spawn.
|
// presetSpawnablePresets maps presets to which presets they can spawn.
|
||||||
|
|
@ -110,13 +138,13 @@ func SandboxConfigForPreset(p Preset, writeRoot string) SandboxConfig {
|
||||||
config.WriteRoot = writeRoot
|
config.WriteRoot = writeRoot
|
||||||
}
|
}
|
||||||
|
|
||||||
// Set ExecPolicy if exec is allowed and pattern is non-empty.
|
// Set ExecPolicy if exec is allowed and rules are defined.
|
||||||
// LocalNetOnly is always true: curl/wget in subagents is for local server
|
// LocalNetOnly is always true: curl/wget in subagents is for local server
|
||||||
// testing only; external HTTP access goes through the web_fetch tool.
|
// testing only; external HTTP access goes through the web_fetch tool.
|
||||||
if allowed["exec"] {
|
if allowed["exec"] {
|
||||||
if pattern := presetExecPatterns[p]; pattern != "" {
|
if rules := presetAllowRules[p]; len(rules) > 0 {
|
||||||
config.ExecPolicy = &ExecPolicy{
|
config.ExecPolicy = &ExecPolicy{
|
||||||
AllowPattern: pattern,
|
AllowRules: rules,
|
||||||
LocalNetOnly: true,
|
LocalNetOnly: true,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -1,7 +1,6 @@
|
||||||
package tools
|
package tools
|
||||||
|
|
||||||
import (
|
import (
|
||||||
"regexp"
|
|
||||||
"testing"
|
"testing"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
@ -167,17 +166,15 @@ func TestSandboxConfigForPreset_Coordinator(t *testing.T) {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// TestPresetExecPatterns_Coder validates coder exec allowlist.
|
// TestPresetAllowRules_Coder validates coder exec allowlist.
|
||||||
func TestPresetExecPatterns_Coder(t *testing.T) {
|
func TestPresetAllowRules_Coder(t *testing.T) {
|
||||||
pattern, ok := presetExecPatterns[PresetCoder]
|
rules := presetAllowRules[PresetCoder]
|
||||||
if !ok || pattern == "" {
|
if len(rules) == 0 {
|
||||||
t.Fatalf("coder pattern missing or empty")
|
t.Fatalf("coder rules missing or empty")
|
||||||
}
|
}
|
||||||
|
|
||||||
re, err := regexp.Compile(pattern)
|
exec := NewExecTool(t.TempDir(), true)
|
||||||
if err != nil {
|
exec.SetAllowRules(rules)
|
||||||
t.Fatalf("failed to compile pattern: %v", err)
|
|
||||||
}
|
|
||||||
|
|
||||||
tests := []struct {
|
tests := []struct {
|
||||||
cmd string
|
cmd string
|
||||||
|
|
@ -185,35 +182,50 @@ func TestPresetExecPatterns_Coder(t *testing.T) {
|
||||||
}{
|
}{
|
||||||
{"go test ./...", true},
|
{"go test ./...", true},
|
||||||
{"go vet ./...", true},
|
{"go vet ./...", true},
|
||||||
|
{"go fmt ./...", true},
|
||||||
{"gofmt -w file.go", true},
|
{"gofmt -w file.go", true},
|
||||||
{"golangci-lint run", true},
|
{"golangci-lint run", true},
|
||||||
{"go build ./...", false},
|
{"cargo test", true},
|
||||||
{"npm install", false},
|
{"cargo fmt", true},
|
||||||
|
{"cargo clippy", true},
|
||||||
{"pnpm test", true},
|
{"pnpm test", true},
|
||||||
|
{"pnpm run test", true},
|
||||||
|
{"pnpm run lint", true},
|
||||||
|
{"pnpm run format", true},
|
||||||
|
{"bun test", true},
|
||||||
|
{"bun run test", true},
|
||||||
|
{"uv run pytest", true},
|
||||||
// curl/wget are in the allowlist; LocalNetOnly enforcement is at runtime
|
// curl/wget are in the allowlist; LocalNetOnly enforcement is at runtime
|
||||||
{"curl http://localhost:3000/health", true},
|
{"curl http://localhost:3000/health", true},
|
||||||
{"wget http://127.0.0.1:8080/status", true},
|
{"wget http://127.0.0.1:8080/status", true},
|
||||||
|
// blocked
|
||||||
|
{"go build ./...", false},
|
||||||
|
{"npm install", false},
|
||||||
|
{"pnpm install", false},
|
||||||
|
{"pnpm run build", false},
|
||||||
|
{"cargo build", false},
|
||||||
|
{"pwd", false},
|
||||||
|
{"ls", false},
|
||||||
}
|
}
|
||||||
|
|
||||||
for _, tt := range tests {
|
for _, tt := range tests {
|
||||||
gotOK := re.MatchString(tt.cmd)
|
result := exec.guardCommand(tt.cmd, t.TempDir())
|
||||||
|
gotOK := result == ""
|
||||||
if gotOK != tt.wantOK {
|
if gotOK != tt.wantOK {
|
||||||
t.Errorf("cmd %q: got %v, want %v", tt.cmd, gotOK, tt.wantOK)
|
t.Errorf("cmd %q: got allowed=%v, want %v (guard: %q)", tt.cmd, gotOK, tt.wantOK, result)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// TestPresetExecPatterns_Analyst validates analyst exec allowlist.
|
// TestPresetAllowRules_Analyst validates analyst exec allowlist.
|
||||||
func TestPresetExecPatterns_Analyst(t *testing.T) {
|
func TestPresetAllowRules_Analyst(t *testing.T) {
|
||||||
pattern, ok := presetExecPatterns[PresetAnalyst]
|
rules := presetAllowRules[PresetAnalyst]
|
||||||
if !ok || pattern == "" {
|
if len(rules) == 0 {
|
||||||
t.Fatalf("analyst pattern missing or empty")
|
t.Fatalf("analyst rules missing or empty")
|
||||||
}
|
}
|
||||||
|
|
||||||
re, err := regexp.Compile(pattern)
|
exec := NewExecTool(t.TempDir(), true)
|
||||||
if err != nil {
|
exec.SetAllowRules(rules)
|
||||||
t.Fatalf("failed to compile pattern: %v", err)
|
|
||||||
}
|
|
||||||
|
|
||||||
tests := []struct {
|
tests := []struct {
|
||||||
cmd string
|
cmd string
|
||||||
|
|
@ -223,16 +235,103 @@ func TestPresetExecPatterns_Analyst(t *testing.T) {
|
||||||
{"go vet ./...", true},
|
{"go vet ./...", true},
|
||||||
{"git log --oneline", true},
|
{"git log --oneline", true},
|
||||||
{"git diff HEAD", true},
|
{"git diff HEAD", true},
|
||||||
|
{"git status", true},
|
||||||
{"grep pattern file", true},
|
{"grep pattern file", true},
|
||||||
|
{"find . -name '*.go'", true},
|
||||||
{"curl http://example.com", true},
|
{"curl http://example.com", true},
|
||||||
|
// blocked
|
||||||
{"go build ./...", false},
|
{"go build ./...", false},
|
||||||
{"npm install", false},
|
{"npm install", false},
|
||||||
|
{"git push", false},
|
||||||
|
{"git checkout", false},
|
||||||
|
{"pwd", false},
|
||||||
|
{"ls", false},
|
||||||
}
|
}
|
||||||
|
|
||||||
for _, tt := range tests {
|
for _, tt := range tests {
|
||||||
gotOK := re.MatchString(tt.cmd)
|
result := exec.guardCommand(tt.cmd, t.TempDir())
|
||||||
|
gotOK := result == ""
|
||||||
if gotOK != tt.wantOK {
|
if gotOK != tt.wantOK {
|
||||||
t.Errorf("cmd %q: got %v, want %v", tt.cmd, gotOK, tt.wantOK)
|
t.Errorf("cmd %q: got allowed=%v, want %v (guard: %q)", tt.cmd, gotOK, tt.wantOK, result)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// TestPresetAllowRules_Worker validates worker exec allowlist.
|
||||||
|
func TestPresetAllowRules_Worker(t *testing.T) {
|
||||||
|
rules := presetAllowRules[PresetWorker]
|
||||||
|
if len(rules) == 0 {
|
||||||
|
t.Fatalf("worker rules missing or empty")
|
||||||
|
}
|
||||||
|
|
||||||
|
exec := NewExecTool(t.TempDir(), true)
|
||||||
|
exec.SetAllowRules(rules)
|
||||||
|
|
||||||
|
tests := []struct {
|
||||||
|
cmd string
|
||||||
|
wantOK bool
|
||||||
|
}{
|
||||||
|
{"go build ./...", true},
|
||||||
|
{"go test ./...", true},
|
||||||
|
{"pnpm install", true},
|
||||||
|
{"pnpm add lodash", true},
|
||||||
|
{"pnpm run dev", true},
|
||||||
|
{"bun install", true},
|
||||||
|
{"bun build", true},
|
||||||
|
{"uv run pytest", true},
|
||||||
|
{"uv sync", true},
|
||||||
|
{"uv pip install flask", true},
|
||||||
|
{"pip install flask", true},
|
||||||
|
{"cargo build", true},
|
||||||
|
{"cargo test", true},
|
||||||
|
{"curl http://localhost:8080", true},
|
||||||
|
// blocked
|
||||||
|
{"npm install", false},
|
||||||
|
{"pwd", false},
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, tt := range tests {
|
||||||
|
result := exec.guardCommand(tt.cmd, t.TempDir())
|
||||||
|
gotOK := result == ""
|
||||||
|
if gotOK != tt.wantOK {
|
||||||
|
t.Errorf("cmd %q: got allowed=%v, want %v (guard: %q)", tt.cmd, gotOK, tt.wantOK, result)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// TestPresetAllowRules_Coordinator validates coordinator exec allowlist.
|
||||||
|
func TestPresetAllowRules_Coordinator(t *testing.T) {
|
||||||
|
rules := presetAllowRules[PresetCoordinator]
|
||||||
|
if len(rules) == 0 {
|
||||||
|
t.Fatalf("coordinator rules missing or empty")
|
||||||
|
}
|
||||||
|
|
||||||
|
exec := NewExecTool(t.TempDir(), true)
|
||||||
|
exec.SetAllowRules(rules)
|
||||||
|
|
||||||
|
tests := []struct {
|
||||||
|
cmd string
|
||||||
|
wantOK bool
|
||||||
|
}{
|
||||||
|
{"go build ./...", true},
|
||||||
|
{"go test ./...", true},
|
||||||
|
{"pnpm install", true},
|
||||||
|
{"pnpm run dev", true},
|
||||||
|
{"bun install", true},
|
||||||
|
{"bun run dev", true},
|
||||||
|
{"curl http://localhost:8080", true},
|
||||||
|
{"wget http://127.0.0.1:3000", true},
|
||||||
|
// blocked
|
||||||
|
{"npm install", false},
|
||||||
|
{"cargo build", false},
|
||||||
|
{"pwd", false},
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, tt := range tests {
|
||||||
|
result := exec.guardCommand(tt.cmd, t.TempDir())
|
||||||
|
gotOK := result == ""
|
||||||
|
if gotOK != tt.wantOK {
|
||||||
|
t.Errorf("cmd %q: got allowed=%v, want %v (guard: %q)", tt.cmd, gotOK, tt.wantOK, result)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -122,7 +122,7 @@ type ExecTool struct {
|
||||||
workingDir string
|
workingDir string
|
||||||
timeout time.Duration
|
timeout time.Duration
|
||||||
denyPatterns []*regexp.Regexp
|
denyPatterns []*regexp.Regexp
|
||||||
allowPatterns []*regexp.Regexp
|
allowRules [][]string // pre-split command prefix allowlist
|
||||||
restrictToWorkspace bool
|
restrictToWorkspace bool
|
||||||
localNetOnly bool // restrict curl/wget to localhost + RFC 1918
|
localNetOnly bool // restrict curl/wget to localhost + RFC 1918
|
||||||
|
|
||||||
|
|
@ -217,7 +217,7 @@ func NewExecToolWithConfig(workingDir string, restrict bool, config *config.Conf
|
||||||
workingDir: workingDir,
|
workingDir: workingDir,
|
||||||
timeout: 5 * time.Minute,
|
timeout: 5 * time.Minute,
|
||||||
denyPatterns: denyPatterns,
|
denyPatterns: denyPatterns,
|
||||||
allowPatterns: nil,
|
allowRules: nil,
|
||||||
restrictToWorkspace: restrict,
|
restrictToWorkspace: restrict,
|
||||||
bgProcesses: make(map[string]*bgProcess),
|
bgProcesses: make(map[string]*bgProcess),
|
||||||
bgCtx: bgCtx,
|
bgCtx: bgCtx,
|
||||||
|
|
@ -708,22 +708,15 @@ func (t *ExecTool) guardCommand(command, cwd string) string {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
if len(t.allowPatterns) > 0 {
|
if len(t.allowRules) > 0 {
|
||||||
allowed := false
|
if !matchAllowRules(lower, t.allowRules) {
|
||||||
for _, pattern := range t.allowPatterns {
|
|
||||||
if pattern.MatchString(lower) {
|
|
||||||
allowed = true
|
|
||||||
break
|
|
||||||
}
|
|
||||||
}
|
|
||||||
if !allowed {
|
|
||||||
var b strings.Builder
|
var b strings.Builder
|
||||||
b.WriteString("Command blocked: not in allowlist [")
|
b.WriteString("Command blocked: not in allowlist [")
|
||||||
for i, p := range t.allowPatterns {
|
for i, rule := range t.allowRules {
|
||||||
if i > 0 {
|
if i > 0 {
|
||||||
b.WriteByte(',')
|
b.WriteByte(',')
|
||||||
}
|
}
|
||||||
b.WriteString(p.String())
|
b.WriteString(strings.Join(rule, " "))
|
||||||
}
|
}
|
||||||
b.WriteByte(']')
|
b.WriteByte(']')
|
||||||
return b.String()
|
return b.String()
|
||||||
|
|
@ -847,16 +840,38 @@ func (t *ExecTool) SetRestrictToWorkspace(restrict bool) {
|
||||||
t.restrictToWorkspace = restrict
|
t.restrictToWorkspace = restrict
|
||||||
}
|
}
|
||||||
|
|
||||||
func (t *ExecTool) SetAllowPatterns(patterns []string) error {
|
// SetAllowRules sets the command prefix allowlist.
|
||||||
t.allowPatterns = make([]*regexp.Regexp, 0, len(patterns))
|
// Each rule is a space-separated command prefix (e.g. "go test", "pnpm run lint").
|
||||||
for _, p := range patterns {
|
// A command is allowed if its first N words match any rule's N words exactly.
|
||||||
re, err := regexp.Compile(p)
|
func (t *ExecTool) SetAllowRules(rules []string) {
|
||||||
if err != nil {
|
t.allowRules = make([][]string, 0, len(rules))
|
||||||
return fmt.Errorf("invalid allow pattern %q: %w", p, err)
|
for _, r := range rules {
|
||||||
|
words := strings.Fields(strings.ToLower(r))
|
||||||
|
if len(words) > 0 {
|
||||||
|
t.allowRules = append(t.allowRules, words)
|
||||||
}
|
}
|
||||||
t.allowPatterns = append(t.allowPatterns, re)
|
|
||||||
}
|
}
|
||||||
return nil
|
}
|
||||||
|
|
||||||
|
// matchAllowRules checks if cmd matches any prefix in the allowlist.
|
||||||
|
func matchAllowRules(cmd string, rules [][]string) bool {
|
||||||
|
cmdWords := strings.Fields(cmd)
|
||||||
|
for _, ruleWords := range rules {
|
||||||
|
if len(cmdWords) < len(ruleWords) {
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
match := true
|
||||||
|
for i, rw := range ruleWords {
|
||||||
|
if cmdWords[i] != rw {
|
||||||
|
match = false
|
||||||
|
break
|
||||||
|
}
|
||||||
|
}
|
||||||
|
if match {
|
||||||
|
return true
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return false
|
||||||
}
|
}
|
||||||
|
|
||||||
func (t *ExecTool) SetLocalNetOnly(v bool) {
|
func (t *ExecTool) SetLocalNetOnly(v bool) {
|
||||||
|
|
|
||||||
|
|
@ -300,7 +300,7 @@ func (sm *SubagentManager) buildPresetRegistry(preset Preset, writeRoot string)
|
||||||
|
|
||||||
// Register exec and bg_monitor if allowed.
|
// Register exec and bg_monitor if allowed.
|
||||||
// Each subagent gets its own ExecTool to avoid mutating the shared instance's
|
// Each subagent gets its own ExecTool to avoid mutating the shared instance's
|
||||||
// allowPatterns (which would leak sandbox restrictions to the conductor).
|
// allowRules (which would leak sandbox restrictions to the conductor).
|
||||||
if config.AllowedTools["exec"] {
|
if config.AllowedTools["exec"] {
|
||||||
execWorkDir := writeRoot
|
execWorkDir := writeRoot
|
||||||
if execWorkDir == "" {
|
if execWorkDir == "" {
|
||||||
|
|
@ -312,7 +312,7 @@ func (sm *SubagentManager) buildPresetRegistry(preset Preset, writeRoot string)
|
||||||
return registry
|
return registry
|
||||||
}
|
}
|
||||||
if config.ExecPolicy != nil {
|
if config.ExecPolicy != nil {
|
||||||
_ = execTool.SetAllowPatterns([]string{config.ExecPolicy.AllowPattern})
|
execTool.SetAllowRules(config.ExecPolicy.AllowRules)
|
||||||
execTool.SetLocalNetOnly(config.ExecPolicy.LocalNetOnly)
|
execTool.SetLocalNetOnly(config.ExecPolicy.LocalNetOnly)
|
||||||
}
|
}
|
||||||
registry.Register(execTool)
|
registry.Register(execTool)
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue