From 5012ddd20f99feb9ce98dbb1bf8026ea233ff1f3 Mon Sep 17 00:00:00 2001
From: Francis Lavoie <lavofr@gmail.com>
Date: Sun, 22 Aug 2021 14:22:56 -0400
Subject: [PATCH] reverseproxy: Move status replacement intercept to
 `replace_status`

---
 .../reverse_proxy_handle_response.txt         | 22 +++---
 modules/caddyhttp/reverseproxy/caddyfile.go   | 79 +++++++++++--------
 2 files changed, 56 insertions(+), 45 deletions(-)

diff --git a/caddytest/integration/caddyfile_adapt/reverse_proxy_handle_response.txt b/caddytest/integration/caddyfile_adapt/reverse_proxy_handle_response.txt
index 35d9631a78c6..88ecbc210db0 100644
--- a/caddytest/integration/caddyfile_adapt/reverse_proxy_handle_response.txt
+++ b/caddytest/integration/caddyfile_adapt/reverse_proxy_handle_response.txt
@@ -1,6 +1,9 @@
 :8884
 
 reverse_proxy 127.0.0.1:65535 {
+	@changeStatus status 500
+	replace_status @changeStatus 400
+
 	@accel header X-Accel-Redirect *
 	handle_response @accel {
 		respond "Header X-Accel-Redirect!"
@@ -38,9 +41,6 @@ reverse_proxy 127.0.0.1:65535 {
 	handle_response @multi {
 		respond "Headers Foo, Bar AND statuses 401, 403 and 404!"
 	}
-
-	@changeStatus status 500
-	handle_response @changeStatus 400
 }
 ----------
 {
@@ -56,6 +56,14 @@ reverse_proxy 127.0.0.1:65535 {
 							"handle": [
 								{
 									"handle_response": [
+										{
+											"match": {
+												"status_code": [
+													500
+												]
+											},
+											"status_code": 400
+										},
 										{
 											"match": {
 												"headers": {
@@ -155,14 +163,6 @@ reverse_proxy 127.0.0.1:65535 {
 												}
 											]
 										},
-										{
-											"match": {
-												"status_code": [
-													500
-												]
-											},
-											"status_code": 400
-										},
 										{
 											"routes": [
 												{
diff --git a/modules/caddyhttp/reverseproxy/caddyfile.go b/modules/caddyhttp/reverseproxy/caddyfile.go
index c7f555f8a44e..6e44751d5ffb 100644
--- a/modules/caddyhttp/reverseproxy/caddyfile.go
+++ b/modules/caddyhttp/reverseproxy/caddyfile.go
@@ -91,14 +91,15 @@ func parseCaddyfile(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHandler, error)
 //             ...
 //         }
 //
-//         # handle responses
+//         # intercepting responses
 //         @name {
 //             status <code...>
 //             header <field> [<value>]
 //         }
-//         handle_response [<matcher>] [status_code] {
+//         handle_response [<matcher>] {
 //             <directives...>
 //         }
+//         replace_status <matcher> <status_code>
 //     }
 //
 // Proxy upstream addresses should be network dial addresses such
@@ -653,6 +654,39 @@ func (h *Handler) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
 				// See h.FinalizeUnmarshalCaddyfile
 				h.handleResponseSegments = append(h.handleResponseSegments, d.NewFromNextSegment())
 
+			case "replace_status":
+				args := d.RemainingArgs()
+				if len(args) != 2 {
+					return d.Errf("must have two arguments: a response matcher and a status code")
+				}
+
+				if !strings.HasPrefix(args[0], matcherPrefix) {
+					return d.Errf("must use a named response matcher, starting with '@'")
+				}
+
+				foundMatcher, ok := h.responseMatchers[args[0]]
+				if !ok {
+					return d.Errf("no named response matcher defined with name '%s'", args[0][1:])
+				}
+
+				_, err := strconv.Atoi(args[1])
+				if err != nil {
+					return d.Errf("bad integer value '%s': %v", args[1], err)
+				}
+
+				// make sure there's no block, cause it doesn't make sense
+				if d.NextBlock(1) {
+					return d.Errf("cannot define routes for 'replace_status', use 'handle_response' instead.")
+				}
+
+				h.HandleResponse = append(
+					h.HandleResponse,
+					caddyhttp.ResponseHandler{
+						Match:      &foundMatcher,
+						StatusCode: caddyhttp.WeakString(args[1]),
+					},
+				)
+
 			default:
 				return d.Errf("unrecognized subdirective %s", d.Val())
 			}
@@ -701,20 +735,20 @@ func (h *Handler) FinalizeUnmarshalCaddyfile(helper httpcaddyfile.Helper) error
 	for _, d := range h.handleResponseSegments {
 		// consume the "handle_response" token
 		d.Next()
-
-		var matcher *caddyhttp.ResponseMatcher
 		args := d.RemainingArgs()
 
-		// the first arg should be a matcher (optional)
-		// the second arg should be a status code (optional)
-		// any more than that isn't currently supported
-		if len(args) > 2 {
+		// TODO: Remove this check at some point in the future
+		if len(args) == 2 {
+			return d.Errf("configuring 'handle_response' for status code replacement is no longer supported. Use 'replace_status' instead.")
+		}
+
+		if len(args) > 1 {
 			return d.Errf("too many arguments for 'handle_response': %s", args)
 		}
 
-		// the first arg should always be a matcher.
-		// it doesn't really make sense to support status code without a matcher.
-		if len(args) > 0 {
+		var matcher *caddyhttp.ResponseMatcher
+		if len(args) == 1 {
+			// the first arg should always be a matcher.
 			if !strings.HasPrefix(args[0], matcherPrefix) {
 				return d.Errf("must use a named response matcher, starting with '@'")
 			}
@@ -726,29 +760,6 @@ func (h *Handler) FinalizeUnmarshalCaddyfile(helper httpcaddyfile.Helper) error
 			matcher = &foundMatcher
 		}
 
-		// a second arg should be a status code, in which case
-		// we skip parsing the block for routes
-		if len(args) == 2 {
-			_, err := strconv.Atoi(args[1])
-			if err != nil {
-				return d.Errf("bad integer value '%s': %v", args[1], err)
-			}
-
-			// make sure there's no block, cause it doesn't make sense
-			if d.NextBlock(1) {
-				return d.Errf("cannot define routes for 'handle_response' when changing the status code")
-			}
-
-			h.HandleResponse = append(
-				h.HandleResponse,
-				caddyhttp.ResponseHandler{
-					Match:      matcher,
-					StatusCode: caddyhttp.WeakString(args[1]),
-				},
-			)
-			continue
-		}
-
 		// parse the block as routes
 		handler, err := httpcaddyfile.ParseSegmentAsSubroute(helper.WithDispenser(d.NewFromNextSegment()))
 		if err != nil {