From 16235cced55f137859aba2c7e590b014a3b92eb2 Mon Sep 17 00:00:00 2001 From: "long.black" <38776487+oksusucha@users.noreply.github.com> Date: Sat, 13 Jun 2026 06:50:58 +0900 Subject: [PATCH] intercept: fix replace_status being silently dropped (#7810) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test: add failing tests for intercept replace_status (#7805) Add integration tests that verify replace_status actually substitutes the HTTP status code sent to the client. Currently these tests fail because replace_status is silently a no-op due to value-receiver boxing and shouldBuffer returning false. Tests added: - TestInterceptReplaceStatusWithMatcher: 500 -> 200 with @err matcher - TestInterceptReplaceStatusWithoutMatcher: 403 -> 200 unconditionally - TestInterceptReplaceStatusNotMatched: 200 passes through unchanged * fix: make intercept replace_status actually substitute the status code Fix #7805: replace_status was silently a no-op because: 1. shouldBuffer returned false when a replacement status was set, causing the original status to be streamed directly to the wire 2. The value-receiver WriteHeader method operated on a stale copy The fix: - shouldBuffer now returns true when replace_status matches, so the response is buffered instead of streamed - After next.ServeHTTP returns, if routes are nil (replace_status only), write the substituted status and buffered body to the client The interceptedResponseHandler.WriteHeader substitution branch is no longer needed for this path since substitution happens post-ServeHTTP. * refactor: remove dead WriteHeader method and resolved TODO The value-receiver WriteHeader on interceptedResponseHandler was unreachable dead code — the substitution is now handled post-ServeHTTP via buffering. Remove it along with the TODO comment that noted status code replacement was unfinished. * style: apply nit suggestions from dunglas code review Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: Claude Sonnet 4.6 --- caddytest/integration/intercept_test.go | 69 ++++++++++++++++++++++++ modules/caddyhttp/intercept/intercept.go | 34 +++++++----- 2 files changed, 89 insertions(+), 14 deletions(-) diff --git a/caddytest/integration/intercept_test.go b/caddytest/integration/intercept_test.go index 6f8ffc929..c1b33b964 100644 --- a/caddytest/integration/intercept_test.go +++ b/caddytest/integration/intercept_test.go @@ -38,3 +38,72 @@ func TestIntercept(t *testing.T) { tester.AssertGetResponse("http://localhost:9080/no-intercept", 200, "I'm not a teapot") } + +func TestInterceptReplaceStatusWithMatcher(t *testing.T) { + tester := caddytest.NewTester(t) + tester.InitServer(`{ + skip_install_trust + admin localhost:2999 + http_port 9080 + https_port 9443 + grace_period 1ns + } + + localhost:9080 { + respond /error "boom" 500 + + intercept { + @err status 5xx + replace_status @err 200 + } + } + `, "caddyfile") + + tester.AssertGetResponse("http://localhost:9080/error", 200, "boom") +} + +func TestInterceptReplaceStatusWithoutMatcher(t *testing.T) { + tester := caddytest.NewTester(t) + tester.InitServer(`{ + skip_install_trust + admin localhost:2999 + http_port 9080 + https_port 9443 + grace_period 1ns + } + + localhost:9080 { + respond /forbidden "denied" 403 + + intercept { + replace_status 200 + } + } + `, "caddyfile") + + tester.AssertGetResponse("http://localhost:9080/forbidden", 200, "denied") +} + +func TestInterceptReplaceStatusNotMatched(t *testing.T) { + tester := caddytest.NewTester(t) + tester.InitServer(`{ + skip_install_trust + admin localhost:2999 + http_port 9080 + https_port 9443 + grace_period 1ns + } + + localhost:9080 { + respond /ok "all good" 200 + + intercept { + @err status 5xx + replace_status @err 503 + } + } + `, "caddyfile") + + // 200 does not match @err (5xx), so status should pass through unchanged + tester.AssertGetResponse("http://localhost:9080/ok", 200, "all good") +} diff --git a/modules/caddyhttp/intercept/intercept.go b/modules/caddyhttp/intercept/intercept.go index bacdc74b5..f0deb1b26 100644 --- a/modules/caddyhttp/intercept/intercept.go +++ b/modules/caddyhttp/intercept/intercept.go @@ -97,8 +97,6 @@ var bufPool = sync.Pool{ }, } -// TODO: handle status code replacement -// // EXPERIMENTAL: Subject to change or removal. type interceptedResponseHandler struct { caddyhttp.ResponseRecorder @@ -108,17 +106,6 @@ type interceptedResponseHandler struct { statusCode int } -// EXPERIMENTAL: Subject to change or removal. -func (irh interceptedResponseHandler) WriteHeader(statusCode int) { - if irh.statusCode != 0 && (statusCode < 100 || statusCode >= 200) { - irh.ResponseRecorder.WriteHeader(irh.statusCode) - - return - } - - irh.ResponseRecorder.WriteHeader(statusCode) -} - // EXPERIMENTAL: Subject to change or removal. func (irh interceptedResponseHandler) Unwrap() http.ResponseWriter { return irh.ResponseRecorder @@ -142,7 +129,7 @@ func (ir Intercept) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddy rec.handlerIndex = i // if configured to only change the status code, - // do that then stream + // buffer the response so we can substitute the status if statusCodeStr := rh.StatusCode.String(); statusCodeStr != "" { sc, err := strconv.Atoi(repl.ReplaceAll(statusCodeStr, "")) if err != nil { @@ -150,6 +137,8 @@ func (ir Intercept) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddy } else { rec.statusCode = sc } + + return true } return rec.statusCode == 0 @@ -176,6 +165,23 @@ func (ir Intercept) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddy c.Write(zap.Int("handler", rec.handlerIndex)) } + // replace_status only: no routes to execute, just substitute status and write body + if rec.handler.Routes == nil { + if rec.statusCode == 0 { + w.WriteHeader(rec.Status()) + } else { + w.WriteHeader(rec.statusCode) + } + + if buf.Len() > 0 { + _, err := io.Copy(w, buf) + + return err + } + + return nil + } + // response recorder doesn't create a new copy of the original headers, they're // present in the original response writer // create a new recorder to see if any response body from the new handler is present,