Skip to content

Conversation

@bryancall
Copy link
Contributor

Summary

The header_rewrite plugin's set-body operator (which calls TSHttpTxnErrorBodySet()) previously only worked for ATS-generated responses. When the origin returned a real response, the body was streamed through the HTTP tunnel and internal_msg_buffer was ignored. This meant plugins could not use set-body to replace or sanitize origin response bodies (e.g., stripping sensitive information from error pages).

This PR adds a check for internal_msg_buffer in the SERVER_READ and TRANSFORM_READ paths of HttpSM::handle_api_return(). When a plugin has set the internal message buffer, ATS now drains the origin response body (if possible for connection reuse) and uses setup_internal_transfer() instead of the tunnel.

Changes

  • HttpSM::handle_api_return() — Check internal_msg_buffer in SERVER_READ and TRANSFORM_READ cases; divert to setup_internal_transfer() when set
  • HttpSM::do_drain_server_response_body() — New function that synchronously consumes the origin body from the buffer when fully available, enabling connection reuse. Falls back to closing the connection for chunked, unknown-length, or partially-received bodies
  • HttpSM::release_server_session() — Widen the pooling condition to allow connection reuse after successful body drain (not just 304/HEAD responses)
  • header_rewrite operators.cc — Add TS_HTTP_READ_RESPONSE_HDR_HOOK to set-body's allowed hooks so plugins can inspect origin response headers and replace the body
  • Documentation — Updated set-body docs with origin response use case, connection reuse behavior, and example rule
  • Autest — New replay-based test covering origin body replacement at both READ_RESPONSE_HDR_HOOK and SEND_RESPONSE_HDR_HOOK, including 403, 200, and empty-body cases

Example Use Case

Sanitize a 403 response from an origin that leaks account information:

cond %{READ_RESPONSE_HDR_HOOK} [AND]
cond %{STATUS} =403
    set-body "Access Denied"

Test Plan

  • Autest: header_rewrite_set_body_origin covering 4 scenarios (403/200 at both hooks, empty body)
  • Manual testing with curl against live proxy
  • Verified connection reuse via debug logs after body drain
  • Verified connection close for chunked/partial bodies

Previously, header_rewrite's set-body operator (which calls
TSHttpTxnErrorBodySet) only worked for ATS-generated responses because
internal_msg_buffer was only consumed by setup_internal_transfer().
When the origin sent a real response, the body was streamed through
the tunnel and internal_msg_buffer was ignored.

This change adds a check for internal_msg_buffer in the SERVER_READ and
TRANSFORM_READ paths of handle_api_return(). When a plugin has set the
internal message buffer, ATS now drains the origin response body (if
possible) and uses setup_internal_transfer() instead of the tunnel.

Key changes:
- Check internal_msg_buffer in SERVER_READ and TRANSFORM_READ paths
- Add do_drain_server_response_body() to synchronously drain small
  origin bodies so the server connection can be reused
- Widen release_server_session() pooling condition to allow connection
  reuse after body drain (not just 304/HEAD)
- Add TS_HTTP_READ_RESPONSE_HDR_HOOK to set-body's allowed hooks
- Add autest covering origin body replacement at both hooks
- Update set-body documentation with origin response examples
@bryancall bryancall self-assigned this Feb 11, 2026
@bryancall bryancall added this to the 11.0.0 milestone Feb 11, 2026
@bryancall bryancall added header_rewrite header_rewrite plugin New Feature labels Feb 11, 2026
@bryancall bryancall requested a review from Copilot February 11, 2026 23:50
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends the header_rewrite plugin's set-body operator to work with origin server responses, not just ATS-generated error responses. Previously, TSHttpTxnErrorBodySet() only affected responses generated by ATS itself, as the origin body would stream through the HTTP tunnel. Now, when internal_msg_buffer is set, ATS drains the origin response body (when feasible for connection reuse) and uses setup_internal_transfer() to send the plugin-provided body instead.

Changes:

  • Added logic in HttpSM::handle_api_return() to detect when a plugin has set an internal message buffer and divert to internal transfer instead of tunneling the origin response
  • Implemented HttpSM::do_drain_server_response_body() to synchronously consume origin response bodies from the buffer to enable connection reuse
  • Extended HttpSM::release_server_session() to allow pooling connections after successful body drains
  • Enabled TS_HTTP_READ_RESPONSE_HDR_HOOK for the set-body operator
  • Added comprehensive test coverage and documentation

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/proxy/http/HttpSM.cc Core implementation: added internal_msg_buffer checks in TRANSFORM_READ and SERVER_READ paths, implemented do_drain_server_response_body() for connection reuse, and extended release_server_session() pooling logic
include/proxy/http/HttpSM.h Added declaration for new do_drain_server_response_body() method
plugins/header_rewrite/operators.cc Registered TS_HTTP_READ_RESPONSE_HDR_HOOK as an allowed hook for the set-body operator
doc/admin-guide/plugins/header_rewrite.en.rst Updated set-body documentation with origin response use cases, connection reuse behavior explanation, and practical examples
tests/gold_tests/pluginTest/header_rewrite/header_rewrite_set_body_origin.test.py Test entry point for replay-based test
tests/gold_tests/pluginTest/header_rewrite/header_rewrite_set_body_origin.replay.yaml Comprehensive test scenarios covering both hooks, multiple status codes, and edge cases
tests/gold_tests/pluginTest/header_rewrite/rules/rule_set_body_origin_send_resp.conf Test rule for SEND_RESPONSE_HDR_HOOK
tests/gold_tests/pluginTest/header_rewrite/rules/rule_set_body_origin_read_resp.conf Test rule for READ_RESPONSE_HDR_HOOK

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@maskit
Copy link
Member

maskit commented Feb 11, 2026

Did you look at how set-body-from work? According to the documentation, it works even if the origin returns a real response. I wonder where the difference comes from.

@djcarlin
Copy link
Contributor

FYI last time we tried doing something similar in txn_box we ran into this issue when the origin response was empty: #8081

@bryancall
Copy link
Contributor Author

Yes, I looked at set-body-from closely. The key difference is in how the transaction is reenabled after setting the body.

set-body-from calls TSHttpTxnReenable(txnp, TS_EVENT_HTTP_ERROR), which routes through HandleApiErrorJump(). That function wipes the existing client response headers (fields_clear()) and rebuilds the response from scratch using build_response(). This effectively creates a new ATS-generated response, which is why setup_internal_transfer() picks up the internal_msg_buffer -- it's already on the internal response path.

set-body, on the other hand, reenables with TS_EVENT_HTTP_CONTINUE, which continues normal processing. The origin response flows through setup_server_transfer() / the HTTP tunnel, and internal_msg_buffer was simply never checked in that path -- until this PR.

The ERROR approach in set-body-from has some trade-offs:

  • It wipes all origin response headers and rebuilds from scratch. This could be a feature when sanitizing responses (you don't leak origin headers), but it also means you can't selectively preserve headers you want. With the CONTINUE approach, origin headers are preserved by default and you can use rm-header to remove specific ones you don't want.
  • Status codes < 400 get clobbered to 500 INKApi Error (there's a TSHttpTxnStatusSet() hack to work around this, but it only preserves codes >= 400). The existing test explicitly documents this as a bug: "TSHttpTxnErrorBodySet will change OK status codes to 500 INKApi Error"
  • It doesn't allow origin connection reuse since it doesn't drain the body

Another important difference is how set-status composes with each operator. With this PR's CONTINUE approach, set-status + set-body works naturally -- set-status modifies the response header directly, set-body replaces the body, and setup_internal_transfer() sends both as-is.

With set-body-from's ERROR approach, set-status is broken regardless of operator order:

  • set-status 200 before set-body-from: set-body-from reads the modified status into http_return_code (200), then HandleApiErrorJump sees 200 < 400 and builds a 500 INKApi Error
  • set-status 200 after set-body-from: set-body-from captures the original status (e.g. 403) into http_return_code, set-status modifies the header to 200, but then HandleApiErrorJump wipes all headers via fields_clear() and rebuilds from http_return_code (403) -- the set-status is completely ignored

This PR takes the cleaner approach: check internal_msg_buffer in handle_api_return() before setting up the tunnel, drain the origin body when possible for connection reuse, and let the existing response headers flow through naturally. I'm planning a follow-up to standardize set-body-from on this same CONTINUE path, which would fix the status code bug, allow connection reuse, make set-status work correctly, and give users the choice of which headers to keep or remove.

Change set-body-from to reenable with TS_EVENT_HTTP_CONTINUE instead of
TS_EVENT_HTTP_ERROR, so the origin response headers and status code are
preserved when the body is replaced. This aligns set-body-from behavior
with set-body.

Changes:
- handleFetchEvents: reenable with TS_EVENT_HTTP_CONTINUE on success
- Remove TSHttpTxnStatusSet workaround that was needed for error path
- Add TS_HTTP_SEND_RESPONSE_HDR_HOOK support to set-body-from
- Update test to expect 200 OK (was 500 INKApi Error)
- Update documentation to reflect new behavior
When set-body (or any plugin using TSHttpTxnErrorBodySet()) is used at a
pre-origin hook like REMAP_PSEUDO_HOOK, ATS now skips the origin connection
entirely and serves a synthetic response. Previously, ATS would still open
a TCP connection to the origin, exchange headers, and then throw away the
body -- wasting resources.

The change adds a check at the top of how_to_open_connection() in
HttpTransact.cc: if internal_msg_buffer is already set, build a synthetic
response and return INTERNAL_CACHE_NOOP. This works with set-status to
support any status code (defaults to 200 OK).

Also improves set-body-from test coverage:
- Add SEND_RESPONSE_HDR_HOOK tests (previously untested)
- Replace fragile gold-file comparisons with ContainsExpression/
  ExcludesExpression verification of status codes and body content
- Add new set-body-remap replay-based test for the origin-skip behavior
- Update documentation with new synthetic response scenario and example
@bryancall
Copy link
Contributor Author

Correction: summary of all three commits in this PR

Commit 1: Allow set-body to replace origin server response bodies

Added internal_msg_buffer checks in HttpSM::handle_api_return() for the SERVER_READ and TRANSFORM_READ states. When a plugin sets a body via TSHttpTxnErrorBodySet() at READ_RESPONSE_HDR_HOOK or SEND_RESPONSE_HDR_HOOK, ATS now diverts to setup_internal_transfer() instead of tunneling the origin body. Drains the origin body when possible to allow connection reuse. Includes replay-based header_rewrite_set_body_origin test covering both hooks, 200/403 status codes, and empty origin bodies.

Commit 2: Standardize set-body-from to preserve origin response headers

Changed set-body-from's successful fetch handler to reenable with TS_EVENT_HTTP_CONTINUE instead of TS_EVENT_HTTP_ERROR. This preserves the origin's status code and headers (previously it returned 500 INKApi Error). Removed the TSHttpTxnStatusSet() workaround. Added SEND_RESPONSE_HDR_HOOK support. Updated documentation.

Commit 3: Skip origin connection when set-body is used at remap hook

When set-body is used at a pre-origin hook like REMAP_PSEUDO_HOOK, ATS now skips the origin connection entirely. Added a single internal_msg_buffer check at the top of how_to_open_connection() -- if the buffer is set, build a synthetic response and return INTERNAL_CACHE_NOOP. Works with set-status for any status code (defaults to 200 OK).

New header_rewrite_set_body_remap replay-based test verifying origin is skipped with both set-status 403 and default 200.

Rewrote header_rewrite_set_body_from tests:

  • Added SEND_RESPONSE_HDR_HOOK tests (previously untested despite the hook being added in commit 2)
  • Replaced fragile gold-file comparisons with ContainsExpression/ExcludesExpression verification of status codes and body content
  • 5 test cases: READ_RESPONSE_HDR x {404, 200}, SEND_RESPONSE_HDR x {404, 200}, fetch-backend-unreachable

Updated set-body documentation with new "Synthetic responses" scenario and example.

All three test suites pass on both regular and ASAN builds.

When remap fails with remap_required=1 but a plugin tunnel exists
(TSHttpTxnServerIntercept or TSHttpTxnIntercept), the error response
body from build_error_response() was left in internal_msg_buffer even
though the error response headers were properly cleared. This caused
how_to_open_connection() to see the stale buffer and short-circuit
the plugin tunnel, serving the remap error page instead of the
plugin-provided response.

Fix: free internal_msg_buffer when discarding the error response in
EndRemapRequest. Also add null guards around server_txn in the
SERVER_READ and TRANSFORM_READ internal_msg_buffer paths, and revert
the release_server_session() condition change which could allow
server session reuse without body draining.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

header_rewrite header_rewrite plugin New Feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants