header_rewrite: fix set-body origin replacement across hooks and transforms by bryancall · Pull Request #13116 · apache/trafficserver

bryancall · 2026-04-23T19:09:56Z

Summary

make set-body robust for origin responses by correctly handling internal body replacement before tunneling, including transform-bypass safety in HttpSM
add READ_RESPONSE_HDR_HOOK as an allowed set-body hook while preserving SEND_RESPONSE_HDR_HOOK, and fix API state interaction in TSHttpTxnErrorBodySet()
consolidate AuTests into one replay-driven test covering read/send hooks with and without transforms, with cache disabled and cache-bypass probes; update docs accordingly

Production validation

Tested with live traffic on a single production edge/cache host using a build from this change set: staged header_rewrite, then traffic_server, with drain/restart and post-restart checks; exercised set-body on READ_RESPONSE_HDR for origin 403 responses via config reload. Behavior matched expectations during rollout observation; the host was returned to rotation and informal bake/monitoring continues.

Ready for review / merge from the author side.

Test plan

AUTEST_PORT_OFFSET=13000 ./autest.sh --ats-bin /tmp/ts-autest/bin --filter=header_rewrite_set_body_origin (eris)
AUTEST_PORT_OFFSET=15000 ./autest.sh --ats-bin /tmp/ts-autest/bin --filter=header_rewrite_bundle (eris)

Supersedes #12879.

Implement robust internal-body handling across READ_RESPONSE and SEND_RESPONSE paths, including transform interactions, and consolidate origin set-body coverage into a single replay-driven AuTest matrix with cache-bypass probes and clearer inline test documentation. Made-with: Cursor

bryancall · 2026-04-23T19:10:08Z

/copilot review

Copilot

Pull request overview

This PR extends header_rewrite’s set-body operator to robustly replace origin response bodies (including across hooks and in the presence of response transforms), updates core HttpSM handling to honor the internal message buffer before tunneling, and adds/updates AuTests and documentation to cover the new behavior.

Changes:

Update HttpSM to divert origin responses to setup_internal_transfer() when a plugin has set internal_msg_buffer (including transform-related paths).
Allow set-body to run at TS_HTTP_READ_RESPONSE_HDR_HOOK (in addition to TS_HTTP_SEND_RESPONSE_HDR_HOOK).
Add a replay-driven gold test matrix (read/send hooks; with/without transform) plus documentation updates.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`src/proxy/http/HttpSM.cc`	Adds internal-body override handling in `SERVER_READ` / `TRANSFORM_READ` and transform-bypass logic in `set_next_state()`.
`src/api/InkAPI.cc`	Ensures `TSHttpTxnErrorBodySet()` clears request-body override mode when reusing the shared buffer.
`plugins/header_rewrite/operators.cc`	Allows `OperatorSetBody` to run on `TS_HTTP_READ_RESPONSE_HDR_HOOK`.
`tests/gold_tests/pluginTest/header_rewrite/header_rewrite_set_body_origin.test.py`	Adds a new replay-driven test wrapper for origin replacement scenarios.
`tests/gold_tests/pluginTest/header_rewrite/header_rewrite_set_body_origin.replay.yaml`	Adds replay matrix for read/send hooks and transform/no-transform scenarios.
`tests/gold_tests/pluginTest/header_rewrite/rules/rule_set_body_origin_read_resp.conf`	New rule file to apply `set-body` at `READ_RESPONSE_HDR_HOOK`.
`tests/gold_tests/pluginTest/header_rewrite/rules/rule_set_body_origin_send_resp.conf`	New rule file to apply `set-body` at `SEND_RESPONSE_HDR_HOOK`.
`doc/admin-guide/plugins/header_rewrite.en.rst`	Documents origin replacement support and related caveats for `set-body`.
`tests/prepare_proxy_verifier.sh`	Updates expected Proxy Verifier tarball SHA1.

Harden transform bypass cleanup before internal transfer, keep no-transform cache probes transform-free, and clarify set-body empty-string behavior in docs. Made-with: Cursor

Guard the bypass path with server-response validity so non-origin regression flows (e.g. connect intercept tests) are not routed through origin replacement logic. Made-with: Cursor

Prevent TSHttpConnect intercept regressions from taking the set-body internal-transfer path by guarding the SERVER_READ override on origin-response hook context and plugin tunnel state. Made-with: Cursor

Prevent null transform hook cleanup crashes when internal response-body replacement bypasses transform setup, fixing -11 failures in header_rewrite_set_body_origin and filter_body AuTests. Made-with: Cursor

Add focused comments in HttpSM transform-read handling to explain why tunnel teardown, transform VC cleanup, response header seeding, and server session release are required before internal transfer. Made-with: Cursor

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 10 comments.

Clarify transform-inactive test/docs wording, assert Content-Type in transform-active set-body cases, and document why transform cleanup remains guarded in the internal-body bypass path to avoid the known -11 crash. Made-with: Cursor

zwoop

I think this looks good, complex code, so love the tests. Other than all the comment slop, my only question really is around that hook check. That seems unnecessary, albeit I'm not 100% sure. Possibly they should be assertions, but I don't see how you'd get there, if you don't have those plugin hooks.

zwoop · 2026-05-06T19:46:44Z

      setup_blind_tunnel(true, initial_data);
+    } else if (t_state.internal_msg_buffer && !t_state.api_server_request_body_set && t_state.hdr_info.server_response.valid() &&
+               plugin_tunnel == nullptr &&
+               (api_hooks.get(TS_HTTP_READ_RESPONSE_HDR_HOOK) != nullptr ||


Is it even possible to get here, without one of these being true ? What's the intent here, that the plugin API wasn't called from somewhere else? But then we wouldn't have a server_response.valid() either.

I don't know for sure, but feels like either this check on the hook is just superfluous, or possibly should be a assert instead?

zwoop · 2026-05-06T19:48:23Z

  s->internal_msg_buffer                     = buf;
  s->internal_msg_buffer_size                = buf ? buflength : 0;
  s->internal_msg_buffer_fast_allocator_size = -1;
+  // TSHttpTxnErrorBodySet() and TSHttpTxnServerRequestBodySet() share the same buffer.


Overall, and I'll start that here:

Claude has a big tendency to add comments that were part of its "thinking" phase. Explaining its reasoning for doing stuff. More often than not, I find those comments to be completely unnecessary, and often distracting, when the code itself is self explanatory. In most cases, the comments can be significantly reduced, and sometimes removed.

I'll refer to this as comment slop :).

zwoop · 2026-05-06T19:49:00Z

-    HttpTunnelProducer *p = setup_transfer_from_transform();
-    perform_transform_cache_write_action();
-    tunnel.tunnel_run(p);
+    // A plugin has installed an internal response body (TSHttpTxnErrorBodySet()).


Comment slop, code is self explanatory.

zwoop · 2026-05-06T19:49:21Z

+    if (t_state.internal_msg_buffer && !t_state.api_server_request_body_set && t_state.hdr_info.server_response.valid()) {
+      SMDbg(dbg_ctl_http, "plugin set internal body, bypassing response transform for internal transfer");
+      t_state.api_info.cache_untransformed = true;
+      // If a tunnel was already set up for transform I/O, shut it down before we re-route.


Comment slop, code says exactly the same thing as the comment.

zwoop · 2026-05-06T19:50:08Z

+        t_state.hdr_info.client_response.create(HTTPType::RESPONSE);
+        t_state.hdr_info.client_response.copy(&t_state.hdr_info.transform_response);
+      }
+      // The server session is not needed for internal body transfer if it was never tunneled.


Comment slop.

zwoop · 2026-05-06T19:50:40Z

+               plugin_tunnel == nullptr &&
+               (api_hooks.get(TS_HTTP_READ_RESPONSE_HDR_HOOK) != nullptr ||
+                api_hooks.get(TS_HTTP_SEND_RESPONSE_HDR_HOOK) != nullptr)) {
+      // A plugin replaced the origin response body via TSHttpTxnErrorBodySet().


Comment slop

zwoop · 2026-05-06T19:50:58Z

 HttpSM::transform_cleanup(TSHttpHookID hook, HttpTransformInfo *info)
 {
+  // Internal-body bypass can skip transform tunnel setup, leaving no transform
+  // entry to clean up. In that case there is nothing safe/useful to close here.


Comment slop.

zwoop · 2026-05-06T19:51:25Z

-      t_hook = t_hook->m_link.next;
+      APIHook *next = t_hook->m_link.next;
+      // Some transform hooks can already be detached by the time kill_this() runs.
+      // Guard against null continuations while still draining the remaining hooks.


The first comment seems useful, the second is slop.

zwoop · 2026-05-06T19:51:45Z

+      bool bypassed_response_transform =
+        t_state.api_info.cache_untransformed && t_state.internal_msg_buffer && !t_state.api_server_request_body_set;
+      // If we intentionally bypassed response transforms for internal-body
+      // transfer, transform_info may be partially detached; skip cleanup in this


Partial comment slop, can be reduced to one line.

zwoop · 2026-05-06T19:53:03Z

 pv_tar="${pv_top_dir}/${pv_tar_filename}"
 pv_tar_url="https://ci.trafficserver.apache.org/bintray/${pv_tar_filename}"
-expected_sha1="e11b5867a56c5ffd496b18c901f1273e9c120a47"
+expected_sha1="0a60c646cbc9326abb2fbc397cb9efa8c08a807a"


What happened here? We bumping proxy verifier version or something as part of this test suite? Feels like that shouldn't be the case, and if we need to, should be a separate PR ?

bryancall mentioned this pull request Apr 23, 2026

Allow set-body to replace origin server response bodies #12879

Closed

4 tasks

bryancall requested a review from Copilot April 23, 2026 19:13

Copilot started reviewing on behalf of bryancall April 23, 2026 19:14 View session

Copilot AI reviewed Apr 23, 2026

View reviewed changes

Comment thread src/proxy/http/HttpSM.cc Outdated

Comment thread tests/gold_tests/pluginTest/header_rewrite/header_rewrite_set_body_origin.replay.yaml

Comment thread doc/admin-guide/plugins/header_rewrite.en.rst Outdated

bryancall added 4 commits April 23, 2026 12:27

Address Copilot review feedback on transform safety and docs.

6a79af5

Harden transform bypass cleanup before internal transfer, keep no-transform cache probes transform-free, and clarify set-body empty-string behavior in docs. Made-with: Cursor

Limit internal-body transform bypass to origin-response contexts.

6b115ce

Guard the bypass path with server-response validity so non-origin regression flows (e.g. connect intercept tests) are not routed through origin replacement logic. Made-with: Cursor

Restrict internal response-body override to response-hook contexts.

ce43606

Prevent TSHttpConnect intercept regressions from taking the set-body internal-transfer path by guarding the SERVER_READ override on origin-response hook context and plugin tunnel state. Made-with: Cursor

Harden transform cleanup for set-body bypass path.

112468f

Prevent null transform hook cleanup crashes when internal response-body replacement bypasses transform setup, fixing -11 failures in header_rewrite_set_body_origin and filter_body AuTests. Made-with: Cursor

bryancall self-assigned this Apr 24, 2026

bryancall added TS API header_rewrite header_rewrite plugin labels Apr 24, 2026

bryancall added this to the 11.0.0 milestone Apr 24, 2026

bryancall requested a review from Copilot April 27, 2026 16:22

Document TRANSFORM_READ internal-body bypass flow.

cf53d32

Add focused comments in HttpSM transform-read handling to explain why tunnel teardown, transform VC cleanup, response header seeding, and server session release are required before internal transfer. Made-with: Cursor

Copilot AI reviewed Apr 27, 2026

View reviewed changes

Copilot started reviewing on behalf of bryancall April 27, 2026 17:09 View session

bryancall requested a review from zwoop April 27, 2026 22:21

zwoop requested changes May 6, 2026

View reviewed changes

Conversation

bryancall commented Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Production validation

Test plan

Uh oh!

bryancall commented Apr 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zwoop left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bryancall commented Apr 23, 2026 •

edited

Loading