From 754fe4f7b4dddbfc7a8ee7fee405cee057da858e Mon Sep 17 00:00:00 2001 From: Matthew Holt Date: Tue, 13 Sep 2022 13:43:21 -0600 Subject: [PATCH] httpcaddyfile: Fix sorting of repeated directives Fixes #5037 --- caddyconfig/httpcaddyfile/directives.go | 43 ++++++++++++++----------- caddyconfig/httpcaddyfile/httptype.go | 2 +- caddytest/caddytest.go | 2 +- modules/caddyhttp/routes.go | 22 +++++++++++++ 4 files changed, 49 insertions(+), 20 deletions(-) diff --git a/caddyconfig/httpcaddyfile/directives.go b/caddyconfig/httpcaddyfile/directives.go index 89706b3a2..e2113ebb4 100644 --- a/caddyconfig/httpcaddyfile/directives.go +++ b/caddyconfig/httpcaddyfile/directives.go @@ -406,7 +406,7 @@ func sortRoutes(routes []ConfigValue) { return false } - // decode the path matchers, if there is just one of them + // decode the path matchers if there is just one matcher set var iPM, jPM caddyhttp.MatchPath if len(iRoute.MatcherSetsRaw) == 1 { _ = json.Unmarshal(iRoute.MatcherSetsRaw[0]["path"], &iPM) @@ -415,38 +415,45 @@ func sortRoutes(routes []ConfigValue) { _ = json.Unmarshal(jRoute.MatcherSetsRaw[0]["path"], &jPM) } - // sort by longer path (more specific) first; missing path - // matchers or multi-matchers are treated as zero-length paths + // if there is only one path in the path matcher, sort by longer path + // (more specific) first; missing path matchers or multi-matchers are + // treated as zero-length paths var iPathLen, jPathLen int - if len(iPM) > 0 { + if len(iPM) == 1 { iPathLen = len(iPM[0]) } - if len(jPM) > 0 { + if len(jPM) == 1 { jPathLen = len(jPM[0]) } // some directives involve setting values which can overwrite - // eachother, so it makes most sense to reverse the order so + // 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" { - // if both directives have no path matcher, use whichever one - // has no matcher first. - if iPathLen == 0 && jPathLen == 0 { - return len(iRoute.MatcherSetsRaw) == 0 && len(jRoute.MatcherSetsRaw) > 0 + // 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 } - // sort with the least-specific (shortest) path first - 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 { - // if both directives have no path matcher, use whichever one - // has any kind of matcher defined first. - if iPathLen == 0 && jPathLen == 0 { - return len(iRoute.MatcherSetsRaw) > 0 && len(jRoute.MatcherSetsRaw) == 0 + // 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 } - // sort with the most-specific (longest) path first - return iPathLen > jPathLen + // if both directives don't have a single path to compare, + // 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 } }) } diff --git a/caddyconfig/httpcaddyfile/httptype.go b/caddyconfig/httpcaddyfile/httptype.go index 72d98bd94..9cf386a47 100644 --- a/caddyconfig/httpcaddyfile/httptype.go +++ b/caddyconfig/httpcaddyfile/httptype.go @@ -955,7 +955,7 @@ func appendSubrouteToRouteList(routeList caddyhttp.RouteList, func buildSubroute(routes []ConfigValue, groupCounter counter) (*caddyhttp.Subroute, error) { for _, val := range routes { if !directiveIsOrdered(val.directive) { - return nil, fmt.Errorf("directive '%s' is not ordered, so it cannot be used here", val.directive) + return nil, fmt.Errorf("directive '%s' is not an ordered HTTP handler, so it cannot be used here", val.directive) } } diff --git a/caddytest/caddytest.go b/caddytest/caddytest.go index 9addd14d3..4fb33941c 100644 --- a/caddytest/caddytest.go +++ b/caddytest/caddytest.go @@ -100,7 +100,7 @@ func (tc *Tester) InitServer(rawConfig string, configType string) { tc.t.Fail() } if err := tc.ensureConfigRunning(rawConfig, configType); err != nil { - tc.t.Logf("failed ensurng config is running: %s", err) + tc.t.Logf("failed ensuring config is running: %s", err) tc.t.Fail() } } diff --git a/modules/caddyhttp/routes.go b/modules/caddyhttp/routes.go index 45cbd80db..ce9bece20 100644 --- a/modules/caddyhttp/routes.go +++ b/modules/caddyhttp/routes.go @@ -109,6 +109,17 @@ func (r Route) Empty() bool { r.Group == "" } +func (r Route) String() string { + handlersRaw := "[" + for _, hr := range r.HandlersRaw { + handlersRaw += " " + string(hr) + } + handlersRaw += "]" + + return fmt.Sprintf(`{Group:"%s" MatcherSetsRaw:%s HandlersRaw:%s Terminal:%t}`, + r.Group, r.MatcherSetsRaw, handlersRaw, r.Terminal) +} + // RouteList is a list of server routes that can // create a middleware chain. type RouteList []Route @@ -331,4 +342,15 @@ func (ms *MatcherSets) FromInterface(matcherSets any) error { return nil } +// TODO: Is this used? +func (ms MatcherSets) String() string { + result := "[" + for _, matcherSet := range ms { + for _, matcher := range matcherSet { + result += fmt.Sprintf(" %#v", matcher) + } + } + return result + " ]" +} + var routeGroupCtxKey = caddy.CtxKey("route_group")