fix(permissions): match glob wildcards across path separators#3605
fix(permissions): match glob wildcards across path separators#3605parveshsaini wants to merge 3 commits into
Conversation
Signed-off-by: parveshsaini <parveshsaini619@gmail.com>
Sayt-0
left a comment
There was a problem hiding this comment.
The diagnosis in #3604 is correct and the glob-to-regexp translation is the right direction: RE2 is linear-time (no ReDoS), the \A/\z anchoring is right, and the fast path is preserved. Two correctness issues in globToRegexp block the merge; both have small fixes, detailed in the inline comments.
| # | Finding | Severity | Effect |
|---|---|---|---|
| 1 | Translated regexp lacks the (?s) flag, so . and .* stop at \n |
blocker | multi-line commands still bypass non-trailing-wildcard deny rules; strict regression vs main for newline-containing values |
| 2 | Byte-wise QuoteMeta(string(c)) corrupts multi-byte UTF-8 runes |
blocker | patterns containing non-ASCII stop matching (café no longer matches café) |
| 3 | \ escape semantics drift outside character classes |
minor | foo\* matched literal foo* on main (non-Windows), now matches foo\ + anything |
| 4 | Classes copied verbatim expose RE2 Perl classes | minor | [\d] meant literal d under filepath.Match, now means any digit |
| 5 | Regexp compiled on every matchGlob call |
nit | avoidable, patterns are static per Checker |
Finding 1 matters most: it recreates the exact failure class this PR sets out to fix, with the boundary moved from / to \n.
Pre-existing and out of scope, noted for a possible follow-up: a pattern that fails to compile silently disables its rule (return false, same direction as ErrBadPattern on main), so a typo in a deny rule fails open; validating patterns at config load would surface this.
| // Full glob match (also handles exact matches). Unlike filepath.Match, the | ||
| // translated "*"/"?" span any character, so patterns match values that | ||
| // contain path separators. | ||
| re, err := globToRegexp(pattern) |
There was a problem hiding this comment.
regexp.Compile runs on every matchGlob call, i.e. per pattern per tool call (tool name plus each argument). Patterns are static for a Checker's lifetime, so regexps could be compiled once at construction, or memoized in a package-level sync.Map keyed by the lowered pattern. Fine as a follow-up; not blocking.
There was a problem hiding this comment.
Did it rather than defer, it was small 😅
Package-level sync.Map keyed by the lowered pattern, so a rule compiles once. Cache is bounded by the number of configured rules since patterns come from config, not from tool input.
| // regexp share, are copied through verbatim; every other character is matched literally. | ||
| func globToRegexp(pattern string) (*regexp.Regexp, error) { | ||
| var b strings.Builder | ||
| b.WriteString(`\A`) |
There was a problem hiding this comment.
In Go regexp, . does not match \n unless the s flag is set, so the translated wildcards stop at newlines exactly the way filepath.Match stopped at /. Shell commands are routinely multi-line (heredocs, && chains), so a non-trailing-wildcard deny rule still fails open for them, and some values that matched on main stop matching:
| pattern | value | main |
this branch |
|---|---|---|---|
*rm -rf* |
"echo hi\nrm -rf /" |
no match | no match (still fails open) |
*b* |
"a\nb" |
match | no match (regression) |
a?b |
"a\nb" |
match | no match (regression) |
(filepath.Match wildcards stop only at path separators; they do cross \n, hence the regression rows.)
Fix: b.WriteString(`(?s)\A`). Included in the full rewrite suggested in the comment below on the default: branch.
There was a problem hiding this comment.
Added your regression rows: {"*b*", "a\nb", true} and {"a?b", "a\nb", true} (both were regressions vs main),
plus *rm -rf* against "echo hi\nrm -rf /", and a multi-line case in the deny test since that's the whole point of the PR.
| i++ | ||
| case '[': | ||
| if end := classEnd(pattern, i); end > 0 { | ||
| b.WriteString(pattern[i : end+1]) |
There was a problem hiding this comment.
Two semantic notes on copying class bodies verbatim:
- RE2 accepts more inside
[...]than glob:[\d]meant literaldunderfilepath.Matchbut is the digit class in RE2 (dno longer matches,5does). Perl/POSIX classes silently leak into glob syntax. Worth either escaping\inside copied classes or documenting the extension. - The leading-
]-is-literal rule ([]a],[^]]) is POSIX glob behavior, notfilepath.Matchbehavior (mainreturnedErrBadPattern, so no match). Arguably an improvement, but the PR description's "keeps all existing matchGlob semantics" does not hold for these; a line in the doc comment would help.
There was a problem hiding this comment.
Both fixed rather than documented. Class bodies are translated member-by-member now: glob \x escapes resolve
to the literal, and the other members get escaped, so [\d] is the literal d and [[:alpha:]] can't smuggle
in a POSIX class. And I dropped my leading-]-is-literal rule , you're right it was POSIX, not
filepath.Match, so []a] is back to matching nothing like it did under ErrBadPattern. Table has [\d]
against both d and 5.
| b.WriteString(regexp.QuoteMeta("[")) | ||
| i++ | ||
| default: | ||
| b.WriteString(regexp.QuoteMeta(string(c))) |
There was a problem hiding this comment.
c is a single byte. For a multi-byte UTF-8 rune, string(c) converts each byte to its own (wrong) rune, so the compiled regexp contains mojibake and literal non-ASCII patterns stop matching:
| pattern | value | main |
this branch |
|---|---|---|---|
café |
café |
match | no match (emitted regexp: \Acafé\z) |
*café* |
xx café yy |
match | no match |
Byte-wise scanning itself is safe (all glob metacharacters are ASCII, UTF-8 continuation bytes are >= 0x80); the fix is to accumulate literal runs and quote whole substrings instead of one byte at a time. Suggested replacement (also includes the (?s) fix):
func globToRegexp(pattern string) (*regexp.Regexp, error) {
var b strings.Builder
// (?s): argument values are often multi-line, "." must match "\n" too.
b.WriteString(`(?s)\A`)
lit := 0
flush := func(end int) {
if lit < end {
b.WriteString(regexp.QuoteMeta(pattern[lit:end]))
}
}
for i := 0; i < len(pattern); {
switch pattern[i] {
case '*':
flush(i)
b.WriteString(`.*`)
i++
lit = i
case '?':
flush(i)
b.WriteByte('.')
i++
lit = i
case '[':
if end := classEnd(pattern, i); end > 0 {
flush(i)
b.WriteString(pattern[i : end+1])
i = end + 1
lit = i
continue
}
// Unterminated class: "[" stays in the literal run.
i++
default:
i++
}
}
flush(len(pattern))
b.WriteString(`\z`)
return regexp.Compile(b.String())
}This variant passes the existing TestMatchGlob table, the new separator-crossing cases, and the newline/UTF-8 cases from this review.
There was a problem hiding this comment.
Good catch! string(c) on a byte was just wrong. Took your rewrite , accumulating literal runs and quoting
whole substrings. Kept the byte-wise scan for the reasons you gave (metacharacters are all ASCII, continuation
bytes are >= 0x80) and noted that in the doc comment so the next reader doesn't "fix" it back. café and
*café* are in the table now.
| } | ||
| for ; i < len(pattern); i++ { | ||
| switch pattern[i] { | ||
| case '\\': |
There was a problem hiding this comment.
classEnd honors \] escapes inside a class, but outside classes \ is now always a literal backslash. On main, filepath.Match treated \ as an escape on non-Windows (foo\* matched literal foo*, and the fast-path comment mentions exactly that case) and as a literal on Windows. This branch makes it a literal everywhere, which is a defensible unification but is a behavior change on non-Windows. Options:
- Handle
\xoutside classes as literalx(one extracase '\\':in the translator), keeping non-Windowsmainbehavior and making escapes work consistently inside and outside classes. - Keep backslash-as-literal and document the divergence in the
matchGlobdoc comment.
Either works; the current state (escape inside classes, literal outside) is the one combination that is hard to explain.
There was a problem hiding this comment.
Went with option 1. \x outside a class is now a literal x, so escapes behave the same on both sides of a
bracket and foo\* matches literal foo* again like it did on main (non-Windows). You were right that the
old split , escape inside, literal outside , was the one combination nobody could explain. The remaining
Windows divergence (\ was a literal there) is deliberate and now in the doc comment.
| // paths, URLs) routinely contain "/", so wildcards must not stop at it. | ||
| {"*rm -rf*", "rm -rf /", true}, | ||
| {"*/etc/*", "cat /etc/passwd", true}, | ||
| {"a?c", "a/c", true}, |
There was a problem hiding this comment.
The new rows cover the / boundary but not the two other boundaries the rewrite is sensitive to. Suggested additions:
// "*" and "?" must also span newlines: shell commands are often multi-line.
{"*rm -rf*", "echo hi\nrm -rf /", true},
{"a?b", "a\nb", true},
// Multi-byte UTF-8 in patterns must keep matching literally.
{"café", "café", true},
{"*café*", "xx café yy", true},All four rows fail on this branch as-is and pass with the fixes from the other comments.
There was a problem hiding this comment.
All four added, plus {"*b*", "a\nb", true} and the escape/class rows (foo\* vs foo*/foobar, [\d] vs
d/5). Also extended TestDenyGlobCrossesPathSeparator with the multi-line command.
Confirmed they fail on the previous commit and pass now 👍
Signed-off-by: parveshsaini <parveshsaini619@gmail.com>
Signed-off-by: parveshsaini <parveshsaini619@gmail.com>
Closes #3604
What
matchGlobinpkg/permissionsfalls back tofilepath.Matchfor any pattern that isn't a plain trailingwildcard.
filepath.Match's*and?match only non-separator characters, so they stop at/(and at\on Windows). This translates the glob to an anchored regexp whose*/?span any character instead.Why
Permission patterns match argument values — shell commands, file paths, URLs — which routinely contain
/.Because of the separator boundary, a rule with a non-trailing wildcard silently fails to match once the
value contains a
/:shellwithcmd = "rm -rf tmp"→ denied ✅shellwithcmd = "rm -rf /"→ not denied (falls through toAsk) ❌For a
denyrule this is the unsafe direction — it fails open. In an interactive session the fall-throughto
Askstill prompts, so the practical exposure is a degraded control; it becomes a real bypass under--yolo, non-interactive / auto-approval runs, or when an overlappingallowrule is present. The sameboundary also silently breaks
allowrules, though those fail closed.The existing docstring already stated the intent — "for argument matching we want
*to match any charactersincluding spaces" — but that only held for the trailing-wildcard fast path (
sudo*), which is why casualtesting with trailing-only patterns never surfaced this.
How
filepath.Matchfallback inmatchGlobwithglobToRegexp+regexp.MatchString.globToRegexptranslates afilepath.Match-style glob into an anchored regexp:*→.*,?→.,character classes (
[...]) copied through (bracket syntax is shared with regexp), everything else matchedliterally via
regexp.QuoteMeta.*prefix-match fast path and the case-insensitive lowercasing are unchanged.regexpis stdlib;path/filepathimport dropped).This keeps all existing
matchGlobsemantics (*,?,[...], case-insensitivity) — only the separatorboundary changes.
Tests
TestDenyGlobCrossesPathSeparator: end-to-end viaCheckWithArgs, asserting a*rm -rf*deny ruleblocks both
rm -rf tmpandrm -rf /.TestMatchGlobtable with separator-crossing cases (*rm -rf*,*/etc/*,a?cvsa/c).These fail on
mainand pass with the fix. Fullpkg/permissionssuite,go vet,gofmt, andgolangci-lintare all green (Go 1.26.4).