Skip to content

fix(permissions): keep colons in argument values from truncating patterns#3767

Merged
Sayt-0 merged 5 commits into
docker:mainfrom
parveshsaini:parveshsaini/fix/permission-arg-colon-truncation
Jul 22, 2026
Merged

fix(permissions): keep colons in argument values from truncating patterns#3767
Sayt-0 merged 5 commits into
docker:mainfrom
parveshsaini:parveshsaini/fix/permission-arg-colon-truncation

Conversation

@parveshsaini

Copy link
Copy Markdown
Contributor

Closes #3766

What

parsePattern split a permission pattern on every :, so any argument value
containing a colon was truncated. A deny rule written against a URL —
fetch:url=https://evil.com/* — parsed as url = "https", which never matches a
real 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 takes
each value up to the next :key= boundary rather than the next colon, so
colons inside a value are preserved. Tool names with colons
(mcp:github:create_issue) are unaffected — they were already handled by
splitting at the first :key=, and that behaviour is unchanged.

Why

This is the sibling of #3605. That fix made matchGlob's wildcards cross
path separators, but with a colon in the value the pattern never reaches
matchGlob intact — so the same fail-open direction was still reachable for URLs
and Windows paths, which are exactly the kind of value an operator writes a
deny rule against. In non-interactive / auto-approval runs, or alongside an
overlapping allow, a corrupted deny is a real bypass. allow rules with
colon values were broken too, but those fail closed.

Tests

Added to pkg/permissions/permissions_test.go:

  • TestParsePattern rows for URL, Windows-drive, and mixed value-colon +
    second-condition patterns.
  • TestDenyRuleWithColonInValue — end-to-end via CheckWithArgs: a deny rule
    with a URL value now returns Deny, plus a shell-command-with-URL case and a
    colon-free control.

Each new assertion fails against main and passes with this change; the existing
TestParsePattern rows (including mcp:github:create_issue) are untouched.

Verification (in golang:1.26.5 + golangci-lint:v2.12.2, matching CI)

$ go test ./pkg/permissions/...
ok  	github.com/docker/docker-agent/pkg/permissions

$ golangci-lint run ./pkg/permissions/...
0 issues.

$ gofmt -l pkg/permissions/    # (no output)

Confirming the tests are meaningful — reverting only permissions.go to main
while keeping the new tests:

--- FAIL: TestParsePattern/fetch:url=https://example.com/*
        expected: map[string]string{"url":"https://example.com/*"}
        actual  : map[string]string{"url":"https"}
--- FAIL: TestParsePattern/shell:cwd=C:\work\*
        expected: map[string]string{"cwd":"C:\\work\\*"}
        actual  : map[string]string{"cwd":"C"}
--- FAIL: TestDenyRuleWithColonInValue
        expected: 2 (Deny)   actual: 0 (Ask)
        Messages: deny rule with a URL value must block the matching URL

…erns

Signed-off-by: parveshsaini <parveshsaini619@gmail.com>
@parveshsaini
parveshsaini requested a review from a team as a code owner July 21, 2026 05:20
@aheritier aheritier added area/security Authentication, authorization, secrets, vulnerabilities kind/fix PR fixes a bug (maps to fix:). Use on PRs only. status/ready-for-review Ready for implementation, design review, or final review labels Jul 21, 2026
Comment thread pkg/permissions/permissions.go
Signed-off-by: parveshsaini <parveshsaini619@gmail.com>
Sayt-0
Sayt-0 previously approved these changes Jul 21, 2026

@Sayt-0 Sayt-0 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me !

@Sayt-0
Sayt-0 enabled auto-merge July 21, 2026 09:22
dgageot added 3 commits July 21, 2026 19:12
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.
@Sayt-0
Sayt-0 merged commit e276bca into docker:main Jul 22, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/security Authentication, authorization, secrets, vulnerabilities kind/fix PR fixes a bug (maps to fix:). Use on PRs only. status/ready-for-review Ready for implementation, design review, or final review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Permission rules fail open when an argument value contains : (parsePattern truncates the value)

4 participants