Skip to content

Commit 40b9535

Browse files
leo-aa88cursoragent
leo-aa88
and
cursoragent
committed
fix(policy): address PR #126 review — shell_safe hardening and preset merge
Fail closed on shell metacharacters; remove dead synthetic requiredFor entries; use tri-state *bool merge for requireAllTools/permissive overlays; unify shell command helpers; simplify evaluator decision path; add adversarial and integration tests. Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent 096393f commit 40b9535

15 files changed

Lines changed: 337 additions & 148 deletions

‎CHANGELOG.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
99
### Added
1010

1111
- **Built-in policy presets** (issue #104): `strict`, `permissive`, and `shell_safe`. Select via `Project.spec.defaults.policy`, by referencing a preset name on agents/workflows, or with `Policy.spec.preset` (local rules layer on top). Presets expand during [NormalizeProjectGraph] so plan/validate show effective rules.
12-
- **`shell_safe` token classification** for native `command.run` / `run` / `exec` / `shell` operations: read-only tokens (`ls`, `cat`, …) run unattended; risky/unknown tokens and tools with side-effect metadata require `--approve`.
12+
- **`shell_safe` token classification** for native `command.run` / `run` / `exec` / `shell` operations: read-only first tokens (`ls`, `cat`, …) run unattended when the command contains no shell metacharacters (`;|&$`, newlines, `` ` ``, `$(…)`); risky tokens, unknown tokens, and side-effecting non-shell tools require `--approve`. **Heuristic only — not a sandbox.**
1313
- **`spec.safety` on Tool resources** (issue #103): optional `trusted`, `sideEffects`, and `requiresApproval` fields. [NormalizeProjectGraph] materializes fail-closed defaults on load.
1414
- **Policy safety fallback**: when no `approvals.requiredFor` entry matches the exact `uses` string, [policy.Derive] consults resolved safety metadata. Unattended mutating tools require `--approve` (exit code **5**, `approval_required`).
1515
- **Plan risk hints** for tools that will require approval at run, including decision source (`explicit_policy_rule`, `safety_metadata`, `fail_closed_default`).

‎internal/cli/init_test.go‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,34 @@ func TestInit_thenValidateSucceeds(t *testing.T) {
3535
}
3636
}
3737

38+
func TestInit_defaultPolicyExpandsShellSafePreset(t *testing.T) {
39+
parent := t.TempDir()
40+
name := "shellsafe"
41+
42+
ResetGlobalsForTest()
43+
cmd := NewRootCmd()
44+
cmd.SetOut(io.Discard)
45+
cmd.SetErr(io.Discard)
46+
cmd.SetArgs([]string{"init", name, "--parent-dir", parent})
47+
if err := cmd.Execute(); err != nil {
48+
t.Fatal(err)
49+
}
50+
51+
ResetGlobalsForTest()
52+
g := &Global{ProjectRoot: filepath.Join(parent, name)}
53+
graph, _, err := prepareProjectGraph(g.ProjectRoot, g)
54+
if err != nil {
55+
t.Fatal(err)
56+
}
57+
pr, ok := graph.Policies["default"]
58+
if !ok || pr == nil {
59+
t.Fatal("expected default policy")
60+
}
61+
if pr.Spec.ResolvedPreset != "shell_safe" {
62+
t.Fatalf("default policy ResolvedPreset = %q want shell_safe", pr.Spec.ResolvedPreset)
63+
}
64+
}
65+
3866
func TestInit_rejectsExistingDir(t *testing.T) {
3967
parent := t.TempDir()
4068
name := "dup"

‎internal/policy/derive.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,20 +50,22 @@ func EffectiveToolDecision(graph *spec.ProjectGraph, pol *spec.PolicySpec, toolN
5050
toolName = strings.TrimSpace(toolName)
5151
safety := resolvedSafetyForTool(graph, toolName)
5252
if pol != nil && pol.Approvals != nil {
53-
if pol.Approvals.Permissive {
53+
if spec.ApprovalPermissive(pol.Approvals) {
5454
return ToolDecision{
5555
Decision: DecisionAllow,
5656
Source: SourceExplicitPolicyRule,
5757
Safety: safety,
5858
}
5959
}
60-
if pol.Approvals.RequireAllTools {
60+
if spec.ApprovalRequireAllTools(pol.Approvals) {
6161
return ToolDecision{
6262
Decision: DecisionRequireApproval,
6363
Source: SourceExplicitPolicyRule,
6464
Safety: safety,
6565
}
6666
}
67+
// shell_safe plan risk is tool-granular (conservative): side-effecting tools flag approval;
68+
// runtime applies per-command token classification via shellSafeRequiresApproval.
6769
if spec.ResolvedPresetName(pol) == spec.PresetShellSafe {
6870
if safety.RequiresApproval || safety.SideEffects {
6971
return ToolDecision{

‎internal/policy/doc.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,9 @@
99
// Project.spec.defaults.policy, a Policy resource spec.preset, or by referencing a preset name
1010
// as the workflow/agent policy. [spec.ExpandPresetsInGraph] materializes effective rules during normalize.
1111
//
12+
// shell_safe uses first-token heuristics plus metacharacter fail-closed checks — not a sandbox.
13+
// Plan risk for shell_safe is tool-granular (conservative); runtime applies per-command classification.
14+
//
1215
// When no explicit approvals.requiredFor rule matches a tool call, [Derive] consults
1316
// [spec.ResolveToolSafety] metadata (fail-closed defaults; issue #103).
1417
//

‎internal/policy/evaluator.go‎

Lines changed: 38 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -54,60 +54,62 @@ func (e *evaluator) CheckStep(ctx context.Context, step StepContext) error {
5454
func (e *evaluator) CheckToolCall(ctx context.Context, call ToolCallContext) error {
5555
_ = ctx
5656
p := e.spec()
57-
if p != nil && p.Approvals != nil && p.Approvals.Permissive {
58-
if err := checkKnownTool(e.graph, call.Uses, p.Tools); err != nil {
59-
return err
60-
}
61-
return nil
62-
}
6357
if p != nil {
6458
if err := checkKnownTool(e.graph, call.Uses, p.Tools); err != nil {
6559
return err
6660
}
67-
if p.Approvals != nil && p.Approvals.RequireAllTools {
68-
if actionApproved(call.Uses, call.Run.ApprovedActions) {
69-
return nil
70-
}
71-
return denied(
72-
ReasonApprovalRequired,
73-
"policy: action requires explicit approval (--approve)",
74-
call.Uses,
75-
map[string]any{"requiredFor": call.Uses, "preset": spec.PresetStrict},
76-
)
77-
}
78-
if spec.ResolvedPresetName(p) == spec.PresetShellSafe && !shellSafeRequiresApproval(e.graph, call) {
61+
if p.Approvals != nil && spec.ApprovalPermissive(p.Approvals) {
7962
return nil
8063
}
81-
if presetRequiresApproval(p, e.graph, call) {
64+
}
65+
switch {
66+
case p != nil && spec.ResolvedPresetName(p) == spec.PresetShellSafe:
67+
if shellSafeRequiresApproval(e.graph, call) {
8268
if actionApproved(call.Uses, call.Run.ApprovedActions) {
8369
return nil
8470
}
85-
return denied(
86-
ReasonApprovalRequired,
87-
"policy: action requires explicit approval (--approve)",
88-
call.Uses,
89-
map[string]any{
90-
"requiredFor": call.Uses,
91-
"preset": spec.ResolvedPresetName(p),
92-
},
93-
)
71+
return toolCallApprovalDenied(call, p)
9472
}
95-
if approvalRequired(call.Uses, p.Approvals) {
96-
return checkApprovalGranted(call.Uses, p.Approvals, call.Run.ApprovedActions)
73+
return nil
74+
case requiresToolCallApproval(e.graph, p, call):
75+
if actionApproved(call.Uses, call.Run.ApprovedActions) {
76+
return nil
9777
}
78+
return toolCallApprovalDenied(call, p)
79+
}
80+
if p != nil && approvalRequired(call.Uses, p.Approvals) {
81+
return checkApprovalGranted(call.Uses, p.Approvals, call.Run.ApprovedActions)
9882
}
9983
return checkSafetyDerived(e.graph, call)
10084
}
10185

102-
func presetRequiresApproval(p *spec.PolicySpec, graph *spec.ProjectGraph, call ToolCallContext) bool {
103-
if p == nil || p.Approvals == nil {
86+
func requiresToolCallApproval(graph *spec.ProjectGraph, pol *spec.PolicySpec, call ToolCallContext) bool {
87+
if pol == nil {
10488
return false
10589
}
106-
if p.Approvals.RequireAllTools {
107-
return true
108-
}
109-
if spec.ResolvedPresetName(p) == spec.PresetShellSafe {
90+
if spec.ResolvedPresetName(pol) == spec.PresetShellSafe {
11091
return shellSafeRequiresApproval(graph, call)
11192
}
93+
if pol.Approvals == nil {
94+
return false
95+
}
96+
if spec.ApprovalRequireAllTools(pol.Approvals) {
97+
return true
98+
}
11299
return false
113100
}
101+
102+
func toolCallApprovalDenied(call ToolCallContext, pol *spec.PolicySpec) error {
103+
extra := map[string]any{"requiredFor": call.Uses}
104+
if pol != nil {
105+
if preset := spec.ResolvedPresetName(pol); preset != "" {
106+
extra["preset"] = preset
107+
}
108+
}
109+
return denied(
110+
ReasonApprovalRequired,
111+
"policy: action requires explicit approval (--approve)",
112+
call.Uses,
113+
extra,
114+
)
115+
}

‎internal/policy/presets_eval_test.go‎

Lines changed: 80 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,21 +57,69 @@ func TestCheckToolCall_shellSafe_gatesRm(t *testing.T) {
5757
}
5858
}
5959

60+
func TestCheckToolCall_shellSafe_gatesChainedCommand(t *testing.T) {
61+
pol, err := spec.BuildPreset(spec.PresetShellSafe)
62+
if err != nil {
63+
t.Fatal(err)
64+
}
65+
ev := NewEvaluator(shellSafeGraph(), &pol)
66+
err = ev.CheckToolCall(context.Background(), ToolCallContext{
67+
Uses: "tool.shell.run",
68+
With: map[string]any{"command": "ls; rm -rf /"},
69+
})
70+
if err == nil {
71+
t.Fatal("expected chained command to require approval")
72+
}
73+
}
74+
75+
func TestCheckToolCall_shellSafe_approveGrantsRm(t *testing.T) {
76+
pol, err := spec.BuildPreset(spec.PresetShellSafe)
77+
if err != nil {
78+
t.Fatal(err)
79+
}
80+
ev := NewEvaluator(shellSafeGraph(), &pol)
81+
uses := "tool.shell.command.run"
82+
err = ev.CheckToolCall(context.Background(), ToolCallContext{
83+
Run: RunContext{ApprovedActions: []string{uses}},
84+
Uses: uses,
85+
With: map[string]any{"command": "rm -rf /tmp/x"},
86+
})
87+
if err != nil {
88+
t.Fatalf("--approve should grant gated command: %v", err)
89+
}
90+
}
91+
6092
func TestCheckToolCall_shellSafe_unknownTokenGated(t *testing.T) {
6193
pol, err := spec.BuildPreset(spec.PresetShellSafe)
6294
if err != nil {
6395
t.Fatal(err)
6496
}
6597
ev := NewEvaluator(shellSafeGraph(), &pol)
6698
err = ev.CheckToolCall(context.Background(), ToolCallContext{
67-
Uses: "tool.shell.command.run",
99+
Uses: "tool.shell.exec",
68100
With: map[string]any{"command": "totally-unknown"},
69101
})
70102
if err == nil {
71103
t.Fatal("expected unknown token to gate")
72104
}
73105
}
74106

107+
func TestCheckToolCall_shellSafe_nonShellSideEffectToolGated(t *testing.T) {
108+
g := testGraphWithTools("slack")
109+
g.Tools["slack"].Spec.Safety = &spec.ToolSafety{SideEffects: spec.BoolPtr(true)}
110+
pol, err := spec.BuildPreset(spec.PresetShellSafe)
111+
if err != nil {
112+
t.Fatal(err)
113+
}
114+
ev := NewEvaluator(g, &pol)
115+
err = ev.CheckToolCall(context.Background(), ToolCallContext{
116+
Uses: "tool.slack.message.send",
117+
})
118+
if err == nil {
119+
t.Fatal("side-effecting non-shell tool should gate under shell_safe")
120+
}
121+
}
122+
75123
func TestCheckToolCall_strict_gatesAllTools(t *testing.T) {
76124
g := testGraphWithTools("helper")
77125
g.Tools["helper"].Spec.Safety = &spec.ToolSafety{SideEffects: spec.BoolPtr(false)}
@@ -104,6 +152,25 @@ func TestCheckToolCall_permissive_allowsMutatingTool(t *testing.T) {
104152
}
105153
}
106154

155+
func TestEngine_Evaluator_resolvesBuiltinShellSafeAfterNormalize(t *testing.T) {
156+
g := &spec.ProjectGraph{
157+
Spec: spec.ProjectSpec{
158+
Defaults: &spec.ProjectDefaults{Policy: spec.PresetShellSafe},
159+
},
160+
Tools: shellSafeGraph().Tools,
161+
}
162+
spec.NormalizeProjectGraph(g)
163+
eng := NewEngine(g)
164+
ev := eng.Evaluator(spec.PresetShellSafe)
165+
err := ev.CheckToolCall(context.Background(), ToolCallContext{
166+
Uses: "tool.shell.command.run",
167+
With: map[string]any{"command": "ls"},
168+
})
169+
if err != nil {
170+
t.Fatalf("builtin shell_safe after normalize should allow ls: %v", err)
171+
}
172+
}
173+
107174
func TestExpandPresetsInGraph_materializesDefault(t *testing.T) {
108175
g := &spec.ProjectGraph{
109176
Spec: spec.ProjectSpec{
@@ -139,3 +206,15 @@ func TestExpandPresetsInGraph_userPolicyOverridesBuiltin(t *testing.T) {
139206
t.Fatal("user policy should not be replaced by builtin")
140207
}
141208
}
209+
210+
func TestEffectiveToolDecision_shellSafe_toolGranularPlan(t *testing.T) {
211+
g := shellSafeGraph()
212+
pol, err := spec.BuildPreset(spec.PresetShellSafe)
213+
if err != nil {
214+
t.Fatal(err)
215+
}
216+
td := EffectiveToolDecision(g, &pol, "shell")
217+
if td.Decision != DecisionRequireApproval {
218+
t.Fatalf("plan should conservatively flag side-effecting shell tool: %+v", td)
219+
}
220+
}

‎internal/policy/shell_safe.go‎

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -12,18 +12,8 @@ func shellSafeRequiresApproval(graph *spec.ProjectGraph, call ToolCallContext) b
1212
return true
1313
}
1414
if spec.IsShellCommandOperation(operation) {
15-
cmd := spec.ExtractShellCommand(call.With)
16-
token := spec.FirstShellToken(cmd)
17-
switch spec.ClassifyShellToken(token) {
18-
case spec.ShellTokenReadOnly:
19-
return false
20-
case spec.ShellTokenGate, spec.ShellTokenUnknown:
21-
return true
22-
}
15+
return spec.ShellCommandRequiresApproval(spec.ExtractShellCommand(call.With))
2316
}
2417
safety := resolvedSafetyForTool(graph, toolName)
25-
if safety.RequiresApproval || safety.SideEffects {
26-
return true
27-
}
28-
return false
18+
return safety.RequiresApproval || safety.SideEffects
2919
}

‎internal/spec/kinds.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -182,10 +182,10 @@ type PolicyTools struct {
182182

183183
type PolicyApprovals struct {
184184
RequiredFor []string `yaml:"requiredFor,omitempty" json:"requiredFor,omitempty"`
185-
// RequireAllTools is set when the strict preset is expanded (every tool call requires approval).
186-
RequireAllTools bool `yaml:"requireAllTools,omitempty" json:"requireAllTools,omitempty"`
187-
// Permissive is set when the permissive preset is expanded (never gate tool calls).
188-
Permissive bool `yaml:"permissive,omitempty" json:"permissive,omitempty"`
185+
// RequireAllTools gates every tool call when true (strict preset). Pointer preserves tri-state merge.
186+
RequireAllTools *bool `yaml:"requireAllTools,omitempty" json:"requireAllTools,omitempty"`
187+
// Permissive skips tool-call approval when true (permissive preset). Pointer preserves tri-state merge.
188+
Permissive *bool `yaml:"permissive,omitempty" json:"permissive,omitempty"`
189189
}
190190

191191
type PolicySecurity struct {

‎internal/spec/policy_expand.go‎

Lines changed: 5 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -19,10 +19,7 @@ func ExpandPresetsInGraph(g *ProjectGraph) {
1919
if _, exists := g.Policies[name]; exists {
2020
continue
2121
}
22-
preset, err := BuildPreset(name)
23-
if err != nil {
24-
continue
25-
}
22+
preset, _ := BuildPreset(name)
2623
g.Policies[name] = &PolicyResource{
2724
APIVersion: APIVersionV0,
2825
Kind: KindPolicy,
@@ -34,11 +31,9 @@ func ExpandPresetsInGraph(g *ProjectGraph) {
3431
if pr == nil {
3532
continue
3633
}
37-
resolved, err := resolvePolicyResourcePreset(&pr.Spec)
38-
if err != nil || resolved == nil {
39-
continue
34+
if resolved, err := resolvePolicyResourcePreset(&pr.Spec); err == nil && resolved != nil {
35+
pr.Spec = *resolved
4036
}
41-
pr.Spec = *resolved
4237
}
4338
}
4439

@@ -47,10 +42,7 @@ func resolvePolicyResourcePreset(pol *PolicySpec) (*PolicySpec, error) {
4742
return nil, nil
4843
}
4944
presetName := strings.TrimSpace(pol.Preset)
50-
if presetName == "" {
51-
if pol.ResolvedPreset != "" {
52-
return nil, nil
53-
}
45+
if presetName == "" || pol.ResolvedPreset != "" {
5446
return nil, nil
5547
}
5648
if !IsBuiltinPreset(presetName) {
@@ -81,9 +73,8 @@ func collectReferencedPolicyNames(g *ProjectGraph) []string {
8173
add(wr.Spec.Policy)
8274
}
8375
}
84-
for name, pr := range g.Policies {
76+
for name := range g.Policies {
8577
add(name)
86-
_ = pr
8778
}
8879
out := make([]string, 0, len(seen))
8980
for name := range seen {

0 commit comments

Comments
 (0)