caddyhttp: Reject conflicting values in query stringsquerymatcher

2 files changed, 43 insertions, 8 deletions

diff --git a/modules/caddyhttp/matchers.go b/modules/caddyhttp/matchers.go
index e39ba3fc0..e90389d82 100644
--- a/modules/caddyhttp/matchers.go
+++ b/modules/caddyhttp/matchers.go
@@ -123,6 +123,7 @@ type (
 	// keyed by the query keys, with an array of string values to match for that key.
 	// Query key matches are exact, but wildcards may be used for value matches. Both
 	// keys and values may be placeholders.
+	//
 	// An example of the structure to match `?key=value&topic=api&query=something` is:
 	//
 	// ```json
@@ -808,19 +809,29 @@ func (m MatchQuery) Match(r *http.Request) bool {
 		return false
 	}
 
-	for param, vals := range m {
+	for param, allowedVals := range m {
 		param = repl.ReplaceAll(param, "")
-		paramVal, found := parsed[param]
+		incomingVals, found := parsed[param]
 		if found {
-			for _, v := range vals {
-				v = repl.ReplaceAll(v, "")
-				if paramVal[0] == v || v == "*" {
+			for _, allowedVal := range allowedVals {
+				allowedVal = repl.ReplaceAll(allowedVal, "")
+				if allowedVal == "*" {
+					return true
+				}
+				matched := true
+				for _, incomingVal := range incomingVals {
+					if incomingVal != allowedVal {
+						matched = false
+						break
+					}
+				}
+				if matched {
 					return true
 				}
 			}
 		}
 	}
-	return len(m) == 0 && len(r.URL.Query()) == 0
+	return len(m) == 0 && len(parsed) == 0
 }
 
 // CELLibrary produces options that expose this matcher for use in CEL
diff --git a/modules/caddyhttp/matchers_test.go b/modules/caddyhttp/matchers_test.go
index 4d5538cd3..9eebfb98f 100644
--- a/modules/caddyhttp/matchers_test.go
+++ b/modules/caddyhttp/matchers_test.go
@@ -718,7 +718,7 @@ func TestQueryMatcher(t *testing.T) {
 			expect:   true,
 		},
 		{
-			scenario: "non match against a wildcarded",
+			scenario: "non match against a wildcard",
 			match:    MatchQuery{"debug": []string{"*"}},
 			input:    "/?other=something",
 			expect:   false,
@@ -765,6 +765,30 @@ func TestQueryMatcher(t *testing.T) {
 			input:    "/?somekey=1",
 			expect:   true,
 		},
+		{
+			scenario: "don't match conflicting values",
+			match:    MatchQuery{"a": []string{"1"}},
+			input:    "/?a=1&a=2",
+			expect:   false,
+		},
+		{
+			scenario: "conflicting values are ambiguous with multiple match values",
+			match:    MatchQuery{"a": []string{"1", "2"}},
+			input:    "/?a=1&a=2",
+			expect:   false,
+		},
+		{
+			scenario: "repeated kv pairs in URI",
+			match:    MatchQuery{"a": []string{"1"}},
+			input:    "/?a=1&a=1",
+			expect:   true,
+		},
+		{
+			scenario: "TODO: it's unclear whether the values should be AND'ed or OR'ed", // perhaps multiple query matchers could be used to "and"
+			match:    MatchQuery{"a": []string{"1", "2"}},
+			input:    "/?a=2",
+			expect:   true,
+		},
 	} {
 
 		u, _ := url.Parse(tc.input)
@@ -777,7 +801,7 @@ func TestQueryMatcher(t *testing.T) {
 		req = req.WithContext(ctx)
 		actual := tc.match.Match(req)
 		if actual != tc.expect {
-			t.Errorf("Test %d %v: Expected %t, got %t for '%s'", i, tc.match, tc.expect, actual, tc.input)
+			t.Errorf("Test %d %v: Expected %t, got %t for '%s' (%s)", i, tc.match, tc.expect, actual, tc.input, tc.scenario)
 			continue
 		}
 	}