fix: match permission rule glob subjects as opaque text - #2747
fix: match permission rule glob subjects as opaque text#2747tarikermis wants to merge 2 commits into
Conversation
Command-like rule subjects (Bash commands, FetchURL URLs, WebSearch/Grep queries, Glob patterns, agent/skill/task names) were matched with picomatch path semantics, so '*' stopped at '/' and refused dot segments: a rule like Bash(rm -rf*) never matched 'rm -rf /tmp/x', and no pattern short of '**' could match such subjects at all. globMatch now first tries the historical path-semantics match (so every pattern that matched before keeps matching, e.g. 'a/**/b' vs 'a/b'), then matches the subject as opaque text with '/' rewritten to a NUL placeholder and dot matching enabled, letting '*' and '**' cross slashes and dot segments as the function's contract already promised. Path subjects (Read/Write/Edit) keep pure path semantics via the extracted pathSegmentGlobMatch helper. Applied identically to agent-core and agent-core-v2, which share this matcher. Refs MoonshotAI#2728
🦋 Changeset detectedLatest commit: ce2d619 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b037001a69
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Try the historical path-semantics match first so rules that matched | ||
| // before keep matching (e.g. `a/**/b` still matches `a/b`). |
There was a problem hiding this comment.
Remove these newly added implementation-narration comments, including the similar comment inside asOpaqueText, or fold only the externally relevant responsibility into the top-of-file header. This package explicitly requires comments to live solely in that header and forbids comments beside functions or statements.
AGENTS.md reference: packages/agent-core-v2/AGENTS.md:L36-L38
Useful? React with 👍 / 👎.
| function asOpaqueText(value: string): string { | ||
| // Strip real NUL bytes first so one cannot be mistaken for a rewritten `/`. | ||
| return value.replaceAll(SLASH_PLACEHOLDER, '').replaceAll('/', SLASH_PLACEHOLDER); |
There was a problem hiding this comment.
When a rule subject can contain \u0000—notably WebSearch, whose query schema accepts any string—deleting NULs makes the permission matcher non-injective: an exact rule for ab now also matches the distinct subject a\u0000b. This can let a later tool call reuse an exact allow/session-approval rule that was granted for different input. Encode slash and NUL injectively (or explicitly reject NUL-bearing subjects) instead of removing the character; the duplicated legacy helper needs the same correction.
Useful? React with 👍 / 👎.
|
Author of #2728 here — I ran this PR against the full probe matrix from that Verification: no functional mismatches (54 probes). Covered: the issue's seven One question — One non-blocking note on backtracking (applies equally to the shape verified in With the above noted, this resolves the B half of #2728 in the shape verified there, |
…ching Address review findings on the previous head: - Stripping NULs in the opaque-text rewrite made the matcher non-injective: an exact rule for 'ab' also matched the distinct subject 'a<NUL>b', so a later call could reuse an exact allow/session-approval granted for different input. NUL-bearing subjects and patterns now skip the opaque phase entirely and match only under the historical literal semantics, which compares NUL bytes as-is; the slash rewrite stays injective on the remaining NUL-free domain. - Remove inline comments from agent-core-v2 rule-match.ts per the package's header-only comment convention, fold the externally relevant contract into the file header, and add a lint probe under test/lint guarding the convention for this file.
2 participants
Related Issue
Resolve #2728
Problem
Permission-rule argument patterns were matched with picomatch path semantics, so
*stopped at/and refused dot segments. For command-like subjects this made rules silently never match:No user-side workaround existed short of
**(which matches everything). This also contradicts the documented exampledeny = "Bash(rm -rf*)"in the config docs and theglobMatchdocstring ("the value is not treated as a file path").What changed
globMatch(in bothagent-coreandagent-core-v2, which share this matcher) now:a/**/bstill matchesa/b) — the change is purely additive;/is rewritten to a NUL placeholder (real NUL bytes are stripped first) anddot: trueis set, so*and**cross slashes and dot segments.Path subjects (
Read/Write/Edit/ReadMediaFile) are untouched:pathGlobMatchnow uses the extractedpathSegmentGlobMatchhelper, a byte-identical copy of the old matcher, so e.g.Edit(src/*)still does not matchsrc/sub/a.ts.On the questions raised in the issue: this takes the shared opaque-text fix for glob subjects (the shape the issue verified against all seven cases); per-tool subject semantics stay as they are. Sub-command decomposition (
Bash(git *)authorizing pipelines) is explicitly out of scope — it is a separate concern that pulls in the opposite direction and deserves its own decision.Honest behavior notes:
allowrules widen too: a rule likeallow Glob(src/*)now also matches pattern subjects containing slashes/traversal segments (e.g.src/../../etc/*), where it previously never fired. That is inherent to making these rules match at all, but calling it out explicitly.!) invert the widened matcher, so e.g.allow Bash(!git *)no longer auto-allows git commands whose arguments contain slashes — which is what such a rule was written to mean, but it is a behavior change for configs that (unknowingly) relied on the silent non-match.Verification
Reproduced the issue's exact cases on current
mainwith failing tests first, then fixed. Added regression tests to the existing matcher test files in both packages covering: the issue's seven cases, URL and search-text subjects, literal-slash negative cases,**patterns, globstar preservation (a/**/bvsa/b), NUL non-forgery, negation, and unchanged path-rule semantics.Checks run locally (Node 24.15.0, pnpm 10.33.0):
pnpm vitest run packages/agent-core packages/agent-core-v2— 535 files, 9027 tests passed, 0 failedpnpm --filter @moonshot-ai/agent-core --filter @moonshot-ai/agent-core-v2 run typecheck— cleanpnpm lint(oxlint --type-aware) — 0 errors; 0 warnings on the changed filesThe diff was reviewed with kiro-cli (claude-opus-5); findings from the review rounds (globstar narrowing, NUL collision, missing negative/URL/search coverage) were verified empirically and addressed.
Limitations: matcher-level and policy-level tests only; I did not run an end-to-end CLI session exercising a live permission prompt. The v2 engine currently does not evaluate config rules, so the v2 change is covered by unit tests only.
Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update. (No doc update: the fix aligns behavior with the already-documentedBash(rm -rf*)example.)