From b037001a691c48779c7238be5d31a032cc377c58 Mon Sep 17 00:00:00 2001 From: Tarik Ermis Date: Sun, 9 Aug 2026 01:34:02 +0200 Subject: [PATCH 1/2] fix: match permission rule glob subjects as opaque text 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 #2728 --- .../glob-rule-subjects-cross-slashes.md | 5 +++ packages/agent-core-v2/src/tool/rule-match.ts | 43 ++++++++++++++++--- .../agent/permissionRules/matchesRule.test.ts | 32 ++++++++++++++ .../src/tools/support/path-glob-match.ts | 40 +++++++++++++++-- .../agent-core/test/agent/permission.test.ts | 32 ++++++++++++++ 5 files changed, 143 insertions(+), 9 deletions(-) create mode 100644 .changeset/glob-rule-subjects-cross-slashes.md diff --git a/.changeset/glob-rule-subjects-cross-slashes.md b/.changeset/glob-rule-subjects-cross-slashes.md new file mode 100644 index 0000000000..fa4c9d196d --- /dev/null +++ b/.changeset/glob-rule-subjects-cross-slashes.md @@ -0,0 +1,5 @@ +--- +"@moonshot-ai/kimi-code": patch +--- + +Fix permission rule argument patterns such as Bash(rm -rf*) not matching commands, URLs, or search text that contain slashes or dot segments. Patterns negated with `!` now exclude those subjects as well. diff --git a/packages/agent-core-v2/src/tool/rule-match.ts b/packages/agent-core-v2/src/tool/rule-match.ts index 94cf3720f7..3c4292c326 100644 --- a/packages/agent-core-v2/src/tool/rule-match.ts +++ b/packages/agent-core-v2/src/tool/rule-match.ts @@ -5,7 +5,8 @@ * and the rule-subject helpers (`literalRulePattern`, * `escapeRuleSubjectLiteral`, `matchesGlobRuleSubject`, * `matchesPathRuleSubject`) that tool implementations use to build their - * `matchesRule` closures and canonical rule strings. Path matching compares + * `matchesRule` closures and canonical rule strings. Glob matching treats + * subjects as opaque text, so `*` crosses `/`. Path matching compares * normalized path variants, so `./a`, `dir/../a`, and Windows separator or * case variants can match the same rule. Pure functions; no scoped service. */ @@ -27,19 +28,51 @@ interface PathMatchSemantics { readonly pathClass: PathClass; } +const SLASH_PLACEHOLDER = '\0'; + export function globMatch(value: string, pattern: string, options?: { nocase?: boolean }): boolean { - if (picomatch.isMatch(value, pattern, options)) return true; + // Try the historical path-semantics match first so rules that matched + // before keep matching (e.g. `a/**/b` still matches `a/b`). + if (pathSegmentGlobMatch(value, pattern, options)) return true; + + // Then match the subject as opaque text: picomatch gives wildcards path + // semantics (`*` stops at `/` and refuses dot segments), so rewrite `/` to + // a placeholder and allow dots instead. + const opaqueOptions = { ...options, dot: true }; + if (picomatch.isMatch(asOpaqueText(value), asOpaqueText(pattern), opaqueOptions)) return true; const normalizedValue = stripLeadingDotSlash(value); const normalizedPattern = stripLeadingDotSlash(pattern); if (normalizedValue === value && normalizedPattern === pattern) return false; - return picomatch.isMatch(normalizedValue, normalizedPattern, options); + return picomatch.isMatch( + asOpaqueText(normalizedValue), + asOpaqueText(normalizedPattern), + opaqueOptions, + ); +} + +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); } function stripLeadingDotSlash(value: string): string { return value.startsWith('./') ? value.slice(2) : value; } +function pathSegmentGlobMatch( + value: string, + pattern: string, + options?: { nocase?: boolean }, +): boolean { + if (picomatch.isMatch(value, pattern, options)) return true; + + const normalizedValue = stripLeadingDotSlash(value); + const normalizedPattern = stripLeadingDotSlash(pattern); + if (normalizedValue === value && normalizedPattern === pattern) return false; + return picomatch.isMatch(normalizedValue, normalizedPattern, options); +} + export function pathGlobMatch( value: string, pattern: string, @@ -48,11 +81,11 @@ export function pathGlobMatch( const semantics = pathMatchSemantics(value, pattern, pathOptions); const nocase = pathOptions?.caseInsensitivePaths ?? true; - if (globMatch(value, pattern, { nocase })) return true; + if (pathSegmentGlobMatch(value, pattern, { nocase })) return true; for (const valueVariant of pathVariants(value, semantics, pathOptions)) { for (const patternVariant of pathVariants(pattern, semantics, pathOptions)) { - if (globMatch(valueVariant, patternVariant, { nocase })) return true; + if (pathSegmentGlobMatch(valueVariant, patternVariant, { nocase })) return true; } } return false; diff --git a/packages/agent-core-v2/test/agent/permissionRules/matchesRule.test.ts b/packages/agent-core-v2/test/agent/permissionRules/matchesRule.test.ts index 173f3d7575..1d63037ffd 100644 --- a/packages/agent-core-v2/test/agent/permissionRules/matchesRule.test.ts +++ b/packages/agent-core-v2/test/agent/permissionRules/matchesRule.test.ts @@ -132,6 +132,38 @@ describe('permissionRules/matchPermissionRule', () => { expect(matches(rule('Bad(unclosed'), 'Bad', noArgs)).toBe(false); }); + it('matches glob rule subjects as opaque text rather than as paths', () => { + expect(matchesGlobRuleSubject('rm -rf*', 'rm -rf x')).toBe(true); + expect(matchesGlobRuleSubject('rm -rf*', 'rm -rf /tmp/x')).toBe(true); + expect(matchesGlobRuleSubject('git *', 'git commit -m "fix src/a.ts"')).toBe(true); + expect(matchesGlobRuleSubject('rm -rf*', 'rm -rf ./build')).toBe(true); + expect(matchesGlobRuleSubject('rm -rf*', 'rm -rf ~/.ssh')).toBe(true); + expect(matchesGlobRuleSubject('rm -rf*', 'rm -rf /home/u/.ssh')).toBe(true); + expect(matchesGlobRuleSubject('https://example.com/*', 'https://example.com/a/b')).toBe(true); + expect(matchesGlobRuleSubject('*acme corp*', 'news about acme corp / rivals')).toBe(true); + expect(matchesGlobRuleSubject('**rm**', 'rm -rf /tmp/x')).toBe(true); + expect(matchesGlobRuleSubject('git *', 'git status')).toBe(true); + expect(matchesGlobRuleSubject('git *', 'git2 status')).toBe(false); + expect(matchesGlobRuleSubject('rm -rf*', 'git status')).toBe(false); + expect(matchesGlobRuleSubject('git log -- src/*.ts', 'git log -- srcXx.ts')).toBe(false); + expect(matchesGlobRuleSubject('https://example.com/a', 'https://example.com/b')).toBe(false); + expect(matchesGlobRuleSubject('!git *', 'git commit -m "fix src/a.ts"')).toBe(false); + expect(matchesGlobRuleSubject('!git *', 'npm test')).toBe(true); + }); + + it('keeps historical glob matches that opaque-text semantics alone would drop', () => { + expect(matchesGlobRuleSubject('**/*.ts', 'a.ts')).toBe(true); + expect(matchesGlobRuleSubject('a/**/b', 'a/b')).toBe(true); + expect(matchesGlobRuleSubject('a/**/b', 'a/x/y/b')).toBe(true); + expect(matchesGlobRuleSubject('a/b', 'a\u0000b')).toBe(false); + }); + + it('keeps path rule subjects on path semantics where * does not cross /', () => { + expect(matchesPathRuleSubject('src/*', 'src/a.ts')).toBe(true); + expect(matchesPathRuleSubject('src/**', 'src/sub/a.ts')).toBe(true); + expect(matchesPathRuleSubject('src/*', 'src/sub/a.ts')).toBe(false); + }); + it('does not match rule arguments without an execution matcher', () => { expect(matches(rule('Custom("query":"a.b")'), 'Custom', noArgs)).toBe(false); expect(matches(rule('Bash("command":"git status")'), 'Bash', noArgs)).toBe(false); diff --git a/packages/agent-core/src/tools/support/path-glob-match.ts b/packages/agent-core/src/tools/support/path-glob-match.ts index d3531fe163..659e513b87 100644 --- a/packages/agent-core/src/tools/support/path-glob-match.ts +++ b/packages/agent-core/src/tools/support/path-glob-match.ts @@ -15,23 +15,55 @@ interface PathMatchSemantics { readonly pathClass: PathClass; } +const SLASH_PLACEHOLDER = '\0'; + /** * Match ordinary string fields, like command text or search patterns. * `*` and `**` work as wildcards, but the value is not treated as a file path. */ export function globMatch(value: string, pattern: string, options?: { nocase?: boolean }): boolean { - if (picomatch.isMatch(value, pattern, options)) return true; + // Try the historical path-semantics match first so rules that matched + // before keep matching (e.g. `a/**/b` still matches `a/b`). + if (pathSegmentGlobMatch(value, pattern, options)) return true; + + // Then match the subject as opaque text: picomatch gives wildcards path + // semantics (`*` stops at `/` and refuses dot segments), so rewrite `/` to + // a placeholder and allow dots instead. + const opaqueOptions = { ...options, dot: true }; + if (picomatch.isMatch(asOpaqueText(value), asOpaqueText(pattern), opaqueOptions)) return true; const normalizedValue = stripLeadingDotSlash(value); const normalizedPattern = stripLeadingDotSlash(pattern); if (normalizedValue === value && normalizedPattern === pattern) return false; - return picomatch.isMatch(normalizedValue, normalizedPattern, options); + return picomatch.isMatch( + asOpaqueText(normalizedValue), + asOpaqueText(normalizedPattern), + opaqueOptions, + ); +} + +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); } function stripLeadingDotSlash(value: string): string { return value.startsWith('./') ? value.slice(2) : value; } +function pathSegmentGlobMatch( + value: string, + pattern: string, + options?: { nocase?: boolean }, +): boolean { + if (picomatch.isMatch(value, pattern, options)) return true; + + const normalizedValue = stripLeadingDotSlash(value); + const normalizedPattern = stripLeadingDotSlash(pattern); + if (normalizedValue === value && normalizedPattern === pattern) return false; + return picomatch.isMatch(normalizedValue, normalizedPattern, options); +} + /** * Match file path fields, like Read/Write/Edit `path`. * Also compares normalized forms, so `./a`, `dir/../a`, and Windows @@ -45,11 +77,11 @@ export function pathGlobMatch( const semantics = pathMatchSemantics(value, pattern, pathOptions); const nocase = pathOptions?.caseInsensitivePaths ?? true; - if (globMatch(value, pattern, { nocase })) return true; + if (pathSegmentGlobMatch(value, pattern, { nocase })) return true; for (const valueVariant of pathVariants(value, semantics, pathOptions)) { for (const patternVariant of pathVariants(pattern, semantics, pathOptions)) { - if (globMatch(valueVariant, patternVariant, { nocase })) return true; + if (pathSegmentGlobMatch(valueVariant, patternVariant, { nocase })) return true; } } return false; diff --git a/packages/agent-core/test/agent/permission.test.ts b/packages/agent-core/test/agent/permission.test.ts index 6ebf967680..10dedb75a8 100644 --- a/packages/agent-core/test/agent/permission.test.ts +++ b/packages/agent-core/test/agent/permission.test.ts @@ -3799,6 +3799,38 @@ describe('Permission rule helpers', () => { expect(ruleMatches(permissionRule('Bad(unclosed'), 'Bad', {})).toBe(false); }); + it('matches glob rule subjects as opaque text rather than as paths', () => { + expect(matchesGlobRuleSubject('rm -rf*', 'rm -rf x')).toBe(true); + expect(matchesGlobRuleSubject('rm -rf*', 'rm -rf /tmp/x')).toBe(true); + expect(matchesGlobRuleSubject('git *', 'git commit -m "fix src/a.ts"')).toBe(true); + expect(matchesGlobRuleSubject('rm -rf*', 'rm -rf ./build')).toBe(true); + expect(matchesGlobRuleSubject('rm -rf*', 'rm -rf ~/.ssh')).toBe(true); + expect(matchesGlobRuleSubject('rm -rf*', 'rm -rf /home/u/.ssh')).toBe(true); + expect(matchesGlobRuleSubject('https://example.com/*', 'https://example.com/a/b')).toBe(true); + expect(matchesGlobRuleSubject('*acme corp*', 'news about acme corp / rivals')).toBe(true); + expect(matchesGlobRuleSubject('**rm**', 'rm -rf /tmp/x')).toBe(true); + expect(matchesGlobRuleSubject('git *', 'git status')).toBe(true); + expect(matchesGlobRuleSubject('git *', 'git2 status')).toBe(false); + expect(matchesGlobRuleSubject('rm -rf*', 'git status')).toBe(false); + expect(matchesGlobRuleSubject('git log -- src/*.ts', 'git log -- srcXx.ts')).toBe(false); + expect(matchesGlobRuleSubject('https://example.com/a', 'https://example.com/b')).toBe(false); + expect(matchesGlobRuleSubject('!git *', 'git commit -m "fix src/a.ts"')).toBe(false); + expect(matchesGlobRuleSubject('!git *', 'npm test')).toBe(true); + }); + + it('keeps historical glob matches that opaque-text semantics alone would drop', () => { + expect(matchesGlobRuleSubject('**/*.ts', 'a.ts')).toBe(true); + expect(matchesGlobRuleSubject('a/**/b', 'a/b')).toBe(true); + expect(matchesGlobRuleSubject('a/**/b', 'a/x/y/b')).toBe(true); + expect(matchesGlobRuleSubject('a/b', 'a\u0000b')).toBe(false); + }); + + it('keeps path rule subjects on path semantics where * does not cross /', () => { + expect(matchesPathRuleSubject('src/*', 'src/a.ts')).toBe(true); + expect(matchesPathRuleSubject('src/**', 'src/sub/a.ts')).toBe(true); + expect(matchesPathRuleSubject('src/*', 'src/sub/a.ts')).toBe(false); + }); + it('does not match rule arguments without an execution matcher', () => { expect( ruleMatches(permissionRule('Custom("query":"a.b")'), 'Custom', { From ce2d6193c7dd2b3702e324ca3d18a704ab7bf981 Mon Sep 17 00:00:00 2001 From: Tarik Ermis Date: Sun, 9 Aug 2026 07:09:05 +0200 Subject: [PATCH 2/2] fix: keep NUL-bearing permission rule subjects out of opaque glob matching 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 'ab', 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. --- packages/agent-core-v2/src/tool/rule-match.ts | 19 ++++------ .../agent/permissionRules/matchesRule.test.ts | 6 +++ .../test/lint/header-comments.test.ts | 37 +++++++++++++++++++ .../src/tools/support/path-glob-match.ts | 9 +++-- .../agent-core/test/agent/permission.test.ts | 6 +++ 5 files changed, 63 insertions(+), 14 deletions(-) create mode 100644 packages/agent-core-v2/test/lint/header-comments.test.ts diff --git a/packages/agent-core-v2/src/tool/rule-match.ts b/packages/agent-core-v2/src/tool/rule-match.ts index 3c4292c326..d120287a88 100644 --- a/packages/agent-core-v2/src/tool/rule-match.ts +++ b/packages/agent-core-v2/src/tool/rule-match.ts @@ -5,10 +5,12 @@ * and the rule-subject helpers (`literalRulePattern`, * `escapeRuleSubjectLiteral`, `matchesGlobRuleSubject`, * `matchesPathRuleSubject`) that tool implementations use to build their - * `matchesRule` closures and canonical rule strings. Glob matching treats - * subjects as opaque text, so `*` crosses `/`. Path matching compares - * normalized path variants, so `./a`, `dir/../a`, and Windows separator or - * case variants can match the same rule. Pure functions; no scoped service. + * `matchesRule` closures and canonical rule strings. Glob matching accepts a + * subject under path-glob semantics or as opaque text where `*` also crosses + * `/`; NUL-bearing subjects match under path-glob semantics only. Path + * matching compares normalized path variants, so `./a`, `dir/../a`, and + * Windows separator or case variants can match the same rule. Pure + * functions; no scoped service. */ import { isAbsolute, join, parse } from 'pathe'; @@ -31,13 +33,9 @@ interface PathMatchSemantics { const SLASH_PLACEHOLDER = '\0'; export function globMatch(value: string, pattern: string, options?: { nocase?: boolean }): boolean { - // Try the historical path-semantics match first so rules that matched - // before keep matching (e.g. `a/**/b` still matches `a/b`). if (pathSegmentGlobMatch(value, pattern, options)) return true; + if (value.includes(SLASH_PLACEHOLDER) || pattern.includes(SLASH_PLACEHOLDER)) return false; - // Then match the subject as opaque text: picomatch gives wildcards path - // semantics (`*` stops at `/` and refuses dot segments), so rewrite `/` to - // a placeholder and allow dots instead. const opaqueOptions = { ...options, dot: true }; if (picomatch.isMatch(asOpaqueText(value), asOpaqueText(pattern), opaqueOptions)) return true; @@ -52,8 +50,7 @@ export function globMatch(value: string, pattern: string, options?: { nocase?: b } 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); + return value.replaceAll('/', SLASH_PLACEHOLDER); } function stripLeadingDotSlash(value: string): string { diff --git a/packages/agent-core-v2/test/agent/permissionRules/matchesRule.test.ts b/packages/agent-core-v2/test/agent/permissionRules/matchesRule.test.ts index 1d63037ffd..1bd1070dfd 100644 --- a/packages/agent-core-v2/test/agent/permissionRules/matchesRule.test.ts +++ b/packages/agent-core-v2/test/agent/permissionRules/matchesRule.test.ts @@ -155,7 +155,13 @@ describe('permissionRules/matchPermissionRule', () => { expect(matchesGlobRuleSubject('**/*.ts', 'a.ts')).toBe(true); expect(matchesGlobRuleSubject('a/**/b', 'a/b')).toBe(true); expect(matchesGlobRuleSubject('a/**/b', 'a/x/y/b')).toBe(true); + }); + + it('keeps NUL-bearing subjects distinct instead of conflating them', () => { + expect(matchesGlobRuleSubject('ab', 'a\u0000b')).toBe(false); expect(matchesGlobRuleSubject('a/b', 'a\u0000b')).toBe(false); + expect(matchesGlobRuleSubject('a\u0000b', 'a\u0000b')).toBe(true); + expect(matchesGlobRuleSubject('a*', 'a\u0000b')).toBe(true); }); it('keeps path rule subjects on path semantics where * does not cross /', () => { diff --git a/packages/agent-core-v2/test/lint/header-comments.test.ts b/packages/agent-core-v2/test/lint/header-comments.test.ts new file mode 100644 index 0000000000..7db771ad0c --- /dev/null +++ b/packages/agent-core-v2/test/lint/header-comments.test.ts @@ -0,0 +1,37 @@ +/** + * Header-only-comment gate — the package AGENTS.md "Comment conventions" + * section requires comments to live solely in the top-of-file block, never + * beside functions, methods, or statements. This probe guards files that + * previously attracted review findings for inline implementation narration; + * extend the list when a file newly trades in subtle encoding or matching + * rules that tempt an explanatory comment. + */ + +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { describe, expect, it } from 'vitest'; + +const SRC_ROOT = join(import.meta.dirname, '..', '..', 'src'); + +const HEADER_ONLY_FILES = ['tool/rule-match.ts']; + +function commentLinesOutsideHeader(source: string): string[] { + const headerEnd = source.indexOf('*/'); + const body = headerEnd === -1 ? source : source.slice(headerEnd + 2); + return body + .split('\n') + .filter( + (line) => + line.includes('//') || line.includes('/*') || line.trimStart().startsWith('*'), + ); +} + +describe('header-only comments', () => { + for (const file of HEADER_ONLY_FILES) { + it(`${file} keeps comments inside the top-of-file header`, () => { + const source = readFileSync(join(SRC_ROOT, file), 'utf8'); + expect(source.startsWith('/**')).toBe(true); + expect(commentLinesOutsideHeader(source)).toEqual([]); + }); + } +}); diff --git a/packages/agent-core/src/tools/support/path-glob-match.ts b/packages/agent-core/src/tools/support/path-glob-match.ts index 659e513b87..00f177e26c 100644 --- a/packages/agent-core/src/tools/support/path-glob-match.ts +++ b/packages/agent-core/src/tools/support/path-glob-match.ts @@ -28,7 +28,11 @@ export function globMatch(value: string, pattern: string, options?: { nocase?: b // Then match the subject as opaque text: picomatch gives wildcards path // semantics (`*` stops at `/` and refuses dot segments), so rewrite `/` to - // a placeholder and allow dots instead. + // a placeholder and allow dots instead. NUL-bearing subjects skip this: + // the historical match above already compares them literally, and the + // rewrite must stay injective so distinct subjects cannot collide. + if (value.includes(SLASH_PLACEHOLDER) || pattern.includes(SLASH_PLACEHOLDER)) return false; + const opaqueOptions = { ...options, dot: true }; if (picomatch.isMatch(asOpaqueText(value), asOpaqueText(pattern), opaqueOptions)) return true; @@ -43,8 +47,7 @@ export function globMatch(value: string, pattern: string, options?: { nocase?: b } 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); + return value.replaceAll('/', SLASH_PLACEHOLDER); } function stripLeadingDotSlash(value: string): string { diff --git a/packages/agent-core/test/agent/permission.test.ts b/packages/agent-core/test/agent/permission.test.ts index 10dedb75a8..9e8dc4ebbe 100644 --- a/packages/agent-core/test/agent/permission.test.ts +++ b/packages/agent-core/test/agent/permission.test.ts @@ -3822,7 +3822,13 @@ describe('Permission rule helpers', () => { expect(matchesGlobRuleSubject('**/*.ts', 'a.ts')).toBe(true); expect(matchesGlobRuleSubject('a/**/b', 'a/b')).toBe(true); expect(matchesGlobRuleSubject('a/**/b', 'a/x/y/b')).toBe(true); + }); + + it('keeps NUL-bearing subjects distinct instead of conflating them', () => { + expect(matchesGlobRuleSubject('ab', 'a\u0000b')).toBe(false); expect(matchesGlobRuleSubject('a/b', 'a\u0000b')).toBe(false); + expect(matchesGlobRuleSubject('a\u0000b', 'a\u0000b')).toBe(true); + expect(matchesGlobRuleSubject('a*', 'a\u0000b')).toBe(true); }); it('keeps path rule subjects on path semantics where * does not cross /', () => {