httpcaddyfile: Adjust path matcher sorting to solve for specificity (#5462)

4 files changed, 82 insertions, 29 deletions

diff --git a/caddyconfig/httpcaddyfile/directives.go b/caddyconfig/httpcaddyfile/directives.go
index a772dba..b8faa4a 100644
--- a/caddyconfig/httpcaddyfile/directives.go
+++ b/caddyconfig/httpcaddyfile/directives.go
@@ -427,26 +427,16 @@ func sortRoutes(routes []ConfigValue) {
 			jPathLen = len(jPM[0])
 		}
 
-		// some directives involve setting values which can overwrite
-		// each other, so it makes most sense to reverse the order so
-		// that the lease specific matcher is first; everything else
-		// has most-specific matcher first
-		if iDir == "vars" {
+		sortByPath := func() bool {
 			// we can only confidently compare path lengths if both
 			// directives have a single path to match (issue #5037)
 			if iPathLen > 0 && jPathLen > 0 {
-				// sort least-specific (shortest) path first
-				return iPathLen < jPathLen
-			}
+				// if both paths are the same except for a trailing wildcard,
+				// sort by the shorter path first (which is more specific)
+				if strings.TrimSuffix(iPM[0], "*") == strings.TrimSuffix(jPM[0], "*") {
+					return iPathLen < jPathLen
+				}
 
-			// if both directives don't have a single path to compare,
-			// sort whichever one has no matcher first; if both have
-			// no matcher, sort equally (stable sort preserves order)
-			return len(iRoute.MatcherSetsRaw) == 0 && len(jRoute.MatcherSetsRaw) > 0
-		} else {
-			// we can only confidently compare path lengths if both
-			// directives have a single path to match (issue #5037)
-			if iPathLen > 0 && jPathLen > 0 {
 				// sort most-specific (longest) path first
 				return iPathLen > jPathLen
 			}
@@ -455,7 +445,18 @@ func sortRoutes(routes []ConfigValue) {
 			// sort whichever one has a matcher first; if both have
 			// a matcher, sort equally (stable sort preserves order)
 			return len(iRoute.MatcherSetsRaw) > 0 && len(jRoute.MatcherSetsRaw) == 0
+		}()
+
+		// some directives involve setting values which can overwrite
+		// each other, so it makes most sense to reverse the order so
+		// that the least-specific matcher is first, allowing the last
+		// matching one to win
+		if iDir == "vars" {
+			return !sortByPath
 		}
+
+		// everything else is most-specific matcher first
+		return sortByPath
 	})
 }
 
diff --git a/caddytest/integration/caddyfile_adapt/map_and_vars_with_raw_types.txt b/caddytest/integration/caddyfile_adapt/map_and_vars_with_raw_types.txt
index af9faf4..cc75630 100644
--- a/caddytest/integration/caddyfile_adapt/map_and_vars_with_raw_types.txt
+++ b/caddytest/integration/caddyfile_adapt/map_and_vars_with_raw_types.txt
@@ -101,15 +101,15 @@ vars {
 													"source": "{http.request.host}"
 												},
 												{
-													"foo": "bar",
-													"handler": "vars"
-												},
-												{
 													"abc": true,
 													"def": 1,
 													"ghi": 2.3,
 													"handler": "vars",
 													"jkl": "mn op"
+												},
+												{
+													"foo": "bar",
+													"handler": "vars"
 												}
 											]
 										}
diff --git a/caddytest/integration/caddyfile_adapt/sort_directives_within_handle.txt b/caddytest/integration/caddyfile_adapt/sort_directives_within_handle.txt
index 0cf9d88..ac0d53c 100644
--- a/caddytest/integration/caddyfile_adapt/sort_directives_within_handle.txt
+++ b/caddytest/integration/caddyfile_adapt/sort_directives_within_handle.txt
@@ -1,12 +1,15 @@
 *.example.com {
 	@foo host foo.example.com
 	handle @foo {
-		handle_path /strip* {
+		handle_path /strip {
 			respond "this should be first"
 		}
-		handle {
+		handle_path /strip* {
 			respond "this should be second"
 		}
+		handle {
+			respond "this should be last"
+		}
 	}
 	handle {
 		respond "this should be last"
@@ -35,13 +38,13 @@
 									"handler": "subroute",
 									"routes": [
 										{
-											"group": "group5",
+											"group": "group6",
 											"handle": [
 												{
 													"handler": "subroute",
 													"routes": [
 														{
-															"group": "group2",
+															"group": "group3",
 															"handle": [
 																{
 																	"handler": "subroute",
@@ -68,13 +71,13 @@
 															"match": [
 																{
 																	"path": [
-																		"/strip*"
+																		"/strip"
 																	]
 																}
 															]
 														},
 														{
-															"group": "group2",
+															"group": "group3",
 															"handle": [
 																{
 																	"handler": "subroute",
@@ -82,6 +85,14 @@
 																		{
 																			"handle": [
 																				{
+																					"handler": "rewrite",
+																					"strip_path_prefix": "/strip"
+																				}
+																			]
+																		},
+																		{
+																			"handle": [
+																				{
 																					"body": "this should be second",
 																					"handler": "static_response"
 																				}
@@ -89,6 +100,31 @@
 																		}
 																	]
 																}
+															],
+															"match": [
+																{
+																	"path": [
+																		"/strip*"
+																	]
+																}
+															]
+														},
+														{
+															"group": "group3",
+															"handle": [
+																{
+																	"handler": "subroute",
+																	"routes": [
+																		{
+																			"handle": [
+																				{
+																					"body": "this should be last",
+																					"handler": "static_response"
+																				}
+																			]
+																		}
+																	]
+																}
 															]
 														}
 													]
@@ -103,7 +139,7 @@
 											]
 										},
 										{
-											"group": "group5",
+											"group": "group6",
 											"handle": [
 												{
 													"handler": "subroute",
diff --git a/caddytest/integration/caddyfile_adapt/sort_vars_in_reverse.txt b/caddytest/integration/caddyfile_adapt/sort_vars_in_reverse.txt
index dff75e1..38a912f 100644
--- a/caddytest/integration/caddyfile_adapt/sort_vars_in_reverse.txt
+++ b/caddytest/integration/caddyfile_adapt/sort_vars_in_reverse.txt
@@ -1,7 +1,8 @@
 :80
 
 vars /foobar foo last
-vars /foo foo middle
+vars /foo foo middle-last
+vars /foo* foo middle-first
 vars * foo first
 ----------
 {
@@ -25,13 +26,28 @@ vars * foo first
 							"match": [
 								{
 									"path": [
+										"/foo*"
+									]
+								}
+							],
+							"handle": [
+								{
+									"foo": "middle-first",
+									"handler": "vars"
+								}
+							]
+						},
+						{
+							"match": [
+								{
+									"path": [
 										"/foo"
 									]
 								}
 							],
 							"handle": [
 								{
-									"foo": "middle",
+									"foo": "middle-last",
 									"handler": "vars"
 								}
 							]