From 95b6ac44a6122d3ca5513a13bbc723cd5f4785f8 Mon Sep 17 00:00:00 2001 From: Francis Lavoie Date: Sat, 24 Apr 2021 18:51:42 -0400 Subject: [PATCH] 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 | 19 +++- .../handle_errors_subroute_fallback.txt | 89 +++++++++++++++++++ modules/caddyhttp/server.go | 70 ++++++++++----- 3 files changed, 152 insertions(+), 26 deletions(-) create mode 100644 caddytest/integration/caddyfile_adapt/handle_errors_subroute_fallback.txt 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.