fix(permissions): keep colons in argument values from truncating patterns#3767
Merged
Sayt-0 merged 5 commits intoJul 22, 2026
Merged
Conversation
…erns Signed-off-by: parveshsaini <parveshsaini619@gmail.com>
dgageot
requested changes
Jul 21, 2026
Signed-off-by: parveshsaini <parveshsaini619@gmail.com>
Sayt-0
enabled auto-merge
July 21, 2026 09:22
cutAtArgCondition/isArgKey/isArgKeyByte detected a ':' immediately followed by a run of word characters and '=', which is exactly the regexp `:(\w+)=`. Splitting on its matches yields the same tool name and argument conditions in a third of the code.
Split already yields the tool name and one value per condition, and the keys come from the same non-overlapping matches, so the index-based FindAllStringSubmatchIndex bookkeeping (submatch offsets, next-match lookahead) is unnecessary.
dgageot
approved these changes
Jul 22, 2026
Sayt-0
approved these changes
Jul 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3766
What
parsePatternsplit a permission pattern on every:, so any argument valuecontaining a colon was truncated. A
denyrule written against a URL —fetch:url=https://evil.com/*— parsed asurl = "https", which never matches areal URL, so the rule silently never fired (fail-open). Windows drive paths
(
shell:cwd=C:\work\*→C) were corrupted the same way.This parses the tool name up to the first
:key=argument condition, then takeseach value up to the next
:key=boundary rather than the next colon, socolons inside a value are preserved. Tool names with colons
(
mcp:github:create_issue) are unaffected — they were already handled bysplitting at the first
:key=, and that behaviour is unchanged.Why
This is the sibling of #3605. That fix made
matchGlob's wildcards crosspath separators, but with a colon in the value the pattern never reaches
matchGlobintact — so the same fail-open direction was still reachable for URLsand Windows paths, which are exactly the kind of value an operator writes a
denyrule against. In non-interactive / auto-approval runs, or alongside anoverlapping
allow, a corrupteddenyis a real bypass.allowrules withcolon values were broken too, but those fail closed.
Tests
Added to
pkg/permissions/permissions_test.go:TestParsePatternrows for URL, Windows-drive, and mixed value-colon +second-condition patterns.
TestDenyRuleWithColonInValue— end-to-end viaCheckWithArgs: adenyrulewith a URL value now returns
Deny, plus a shell-command-with-URL case and acolon-free control.
Each new assertion fails against
mainand passes with this change; the existingTestParsePatternrows (includingmcp:github:create_issue) are untouched.Verification (in
golang:1.26.5+golangci-lint:v2.12.2, matching CI)Confirming the tests are meaningful — reverting only
permissions.gotomainwhile keeping the new tests: