From b092061591d673d5a886881531df2b39cd7722e3 Mon Sep 17 00:00:00 2001 From: Francis Lavoie Date: Mon, 18 Oct 2021 14:00:43 -0400 Subject: reverseproxy: Prevent copying the response if a response handler ran (#4388) --- modules/caddyhttp/reverseproxy/reverseproxy.go | 39 ++++++++++++++++++-------- 1 file changed, 27 insertions(+), 12 deletions(-) (limited to 'modules/caddyhttp/reverseproxy/reverseproxy.go') diff --git a/modules/caddyhttp/reverseproxy/reverseproxy.go b/modules/caddyhttp/reverseproxy/reverseproxy.go index 360b018..e626962 100644 --- a/modules/caddyhttp/reverseproxy/reverseproxy.go +++ b/modules/caddyhttp/reverseproxy/reverseproxy.go @@ -614,6 +614,11 @@ func (h *Handler) reverseProxy(rw http.ResponseWriter, req *http.Request, repl * res.Body = h.bufferedBody(res.Body) } + // the response body may get closed by a response handler, + // and we need to keep track to make sure we don't try to copy + // the response if it was already closed + bodyClosed := false + // see if any response handler is configured for this response from the backend for i, rh := range h.HandleResponse { if rh.Match != nil && !rh.Match.Match(res.StatusCode, res.Header) { @@ -638,8 +643,6 @@ func (h *Handler) reverseProxy(rw http.ResponseWriter, req *http.Request, repl * continue } - res.Body.Close() - // set up the replacer so that parts of the original response can be // used for routing decisions for field, value := range res.Header { @@ -649,7 +652,17 @@ func (h *Handler) reverseProxy(rw http.ResponseWriter, req *http.Request, repl * repl.Set("http.reverse_proxy.status_text", res.Status) h.logger.Debug("handling response", zap.Int("handler", i)) - if routeErr := rh.Routes.Compile(next).ServeHTTP(rw, req); routeErr != nil { + + // pass the request through the response handler routes + routeErr := rh.Routes.Compile(next).ServeHTTP(rw, req) + + // always close the response body afterwards since it's expected + // that the response handler routes will have written to the + // response writer with a new body + res.Body.Close() + bodyClosed = true + + if routeErr != nil { // wrap error in roundtripSucceeded so caller knows that // the roundtrip was successful and to not retry return roundtripSucceeded{routeErr} @@ -690,15 +703,17 @@ func (h *Handler) reverseProxy(rw http.ResponseWriter, req *http.Request, repl * } rw.WriteHeader(res.StatusCode) - err = h.copyResponse(rw, res.Body, h.flushInterval(req, res)) - res.Body.Close() // close now, instead of defer, to populate res.Trailer - if err != nil { - // we're streaming the response and we've already written headers, so - // there's nothing an error handler can do to recover at this point; - // the standard lib's proxy panics at this point, but we'll just log - // the error and abort the stream here - h.logger.Error("aborting with incomplete response", zap.Error(err)) - return nil + if !bodyClosed { + err = h.copyResponse(rw, res.Body, h.flushInterval(req, res)) + res.Body.Close() // close now, instead of defer, to populate res.Trailer + if err != nil { + // we're streaming the response and we've already written headers, so + // there's nothing an error handler can do to recover at this point; + // the standard lib's proxy panics at this point, but we'll just log + // the error and abort the stream here + h.logger.Error("aborting with incomplete response", zap.Error(err)) + return nil + } } if len(res.Trailer) > 0 { -- cgit v1.2.3