caddyhttp: Fix fallback for the error handler chain

This is mainly a problem with the default behaviour of the Caddyfile's `handle_errors` routes, but it's kinda tricky to solve well. I went with an approach that involves a smidge of magic which might not really be desirable.

See https://caddy.community/t/problem-with-basicauth-handle-errors/12243/9 for context.

So we do already have a default fallback for error routes, i.e. `errorEmptyHandler` in `caddyhttp.go` which is always configured as the last handler in the chain (implicitly, not visible in the config).

The problem is that when subroutes come into play with `"terminal": true`, then this fallback handler will never be reached. This is the case when the Caddyfile generates a config which has a host matcher from a site block (which is most of the time) when the user configured `handle_errors` to handle specific errors (such as 502s or 404s to serve HTML pages for those, etc). If other errors, like `basicauth`'s 401s are emitted in that case, then the result is that the default of HTTP status 200 will be served instead of the 401, which breaks `basicauth` completely.

The fix I went with is to make the Caddyfile adapter append special `error` handlers inside of the `handle_errors` subroutes which throw error `-1`, which `server.go` then picks up, and seeing `-1` responds with the original error code of `401` instead. The `-1` thing is the aforementioned magic.

At first, I had this implemented with `static_response` setting the StatusCode to `{http.error.status_code}`, but it didn't feel right to use a placeholder because it's inherently slightly less efficient, and it wasn't 100% correct because non-handler errors wouldn't be handled as 500s properly I think (because if it's not a `HandlerError`, then `http.error.status_code` doesn't exist, so it would maybe try to write an the placeholder replacement result of an empty string as `0` for the status code).

caddyconfig/httpcaddyfile/httptype.go

@@ -614,7 +614,7 @@ func (st *ServerType) serversFromPairings(
 			}
 
 			// add the site block's route(s) to the server
-			srv.Routes = appendSubrouteToRouteList(srv.Routes, siteSubroute, matcherSetsEnc, p, warnings)
+			srv.Routes = appendSubrouteToRouteList(srv.Routes, siteSubroute, matcherSetsEnc, p, false, warnings)
 
 			// if error routes are defined, add those too
 			if errorSubrouteVals, ok := sblock.pile["error_route"]; ok {
@@ -623,7 +623,7 @@ func (st *ServerType) serversFromPairings(
 				}
 				for _, val := range errorSubrouteVals {
 					sr := val.Value.(*caddyhttp.Subroute)
-					srv.Errors.Routes = appendSubrouteToRouteList(srv.Errors.Routes, sr, matcherSetsEnc, p, warnings)
+					srv.Errors.Routes = appendSubrouteToRouteList(srv.Errors.Routes, sr, matcherSetsEnc, p, true, warnings)
 				}
 			}
 
@@ -901,6 +901,7 @@ func appendSubrouteToRouteList(routeList caddyhttp.RouteList,
 	subroute *caddyhttp.Subroute,
 	matcherSetsEnc []caddy.ModuleMap,
 	p sbAddrAssociation,
+	isError bool,
 	warnings *[]caddyconfig.Warning) caddyhttp.RouteList {
 
 	// nothing to do if... there's nothing to do
@@ -927,6 +928,20 @@ func appendSubrouteToRouteList(routeList caddyhttp.RouteList,
 				caddyconfig.JSONModuleObject(subroute, "handler", "subroute", warnings),
 			}
 		}
+
+		// for error subroutes, we need to append a fallback static error
+		// because the default fallback won't be reached due to the route
+		// being marked as terminal. Using -1 as a special value to tell
+		// the handling in server.go to write the original error status instead.
+		if isError {
+			route.HandlersRaw = append(
+				route.HandlersRaw,
+				caddyconfig.JSONModuleObject(caddyhttp.StaticError{
+					StatusCode: caddyhttp.WeakString(strconv.Itoa(caddyhttp.WriteOriginalErrorCode)),
+				}, "handler", "error", warnings),
+			)
+		}
+
 		if len(route.MatcherSetsRaw) > 0 || len(route.HandlersRaw) > 0 {
 			routeList = append(routeList, route)
 		}

caddytest/integration/caddyfile_adapt/handle_errors_subroute_fallback.txt

@@ -0,0 +1,89 @@
+http://127.0.0.1 {
+    handle_errors {
+	@404 expression `{http.error.status_code} == 404`
+	respond @404 "Not Found"
+    }
+
+    file_server
+}
+----------
+{
+	"apps": {
+		"http": {
+			"servers": {
+				"srv0": {
+					"listen": [
+						":80"
+					],
+					"routes": [
+						{
+							"match": [
+								{
+									"host": [
+										"127.0.0.1"
+									]
+								}
+							],
+							"handle": [
+								{
+									"handler": "subroute",
+									"routes": [
+										{
+											"handle": [
+												{
+													"handler": "file_server",
+													"hide": [
+														"./Caddyfile"
+													]
+												}
+											]
+										}
+									]
+								}
+							],
+							"terminal": true
+						}
+					],
+					"errors": {
+						"routes": [
+							{
+								"match": [
+									{
+										"host": [
+											"127.0.0.1"
+										]
+									}
+								],
+								"handle": [
+									{
+										"handler": "subroute",
+										"routes": [
+											{
+												"handle": [
+													{
+														"body": "Not Found",
+														"handler": "static_response"
+													}
+												],
+												"match": [
+													{
+														"expression": "{http.error.status_code} == 404"
+													}
+												]
+											}
+										]
+									},
+									{
+										"handler": "error",
+										"status_code": -1
+									}
+								],
+								"terminal": true
+							}
+						]
+					}
+				}
+			}
+		}
+	}
+}

modules/caddyhttp/server.go

@@ -233,34 +233,51 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	// add HTTP error information to request context
 	r = s.Errors.WithError(r, err)
 
-	if s.Errors != nil && len(s.Errors.Routes) > 0 {
-		// execute user-defined error handling route
-		err2 := s.errorHandlerChain.ServeHTTP(w, r)
-		if err2 == nil {
-			// user's error route handled the error response
-			// successfully, so now just log the error
+	// if there's no error routes, then just write out the error status
+	if s.Errors == nil || len(s.Errors.Routes) == 0 {
+		if errStatus >= 500 {
+			logger.Error(errMsg, errFields...)
+		}
+		w.WriteHeader(errStatus)
+		return
+	}
+
+	// execute user-defined error handling route
+	err2 := s.errorHandlerChain.ServeHTTP(w, r)
+
+	// if the error handlers emit an error, it's usually
+	// a problem, but in some cases it's intended
+	if err2 != nil {
+		// if the error was WriteOriginalErrorCode, then it was meant as a signal
+		// from the error routes to just write the original error
+		// see https://caddy.community/t/problem-with-basicauth-handle-errors/12243/9
+		if handlerErr, ok := err2.(HandlerError); ok && handlerErr.StatusCode == WriteOriginalErrorCode {
 			if errStatus >= 500 {
 				logger.Error(errMsg, errFields...)
 			}
-		} else {
-			// well... this is awkward
-			errFields = append([]zapcore.Field{
-				zap.String("error", err2.Error()),
-				zap.Namespace("first_error"),
-				zap.String("msg", errMsg),
-			}, errFields...)
-			logger.Error("error handling handler error", errFields...)
-			if handlerErr, ok := err.(HandlerError); ok {
-				w.WriteHeader(handlerErr.StatusCode)
-			} else {
-				w.WriteHeader(http.StatusInternalServerError)
-			}
+			w.WriteHeader(errStatus)
+			return
 		}
-	} else {
-		if errStatus >= 500 {
-			logger.Error(errMsg, errFields...)
+
+		// well... this is awkward, the error handler
+		// returned an unexpected error
+		logger.Error("error handling handler error", append([]zapcore.Field{
+			zap.String("error", err2.Error()),
+			zap.Namespace("first_error"),
+			zap.String("msg", errMsg),
+		}, errFields...)...)
+		if handlerErr, ok := err.(HandlerError); ok {
+			w.WriteHeader(handlerErr.StatusCode)
+		} else {
+			w.WriteHeader(http.StatusInternalServerError)
 		}
-		w.WriteHeader(errStatus)
+		return
+	}
+
+	// user's error route handled the error response
+	// successfully, so now just log the error if necessary
+	if errStatus >= 500 {
+		logger.Error(errMsg, errFields...)
 	}
 }
 
@@ -615,6 +632,11 @@ const (
 
 	// commonLogEmptyValue is the common empty log value.
 	commonLogEmptyValue = "-"
+
+	// WriteOriginalErrorCode is the special error code to use to signal
+	// the server error handling logic to write the original error code
+	// it received as a fallback if the error routes don't handle the error.
+	WriteOriginalErrorCode = -1
 )
 
 // Context keys for HTTP request context values.