Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Mar 27, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 17 additions & 16 deletions caddyconfig/httpcaddyfile/directives.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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
})
}

Expand Down
8 changes: 4 additions & 4 deletions caddytest/integration/caddyfile_adapt/map_and_vars_with_raw_types.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
]
}
Expand Down
50 changes: 43 additions & 7 deletions caddytest/integration/caddyfile_adapt/sort_directives_within_handle.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"
Expand Down Expand Up @@ -35,13 +38,13 @@
"handler": "subroute",
"routes": [
{
"group": "group5",
"group": "group6",
"handle": [
{
"handler": "subroute",
"routes": [
{
"group": "group2",
"group": "group3",
"handle": [
{
"handler": "subroute",
Expand All @@ -68,17 +71,25 @@
"match": [
{
"path": [
"/strip*"
"/strip"
]
}
]
},
{
"group": "group2",
"group": "group3",
"handle": [
{
"handler": "subroute",
"routes": [
{
"handle": [
{
"handler": "rewrite",
"strip_path_prefix": "/strip"
}
]
},
{
"handle": [
{
Expand All @@ -89,6 +100,31 @@
}
]
}
],
"match": [
{
"path": [
"/strip*"
]
}
]
},
{
"group": "group3",
"handle": [
{
"handler": "subroute",
"routes": [
{
"handle": [
{
"body": "this should be last",
"handler": "static_response"
}
]
}
]
}
]
}
]
Expand All @@ -103,7 +139,7 @@
]
},
{
"group": "group5",
"group": "group6",
"handle": [
{
"handler": "subroute",
Expand Down
20 changes: 18 additions & 2 deletions caddytest/integration/caddyfile_adapt/sort_vars_in_reverse.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
----------
{
Expand All @@ -21,6 +22,21 @@ vars * foo first
}
]
},
{
"match": [
{
"path": [
"/foo*"
]
}
],
"handle": [
{
"foo": "middle-first",
"handler": "vars"
}
]
},
{
"match": [
{
Expand All @@ -31,7 +47,7 @@ vars * foo first
],
"handle": [
{
"foo": "middle",
"foo": "middle-last",
"handler": "vars"
}
]
Expand Down