From 330be2d8c793147d3914f944eecb96c18f2eabff Mon Sep 17 00:00:00 2001 From: Francis Lavoie Date: Mon, 27 Mar 2023 15:43:44 -0400 Subject: [PATCH] httpcaddyfile: Adjust path matcher sorting to solve for specificity (#5462) --- caddyconfig/httpcaddyfile/directives.go | 33 ++++++------ .../map_and_vars_with_raw_types.txt | 8 +-- .../sort_directives_within_handle.txt | 50 ++++++++++++++++--- .../caddyfile_adapt/sort_vars_in_reverse.txt | 20 +++++++- 4 files changed, 82 insertions(+), 29 deletions(-) diff --git a/caddyconfig/httpcaddyfile/directives.go b/caddyconfig/httpcaddyfile/directives.go index a772dbaa..b8faa4a9 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 af9faf47..cc756301 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 @@ -100,16 +100,16 @@ 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 0cf9d880..ac0d53cc 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,17 +71,25 @@ "match": [ { "path": [ - "/strip*" + "/strip" ] } ] }, { - "group": "group2", + "group": "group3", "handle": [ { "handler": "subroute", "routes": [ + { + "handle": [ + { + "handler": "rewrite", + "strip_path_prefix": "/strip" + } + ] + }, { "handle": [ { @@ -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 dff75e1e..38a912f9 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 ---------- { @@ -21,6 +22,21 @@ vars * foo first } ] }, + { + "match": [ + { + "path": [ + "/foo*" + ] + } + ], + "handle": [ + { + "foo": "middle-first", + "handler": "vars" + } + ] + }, { "match": [ { @@ -31,7 +47,7 @@ vars * foo first ], "handle": [ { - "foo": "middle", + "foo": "middle-last", "handler": "vars" } ]