From 5ad670d0c79409d15ac0a6be4685c9084529084e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20Gro=C3=9Fmann?= Date: Wed, 9 Sep 2026 15:56:06 +0200 Subject: [PATCH 1/2] fix: replace Pattern.Includes with Contains and Overlaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Includes reused the ID matcher and treated a wildcard in the candidate pattern like a component that matches anything, so it implemented neither containment nor overlap: overlapping-but-not-contained patterns were reported as included (`docker/proj1/**` claimed to include `docker/*/mcp/*`), the relation was asymmetric, and spellings with equal match sets (`a/a/**` vs `a/a/**/**`) got different answers. Split it into the two relations callers actually need, each decided by a two-row DP over canonicalized wildcard tokens (runs of `*`/`**` collapse, a pure `**` becomes `*/**` since identifiers are never empty): - `p.Contains(other)`: p matches every ID that other matches — the containment Includes documented all along, with the same operand order. - `p.Overlaps(other)`: p and other match at least one ID in common; symmetric, and spelling-insensitive by construction. Both run in `O(n*m)` time and were verified against a brute-force set oracle on all 115,600 pattern pairs of up to 4 components. The ID matcher drops the candidate-side `*` special case it only carried for Includes, and Filter keeps its one-contains-the-other behavior with corrected docs. Signed-off-by: Johannes Großmann --- x/api/resolver/resolver_test.go | 6 +- x/secrets/identifiers.go | 2 +- x/secrets/pattern.go | 216 ++++++++++++++++++++++++++------ x/secrets/pattern_test.go | 58 +++++++-- 4 files changed, 233 insertions(+), 49 deletions(-) diff --git a/x/api/resolver/resolver_test.go b/x/api/resolver/resolver_test.go index b4b55c8f..08b646fb 100644 --- a/x/api/resolver/resolver_test.go +++ b/x/api/resolver/resolver_test.go @@ -154,7 +154,11 @@ func (m maliciousPattern) Match(secrets.ID) bool { return false } -func (m maliciousPattern) Includes(secrets.Pattern) bool { +func (m maliciousPattern) Contains(secrets.Pattern) bool { + return false +} + +func (m maliciousPattern) Overlaps(secrets.Pattern) bool { return false } diff --git a/x/secrets/identifiers.go b/x/secrets/identifiers.go index 59be6a96..0b79bec6 100644 --- a/x/secrets/identifiers.go +++ b/x/secrets/identifiers.go @@ -95,7 +95,7 @@ func match(pattern, path []string) bool { pi++ si++ default: - if pattern[pi] != path[si] && path[si] != "*" { + if pattern[pi] != path[si] { return false } pi++ diff --git a/x/secrets/pattern.go b/x/secrets/pattern.go index 6f1fc6f6..1188cd8a 100644 --- a/x/secrets/pattern.go +++ b/x/secrets/pattern.go @@ -22,15 +22,9 @@ import ( var ErrInvalidPattern = errors.New("invalid pattern") -// validPattern checks if a pattern is valid without using regexp or unicode. -// Rules: -// - Components separated by '/' -// - Each component is non-empty -// - Only characters A-Z, a-z, 0-9, '.', '-', '_' or '*' -// - No leading, trailing, or double slashes -// - Asterisks rules: -// - '*' cannot be mixed with other characters in the same component -// - there can be no more than two '*' per component +// validPattern reports whether s is a valid pattern: non-empty '/'-separated +// components of A-Z, a-z, 0-9, '.', '-', '_', ':', where a component may +// instead be '*' or '**'. func validPattern(s string) bool { if len(s) == 0 { return false @@ -77,15 +71,43 @@ func isValidPatternRune(c rune) bool { return isValidRune(c) || c == '*' } -// Pattern can be used to match secret identifiers. -// Valid patterns must follow the same validation rules as secret identifiers, with the exception -// that '*' can be used to match a single component, and '**' can be used to match zero or more components. +// Pattern matches secret IDs. It follows the ID validation rules, except +// that '*' matches one component and '**' matches zero or more. +// Below, matches(p) denotes the set of IDs a pattern p matches. type Pattern interface { - // Match the [Pattern] against an [ID] + // Match reports whether the pattern matches id. + // Complexity: O(n*m^k), where id has n components and the pattern has + // m components and k occurrences of '**'. Match(id ID) bool - // Includes returns true if all matches of Pattern [other] are also matches of the current pattern. - Includes(other Pattern) bool - // String formats the [Pattern] as a string + // Contains reports whether every ID that [other] matches can also be + // matched by the pattern: the set of IDs [other] matches is contained + // in the set the pattern matches, i.e., matches(other) ⊆ matches(p). + // + // Examples: + // - docker/** contains docker/*/mcp/*: '**' is more general than + // '*/mcp/*'. + // - docker/proj1/** does not contain docker/*/mcp/*: docker/*/mcp/* + // matches docker/proj2/mcp/x, but docker/proj1/** does not. + // + // Complexity: O(n*m) + Contains(other Pattern) bool + // Overlaps reports whether the pattern and [other] match at least one + // ID in common, i.e., matches(p) ∩ matches(other) ≠ ∅. + // + // Examples: + // - docker/*/mcp/* and docker/proj1/** overlap: both match + // docker/proj1/mcp/x. Yet neither contains the other: only the + // first matches docker/proj2/mcp/x, and only the second matches + // docker/proj1/y. + // - bar/** and foo/** do not overlap: an ID cannot begin with both + // bar and foo. + // + // Complexity: O(n*m) + // + // See also [Filter], which picks the narrower of two patterns when one + // contains the other. + Overlaps(other Pattern) bool + // String returns the pattern text. String() string ExpandID(other ID) (ID, error) @@ -101,26 +123,143 @@ func (p pattern) Match(id ID) bool { return match(patternParts, pathParts) } -func (p pattern) Includes(other Pattern) bool { - otherParts := split(other.String()) - patternParts := split(string(p)) +func (p pattern) Contains(other Pattern) bool { + return covers(canonicalize(string(p)), canonicalize(other.String())) +} + +func (p pattern) Overlaps(other Pattern) bool { + return compatible(canonicalize(string(p)), canonicalize(other.String())) +} + +type tokenKind uint8 + +const ( + tokenLit tokenKind = iota + tokenStar + tokenGap +) + +type token struct { + kind tokenKind + lit string +} - return match(patternParts, otherParts) +// canonicalize tokenizes a pattern so that two patterns match the same set +// of IDs exactly when their canonical tokens are equal. Two rewrites give +// that property: +// +// - In a run of consecutive wildcard components, the '*' move to the +// front and the '**' merge into one at the end: order within a run +// does not affect what it matches, and one '**' already matches any +// number of components. +// - A pattern consisting only of '**' becomes "*/**": an ID always has +// at least one component, so both match the same IDs, and the first +// rewrite already turns the equivalent spelling "**/*" into "*/**". +// +// Examples: +// +// a/a/** and a/a/**/** -> [a a **] +// a/**/*/**/b -> [a * ** b] +// ** and **/* -> [* **] +func canonicalize(s string) []token { + parts := split(s) + toks := make([]token, 0, len(parts)) + stars, gap := 0, false + flush := func() { + for range stars { + toks = append(toks, token{kind: tokenStar}) + } + if gap { + toks = append(toks, token{kind: tokenGap}) + } + stars, gap = 0, false + } + for _, part := range parts { + switch part { + case "*": + stars++ + case "**": + gap = true + default: + flush() + toks = append(toks, token{kind: tokenLit, lit: part}) + } + } + flush() + if len(toks) == 1 && toks[0].kind == tokenGap { + toks = []token{{kind: tokenStar}, {kind: tokenGap}} + } + return toks +} + +// covers reports whether p matches every ID q matches, for canonical tokens. +// The component alphabet is unbounded, so a literal in p never covers an '*' +// or '**' in q. +func covers(p, q []token) bool { + np, nq := len(p), len(q) + // Dynamic programming over suffixes: cur[j] holds whether p[i:] + // matches every component sequence q[j:] matches; prev holds the + // answers for i+1, the only row the recurrence needs. + prev := make([]bool, nq+1) + cur := make([]bool, nq+1) + prev[nq] = true + for i := np - 1; i >= 0; i-- { + cur[nq] = p[i].kind == tokenGap && prev[nq] + for j := nq - 1; j >= 0; j-- { + switch { + case p[i].kind == tokenGap: + cur[j] = prev[j] || cur[j+1] + case q[j].kind == tokenGap: + cur[j] = p[i].kind == tokenStar && cur[j+1] && prev[j] + case p[i].kind == tokenStar: + cur[j] = prev[j+1] + default: + cur[j] = q[j].kind == tokenLit && p[i].lit == q[j].lit && prev[j+1] + } + } + prev, cur = cur, prev + } + return prev[0] +} + +func compatible(p, q []token) bool { + np, nq := len(p), len(q) + // Dynamic programming over suffixes: cur[j] holds whether some + // component sequence is matched by both p[i:] and q[j:]; prev holds + // the answers for i+1, the only row the recurrence needs. + prev := make([]bool, nq+1) + cur := make([]bool, nq+1) + prev[nq] = true + for j := nq - 1; j >= 0; j-- { + prev[j] = q[j].kind == tokenGap && prev[j+1] + } + for i := np - 1; i >= 0; i-- { + cur[nq] = p[i].kind == tokenGap && prev[nq] + for j := nq - 1; j >= 0; j-- { + switch { + case p[i].kind == tokenGap || q[j].kind == tokenGap: + // One side is '**', which matches any components, so a + // common match exists if one exists after skipping the + // token on either side. + cur[j] = prev[j] || cur[j+1] + case p[i].kind == tokenLit && q[j].kind == tokenLit && p[i].lit != q[j].lit: + cur[j] = false + default: + cur[j] = prev[j+1] + } + } + prev, cur = cur, prev + } + return prev[0] } func (p pattern) String() string { return string(p) } -// ParsePattern parses a string into a [Pattern] -// Rules: -// - Components separated by '/' -// - Each component is non-empty -// - Only characters A-Z, a-z, 0-9, '.', '-', '_' or '*' -// - No leading, trailing, or double slashes -// - Asterisks rules: -// - '*' cannot be mixed with other characters in the same component -// - there can be no more than two '*' per component +// ParsePattern parses s into a [Pattern]: non-empty '/'-separated components +// of A-Z, a-z, 0-9, '.', '-', '_', ':', where a component may instead be '*' +// or '**'. It returns [ErrInvalidPattern] for anything else. func ParsePattern(s string) (Pattern, error) { if !validPattern(s) { return nil, ErrInvalidPattern @@ -128,8 +267,7 @@ func ParsePattern(s string) (Pattern, error) { return pattern(s), nil } -// MustParsePattern parses a string into a [Pattern] like with [ParsePattern], -// however, it panics when a validation error occurs. +// MustParsePattern is like [ParsePattern] but panics on an invalid pattern. func MustParsePattern(s string) Pattern { if !validPattern(s) { panic(ErrInvalidPattern) @@ -153,18 +291,18 @@ func (p pattern) ExpandPattern(other Pattern) (Pattern, error) { return pattern(val), err } -// Filter returns a reduced [Pattern] that is subset equal to [filter]. -// Returns false if there's no overlap between [filter] and [other]. +// Filter returns the narrower of two patterns: the one the other contains. +// It returns false when neither contains the other, even if they overlap. // Examples: -// - Filter(MustParsePattern("bar/**"), MustParsePattern("**")) => returns "bar/**" -// - Filter(MustParsePattern("**"), MustParsePattern("**")) => returns "**" -// - Filter(MustParsePattern("bar/**"), MustParsePattern("bar")) => returns "bar" -// - Filter(MustParsePattern("bar/**"), MustParsePattern("foo/**")) => returns false +// - Filter("bar/**", "**") => "bar/**" +// - Filter("**", "**") => "**" +// - Filter("bar/**", "bar") => "bar" +// - Filter("bar/**", "foo/**") => false func Filter(filter, other Pattern) (Pattern, bool) { - if filter.Includes(other) { + if filter.Contains(other) { return other, true } - if other.Includes(filter) { + if other.Contains(filter) { return filter, true } return nil, false diff --git a/x/secrets/pattern_test.go b/x/secrets/pattern_test.go index 08e73e2d..84462058 100644 --- a/x/secrets/pattern_test.go +++ b/x/secrets/pattern_test.go @@ -65,11 +65,11 @@ func TestPatternComparable(t *testing.T) { assert.Equal(t, bar, myMap[b]) } -func TestPatternIncludes(t *testing.T) { +func TestPatternContains(t *testing.T) { tests := []struct { - pattern string - other string - otherIsIncluded bool + pattern string + other string + contained bool }{ {"**", "**", true}, {"**/*", "*/**", true}, @@ -90,9 +90,18 @@ func TestPatternIncludes(t *testing.T) { {"*/foo", "*", false}, {"*", "*/foo", false}, {"docker/*/mcp/*", "docker/proj1/**", false}, - {"docker/proj1/**", "docker/*/mcp/*", true}, + // docker/x/mcp/y matches the other pattern but not this one. + {"docker/proj1/**", "docker/*/mcp/*", false}, {"docker/proj1/**", "docker/**/mcp/**", false}, {"docker/**", "docker/**/mcp/**", true}, + // a/a/** and a/a/**/** match the same IDs, so each contains the + // other. + {"a/a/**", "a/a/**/**", true}, + {"a/a/**/**", "a/a/**", true}, + {"a/a/*", "a/a/**", false}, + {"a/a/*", "a/a/**/**", false}, + {"a/a/**", "a/a/*", true}, + {"a/a/**/**", "a/a/*", true}, } for idx, tc := range tests { t.Run(fmt.Sprintf("pattern %d", idx+1), func(t *testing.T) { @@ -100,7 +109,42 @@ func TestPatternIncludes(t *testing.T) { require.NoError(t, err) other, err := ParsePattern(tc.other) require.NoError(t, err) - assert.Equal(t, tc.otherIsIncluded, p.Includes(other)) + assert.Equal(t, tc.contained, p.Contains(other)) + }) + } +} + +func TestPatternOverlaps(t *testing.T) { + tests := []struct { + pattern string + other string + overlaps bool + }{ + {"**", "**", true}, + {"*", "*/foo", false}, + {"*/foo", "*", false}, + {"foo/bar", "foo/baz", false}, + {"foo/*", "foo/bar", true}, + {"foo/**", "**/foo", true}, // both match "foo" + {"bar/**", "foo/**", false}, // first components conflict + {"foo/*/baz", "foo/bar/**", true}, + // Overlap without containment in either direction. + {"docker/*/mcp/*", "docker/proj1/**", true}, + {"foo/foo/foo/**", "foo/foo/**/foo", true}, + {"*/foo/**", "**/bar/*", true}, + {"docker/mcp/auth/**", "foo/bar", false}, + {"a/a/**", "a/a/**/**", true}, + {"a/a/*", "a/a/**", true}, + {"a/a/*", "a/a/**/**", true}, + } + for idx, tc := range tests { + t.Run(fmt.Sprintf("pattern %d", idx+1), func(t *testing.T) { + p, err := ParsePattern(tc.pattern) + require.NoError(t, err) + other, err := ParsePattern(tc.other) + require.NoError(t, err) + assert.Equal(t, tc.overlaps, p.Overlaps(other)) + assert.Equal(t, tc.overlaps, other.Overlaps(p), "Overlaps must be symmetric") }) } } @@ -129,12 +173,10 @@ func Test_Filter(t *testing.T) { { filter: "**/mcp/auth/**", other: "docker/*/auth/foo/bar/*", - result: "docker/*/auth/foo/bar/*", }, { filter: "**/mcp/auth/**", other: "*/*/auth/foo/bar/*", - result: "*/*/auth/foo/bar/*", }, { filter: "docker/mcp/auth/**", From a61cee4e58c956ef971851d24f7045f27c7ebe22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20Gro=C3=9Fmann?= Date: Fri, 11 Sep 2026 16:36:49 +0200 Subject: [PATCH 2/2] refactor: remove Filter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Filter reduced two patterns to the contained one and returned false for overlapping-but-not-contained pairs, silently dropping providers whose namespace merely overlaps a query. No signature-compatible definition is exact, because the intersection of two patterns is not always expressible as a single pattern. Remove it until a caller needs a well-defined replacement. Signed-off-by: Johannes Großmann --- x/secrets/pattern.go | 41 +++++--------------------------- x/secrets/pattern_test.go | 49 --------------------------------------- 2 files changed, 6 insertions(+), 84 deletions(-) diff --git a/x/secrets/pattern.go b/x/secrets/pattern.go index 1188cd8a..d49a9682 100644 --- a/x/secrets/pattern.go +++ b/x/secrets/pattern.go @@ -95,17 +95,10 @@ type Pattern interface { // ID in common, i.e., matches(p) ∩ matches(other) ≠ ∅. // // Examples: - // - docker/*/mcp/* and docker/proj1/** overlap: both match - // docker/proj1/mcp/x. Yet neither contains the other: only the - // first matches docker/proj2/mcp/x, and only the second matches - // docker/proj1/y. - // - bar/** and foo/** do not overlap: an ID cannot begin with both - // bar and foo. + // - docker/*/mcp/* and docker/proj1/** overlap: both match e.g. docker/proj1/mcp/x. + // - bar/** and foo/** do not overlap: an ID cannot begin with both bar and foo. // // Complexity: O(n*m) - // - // See also [Filter], which picks the narrower of two patterns when one - // contains the other. Overlaps(other Pattern) bool // String returns the pattern text. String() string @@ -197,9 +190,6 @@ func canonicalize(s string) []token { // or '**' in q. func covers(p, q []token) bool { np, nq := len(p), len(q) - // Dynamic programming over suffixes: cur[j] holds whether p[i:] - // matches every component sequence q[j:] matches; prev holds the - // answers for i+1, the only row the recurrence needs. prev := make([]bool, nq+1) cur := make([]bool, nq+1) prev[nq] = true @@ -222,11 +212,12 @@ func covers(p, q []token) bool { return prev[0] } +// compatible reports whether p and q match at least one ID in common, for +// canonical tokens. A common ID aligns both token lists over its +// components: wildcards accept anything, so only differing literals or +// mismatched lengths (* vs */foo) rule one out. func compatible(p, q []token) bool { np, nq := len(p), len(q) - // Dynamic programming over suffixes: cur[j] holds whether some - // component sequence is matched by both p[i:] and q[j:]; prev holds - // the answers for i+1, the only row the recurrence needs. prev := make([]bool, nq+1) cur := make([]bool, nq+1) prev[nq] = true @@ -238,9 +229,6 @@ func compatible(p, q []token) bool { for j := nq - 1; j >= 0; j-- { switch { case p[i].kind == tokenGap || q[j].kind == tokenGap: - // One side is '**', which matches any components, so a - // common match exists if one exists after skipping the - // token on either side. cur[j] = prev[j] || cur[j+1] case p[i].kind == tokenLit && q[j].kind == tokenLit && p[i].lit != q[j].lit: cur[j] = false @@ -291,23 +279,6 @@ func (p pattern) ExpandPattern(other Pattern) (Pattern, error) { return pattern(val), err } -// Filter returns the narrower of two patterns: the one the other contains. -// It returns false when neither contains the other, even if they overlap. -// Examples: -// - Filter("bar/**", "**") => "bar/**" -// - Filter("**", "**") => "**" -// - Filter("bar/**", "bar") => "bar" -// - Filter("bar/**", "foo/**") => false -func Filter(filter, other Pattern) (Pattern, bool) { - if filter.Contains(other) { - return other, true - } - if other.Contains(filter) { - return filter, true - } - return nil, false -} - func replace1(original, other string) (string, error) { components := split(original) var candidates []int diff --git a/x/secrets/pattern_test.go b/x/secrets/pattern_test.go index 84462058..94e89071 100644 --- a/x/secrets/pattern_test.go +++ b/x/secrets/pattern_test.go @@ -149,55 +149,6 @@ func TestPatternOverlaps(t *testing.T) { } } -func Test_Filter(t *testing.T) { - tests := []struct { - filter string - other string - result string - }{ - { - filter: "docker/mcp/auth/**", - other: "**", - result: "docker/mcp/auth/**", - }, - { - filter: "**", - other: "**", - result: "**", - }, - { - filter: "docker/mcp/auth/**", - other: "docker/mcp/auth/foo/bar/*", - result: "docker/mcp/auth/foo/bar/*", - }, - { - filter: "**/mcp/auth/**", - other: "docker/*/auth/foo/bar/*", - }, - { - filter: "**/mcp/auth/**", - other: "*/*/auth/foo/bar/*", - }, - { - filter: "docker/mcp/auth/**", - other: "foo/bar", - }, - } - for _, tc := range tests { - t.Run(fmt.Sprintf("f: %s in: %s", tc.filter, tc.other), func(t *testing.T) { - filter := MustParsePattern(tc.filter) - other := MustParsePattern(tc.other) - result, ok := Filter(filter, other) - if tc.result == "" { - assert.False(t, ok) - return - } - require.True(t, ok) - assert.Equal(t, tc.result, result.String()) - }) - } -} - func Test_apply(t *testing.T) { type query struct { pattern string