diff --git a/internal/forge/gen.go b/internal/forge/gen.go index 314563e6..5dbeb51f 100644 --- a/internal/forge/gen.go +++ b/internal/forge/gen.go @@ -77,6 +77,8 @@ func matchCondition(expr rules.MatchExpr) string { return "Any skill containing instruction-override phrasing, invisible Unicode, or encoded blobs." case len(expr.SkillAllowsTool) > 0: return "Any skill pre-approving Bash, Write, Edit, WebFetch, or NotebookEdit in allowed-tools." + case len(expr.SkillAllowsUnrestrictedTool) > 0: + return "Any skill pre-approving Bash, Write, Edit, WebFetch, or NotebookEdit in allowed-tools with a genuinely unrestricted grant." case expr.SkillModelInvocable != nil: return "Any skill where disable-model-invocation is not set to true." case expr.SkillBundledScriptNetworkEgress != nil: diff --git a/internal/rules/evaluator.go b/internal/rules/evaluator.go index f7eb6d10..7114575b 100644 --- a/internal/rules/evaluator.go +++ b/internal/rules/evaluator.go @@ -202,6 +202,9 @@ func (e MatchExpr) EvaluateSkill(s models.SkillDef, inv models.RepoInventory) bo if len(e.SkillAllowsTool) > 0 && !PredSkillAllowsTool(s, e.SkillAllowsTool) { return false } + if len(e.SkillAllowsUnrestrictedTool) > 0 && !PredSkillAllowsUnrestrictedTool(s, e.SkillAllowsUnrestrictedTool) { + return false + } if e.SkillModelInvocable != nil && PredSkillModelInvocable(s) != *e.SkillModelInvocable { return false } @@ -398,6 +401,7 @@ var predicatesByScope = map[models.Scope]map[string]bool{ models.ScopeSkill: { "skill_allows_unrestricted_shell": true, "skill_allows_tool": true, + "skill_allows_unrestricted_tool": true, "skill_model_invocable": true, "skill_body_has_dynamic_exec": true, "skill_dynamic_exec_touches_network_or_secrets": true, @@ -478,6 +482,7 @@ func (e MatchExpr) setPredicateNames() []string { // Skill scope add(e.SkillAllowsUnrestrictedShell != nil, "skill_allows_unrestricted_shell") add(len(e.SkillAllowsTool) > 0, "skill_allows_tool") + add(len(e.SkillAllowsUnrestrictedTool) > 0, "skill_allows_unrestricted_tool") add(e.SkillModelInvocable != nil, "skill_model_invocable") add(e.SkillBodyHasDynamicExec != nil, "skill_body_has_dynamic_exec") add(e.SkillDynamicExecTouchesNetworkOrSecrets != nil, "skill_dynamic_exec_touches_network_or_secrets") diff --git a/internal/rules/policies_test.go b/internal/rules/policies_test.go index 88eb2550..2ccec328 100644 --- a/internal/rules/policies_test.go +++ b/internal/rules/policies_test.go @@ -3509,6 +3509,9 @@ var policySkillRuleCases = []policySkillCase{ {"CSKILL-001 fires on Bash(*) grant", "CSKILL-001", models.SkillDef{Name: "leak", Location: models.Location{FilePath: ".claude/skills/leak/SKILL.md"}, ToolGrants: []models.ToolGrant{{Tool: "Bash", Pattern: "*"}}}, models.RepoInventory{}, true}, + {"CSKILL-001 fires on Bash(:*) grant", "CSKILL-001", + models.SkillDef{Name: "leak2", Location: models.Location{FilePath: ".claude/skills/leak2/SKILL.md"}, + ToolGrants: []models.ToolGrant{{Tool: "Bash", Pattern: ":*"}}}, models.RepoInventory{}, true}, {"CSKILL-001 silent on scoped Bash grant", "CSKILL-001", models.SkillDef{Name: "safe", Location: models.Location{FilePath: ".claude/skills/safe/SKILL.md"}, ToolGrants: []models.ToolGrant{{Tool: "Bash", Pattern: "git status *"}}}, models.RepoInventory{}, false}, @@ -3544,10 +3547,25 @@ var policySkillRuleCases = []policySkillCase{ {"CSKILL-050 fires on model-invocable skill granting Write", "CSKILL-050", models.SkillDef{Name: "auto", Location: models.Location{FilePath: ".claude/skills/auto/SKILL.md"}, ToolGrants: []models.ToolGrant{{Tool: "Write"}}}, models.RepoInventory{}, true}, + {"CSKILL-050 fires on unrestricted Bash(*) grant", "CSKILL-050", + models.SkillDef{Name: "auto2", Location: models.Location{FilePath: ".claude/skills/auto2/SKILL.md"}, + ToolGrants: []models.ToolGrant{{Tool: "Bash", Pattern: "*"}}}, models.RepoInventory{}, true}, + {"CSKILL-050 fires on Write with a path pattern (Claude Code never enforces it)", "CSKILL-050", + models.SkillDef{Name: "auto3", Location: models.Location{FilePath: ".claude/skills/auto3/SKILL.md"}, + ToolGrants: []models.ToolGrant{{Tool: "Write", Pattern: "src/**"}}}, models.RepoInventory{}, true}, {"CSKILL-050 silent when manual-only", "CSKILL-050", models.SkillDef{Name: "manual", Location: models.Location{FilePath: ".claude/skills/manual/SKILL.md"}, DisableModelInvocation: true, ToolGrants: []models.ToolGrant{{Tool: "Write"}}}, models.RepoInventory{}, false}, + {"CSKILL-050 silent on scoped Bash grant", "CSKILL-050", + models.SkillDef{Name: "scoped", Location: models.Location{FilePath: ".claude/skills/scoped/SKILL.md"}, + ToolGrants: []models.ToolGrant{{Tool: "Bash", Pattern: "git status:*"}}}, models.RepoInventory{}, false}, + {"CSKILL-050 silent on scoped Edit and WebFetch grants", "CSKILL-050", + models.SkillDef{Name: "scoped2", Location: models.Location{FilePath: ".claude/skills/scoped2/SKILL.md"}, + ToolGrants: []models.ToolGrant{ + {Tool: "Edit", Pattern: "docs/**"}, + {Tool: "WebFetch", Pattern: "domain:example.com"}, + }}, models.RepoInventory{}, false}, {"CSKILL-010 fires on bundled script network egress", "CSKILL-010", models.SkillDef{Name: "boot", Location: models.Location{FilePath: ".claude/skills/boot/SKILL.md"}, @@ -3582,6 +3600,10 @@ var policySkillRuleCases = []policySkillCase{ models.SkillDef{Name: "reader", Description: "Read-only summarizer; does not modify anything.", Location: models.Location{FilePath: ".claude/skills/reader/SKILL.md"}, ToolGrants: []models.ToolGrant{{Tool: "Read"}, {Tool: "Grep"}}}, models.RepoInventory{}, false}, + {"CSKILL-060 silent when the read-only claim's Bash grant is scoped", "CSKILL-060", + models.SkillDef{Name: "scoped-reader", Description: "Read-only: summarizes git history. Cannot modify anything.", + Location: models.Location{FilePath: ".claude/skills/scoped-reader/SKILL.md"}, + ToolGrants: []models.ToolGrant{{Tool: "Bash", Pattern: "git status:*"}}}, models.RepoInventory{}, false}, {"CSKILL-070 fires on skill with empty description", "CSKILL-070", models.SkillDef{Name: "helper", Location: models.Location{FilePath: ".claude/skills/helper/SKILL.md"}, Description: ""}, @@ -3627,10 +3649,18 @@ var policySkillRuleCases = []policySkillCase{ models.SkillDef{Name: "security-audit", Location: models.Location{FilePath: ".claude/skills/security-audit/SKILL.md"}, ToolGrants: []models.ToolGrant{{Tool: "Bash", Pattern: "*"}}}, models.RepoInventory{}, true}, + {"CSKILL-082 fires when a security-purpose skill grants bare Bash", "CSKILL-082", + models.SkillDef{Name: "security-audit2", + Location: models.Location{FilePath: ".claude/skills/security-audit2/SKILL.md"}, + ToolGrants: []models.ToolGrant{{Tool: "Bash"}}}, models.RepoInventory{}, true}, {"CSKILL-082 silent when a security-purpose skill only reads", "CSKILL-082", models.SkillDef{Name: "security-audit", Location: models.Location{FilePath: ".claude/skills/security-audit/SKILL.md"}, ToolGrants: []models.ToolGrant{{Tool: "Read"}, {Tool: "Grep"}}}, models.RepoInventory{}, false}, + {"CSKILL-082 silent when a security-purpose skill's Bash grant is scoped", "CSKILL-082", + models.SkillDef{Name: "security-audit3", + Location: models.Location{FilePath: ".claude/skills/security-audit3/SKILL.md"}, + ToolGrants: []models.ToolGrant{{Tool: "Bash", Pattern: "git status:*"}}}, models.RepoInventory{}, false}, {"CSKILL-083 fires when the body has no error-handling language", "CSKILL-083", models.SkillDef{Name: "helper", diff --git a/internal/rules/predicates.go b/internal/rules/predicates.go index 3dd1834e..82d87889 100644 --- a/internal/rules/predicates.go +++ b/internal/rules/predicates.go @@ -989,6 +989,10 @@ func PredSkillBodyHasInjectionMarker(s models.SkillDef) bool { // PredSkillAllowsTool reports whether the skill pre-approves any of names via // allowed-tools. It matches the parsed grant's Tool first (so "Bash(git *)" // matches "Bash") then the raw allowed-tools tokens. +// +// This predicate does NOT consult the grant's Pattern — "Bash(git status:*)" +// matches identically to a bare "Bash". Use PredSkillAllowsUnrestrictedTool +// when a narrowly-scoped grant should not count. func PredSkillAllowsTool(s models.SkillDef, names []string) bool { want := make(map[string]bool, len(names)) for _, n := range names { @@ -1007,17 +1011,82 @@ func PredSkillAllowsTool(s models.SkillDef, names []string) bool { return false } +// skillGrantIsUnrestricted reports whether a parsed ToolGrant places no real +// constraint on what it authorizes, per Claude Code's own permission-rule +// semantics (see code.claude.com/docs/en/permissions): +// +// - Bash / PowerShell: unrestricted only for an empty, "*", or ":*" pattern +// — Bash(git status:*) is a genuine command-prefix restriction. +// - Edit (the tool Claude Code actually consults for file-path rules; see +// below): unrestricted only for an empty pattern or a catch-all glob +// ("*", "**", "/**", "//**"), never a real gitignore-style path pattern +// like "docs/**". +// - WebFetch: unrestricted only for an empty or "*" pattern — a +// "domain:example.com" specifier is a genuine restriction. +// - Write and NotebookEdit: ALWAYS unrestricted, regardless of any pattern +// written in the grant. Claude Code accepts a path specifier on these +// tools but never consults it — only an Edit rule is checked for file +// writes (it warns at startup that a Write/NotebookEdit path rule is +// dead). So "Write(src/**)" grants exactly as much as bare "Write". +func skillGrantIsUnrestricted(g models.ToolGrant) bool { + pattern := strings.TrimSpace(g.Pattern) + switch g.Tool { + case "Write", "NotebookEdit": + return true + case "Bash", "PowerShell": + switch pattern { + case "", "*", ":*": + return true + } + return false + case "Edit": + switch pattern { + case "", "*", "**", "/**", "//**": + return true + } + return false + case "WebFetch": + switch pattern { + case "", "*": + return true + } + return false + default: + return pattern == "" + } +} + // PredSkillAllowsUnrestrictedShell reports whether the skill pre-approves // unrestricted shell via allowed-tools — a bare `Bash` grant or a wildcard // pattern (`Bash(*)` / `Bash(:*)`). allowed-tools is an auto-approval list, not a // sandbox, so this lets the skill run any shell command without prompting. func PredSkillAllowsUnrestrictedShell(s models.SkillDef) bool { for _, g := range s.ToolGrants { - if g.Tool != "Bash" { - continue + if g.Tool == "Bash" && skillGrantIsUnrestricted(g) { + return true } - switch strings.TrimSpace(g.Pattern) { - case "", "*", ":*": + } + return false +} + +// PredSkillAllowsUnrestrictedTool reports whether the skill pre-approves any +// of names via allowed-tools with a genuinely unrestricted grant — unlike +// PredSkillAllowsTool, a narrowly-scoped grant (Bash(git status:*), +// Edit(docs/**), WebFetch(domain:example.com)) does not count. A raw +// allowed-tools token with no parsed grant (s.AllowedTools fallback) is +// treated as unrestricted, since a bare token is unrestricted by definition. +func PredSkillAllowsUnrestrictedTool(s models.SkillDef, names []string) bool { + want := make(map[string]bool, len(names)) + for _, n := range names { + want[n] = true + } + for _, g := range s.ToolGrants { + if want[g.Tool] && skillGrantIsUnrestricted(g) { + return true + } + } + for _, t := range s.AllowedTools { + if want[t] { return true } } @@ -1090,14 +1159,16 @@ func PredSkillBundledFileHasHardcodedSecret(s models.SkillDef) bool { var sideEffectingSkillTools = []string{"Bash", "Write", "Edit", "WebFetch", "NotebookEdit"} // PredSkillDescriptionToolMismatch reports whether the skill's description -// explicitly claims to be read-only / side-effect-free while it pre-approves a -// side-effecting tool (or unrestricted shell) — the metadata then understates -// the real capability, which is the signal a reviewer relies on. +// explicitly claims to be read-only / side-effect-free while it pre-approves an +// unrestricted side-effecting tool grant — the metadata then understates the +// real capability, which is the signal a reviewer relies on. A narrowly-scoped +// grant (Bash(git status:*), Edit(docs/**)) does not misrepresent the +// description, so it is not counted. func PredSkillDescriptionToolMismatch(s models.SkillDef) bool { if !skillReadOnlyClaimRe.MatchString(s.Description) { return false } - return PredSkillAllowsUnrestrictedShell(s) || PredSkillAllowsTool(s, sideEffectingSkillTools) + return PredSkillAllowsUnrestrictedTool(s, sideEffectingSkillTools) } func PredSkillHasDescription(s models.SkillDef) bool { diff --git a/internal/rules/predicates_test.go b/internal/rules/predicates_test.go index 09d8f1e7..a569939b 100644 --- a/internal/rules/predicates_test.go +++ b/internal/rules/predicates_test.go @@ -1608,3 +1608,89 @@ func TestPredSubagentGrantsTool(t *testing.T) { t.Errorf("expected true: Bash(npm run *) grants Bash via ToolGrants") } } + +// TestPredSkillAllowsUnrestrictedTool exercises the per-tool "unrestricted" +// table: Bash/PowerShell and WebFetch/Edit have real pattern-argument +// restrictions Claude Code enforces, while Write and NotebookEdit accept a +// path specifier but never consult it — so any grant on those two tools is +// unrestricted regardless of pattern. +func TestPredSkillAllowsUnrestrictedTool(t *testing.T) { + names := []string{"Bash", "Write", "Edit", "WebFetch", "NotebookEdit"} + skillWith := func(grant models.ToolGrant) models.SkillDef { + return models.SkillDef{Name: "x", ToolGrants: []models.ToolGrant{grant}} + } + + cases := []struct { + name string + grant models.ToolGrant + want bool + }{ + // Bash: unrestricted only for "", "*", ":*". + {"bare Bash", models.ToolGrant{Tool: "Bash"}, true}, + {"Bash(*)", models.ToolGrant{Tool: "Bash", Pattern: "*"}, true}, + {"Bash(:*)", models.ToolGrant{Tool: "Bash", Pattern: ":*"}, true}, + {"Bash(git status:*)", models.ToolGrant{Tool: "Bash", Pattern: "git status:*"}, false}, + {"Bash(npm run *)", models.ToolGrant{Tool: "Bash", Pattern: "npm run *"}, false}, + + // Edit: unrestricted only for "" or a catch-all glob. + {"bare Edit", models.ToolGrant{Tool: "Edit"}, true}, + {"Edit(*)", models.ToolGrant{Tool: "Edit", Pattern: "*"}, true}, + {"Edit(**)", models.ToolGrant{Tool: "Edit", Pattern: "**"}, true}, + {"Edit(/**)", models.ToolGrant{Tool: "Edit", Pattern: "/**"}, true}, + {"Edit(//**)", models.ToolGrant{Tool: "Edit", Pattern: "//**"}, true}, + {"Edit(docs/**)", models.ToolGrant{Tool: "Edit", Pattern: "docs/**"}, false}, + {"Edit(src/**/*.ts)", models.ToolGrant{Tool: "Edit", Pattern: "src/**/*.ts"}, false}, + + // WebFetch: unrestricted only for "" or "*". + {"bare WebFetch", models.ToolGrant{Tool: "WebFetch"}, true}, + {"WebFetch(*)", models.ToolGrant{Tool: "WebFetch", Pattern: "*"}, true}, + {"WebFetch(domain:example.com)", models.ToolGrant{Tool: "WebFetch", Pattern: "domain:example.com"}, false}, + + // Write / NotebookEdit: ALWAYS unrestricted — Claude Code never + // consults a path rule written for these tools. + {"bare Write", models.ToolGrant{Tool: "Write"}, true}, + {"Write(src/**)", models.ToolGrant{Tool: "Write", Pattern: "src/**"}, true}, + {"bare NotebookEdit", models.ToolGrant{Tool: "NotebookEdit"}, true}, + {"NotebookEdit(notebooks/**)", models.ToolGrant{Tool: "NotebookEdit", Pattern: "notebooks/**"}, true}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := rules.PredSkillAllowsUnrestrictedTool(skillWith(tc.grant), names) + if got != tc.want { + t.Errorf("PredSkillAllowsUnrestrictedTool(%+v) = %v, want %v", tc.grant, got, tc.want) + } + }) + } + + // A skill with only a raw allowed-tools token (no parsed ToolGrants) is + // treated as unrestricted, since a bare token is unrestricted by + // definition — mirrors PredSkillAllowsTool's raw-token fallback. + raw := models.SkillDef{Name: "raw", AllowedTools: []string{"Bash"}} + if !rules.PredSkillAllowsUnrestrictedTool(raw, names) { + t.Errorf("expected true: raw AllowedTools token %q falls back to unrestricted", "Bash") + } +} + +// TestPredSkillDescriptionToolMismatch_ScopedGrantIsSilent guards the +// CSKILL-060 fix end-to-end: a scoped grant must not trip the description +// mismatch even though the description makes a read-only claim. +func TestPredSkillDescriptionToolMismatch_ScopedGrantIsSilent(t *testing.T) { + scoped := models.SkillDef{ + Name: "scoped-reader", + Description: "Read-only: summarizes git history. Cannot modify anything.", + ToolGrants: []models.ToolGrant{{Tool: "Bash", Pattern: "git status:*"}}, + } + if rules.PredSkillDescriptionToolMismatch(scoped) { + t.Errorf("expected false: Bash(git status:*) is a scoped grant, not a mismatch") + } + + unrestricted := models.SkillDef{ + Name: "sneaky", + Description: "Read-only: summarizes the diff. Cannot run commands.", + ToolGrants: []models.ToolGrant{{Tool: "Bash", Pattern: "*"}}, + } + if !rules.PredSkillDescriptionToolMismatch(unrestricted) { + t.Errorf("expected true: Bash(*) is unrestricted and contradicts the read-only claim") + } +} diff --git a/internal/rules/schema.go b/internal/rules/schema.go index c9476f1f..071d6722 100644 --- a/internal/rules/schema.go +++ b/internal/rules/schema.go @@ -100,6 +100,7 @@ type MatchExpr struct { // Skill-scope predicates SkillAllowsUnrestrictedShell *bool `yaml:"skill_allows_unrestricted_shell,omitempty"` SkillAllowsTool []string `yaml:"skill_allows_tool,omitempty"` + SkillAllowsUnrestrictedTool []string `yaml:"skill_allows_unrestricted_tool,omitempty"` SkillModelInvocable *bool `yaml:"skill_model_invocable,omitempty"` SkillBodyHasDynamicExec *bool `yaml:"skill_body_has_dynamic_exec,omitempty"` SkillDynamicExecTouchesNetworkOrSecrets *bool `yaml:"skill_dynamic_exec_touches_network_or_secrets,omitempty"` diff --git a/internal/rules/schema.yaml b/internal/rules/schema.yaml index 1fa834d7..11cd8422 100644 --- a/internal/rules/schema.yaml +++ b/internal/rules/schema.yaml @@ -399,12 +399,28 @@ rules: # # skill_allows_tool: [Bash, Write, WebFetch, ...] # Fires when allowed-tools pre-approves any of the given Claude Code tool -# names (matches the parsed grant Tool, then the raw token). +# names (matches the parsed grant Tool, then the raw token). PATTERN-BLIND: +# ignores the grant's Pattern, so "Bash(git status:*)" matches identically +# to a bare "Bash". Prefer skill_allows_unrestricted_tool when a narrowly- +# scoped grant should not count. +# +# skill_allows_unrestricted_tool: [Bash, Write, WebFetch, ...] +# Fires when allowed-tools pre-approves any of the given tool names with a +# genuinely unrestricted grant. Unlike skill_allows_tool, this consults the +# grant's Pattern per Claude Code's own permission-rule semantics: Bash/ +# PowerShell are unrestricted only for "" / "*" / ":*"; WebFetch only for +# "" / "*" (a "domain:example.com" specifier is a real restriction); Edit +# only for "" or a catch-all glob ("*", "**", "/**", "//**") — a real path +# pattern like "docs/**" is a real restriction. Write and NotebookEdit are +# ALWAYS unrestricted regardless of any pattern written in the grant, +# because Claude Code accepts but never consults a path specifier on those +# two tools (only an Edit rule is checked for file writes). # # skill_model_invocable: true # Fires when the model can auto-invoke the skill (disable-model-invocation -# is not set). Pair with skill_allows_tool to flag a model-triggerable -# side-effecting skill. Bool predicate. +# is not set). Pair with skill_allows_unrestricted_tool to flag a +# model-triggerable skill with a genuinely unrestricted side-effecting +# grant. Bool predicate. # # skill_body_has_dynamic_exec: true # Fires when the SKILL.md body uses dynamic-context shell execution (the @@ -505,8 +521,16 @@ rules: # # repo_uses_default_tracing: true | false # Fires when the scanner's derived UsesDefaultTracing flag matches. -# UsesDefaultTracing is true when no add_trace_processor or -# OPENAI_AGENTS_DISABLE_TRACING appears anywhere in parsed Python files. +# UsesDefaultTracing is false (tracing is NOT on the pure default path) +# when any parsed Python file's AST contains one of four documented +# disable mechanisms: an add_trace_processor / set_trace_processors call, +# an OPENAI_AGENTS_DISABLE_TRACING string literal, a set_tracing_disabled +# (True) call, or a RunConfig(tracing_disabled=True) construction +# (matched anywhere in the file, so both the inline +# Runner.run(agent, run_config=RunConfig(...)) form and an assigned- +# variable form are covered without dataflow). The match is AST-scoped +# (call-function names, call arguments, string literals) — a mention +# inside a comment or an unrelated identifier does not count. # repo_claude_default_mode_is: [bypassPermissions, acceptEdits, ...] # Fires when any discovered .claude/settings.json (or settings.local.json) # declares a defaultMode equal to one of the listed modes. Reads diff --git a/internal/rules/schema_version.go b/internal/rules/schema_version.go index 083f3624..cea14c75 100644 --- a/internal/rules/schema_version.go +++ b/internal/rules/schema_version.go @@ -27,4 +27,4 @@ package rules // that rule (safe) instead of mis-evaluating a predicate whose meaning changed. // That rename discipline is what keeps lenient loading safe without a hard // version gate. -const SupportedSchemaVersion = 16 +const SupportedSchemaVersion = 17 diff --git a/internal/scanner/scanner.go b/internal/scanner/scanner.go index 0600dda2..02a601f4 100644 --- a/internal/scanner/scanner.go +++ b/internal/scanner/scanner.go @@ -668,11 +668,22 @@ var tracingProcessorFuncs = map[string]bool{ "set_trace_processors": true, } -// disablesDefaultTracing reports whether a parsed file installs a custom trace -// processor or references the tracing-disable env var. It inspects typed AST -// nodes — call-function names and string literals — rather than substring- -// scanning raw source, so a mention inside a comment or an unrelated identifier -// no longer produces a false signal (the inventory-owns-AST-facts contract). +// tracingDisableFuncs are OpenAI Agents SDK calls that disable tracing outright +// when their first positional argument is the literal True — the SDK's +// module-level `set_tracing_disabled(True)` switch. +var tracingDisableFuncs = map[string]bool{ + "set_tracing_disabled": true, +} + +// disablesDefaultTracing reports whether a parsed file disables or replaces +// OpenAI Agents SDK default tracing, by any of four documented mechanisms: +// installing a custom trace processor (add_trace_processor / +// set_trace_processors), referencing the OPENAI_AGENTS_DISABLE_TRACING env +// var, calling set_tracing_disabled(True), or constructing +// RunConfig(tracing_disabled=True). It inspects typed AST nodes — call-function +// names, call arguments, and string literals — rather than substring-scanning +// raw source, so a mention inside a comment or an unrelated identifier no +// longer produces a false signal (the inventory-owns-AST-facts contract). func disablesDefaultTracing(pf analysis.ParsedFile) bool { found := false astutil.Walk(pf.Tree.RootNode(), func(n *sitter.Node) bool { @@ -691,7 +702,19 @@ func disablesDefaultTracing(pf analysis.ParsedFile) bool { if i := strings.LastIndex(name, "."); i >= 0 { name = name[i+1:] } - if tracingProcessorFuncs[name] { + switch { + case tracingProcessorFuncs[name]: + found = true + return false + case tracingDisableFuncs[name] && callFirstPositionalArgIsTrue(n): + found = true + return false + case name == "RunConfig" && callKeywordArgIsTrue(n, "tracing_disabled", pf.Source): + // Matches both the inline Runner.run(agent, run_config=RunConfig(...)) + // form and an assigned-variable form (cfg = RunConfig(...)) — the + // walk finds the RunConfig(...) construction wherever it appears in + // the file, with no dataflow needed to connect it back to the run + // call. found = true return false } @@ -709,6 +732,42 @@ func disablesDefaultTracing(pf analysis.ParsedFile) bool { return found } +// callFirstPositionalArgIsTrue reports whether callNode's first positional +// argument (i.e. not a keyword argument) is the literal True. +func callFirstPositionalArgIsTrue(callNode *sitter.Node) bool { + args := callNode.ChildByFieldName("arguments") + if args == nil { + return false + } + if args.NamedChildCount() == 0 { + return false + } + first := args.NamedChild(0) + return first != nil && first.Type() == "true" +} + +// callKeywordArgIsTrue reports whether callNode has a keyword argument named +// kwarg whose value is the literal True. +func callKeywordArgIsTrue(callNode *sitter.Node, kwarg string, src []byte) bool { + args := callNode.ChildByFieldName("arguments") + if args == nil { + return false + } + for i := 0; i < int(args.NamedChildCount()); i++ { + child := args.NamedChild(i) + if child.Type() != "keyword_argument" { + continue + } + name := astutil.NodeText(child.ChildByFieldName("name"), src) + if name != kwarg { + continue + } + value := child.ChildByFieldName("value") + return value != nil && value.Type() == "true" + } + return false +} + // languagesLabel renders a stable, comma-separated language list for progress. // sortedUnique returns the input sorted with duplicates removed. Used to make // the Coverage.SkippedFiles list deterministic. diff --git a/internal/scanner/scanner_test.go b/internal/scanner/scanner_test.go index 688ce23e..c2261cb3 100644 --- a/internal/scanner/scanner_test.go +++ b/internal/scanner/scanner_test.go @@ -721,6 +721,70 @@ func TestScanExamples_EmailAgent_SubagentDiscoveredAndAudited(t *testing.T) { } } +// TestScan_OAI201_TracingDisabled is the source-level fire/silent pair for +// OAI-201 (testdata/rules-fixture/CLAUDE.md's known gap (d)): it runs real +// .py source through the full scanner rather than a hand-built RepoInventory, +// so a discovery regression that stops producing the shape OAI-201 depends on +// fails a test instead of silently killing the rule. +func TestScan_OAI201_TracingDisabled(t *testing.T) { + writeRepo := func(t *testing.T, agentBody string) string { + t.Helper() + dir := t.TempDir() + writeFile := func(rel, body string) { + t.Helper() + full := filepath.Join(dir, rel) + if err := os.MkdirAll(filepath.Dir(full), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(full, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + } + writeFile("pyproject.toml", "[project]\nname = \"f\"\ndependencies = [\"openai-agents\"]\n") + writeFile("agent.py", agentBody) + return dir + } + + fires := func(t *testing.T, res models.ScanResult) bool { + t.Helper() + for _, f := range res.Findings { + if f.RuleID == "OAI-201" { + return true + } + } + return false + } + + t.Run("fires with default tracing", func(t *testing.T) { + dir := writeRepo(t, `from agents import Agent + +agent = Agent(name="researcher") +`) + res, err := scanner.Run(scanner.Config{Target: dir, RulesFS: rulesFixture(t)}) + if err != nil { + t.Fatalf("scanner.Run: %v", err) + } + if !fires(t, res) { + t.Errorf("expected OAI-201 to fire on a repo with no tracing configuration; findings=%v", res.Findings) + } + }) + + t.Run("silent with RunConfig(tracing_disabled=True)", func(t *testing.T) { + dir := writeRepo(t, `from agents import Agent, Runner, RunConfig + +agent = Agent(name="researcher") +Runner.run(agent, run_config=RunConfig(tracing_disabled=True)) +`) + res, err := scanner.Run(scanner.Config{Target: dir, RulesFS: rulesFixture(t)}) + if err != nil { + t.Fatalf("scanner.Run: %v", err) + } + if fires(t, res) { + t.Errorf("expected OAI-201 to stay silent when RunConfig(tracing_disabled=True) is set; findings=%v", res.Findings) + } + }) +} + // TestScanResult_JSONLineRangeFields asserts that every new JSON field path // added by the inventory line-attribution work is present in --format json // output. This is a "the JSON shape is what we promised" contract test: it diff --git a/internal/scanner/tracing_test.go b/internal/scanner/tracing_test.go index 620676fe..b5b5dc28 100644 --- a/internal/scanner/tracing_test.go +++ b/internal/scanner/tracing_test.go @@ -56,6 +56,48 @@ func TestComputeUsesDefaultTracing(t *testing.T) { src: "my_add_trace_processor_helper = None\n", want: true, }, + { + name: "set_tracing_disabled(True) disables default", + src: "from agents import set_tracing_disabled\nset_tracing_disabled(True)\n", + want: false, + }, + { + name: "set_tracing_disabled(False) does not disable default", + src: "from agents import set_tracing_disabled\nset_tracing_disabled(False)\n", + want: true, + }, + { + name: "attribute-form set_tracing_disabled(True) disables default", + src: "import agents\nagents.set_tracing_disabled(True)\n", + want: false, + }, + { + name: "RunConfig(tracing_disabled=True) disables default", + src: "from agents import RunConfig\ncfg = RunConfig(tracing_disabled=True)\n", + want: false, + }, + { + name: "RunConfig(tracing_disabled=False) does not disable default", + src: "from agents import RunConfig\ncfg = RunConfig(tracing_disabled=False)\n", + want: true, + }, + { + name: "inline run_config=RunConfig(tracing_disabled=True) disables default", + src: "from agents import Agent, Runner, RunConfig\n" + + "agent = Agent(name='a')\n" + + "Runner.run(agent, run_config=RunConfig(tracing_disabled=True))\n", + want: false, + }, + { + name: "RunConfig with an unrelated kwarg does not disable default", + src: "from agents import RunConfig\ncfg = RunConfig(workflow_name='wf')\n", + want: true, + }, + { + name: "lookalike RunConfig-suffixed identifier does not disable default", + src: "MyRunConfig(tracing_disabled=True)\n", + want: true, + }, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { diff --git a/testdata/forge/claude_skill/expected/SKILL.md b/testdata/forge/claude_skill/expected/SKILL.md index d69def85..e226db02 100644 --- a/testdata/forge/claude_skill/expected/SKILL.md +++ b/testdata/forge/claude_skill/expected/SKILL.md @@ -95,11 +95,11 @@ in post-build scan — prevent it now. ## [CSKILL-050] Model-invocable skill grants side-effecting tools **Severity:** high | **Confidence:** 0.80 -**Directive:** Add `disable-model-invocation: true` so only the user can invoke this skill, or narrow allowed-tools to read-only tools (Read, Grep, Glob). +**Directive:** Add `disable-model-invocation: true` so only the user can invoke this skill, or narrow allowed-tools to read-only tools (Read, Grep, Glob) or to scoped grants such as `Bash(git status:*)`, `Edit(docs/**)`, or `WebFetch(domain:example.com)`. -**Why:** Claude can auto-invoke this skill (disable-model-invocation is not set) and it pre-approves a side-effecting or exfiltration-capable tool (Bash / Write / Edit / WebFetch / NotebookEdit). +**Why:** Claude can auto-invoke this skill (disable-model-invocation is not set) and it pre-approves an unrestricted side-effecting or exfiltration-capable tool (Bash / Write / Edit / WebFetch / NotebookEdit). -**When this applies:** Any skill where disable-model-invocation is not set to true, and Any skill pre-approving Bash, Write, Edit, WebFetch, or NotebookEdit in allowed-tools. +**When this applies:** Any skill where disable-model-invocation is not set to true, and Any skill pre-approving Bash, Write, Edit, WebFetch, or NotebookEdit in allowed-tools with a genuinely unrestricted grant. --- @@ -128,9 +128,9 @@ in post-build scan — prevent it now. ## [CSKILL-060] Skill description claims read-only but grants side-effecting tools **Severity:** medium | **Confidence:** 0.50 -**Directive:** Make the description match the grants: either narrow allowed-tools to the read-only set the description promises (Read, Grep, Glob), or correct the description to disclose the side-effecting tools the skill actually uses. +**Directive:** Make the description match the grants: either narrow allowed-tools to the read-only set the description promises (Read, Grep, Glob) or to scoped grants such as `Bash(git status:*)` or `Edit(docs/**)`, or correct the description to disclose the side-effecting tools the skill actually uses. -**Why:** This skill's description claims it is read-only or side-effect-free, yet its allowed-tools pre-approve a side-effecting or exfiltration-capable tool (Bash / Write / Edit / WebFetch / NotebookEdit, or unrestricted shell). +**Why:** This skill's description claims it is read-only or side-effect-free, yet its allowed-tools pre-approve an unrestricted side-effecting or exfiltration- capable tool (Bash / Write / Edit / WebFetch / NotebookEdit). **When this applies:** Any skill whose description claims read-only but grants side-effecting tools. diff --git a/testdata/forge/claude_skill/input/skill_safety.yaml b/testdata/forge/claude_skill/input/skill_safety.yaml index 42a3fdf2..7c1bee6e 100644 --- a/testdata/forge/claude_skill/input/skill_safety.yaml +++ b/testdata/forge/claude_skill/input/skill_safety.yaml @@ -132,7 +132,7 @@ rules: match: all: - skill_model_invocable: true - - skill_allows_tool: + - skill_allows_unrestricted_tool: - Bash - Write - Edit @@ -140,14 +140,17 @@ rules: - NotebookEdit explanation: > Claude can auto-invoke this skill (disable-model-invocation is not set) and - it pre-approves a side-effecting or exfiltration-capable tool (Bash / Write - / Edit / WebFetch / NotebookEdit). The model can therefore be steered — by - an ambiguous request or an injected instruction — into triggering the skill - and its tools without the user choosing to. + it pre-approves an unrestricted side-effecting or exfiltration-capable tool + (Bash / Write / Edit / WebFetch / NotebookEdit). The model can therefore be + steered — by an ambiguous request or an injected instruction — into + triggering the skill and its tools without the user choosing to, with no + command, path, or domain scoping to contain the blast radius. fix: > Add `disable-model-invocation: true` so only the user can invoke this skill, - or narrow allowed-tools to read-only tools (Read, Grep, Glob). Reserve - auto-invocation for skills that carry knowledge, not side effects. + or narrow allowed-tools to read-only tools (Read, Grep, Glob) or to scoped + grants such as `Bash(git status:*)`, `Edit(docs/**)`, or + `WebFetch(domain:example.com)`. Reserve unscoped auto-invocation for skills + that carry knowledge, not side effects. - id: CSKILL-010 title: Bundled skill script performs network egress @@ -228,14 +231,15 @@ rules: skill_description_tool_mismatch: true explanation: > This skill's description claims it is read-only or side-effect-free, yet its - allowed-tools pre-approve a side-effecting or exfiltration-capable tool - (Bash / Write / Edit / WebFetch / NotebookEdit, or unrestricted shell). The + allowed-tools pre-approve an unrestricted side-effecting or exfiltration- + capable tool (Bash / Write / Edit / WebFetch / NotebookEdit). The description is the signal a user relies on when deciding to install or trust a skill, so a description that understates the real capability is a metadata-vs-behavior mismatch an attacker can hide behind. fix: > Make the description match the grants: either narrow allowed-tools to the - read-only set the description promises (Read, Grep, Glob), or correct the + read-only set the description promises (Read, Grep, Glob) or to scoped + grants such as `Bash(git status:*)` or `Edit(docs/**)`, or correct the description to disclose the side-effecting tools the skill actually uses. - id: CSKILL-070 diff --git a/testdata/rules-fixture/CLAUDE.md b/testdata/rules-fixture/CLAUDE.md index 8a492d9d..63e62ca7 100644 --- a/testdata/rules-fixture/CLAUDE.md +++ b/testdata/rules-fixture/CLAUDE.md @@ -221,12 +221,19 @@ production-grade, in priority order: `buildTool`, TS in `tsHandlerFacts`) and `PredAgentUsesToolKind` honors it for `shell_invocation`. Residual: the agent rules still only key on shell reach, not the broader "filesystem-touching" half their titles also name. -- **(d) Source-level fire/silent fixtures.** Per-rule cases in - `policies_test.go` feed hand-constructed typed inputs, so they prove - predicate logic but not discovery → detection end-to-end. Add fixtures - that run real `.py` snippets through the full scanner, so a discovery - change that stops producing a shape a rule depends on fails a test - instead of silently killing the rule. +- **(d) Source-level fire/silent fixtures — started, not general.** Per-rule + cases in `policies_test.go` feed hand-constructed typed inputs, so they + prove predicate logic but not discovery → detection end-to-end. OAI-201 + (`internal/scanner/scanner_test.go`, + `TestScan_OAI201_TracingDisabled`) now has a fire/silent pair that runs + real `.py` source through the full scanner rather than a hand-built + `RepoInventory` — it fires on a plain `Agent(...)` repo and goes silent on + `Runner.run(agent, run_config=RunConfig(tracing_disabled=True))`. The rest + of the pack still uses hand-constructed inputs; add more fixtures of this + shape as rules whose detection lives inside computed-fact logic (like + `disablesDefaultTracing`) are touched, so a discovery change that stops + producing a shape a rule depends on fails a test instead of silently + killing the rule. ## Output discipline for explanation/fix text diff --git a/testdata/rules-fixture/claude_skill/skill_quality_text.yaml b/testdata/rules-fixture/claude_skill/skill_quality_text.yaml index a8049931..6bba84f9 100644 --- a/testdata/rules-fixture/claude_skill/skill_quality_text.yaml +++ b/testdata/rules-fixture/claude_skill/skill_quality_text.yaml @@ -118,7 +118,7 @@ rules: - scan - vulnerability - compliance - - skill_allows_tool: + - skill_allows_unrestricted_tool: - Bash - Write - Edit @@ -127,15 +127,16 @@ rules: explanation: > This skill's name claims a security, audit, pentest, or compliance purpose — a role that is expected to observe and report, not mutate — - yet its allowed-tools pre-approve a side-effecting or exfiltration- - capable tool (Bash / Write / Edit / WebFetch / NotebookEdit). A skill - that can be steered into "auditing" a system while holding write or - shell access turns a read-first review into a privilege-escalation - path, whether by an ambiguous request or an injected instruction - encountered mid-audit. + yet its allowed-tools pre-approve an unrestricted side-effecting or + exfiltration-capable tool (Bash / Write / Edit / WebFetch / + NotebookEdit). A skill that can be steered into "auditing" a system + while holding unscoped write or shell access turns a read-first review + into a privilege-escalation path, whether by an ambiguous request or an + injected instruction encountered mid-audit. fix: > Narrow allowed-tools to the read-only set an audit role actually needs - (Read, Grep, Glob). If the skill genuinely must remediate what it finds, + (Read, Grep, Glob), or to scoped grants such as `Bash(git status:*)` or + `Edit(docs/**)`. If the skill genuinely must remediate what it finds, split remediation into a separate, explicitly-named skill so the audit-only skill's grants match what its name promises. diff --git a/testdata/rules-fixture/claude_skill/skill_safety.yaml b/testdata/rules-fixture/claude_skill/skill_safety.yaml index 42a3fdf2..7c1bee6e 100644 --- a/testdata/rules-fixture/claude_skill/skill_safety.yaml +++ b/testdata/rules-fixture/claude_skill/skill_safety.yaml @@ -132,7 +132,7 @@ rules: match: all: - skill_model_invocable: true - - skill_allows_tool: + - skill_allows_unrestricted_tool: - Bash - Write - Edit @@ -140,14 +140,17 @@ rules: - NotebookEdit explanation: > Claude can auto-invoke this skill (disable-model-invocation is not set) and - it pre-approves a side-effecting or exfiltration-capable tool (Bash / Write - / Edit / WebFetch / NotebookEdit). The model can therefore be steered — by - an ambiguous request or an injected instruction — into triggering the skill - and its tools without the user choosing to. + it pre-approves an unrestricted side-effecting or exfiltration-capable tool + (Bash / Write / Edit / WebFetch / NotebookEdit). The model can therefore be + steered — by an ambiguous request or an injected instruction — into + triggering the skill and its tools without the user choosing to, with no + command, path, or domain scoping to contain the blast radius. fix: > Add `disable-model-invocation: true` so only the user can invoke this skill, - or narrow allowed-tools to read-only tools (Read, Grep, Glob). Reserve - auto-invocation for skills that carry knowledge, not side effects. + or narrow allowed-tools to read-only tools (Read, Grep, Glob) or to scoped + grants such as `Bash(git status:*)`, `Edit(docs/**)`, or + `WebFetch(domain:example.com)`. Reserve unscoped auto-invocation for skills + that carry knowledge, not side effects. - id: CSKILL-010 title: Bundled skill script performs network egress @@ -228,14 +231,15 @@ rules: skill_description_tool_mismatch: true explanation: > This skill's description claims it is read-only or side-effect-free, yet its - allowed-tools pre-approve a side-effecting or exfiltration-capable tool - (Bash / Write / Edit / WebFetch / NotebookEdit, or unrestricted shell). The + allowed-tools pre-approve an unrestricted side-effecting or exfiltration- + capable tool (Bash / Write / Edit / WebFetch / NotebookEdit). The description is the signal a user relies on when deciding to install or trust a skill, so a description that understates the real capability is a metadata-vs-behavior mismatch an attacker can hide behind. fix: > Make the description match the grants: either narrow allowed-tools to the - read-only set the description promises (Read, Grep, Glob), or correct the + read-only set the description promises (Read, Grep, Glob) or to scoped + grants such as `Bash(git status:*)` or `Edit(docs/**)`, or correct the description to disclose the side-effecting tools the skill actually uses. - id: CSKILL-070 diff --git a/testdata/rules-fixture/manifest.yaml b/testdata/rules-fixture/manifest.yaml index afb3ac0a..50baf504 100644 --- a/testdata/rules-fixture/manifest.yaml +++ b/testdata/rules-fixture/manifest.yaml @@ -11,4 +11,4 @@ # # This file is metadata, not a rule: the engine's loader skips manifest.yaml # when walking the pack for policy files. -schema_version: 16 +schema_version: 17