Mirrors/caddy
1
0
Fork 0
mirror of https://github.com/caddyserver/caddy.git synced 2025-01-22 16:46:53 +01:00
Code Issues Projects Releases Packages Wiki Activity

caddyhttp: Fix fallback for the error handler chain

Browse source 
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).
This commit is contained in:
Francis Lavoie 2021-04-24 18:51:42 -04:00
parent 74f5d66c48
commit 95b6ac44a6
No known key found for this signature in database
GPG key ID: 7D1A27F0725BE5D8
3 changed files with 152 additions and 26 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

19
caddyconfig/httpcaddyfile/httptype.go
View file

@ -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)
}

89
caddytest/integration/caddyfile_adapt/handle_errors_subroute_fallback.txt Normal file
View file

@ -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
}
]
}
}
}
}
}
}

70
modules/caddyhttp/server.go
View file

@ -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.