fix: replace Pattern.Includes with Contains and Overlaps

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
 }
 

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++

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,36 @@ 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 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)
+	Overlaps(other Pattern) bool
+	// String returns the pattern text.
 	String() string
 
 	ExpandID(other ID) (ID, error)
@@ -101,35 +116,146 @@ 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
+}
+
+// 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
+}
 
-	return match(patternParts, otherParts)
+// 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)
+	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]
+}
+
+// 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)
+	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:
+				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
 	}
 	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,23 +279,6 @@ 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].
-// 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
-func Filter(filter, other Pattern) (Pattern, bool) {
-	if filter.Includes(other) {
-		return other, true
-	}
-	if other.Includes(filter) {
-		return filter, true
-	}
-	return nil, false
-}
-
 func replace1(original, other string) (string, error) {
 	components := split(original)
 	var candidates []int

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,68 +90,61 @@ 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) {
 			p, err := ParsePattern(tc.pattern)
 			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 Test_Filter(t *testing.T) {
+func TestPatternOverlaps(t *testing.T) {
 	tests := []struct {
-		filter string
-		other  string
-		result string
+		pattern  string
+		other    string
+		overlaps bool
 	}{
-		{
-			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/*",
-			result: "docker/*/auth/foo/bar/*",
-		},
-		{
-			filter: "**/mcp/auth/**",
-			other:  "*/*/auth/foo/bar/*",
-			result: "*/*/auth/foo/bar/*",
-		},
-		{
-			filter: "docker/mcp/auth/**",
-			other:  "foo/bar",
-		},
+		{"**", "**", 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 _, 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())
+	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")
 		})
 	}
 }