reverseproxy: Fix double headers in response handlers (#4847)

3 files changed, 34 insertions, 86 deletions

diff --git a/modules/caddyhttp/reverseproxy/copyresponse.go b/modules/caddyhttp/reverseproxy/copyresponse.go
index 72a55d4..c1c9de9 100644
--- a/modules/caddyhttp/reverseproxy/copyresponse.go
+++ b/modules/caddyhttp/reverseproxy/copyresponse.go
@@ -80,7 +80,7 @@ func (h CopyResponseHandler) ServeHTTP(rw http.ResponseWriter, req *http.Request
 	hrc.isFinalized = true
 
 	// write the response
-	return hrc.handler.finalizeResponse(rw, req, hrc.response, repl, hrc.start, hrc.logger, false)
+	return hrc.handler.finalizeResponse(rw, req, hrc.response, repl, hrc.start, hrc.logger)
 }
 
 // CopyResponseHeadersHandler is a special HTTP handler which may
diff --git a/modules/caddyhttp/reverseproxy/forwardauth/caddyfile.go b/modules/caddyhttp/reverseproxy/forwardauth/caddyfile.go
index c22ddde..8230216 100644
--- a/modules/caddyhttp/reverseproxy/forwardauth/caddyfile.go
+++ b/modules/caddyhttp/reverseproxy/forwardauth/caddyfile.go
@@ -59,13 +59,6 @@ func init() {
 //                 Remote-Email {http.reverse_proxy.header.Remote-Email}
 //             }
 //         }
-//
-//         handle_response {
-//             copy_response_headers {
-//                 exclude Connection Keep-Alive Te Trailers Transfer-Encoding Upgrade
-//             }
-//             copy_response
-//         }
 //     }
 //
 func parseCaddyfile(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error) {
@@ -217,41 +210,13 @@ func parseCaddyfile(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error)
 			},
 		)
 	}
-	rpHandler.HandleResponse = append(rpHandler.HandleResponse, goodResponseHandler)
 
-	// set up handler for denial responses; when a response
-	// has any other status than 2xx, then we copy the response
-	// back to the client, and terminate handling.
-	denialResponseHandler := caddyhttp.ResponseHandler{
-		Routes: []caddyhttp.Route{
-			{
-				HandlersRaw: []json.RawMessage{caddyconfig.JSONModuleObject(
-					&reverseproxy.CopyResponseHeadersHandler{
-						Exclude: []string{
-							"Connection",
-							"Keep-Alive",
-							"Te",
-							"Trailers",
-							"Transfer-Encoding",
-							"Upgrade",
-						},
-					},
-					"handler",
-					"copy_response_headers",
-					nil,
-				)},
-			},
-			{
-				HandlersRaw: []json.RawMessage{caddyconfig.JSONModuleObject(
-					&reverseproxy.CopyResponseHandler{},
-					"handler",
-					"copy_response",
-					nil,
-				)},
-			},
-		},
-	}
-	rpHandler.HandleResponse = append(rpHandler.HandleResponse, denialResponseHandler)
+	// note that when a response has any other status than 2xx, then we
+	// use the reverse proxy's default behaviour of copying the response
+	// back to the client, so we don't need to explicitly add a response
+	// handler specifically for that behaviour; we do need the 2xx handler
+	// though, to make handling fall through to handlers deeper in the chain.
+	rpHandler.HandleResponse = append(rpHandler.HandleResponse, goodResponseHandler)
 
 	// the rest of the config is specified by the user
 	// using the reverse_proxy directive syntax
diff --git a/modules/caddyhttp/reverseproxy/reverseproxy.go b/modules/caddyhttp/reverseproxy/reverseproxy.go
index 1068c23..15e3104 100644
--- a/modules/caddyhttp/reverseproxy/reverseproxy.go
+++ b/modules/caddyhttp/reverseproxy/reverseproxy.go
@@ -784,18 +784,14 @@ func (h *Handler) reverseProxy(rw http.ResponseWriter, req *http.Request, origRe
 		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) {
 			continue
 		}
 
-		// if configured to only change the status code, do that then continue regular proxy response
+		// if configured to only change the status code,
+		// do that then continue regular proxy response
 		if statusCodeStr := rh.StatusCode.String(); statusCodeStr != "" {
 			statusCode, err := strconv.Atoi(repl.ReplaceAll(statusCodeStr, ""))
 			if err != nil {
@@ -840,33 +836,29 @@ func (h *Handler) reverseProxy(rw http.ResponseWriter, req *http.Request, origRe
 		// pass the request through the response handler routes
 		routeErr := rh.Routes.Compile(next).ServeHTTP(rw, origReq.WithContext(ctx))
 
-		// if the response handler routes already finalized the response,
-		// we can return early. It should be finalized if the routes executed
-		// included a copy_response handler. If a fresh response was written
-		// by the routes instead, then we still need to finalize the response
-		// without copying the body.
-		if routeErr == nil && hrc.isFinalized {
-			return nil
+		// close the response body afterwards, since we don't need it anymore;
+		// either a route had 'copy_response' which already consumed the body,
+		// or some other terminal handler ran which doesn't need the response
+		// body after that point (e.g. 'file_server' for X-Accel-Redirect flow),
+		// or we fell through to subsequent handlers past this proxy
+		// (e.g. forward auth's 2xx response flow).
+		if !hrc.isFinalized {
+			res.Body.Close()
 		}
 
-		// 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, if it wasn't already finalized.
-		res.Body.Close()
-		bodyClosed = true
-
+		// wrap any route error in roundtripSucceeded so caller knows that
+		// the roundtrip was successful and to not retry
 		if routeErr != nil {
-			// wrap error in roundtripSucceeded so caller knows that
-			// the roundtrip was successful and to not retry
 			return roundtripSucceeded{routeErr}
 		}
 
-		// we've already closed the body, so there's no use allowing
-		// another response handler to run as well
-		break
+		// we're done handling the response, and we don't want to
+		// fall through to the default finalize/copy behaviour
+		return nil
 	}
 
-	return h.finalizeResponse(rw, req, res, repl, start, logger, bodyClosed)
+	// copy the response body and headers back to the upstream client
+	return h.finalizeResponse(rw, req, res, repl, start, logger)
 }
 
 // finalizeResponse prepares and copies the response.
@@ -877,7 +869,6 @@ func (h Handler) finalizeResponse(
 	repl *caddy.Replacer,
 	start time.Time,
 	logger *zap.Logger,
-	bodyClosed bool,
 ) error {
 	// deal with 101 Switching Protocols responses: (WebSocket, h2c, etc)
 	if res.StatusCode == http.StatusSwitchingProtocols {
@@ -891,13 +882,6 @@ func (h Handler) finalizeResponse(
 		res.Header.Del(h)
 	}
 
-	// remove the content length if we're not going to be copying
-	// from the response, because otherwise there'll be a mismatch
-	// between bytes written and the advertised length
-	if bodyClosed {
-		res.Header.Del("Content-Length")
-	}
-
 	// apply any response header operations
 	if h.Headers != nil && h.Headers.Response != nil {
 		if h.Headers.Response.Require == nil ||
@@ -920,17 +904,16 @@ func (h Handler) finalizeResponse(
 	}
 
 	rw.WriteHeader(res.StatusCode)
-	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
-		}
+
+	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 {