diff --git a/caddyconfig/httpcaddyfile/httptype.go b/caddyconfig/httpcaddyfile/httptype.go index 1ccaed244..d9c4d3246 100644 --- a/caddyconfig/httpcaddyfile/httptype.go +++ b/caddyconfig/httpcaddyfile/httptype.go @@ -617,7 +617,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 { @@ -626,7 +626,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) } } @@ -904,6 +904,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 @@ -930,6 +931,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) } diff --git a/caddytest/integration/caddyfile_adapt/handle_errors_subroute_fallback.txt b/caddytest/integration/caddyfile_adapt/handle_errors_subroute_fallback.txt new file mode 100644 index 000000000..6b3e09b83 --- /dev/null +++ b/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 + } + ] + } + } + } + } + } +} \ No newline at end of file diff --git a/modules/caddyhttp/server.go b/modules/caddyhttp/server.go index 294ee6afd..1f68a0753 100644 --- a/modules/caddyhttp/server.go +++ b/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 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) - } - } - } else { + // 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...) + } + w.WriteHeader(errStatus) + return + } + + // 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) + } + 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.