intercept: fix replace_status being silently dropped (#7810)
Some checks are pending
Tests / test (./cmd/caddy/caddy, ~1.26.0, macos-14, 0, 1.26, mac) (push) Waiting to run
Tests / test (./cmd/caddy/caddy, ~1.26.0, ubuntu-latest, 0, 1.26, linux) (push) Waiting to run
Tests / test (./cmd/caddy/caddy.exe, ~1.26.0, windows-latest, True, 1.26, windows) (push) Waiting to run
Tests / test (s390x on IBM Z) (push) Waiting to run
Tests / goreleaser-check (push) Waiting to run
Cross-Build / build (~1.26.0, 1.26, aix) (push) Waiting to run
Cross-Build / build (~1.26.0, 1.26, darwin) (push) Waiting to run
Cross-Build / build (~1.26.0, 1.26, dragonfly) (push) Waiting to run
Cross-Build / build (~1.26.0, 1.26, freebsd) (push) Waiting to run
Cross-Build / build (~1.26.0, 1.26, illumos) (push) Waiting to run
Cross-Build / build (~1.26.0, 1.26, linux) (push) Waiting to run
Cross-Build / build (~1.26.0, 1.26, netbsd) (push) Waiting to run
Cross-Build / build (~1.26.0, 1.26, openbsd) (push) Waiting to run
Cross-Build / build (~1.26.0, 1.26, solaris) (push) Waiting to run
Cross-Build / build (~1.26.0, 1.26, windows) (push) Waiting to run
Lint / lint (push) Waiting to run
Lint / lint-1 (push) Waiting to run
Lint / lint-2 (push) Waiting to run
Lint / govulncheck (push) Waiting to run
Lint / dependency-review (push) Waiting to run
OpenSSF Scorecard supply-chain security / Scorecard analysis (push) Waiting to run

* 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 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
long.black 2026-06-13 06:50:58 +09:00 • committed by GitHub
parent 52dc6709fb
commit 16235cced5
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 89 additions and 14 deletions

View file

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

View file

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