caddyhttp: Add `MatchWithError` to replace SetVar hack

8 files changed, 222 insertions, 58 deletions

diff --git a/modules/caddyhttp/caddyhttp.go b/modules/caddyhttp/caddyhttp.go
index e1e71f4a0..218c0a069 100644
--- a/modules/caddyhttp/caddyhttp.go
+++ b/modules/caddyhttp/caddyhttp.go
@@ -40,6 +40,18 @@ type RequestMatcher interface {
 	Match(*http.Request) bool
 }
 
+// RequestMatcherWithError is like RequestMatcher but can return an error.
+// An error during matching will abort the request middleware chain and
+// invoke the error middleware chain.
+//
+// This will eventually replace RequestMatcher. Matcher modules
+// should implement both interfaces, and once all modules have
+// been updated to use RequestMatcherWithError, the RequestMatcher
+// interface may eventually be dropped.
+type RequestMatcherWithError interface {
+	MatchWithError(*http.Request) (bool, error)
+}
+
 // Handler is like http.Handler except ServeHTTP may return an error.
 //
 // If any handler encounters an error, it should be returned for proper
diff --git a/modules/caddyhttp/celmatcher.go b/modules/caddyhttp/celmatcher.go
index 2a03ebba7..a54cf72e8 100644
--- a/modules/caddyhttp/celmatcher.go
+++ b/modules/caddyhttp/celmatcher.go
@@ -202,17 +202,25 @@ func (m *MatchExpression) Provision(ctx caddy.Context) error {
 
 // Match returns true if r matches m.
 func (m MatchExpression) Match(r *http.Request) bool {
+	match, err := m.MatchWithError(r)
+	if err != nil {
+		SetVar(r.Context(), MatcherErrorVarKey, err)
+	}
+	return match
+}
+
+// MatchWithError returns true if r matches m.
+func (m MatchExpression) MatchWithError(r *http.Request) (bool, error) {
 	celReq := celHTTPRequest{r}
 	out, _, err := m.prg.Eval(celReq)
 	if err != nil {
 		m.log.Error("evaluating expression", zap.Error(err))
-		SetVar(r.Context(), MatcherErrorVarKey, err)
-		return false
+		return false, err
 	}
 	if outBool, ok := out.Value().(bool); ok {
-		return outBool
+		return outBool, nil
 	}
-	return false
+	return false, nil
 }
 
 // UnmarshalCaddyfile implements caddyfile.Unmarshaler.
@@ -494,6 +502,13 @@ func CELMatcherDecorator(funcName string, fac CELMatcherFactory) interpreter.Int
 				// If needed this call could be changed to convert the value
 				// to a *http.Request using CEL's ConvertToNative method.
 				httpReq := celReq.Value().(celHTTPRequest)
+				if m, ok := matcher.(RequestMatcherWithError); ok {
+					match, err := m.MatchWithError(httpReq.Request)
+					if err != nil {
+						return types.WrapErr(err)
+					}
+					return types.Bool(match)
+				}
 				return types.Bool(matcher.Match(httpReq.Request))
 			},
 		), nil
@@ -509,6 +524,13 @@ func CELMatcherRuntimeFunction(funcName string, fac CELMatcherFactory) functions
 			return types.WrapErr(err)
 		}
 		httpReq := celReq.Value().(celHTTPRequest)
+		if m, ok := matcher.(RequestMatcherWithError); ok {
+			match, err := m.MatchWithError(httpReq.Request)
+			if err != nil {
+				return types.WrapErr(err)
+			}
+			return types.Bool(match)
+		}
 		return types.Bool(matcher.Match(httpReq.Request))
 	}
 }
@@ -733,9 +755,10 @@ const MatcherNameCtxKey = "matcher_name"
 
 // Interface guards
 var (
-	_ caddy.Provisioner     = (*MatchExpression)(nil)
-	_ RequestMatcher        = (*MatchExpression)(nil)
-	_ caddyfile.Unmarshaler = (*MatchExpression)(nil)
-	_ json.Marshaler        = (*MatchExpression)(nil)
-	_ json.Unmarshaler      = (*MatchExpression)(nil)
+	_ caddy.Provisioner       = (*MatchExpression)(nil)
+	_ RequestMatcher          = (*MatchExpression)(nil)
+	_ RequestMatcherWithError = (*MatchExpression)(nil)
+	_ caddyfile.Unmarshaler   = (*MatchExpression)(nil)
+	_ json.Marshaler          = (*MatchExpression)(nil)
+	_ json.Unmarshaler        = (*MatchExpression)(nil)
 )
diff --git a/modules/caddyhttp/fileserver/matcher.go b/modules/caddyhttp/fileserver/matcher.go
index 28f7b89be..0a5eece16 100644
--- a/modules/caddyhttp/fileserver/matcher.go
+++ b/modules/caddyhttp/fileserver/matcher.go
@@ -313,12 +313,21 @@ func (m MatchFile) Validate() error {
 //   - http.matchers.file.type: file or directory
 //   - http.matchers.file.remainder: Portion remaining after splitting file path (if configured)
 func (m MatchFile) Match(r *http.Request) bool {
+	match, err := m.selectFile(r)
+	if err != nil {
+		caddyhttp.SetVar(r.Context(), caddyhttp.MatcherErrorVarKey, err)
+	}
+	return match
+}
+
+// MatchWithError returns true if r matches m.
+func (m MatchFile) MatchWithError(r *http.Request) (bool, error) {
 	return m.selectFile(r)
 }
 
 // selectFile chooses a file according to m.TryPolicy by appending
 // the paths in m.TryFiles to m.Root, with placeholder replacements.
-func (m MatchFile) selectFile(r *http.Request) (matched bool) {
+func (m MatchFile) selectFile(r *http.Request) (bool, error) {
 	repl := r.Context().Value(caddy.ReplacerCtxKey).(*caddy.Replacer)
 
 	root := filepath.Clean(repl.ReplaceAll(m.Root, "."))
@@ -330,7 +339,7 @@ func (m MatchFile) selectFile(r *http.Request) (matched bool) {
 		if c := m.logger.Check(zapcore.ErrorLevel, "use of unregistered filesystem"); c != nil {
 			c.Write(zap.String("fs", fsName))
 		}
-		return false
+		return false, nil
 	}
 	type matchCandidate struct {
 		fullpath, relative, splitRemainder string
@@ -422,14 +431,13 @@ func (m MatchFile) selectFile(r *http.Request) (matched bool) {
 	case "", tryPolicyFirstExist:
 		for _, pattern := range m.TryFiles {
 			if err := parseErrorCode(pattern); err != nil {
-				caddyhttp.SetVar(r.Context(), caddyhttp.MatcherErrorVarKey, err)
-				return
+				return false, err
 			}
 			candidates := makeCandidates(pattern)
 			for _, c := range candidates {
 				if info, exists := m.strictFileExists(fileSystem, c.fullpath); exists {
 					setPlaceholders(c, info)
-					return true
+					return true, nil
 				}
 			}
 		}
@@ -450,10 +458,10 @@ func (m MatchFile) selectFile(r *http.Request) (matched bool) {
 			}
 		}
 		if largestInfo == nil {
-			return false
+			return false, nil
 		}
 		setPlaceholders(largest, largestInfo)
-		return true
+		return true, nil
 
 	case tryPolicySmallestSize:
 		var smallestSize int64
@@ -471,10 +479,10 @@ func (m MatchFile) selectFile(r *http.Request) (matched bool) {
 			}
 		}
 		if smallestInfo == nil {
-			return false
+			return false, nil
 		}
 		setPlaceholders(smallest, smallestInfo)
-		return true
+		return true, nil
 
 	case tryPolicyMostRecentlyMod:
 		var recent matchCandidate
@@ -491,13 +499,13 @@ func (m MatchFile) selectFile(r *http.Request) (matched bool) {
 			}
 		}
 		if recentInfo == nil {
-			return false
+			return false, nil
 		}
 		setPlaceholders(recent, recentInfo)
-		return true
+		return true, nil
 	}
 
-	return
+	return false, nil
 }
 
 // parseErrorCode checks if the input is a status
diff --git a/modules/caddyhttp/ip_matchers.go b/modules/caddyhttp/ip_matchers.go
index 99eb39dff..dec2f75de 100644
--- a/modules/caddyhttp/ip_matchers.go
+++ b/modules/caddyhttp/ip_matchers.go
@@ -166,6 +166,11 @@ func (m MatchRemoteIP) Match(r *http.Request) bool {
 	return matches
 }
 
+// MatchWithError returns true if r matches m.
+func (m MatchRemoteIP) MatchWithError(r *http.Request) (bool, error) {
+	return m.Match(r), nil
+}
+
 // CaddyModule returns the Caddy module information.
 func (MatchClientIP) CaddyModule() caddy.ModuleInfo {
 	return caddy.ModuleInfo{
@@ -254,6 +259,11 @@ func (m MatchClientIP) Match(r *http.Request) bool {
 	return matches
 }
 
+// MatchWithError returns true if r matches m.
+func (m MatchClientIP) MatchWithError(r *http.Request) (bool, error) {
+	return m.Match(r), nil
+}
+
 func provisionCidrsZonesFromRanges(ranges []string) ([]*netip.Prefix, []string, error) {
 	cidrs := []*netip.Prefix{}
 	zones := []string{}
@@ -326,13 +336,15 @@ func matchIPByCidrZones(clientIP netip.Addr, zoneID string, cidrs []*netip.Prefi
 
 // Interface guards
 var (
-	_ RequestMatcher        = (*MatchRemoteIP)(nil)
-	_ caddy.Provisioner     = (*MatchRemoteIP)(nil)
-	_ caddyfile.Unmarshaler = (*MatchRemoteIP)(nil)
-	_ CELLibraryProducer    = (*MatchRemoteIP)(nil)
-
-	_ RequestMatcher        = (*MatchClientIP)(nil)
-	_ caddy.Provisioner     = (*MatchClientIP)(nil)
-	_ caddyfile.Unmarshaler = (*MatchClientIP)(nil)
-	_ CELLibraryProducer    = (*MatchClientIP)(nil)
+	_ RequestMatcher          = (*MatchRemoteIP)(nil)
+	_ RequestMatcherWithError = (*MatchRemoteIP)(nil)
+	_ caddy.Provisioner       = (*MatchRemoteIP)(nil)
+	_ caddyfile.Unmarshaler   = (*MatchRemoteIP)(nil)
+	_ CELLibraryProducer      = (*MatchRemoteIP)(nil)
+
+	_ RequestMatcher          = (*MatchClientIP)(nil)
+	_ RequestMatcherWithError = (*MatchClientIP)(nil)
+	_ caddy.Provisioner       = (*MatchClientIP)(nil)
+	_ caddyfile.Unmarshaler   = (*MatchClientIP)(nil)
+	_ CELLibraryProducer      = (*MatchClientIP)(nil)
 )
diff --git a/modules/caddyhttp/matchers.go b/modules/caddyhttp/matchers.go
index 93a36237b..d5f9e31c5 100644
--- a/modules/caddyhttp/matchers.go
+++ b/modules/caddyhttp/matchers.go
@@ -355,6 +355,11 @@ outer:
 	return false
 }
 
+// MatchWithError returns true if r matches m.
+func (m MatchHost) MatchWithError(r *http.Request) (bool, error) {
+	return m.Match(r), nil
+}
+
 // CELLibrary produces options that expose this matcher for use in CEL
 // expression matchers.
 //
@@ -627,6 +632,11 @@ func (MatchPath) matchPatternWithEscapeSequence(escapedPath, matchPath string) b
 	return matches
 }
 
+// MatchWithError returns true if r matches m.
+func (m MatchPath) MatchWithError(r *http.Request) (bool, error) {
+	return m.Match(r), nil
+}
+
 // CELLibrary produces options that expose this matcher for use in CEL
 // expression matchers.
 //
@@ -687,6 +697,11 @@ func (m MatchPathRE) Match(r *http.Request) bool {
 	return m.MatchRegexp.Match(cleanedPath, repl)
 }
 
+// MatchWithError returns true if r matches m.
+func (m MatchPathRE) MatchWithError(r *http.Request) (bool, error) {
+	return m.Match(r), nil
+}
+
 // CELLibrary produces options that expose this matcher for use in CEL
 // expression matchers.
 //
@@ -767,6 +782,11 @@ func (m MatchMethod) Match(r *http.Request) bool {
 	return slices.Contains(m, r.Method)
 }
 
+// MatchWithError returns true if r matches m.
+func (m MatchMethod) MatchWithError(r *http.Request) (bool, error) {
+	return m.Match(r), nil
+}
+
 // CELLibrary produces options that expose this matcher for use in CEL
 // expression matchers.
 //
@@ -867,6 +887,11 @@ func (m MatchQuery) Match(r *http.Request) bool {
 	return matchedKeys == len(m)
 }
 
+// MatchWithError returns true if r matches m.
+func (m MatchQuery) MatchWithError(r *http.Request) (bool, error) {
+	return m.Match(r), nil
+}
+
 // CELLibrary produces options that expose this matcher for use in CEL
 // expression matchers.
 //
@@ -944,6 +969,11 @@ func (m MatchHeader) Match(r *http.Request) bool {
 	return matchHeaders(r.Header, http.Header(m), r.Host, repl)
 }
 
+// MatchWithError returns true if r matches m.
+func (m MatchHeader) MatchWithError(r *http.Request) (bool, error) {
+	return m.Match(r), nil
+}
+
 // CELLibrary produces options that expose this matcher for use in CEL
 // expression matchers.
 //
@@ -1093,6 +1123,11 @@ func (m MatchHeaderRE) Match(r *http.Request) bool {
 	return true
 }
 
+// MatchWithError returns true if r matches m.
+func (m MatchHeaderRE) MatchWithError(r *http.Request) (bool, error) {
+	return m.Match(r), nil
+}
+
 // Provision compiles m's regular expressions.
 func (m MatchHeaderRE) Provision(ctx caddy.Context) error {
 	for _, rm := range m {
@@ -1214,6 +1249,11 @@ func (m MatchProtocol) Match(r *http.Request) bool {
 	return false
 }
 
+// MatchWithError returns true if r matches m.
+func (m MatchProtocol) MatchWithError(r *http.Request) (bool, error) {
+	return m.Match(r), nil
+}
+
 // UnmarshalCaddyfile implements caddyfile.Unmarshaler.
 func (m *MatchProtocol) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
 	// iterate to merge multiple matchers into one
@@ -1270,6 +1310,11 @@ func (m MatchTLS) Match(r *http.Request) bool {
 	return true
 }
 
+// MatchWithError returns true if r matches m.
+func (m MatchTLS) MatchWithError(r *http.Request) (bool, error) {
+	return m.Match(r), nil
+}
+
 // UnmarshalCaddyfile parses Caddyfile tokens for this matcher. Syntax:
 //
 // ... tls [early_data]
@@ -1356,6 +1401,11 @@ func (m MatchNot) Match(r *http.Request) bool {
 	return true
 }
 
+// MatchWithError returns true if r matches m.
+func (m MatchNot) MatchWithError(r *http.Request) (bool, error) {
+	return m.Match(r), nil
+}
+
 // MatchRegexp is an embedable type for matching
 // using regular expressions. It adds placeholders
 // to the request's replacer.
@@ -1528,20 +1578,29 @@ const MatcherErrorVarKey = "matchers.error"
 
 // Interface guards
 var (
-	_ RequestMatcher    = (*MatchHost)(nil)
-	_ caddy.Provisioner = (*MatchHost)(nil)
-	_ RequestMatcher    = (*MatchPath)(nil)
-	_ RequestMatcher    = (*MatchPathRE)(nil)
-	_ caddy.Provisioner = (*MatchPathRE)(nil)
-	_ RequestMatcher    = (*MatchMethod)(nil)
-	_ RequestMatcher    = (*MatchQuery)(nil)
-	_ RequestMatcher    = (*MatchHeader)(nil)
-	_ RequestMatcher    = (*MatchHeaderRE)(nil)
-	_ caddy.Provisioner = (*MatchHeaderRE)(nil)
-	_ RequestMatcher    = (*MatchProtocol)(nil)
-	_ RequestMatcher    = (*MatchNot)(nil)
-	_ caddy.Provisioner = (*MatchNot)(nil)
-	_ caddy.Provisioner = (*MatchRegexp)(nil)
+	_ RequestMatcher          = (*MatchHost)(nil)
+	_ RequestMatcherWithError = (*MatchHost)(nil)
+	_ caddy.Provisioner       = (*MatchHost)(nil)
+	_ RequestMatcher          = (*MatchPath)(nil)
+	_ RequestMatcherWithError = (*MatchPath)(nil)
+	_ RequestMatcher          = (*MatchPathRE)(nil)
+	_ RequestMatcherWithError = (*MatchPathRE)(nil)
+	_ caddy.Provisioner       = (*MatchPathRE)(nil)
+	_ RequestMatcher          = (*MatchMethod)(nil)
+	_ RequestMatcherWithError = (*MatchMethod)(nil)
+	_ RequestMatcher          = (*MatchQuery)(nil)
+	_ RequestMatcherWithError = (*MatchQuery)(nil)
+	_ RequestMatcher          = (*MatchHeader)(nil)
+	_ RequestMatcherWithError = (*MatchHeader)(nil)
+	_ RequestMatcher          = (*MatchHeaderRE)(nil)
+	_ RequestMatcherWithError = (*MatchHeaderRE)(nil)
+	_ caddy.Provisioner       = (*MatchHeaderRE)(nil)
+	_ RequestMatcher          = (*MatchProtocol)(nil)
+	_ RequestMatcherWithError = (*MatchProtocol)(nil)
+	_ RequestMatcher          = (*MatchNot)(nil)
+	_ RequestMatcherWithError = (*MatchNot)(nil)
+	_ caddy.Provisioner       = (*MatchNot)(nil)
+	_ caddy.Provisioner       = (*MatchRegexp)(nil)
 
 	_ caddyfile.Unmarshaler = (*MatchHost)(nil)
 	_ caddyfile.Unmarshaler = (*MatchPath)(nil)
diff --git a/modules/caddyhttp/reverseproxy/reverseproxy.go b/modules/caddyhttp/reverseproxy/reverseproxy.go
index 1250eae6c..5ef37a4e8 100644
--- a/modules/caddyhttp/reverseproxy/reverseproxy.go
+++ b/modules/caddyhttp/reverseproxy/reverseproxy.go
@@ -496,7 +496,7 @@ func (h *Handler) proxyLoopIteration(r *http.Request, origReq *http.Request, w h
 		if proxyErr == nil {
 			proxyErr = caddyhttp.Error(http.StatusServiceUnavailable, errNoUpstream)
 		}
-		if !h.LoadBalancing.tryAgain(h.ctx, start, retries, proxyErr, r) {
+		if !h.LoadBalancing.tryAgain(h.ctx, start, retries, proxyErr, r, h.logger) {
 			return true, proxyErr
 		}
 		return false, proxyErr
@@ -562,7 +562,7 @@ func (h *Handler) proxyLoopIteration(r *http.Request, origReq *http.Request, w h
 	h.countFailure(upstream)
 
 	// if we've tried long enough, break
-	if !h.LoadBalancing.tryAgain(h.ctx, start, retries, proxyErr, r) {
+	if !h.LoadBalancing.tryAgain(h.ctx, start, retries, proxyErr, r, h.logger) {
 		return true, proxyErr
 	}
 
@@ -1089,7 +1089,7 @@ func (h *Handler) finalizeResponse(
 // If true is returned, it has already blocked long enough before
 // the next retry (i.e. no more sleeping is needed). If false is
 // returned, the handler should stop trying to proxy the request.
-func (lb LoadBalancing) tryAgain(ctx caddy.Context, start time.Time, retries int, proxyErr error, req *http.Request) bool {
+func (lb LoadBalancing) tryAgain(ctx caddy.Context, start time.Time, retries int, proxyErr error, req *http.Request, logger *zap.Logger) bool {
 	// no retries are configured
 	if lb.TryDuration == 0 && lb.Retries == 0 {
 		return false
@@ -1124,7 +1124,12 @@ func (lb LoadBalancing) tryAgain(ctx caddy.Context, start time.Time, retries int
 				return false
 			}
 
-			if !lb.RetryMatch.AnyMatch(req) {
+			match, err := lb.RetryMatch.AnyMatchWithError(req)
+			if err != nil {
+				logger.Error("error matching request for retry", zap.Error(err))
+				return false
+			}
+			if !match {
 				return false
 			}
 		}
diff --git a/modules/caddyhttp/routes.go b/modules/caddyhttp/routes.go
index 939d01e55..1e721229d 100644
--- a/modules/caddyhttp/routes.go
+++ b/modules/caddyhttp/routes.go
@@ -254,18 +254,13 @@ func wrapRoute(route Route) Middleware {
 			nextCopy := next
 
 			// route must match at least one of the matcher sets
-			if !route.MatcherSets.AnyMatch(req) {
+			matches, err := route.MatcherSets.AnyMatchWithError(req)
+			if err != nil {
 				// allow matchers the opportunity to short circuit
 				// the request and trigger the error handling chain
-				err, ok := GetVar(req.Context(), MatcherErrorVarKey).(error)
-				if ok {
-					// clear out the error from context, otherwise
-					// it will cascade to the error routes (#4916)
-					SetVar(req.Context(), MatcherErrorVarKey, nil)
-					// return the matcher's error
-					return err
-				}
-
+				return err
+			}
+			if !matches {
 				// call the next handler, and skip this one,
 				// since the matcher didn't match
 				return nextCopy.ServeHTTP(rw, req)
@@ -354,6 +349,30 @@ func (mset MatcherSet) Match(r *http.Request) bool {
 	return true
 }
 
+// MatchWithError returns true if r matches m.
+func (mset MatcherSet) MatchWithError(r *http.Request) (bool, error) {
+	for _, m := range mset {
+		if me, ok := m.(RequestMatcherWithError); ok {
+			match, err := me.MatchWithError(r)
+			if err != nil || !match {
+				return match, err
+			}
+		} else {
+			if !m.Match(r) {
+				// for backwards compatibility
+				err, ok := GetVar(r.Context(), MatcherErrorVarKey).(error)
+				if ok {
+					// clear out the error from context since we've consumed it
+					SetVar(r.Context(), MatcherErrorVarKey, nil)
+					return false, err
+				}
+				return false, nil
+			}
+		}
+	}
+	return true, nil
+}
+
 // RawMatcherSets is a group of matcher sets
 // in their raw, JSON form.
 type RawMatcherSets []caddy.ModuleMap
@@ -366,6 +385,8 @@ type MatcherSets []MatcherSet
 // AnyMatch returns true if req matches any of the
 // matcher sets in ms or if there are no matchers,
 // in which case the request always matches.
+//
+// Deprecated: Use AnyMatchWithError instead.
 func (ms MatcherSets) AnyMatch(req *http.Request) bool {
 	for _, m := range ms {
 		if m.Match(req) {
@@ -375,6 +396,20 @@ func (ms MatcherSets) AnyMatch(req *http.Request) bool {
 	return len(ms) == 0
 }
 
+// AnyMatchWithError returns true if req matches any of the
+// matcher sets in ms or if there are no matchers, in which
+// case the request always matches. If any matcher returns
+// an error, we cut short and return the error.
+func (ms MatcherSets) AnyMatchWithError(req *http.Request) (bool, error) {
+	for _, m := range ms {
+		match, err := m.MatchWithError(req)
+		if err != nil || match {
+			return match, err
+		}
+	}
+	return len(ms) == 0, nil
+}
+
 // FromInterface fills ms from an 'any' value obtained from LoadModule.
 func (ms *MatcherSets) FromInterface(matcherSets any) error {
 	for _, matcherSetIfaces := range matcherSets.([]map[string]any) {
diff --git a/modules/caddyhttp/vars.go b/modules/caddyhttp/vars.go
index 77e06e3cb..280843e70 100644
--- a/modules/caddyhttp/vars.go
+++ b/modules/caddyhttp/vars.go
@@ -207,6 +207,11 @@ func (m VarsMatcher) Match(r *http.Request) bool {
 	return false
 }
 
+// MatchWithError returns true if r matches m.
+func (m VarsMatcher) MatchWithError(r *http.Request) (bool, error) {
+	return m.Match(r), nil
+}
+
 // CELLibrary produces options that expose this matcher for use in CEL
 // expression matchers.
 //
@@ -328,6 +333,11 @@ func (m MatchVarsRE) Match(r *http.Request) bool {
 	return false
 }
 
+// MatchWithError returns true if r matches m.
+func (m MatchVarsRE) MatchWithError(r *http.Request) (bool, error) {
+	return m.Match(r), nil
+}
+
 // CELLibrary produces options that expose this matcher for use in CEL
 // expression matchers.
 //