From f10dd3124ff26b41cdecbef6855cfce54fb0bf85 Mon Sep 17 00:00:00 2001 From: Shashank Shekhar Singh Date: Tue, 4 Aug 2026 00:36:54 +0530 Subject: [PATCH] A deny rule naming a tool literally failed open when the name was also a glob MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `PermissionPolicy.decide` matched rules with `fnmatch(tool_name, rule.pattern)` and nothing else. `pattern` is documented as a glob, so the obvious way to write a rule — paste the tool's exact name — silently stopped matching the moment that name carried `[`…`]`, because fnmatch reads it as a character class: DENY "exfil[all]" does not match the tool exfil[all] ALLOW "*" does -> allow The operator got no error, no warning and no deny. `ToolRegistry.visible()` decides through the same call, so the tool the operator had just forbidden was also described to the model as available — the model was actively invited to call the thing it may not have, and the call went through. Verified end to end, not only at the policy layer. The failure was inconsistent as well as silent: `DENY "tool?x"` happened to hold, because a `?` glob matches a literal `?`. And it was the one place in this tree where a *deny* failed open — an unmatched tool defaults to DENY, an unregistered kind is refused, an unreachable backend raises. A rule now matches on `fnmatch(name, pattern) or name == pattern`, with the equality bound to the DENY and ASK tiers. That binding is the whole of the safety argument: adding a match to deny or ask can only ever refuse or gate a call that would otherwise have run, so it cannot loosen any policy. The same widening on ALLOW could grant a tool on a pattern the operator wrote as a glob, so ALLOW stays glob-only, and the case it needs is served by `PermissionRule.literal(action, name)`, which stores `glob.escape(name)` and so names one tool exactly at any tier. `default_harness` builds its ALLOW rules from registry names rather than from operator patterns, so it uses `literal` now; the core tool names carry no metacharacters, so nothing there changes behaviour today. Glob semantics are untouched, and tested as such: `rm*` still spans `rmdir`, `*` still matches everything, a glob still matches through every tier rather than only its own text, the deny -> ask -> allow ordering is unchanged, and an unmatched tool still falls to the DENY default. Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 1 + docs/cookbook/03-agents-and-tools.md | 10 ++++ grapharc/harness/permissions.py | 49 ++++++++++++++++++- grapharc/stdlib.py | 5 +- tests/test_harness_gate.py | 73 ++++++++++++++++++++++++++++ 5 files changed, 135 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a3cad53..1d0a1c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,3 +16,4 @@ Entries are newest-last within a release, matching the order they were written. - a bracket anywhere in a model's **prose** hijacked JSON extraction, because only the first `{`/`[` was ever tried. `Based on the context [lines 3-5]: {…}` was rejected as unparseable, and — worse — `Analysis (note [1]): {"supported": false}` returned a perfectly valid `[1]`, substituting a fabricated value for the verifier's actual answer. Every opener is tried now, and length alone turned out not to be a safe rank — a citation list like `[101, 205, 309, …]` *longer* than the verdict still won — so object spans are tried before array spans, each longest-first; junk still returns `None`, so fail-closed is unchanged. - a **bare backend name was read as a model name**, because `split_spec` only consulted the backend list when the spec contained a slash. `--model claude-cli` — the backend `models --check` reports as `usable` — shelled out to `claude -p --model claude-cli` and was refused by the CLI on *every* call, and `--model mock` named the paid subscription backend and spawned the real binary, so the double documented as "never reaches a provider" reached for one. A bare backend name now resolves to that backend (`claude-cli` to its own default model, `mock` to the scripted double, which ignores the model segment anyway); `openrouter`, `openai` and `ollama` front catalogues rather than a model, so those are refused with an example spelling instead of a guess about what to bill you for. The slash forms and bare *model* names are unchanged. - a failing `claude -p` **reported no reason at all**. The CLI exits non-zero with an empty stderr and puts its explanation in the JSON envelope on stdout; the gateway read only stderr, so the error was `claude -p exited 1: ` — a sentence that stops at the colon. Since the wrong-model bug above presented itself exactly that way, the two hid each other. stdout is read first now, and the recovered text also feeds the transient-vs-deterministic classifier, which was previously deciding from `""`. +- a **`deny` rule naming a tool literally failed open** when the name carried fnmatch metacharacters. `PermissionPolicy.decide` matched with `fnmatch(name, pattern)` alone, so `DENY "exfil[all]"` read as a character class, did not match the tool it spells, and evaluation fell through to whatever came next — typically a broad `ALLOW "*"`. The operator got no error, no warning and no deny; worse, `visible()` decides the same way, so the tool the operator had just forbidden was described to the model as available and then ran when it asked. The failure was inconsistent as well as silent: `DENY "tool?x"` happened to hold, because a `?` glob matches a literal `?`. This was the one place in the tree where a deny failed open — an unmatched tool, an unregistered kind and an unreachable backend all refuse. A `deny` or `ask` rule now also fires on an exact literal match. The widening is bound to those two tiers on purpose: equality can only add a rule that refuses or gates a call, never one that permits it, so it cannot loosen a policy the way the same change on `allow` could. For the `allow` case there is `PermissionRule.literal(action, name)`, which `glob.escape`s the name rather than widening the match, and which `default_harness` now uses for the registry names it allows. Glob semantics are untouched: `rm*` still spans `rmdir`, `*` still matches everything, the tier order and the `deny` default are unchanged. diff --git a/docs/cookbook/03-agents-and-tools.md b/docs/cookbook/03-agents-and-tools.md index 6ba4788..e48aa0b 100644 --- a/docs/cookbook/03-agents-and-tools.md +++ b/docs/cookbook/03-agents-and-tools.md @@ -546,6 +546,16 @@ Patterns match the **tool name** only, never its arguments. `DENY "run_command"` stops the shell tool entirely; it cannot express "deny `rm` but allow `ls`". That distinction belongs in a pre-hook, two recipes down. +**A name that is also a pattern.** `fnmatch` reads `[`…`]` as a character class, +so a tool called `exfil[all]` — or an MCP-style `mcp__srv__do[all]` — is not the +same string as the pattern that spells it. A `deny` or `ask` rule therefore also +fires on an **exact literal match**, so pasting a tool's name into a rule refuses +it whatever characters it holds. That fallback is deliberately not extended to +`allow`: equality can only ever add a refusal, never a grant. To *allow* one tool +whose name carries `*`, `?` or `[`, build the rule with +`PermissionRule.literal(Decision.ALLOW, name)`, which escapes the name instead of +widening the match. + --- ## How do I make sure a denied tool is never even offered to the model? diff --git a/grapharc/harness/permissions.py b/grapharc/harness/permissions.py index d174448..5b082c7 100644 --- a/grapharc/harness/permissions.py +++ b/grapharc/harness/permissions.py @@ -4,10 +4,17 @@ harness, never by prompts — instructions are advisory, this is not. A broad deny always beats a narrower allow (no allowlist exceptions inside a deny), matching the semantics that survived contact with reality in Claude Code. + +A pattern is a glob, but a DENY or ASK rule also fires on an exact literal +match, so a rule that simply names a tool refuses it even when the name carries +fnmatch metacharacters (`exfil[all]`). ALLOW keeps glob-only matching — see +`PermissionRule.matches` for why the two tiers differ, and +`PermissionRule.literal` for naming one tool exactly at any tier. """ from __future__ import annotations +import glob from enum import StrEnum from fnmatch import fnmatch @@ -24,9 +31,47 @@ class PermissionDenied(Exception): """A tool call was refused by policy (or by an absent/negative approval).""" +#: Tiers where an exact literal name is also honoured as a match, on top of the +#: glob. Restricting a tool can only ever *narrow* what runs, so widening these +#: two is safe in the direction this module already fails; see `matches`. +_LITERAL_TIERS = frozenset({Decision.DENY, Decision.ASK}) + + class PermissionRule(BaseModel): action: Decision - pattern: str # fnmatch pattern over the tool name + pattern: str # fnmatch pattern over the tool name; DENY/ASK also match literally + + @classmethod + def literal(cls, action: Decision, name: str) -> PermissionRule: + """A rule matching exactly one tool name, whatever characters it holds. + + `pattern` is a glob, so a name containing `*`, `?` or `[` is not the + rule that names it — `exfil[all]` reads as a character class. This + escapes the name (`glob.escape`) so the rule means the tool and nothing + else. Use it whenever the name comes from a registry rather than from an + operator writing a pattern by hand, and especially for ALLOW, where the + literal fallback in `matches` deliberately does not apply. + """ + return cls(action=action, pattern=glob.escape(name)) + + def matches(self, tool_name: str) -> bool: + """Does this rule fire for `tool_name`? + + The glob, plus — for DENY and ASK only — an exact string equality. A + tool whose name contains fnmatch metacharacters (`mcp__srv__do[all]`) + would otherwise slip past the rule that names it exactly, and the + evaluation would fall through to a broader ALLOW: the one place in this + tree where a *deny* failed open. + + Bound to DENY/ASK on purpose. Equality can only add rules that refuse or + gate a call, never ones that permit it, so it cannot loosen a policy; + the same widening on ALLOW could grant a tool the operator never allowed. + For an ALLOW rule naming a metacharacter-bearing tool, write it with + `PermissionRule.literal`, which escapes rather than widens. + """ + return fnmatch(tool_name, self.pattern) or ( + self.action in _LITERAL_TIERS and tool_name == self.pattern + ) class PermissionPolicy(BaseModel): @@ -42,6 +87,6 @@ class PermissionPolicy(BaseModel): def decide(self, tool_name: str) -> Decision: for tier in (Decision.DENY, Decision.ASK, Decision.ALLOW): for rule in self.rules: - if rule.action == tier and fnmatch(tool_name, rule.pattern): + if rule.action == tier and rule.matches(tool_name): return tier return self.default diff --git a/grapharc/stdlib.py b/grapharc/stdlib.py index 414bf7e..0ecef68 100644 --- a/grapharc/stdlib.py +++ b/grapharc/stdlib.py @@ -252,8 +252,11 @@ def default_harness(tools: tuple[str, ...], workspace: Any = None) -> Any: registry = ToolRegistry() for spec in core_tools(Path(workspace or Path.cwd()), include=tools): registry.register(spec) + # `literal`, not a bare pattern: these names come from a registry, not from + # an operator writing globs, and an ALLOW rule is the one tier where a name + # read as a pattern could grant more than was asked for. policy = PermissionPolicy( - rules=[PermissionRule(action=Decision.ALLOW, pattern=name) for name in tools], + rules=[PermissionRule.literal(Decision.ALLOW, name) for name in tools], default=Decision.DENY, ) return Harness(registry=registry, policy=policy, executor=LocalExecutor()) diff --git a/tests/test_harness_gate.py b/tests/test_harness_gate.py index c502d49..df61f24 100644 --- a/tests/test_harness_gate.py +++ b/tests/test_harness_gate.py @@ -68,6 +68,79 @@ def test_denied_tools_are_never_visible(): assert visible == ["read"] # rm's schema is never exposed +def test_deny_by_literal_name_beats_a_broad_allow_when_the_name_is_a_glob(): + """A DENY rule that *is* the tool's name refuses it, metacharacters and all. + + `exfil[all]` reads as a character class to fnmatch, so the rule naming it + used to miss, evaluation fell through to `ALLOW "*"`, and the tool both + appeared in `visible()` and ran. This is the one place a deny failed open. + """ + name = "exfil[all]" + reg = ToolRegistry() + reg.register(ToolSpec(name=name, description="dangerous", fn=_echo)) + policy = _policy([{"action": "deny", "pattern": name}, {"action": "allow", "pattern": "*"}]) + + assert policy.decide(name) is Decision.DENY + assert [t.name for t in reg.visible(policy)] == [] # never offered to the model + with pytest.raises(PermissionDenied): + Harness(reg, policy).call(name, {}) # and never runs if it asks anyway + + +def test_ask_by_literal_name_gates_a_glob_shaped_name(): + """The same widening on ASK: a gate that reads right is a gate that holds.""" + name = "mcp__srv__do[all]" + reg = ToolRegistry() + reg.register(ToolSpec(name=name, description="", fn=_echo)) + policy = _policy([{"action": "ask", "pattern": name}, {"action": "allow", "pattern": "*"}]) + + assert policy.decide(name) is Decision.ASK + assert [t.name for t in reg.visible(policy)] == [name] # ASK is not DENY + with pytest.raises(PermissionDenied, match="requires approval"): + Harness(reg, policy).call(name, {}) # no approval callback + + +def test_allow_stays_glob_only_and_literal_escapes_instead(): + """Literal equality is bound to DENY/ASK — it may only ever refuse more. + + Widening ALLOW the same way would grant a tool on a pattern the operator + wrote as a glob, so `PermissionRule.literal` escapes the name instead. + """ + name = "exfil[all]" + glob_rule = PermissionPolicy(rules=[PermissionRule(action=Decision.ALLOW, pattern=name)]) + assert glob_rule.decide(name) is Decision.DENY # the DENY default still holds + + literal_rule = PermissionPolicy(rules=[PermissionRule.literal(Decision.ALLOW, name)]) + assert literal_rule.decide(name) is Decision.ALLOW + assert literal_rule.decide("exfila") is Decision.DENY # and nothing the class covers + + +def test_glob_matching_is_unchanged_by_the_literal_fallback(): + """Regression: every existing glob semantic, at every tier.""" + policy = _policy([{"action": "deny", "pattern": "rm*"}, {"action": "allow", "pattern": "*"}]) + assert policy.decide("rmdir") is Decision.DENY # prefix glob still spans + assert policy.decide("rm") is Decision.DENY + assert policy.decide("read_file") is Decision.ALLOW # "*" still matches everything + + # A glob still matches through every tier, and never only its own text. + for action in ("deny", "ask", "allow"): + tier = _policy([{"action": action, "pattern": "danger_?"}]) + assert tier.decide("danger_1") is Decision(action) + assert tier.decide("danger_1x") is Decision.DENY # unmatched -> default + # deny -> ask -> allow ordering, independent of rule order + ordered = _policy( + [ + {"action": "allow", "pattern": "x_*"}, + {"action": "ask", "pattern": "x_a*"}, + {"action": "deny", "pattern": "x_ab*"}, + ] + ) + assert (ordered.decide("x_abc"), ordered.decide("x_ax"), ordered.decide("x_b")) == ( + Decision.DENY, + Decision.ASK, + Decision.ALLOW, + ) + + def test_ask_without_approval_fails_closed(): reg = ToolRegistry() reg.register(ToolSpec(name="send", description="", fn=_echo))