From 4d912faff21c1476400885094e6236f252077839 Mon Sep 17 00:00:00 2001 From: bneradt Date: Mon, 6 Jul 2026 12:18:23 -0400 Subject: [PATCH] Fix H2 origin payload handling HTTP/2 origin responses can legally carry a non-zero Content-Length on responses that do not carry a payload, such as responses to HEAD requests. ATS was validating these responses without retaining the request method that had been sent on the outbound H2 stream, so it could mistake a valid no-body response for a payload length error. Outbound H2 DATA generation also needs to respect the transaction write VIO byte count when a buffer reader has more bytes available than should be sent on the stream. This caps the frame payload length to the remaining write VIO bytes and still marks END_STREAM when that payload finishes the transaction body. This extends the H2 origin replay coverage with a HEAD response that has a non-zero Content-Length and no body, plus a large PUT request and response over an H2 client and H2 origin path. --- include/proxy/http2/Http2Stream.h | 21 ++- src/proxy/http2/Http2ConnectionState.cc | 15 ++- .../h2/gold/http-request-method-metrics.gold | 2 +- tests/gold_tests/h2/h2origin.test.py | 6 +- .../h2/replay_h2origin/h2-origin.yaml | 123 ++++++++++++++++++ 5 files changed, 160 insertions(+), 7 deletions(-) diff --git a/include/proxy/http2/Http2Stream.h b/include/proxy/http2/Http2Stream.h index bc4b0743c51..67e8525d5a3 100644 --- a/include/proxy/http2/Http2Stream.h +++ b/include/proxy/http2/Http2Stream.h @@ -163,6 +163,7 @@ class Http2Stream : public ProxyTransaction void increment_data_length(uint64_t length); bool payload_length_is_valid() const; bool is_write_vio_done() const; + int64_t write_vio_ntodo() const; void update_sent_count(unsigned num_bytes); Http2StreamId get_id() const; Http2StreamState get_state() const; @@ -174,6 +175,7 @@ class Http2Stream : public ProxyTransaction void set_receive_headers(HTTPHdr &h2_headers); void reset_receive_headers(); void reset_send_headers(); + void set_sent_request_method(int method); MIOBuffer *read_vio_writer() const; int64_t read_vio_read_avail(); bool is_read_enabled() const; @@ -214,6 +216,7 @@ class Http2Stream : public ProxyTransaction Http2StreamId _id = -1; Http2StreamState _state = Http2StreamState::HTTP2_STREAM_STATE_IDLE; int64_t _http_sm_id = -1; + int _sent_request_method{-1}; HTTPHdr _receive_header; #if TS_USE_MALLOC_ALLOCATOR @@ -314,6 +317,12 @@ Http2Stream::is_write_vio_done() const return this->write_vio.ntodo() == 0; } +inline int64_t +Http2Stream::write_vio_ntodo() const +{ + return this->write_vio.ntodo(); +} + inline void Http2Stream::update_sent_count(unsigned num_bytes) { @@ -389,6 +398,12 @@ Http2Stream::reset_send_headers() this->_send_header.create(HTTPType::RESPONSE); } +inline void +Http2Stream::set_sent_request_method(int method) +{ + _sent_request_method = method; +} + // Check entire DATA payload length if content-length: header exists inline void Http2Stream::increment_data_length(uint64_t length) @@ -405,9 +420,9 @@ Http2Stream::payload_length_is_valid() const // Skip Content-Length check on [RFC 7230] 3.3.2 conditions bool is_payload_precluded = - this->is_outbound_connection() && (_send_header.method_get_wksidx() == HTTP_WKSIDX_HEAD || - (_send_header.method_get_wksidx() == HTTP_WKSIDX_GET && _send_header.presence(mask) && - _receive_header.status_get() == HTTPStatus::NOT_MODIFIED)); + this->is_outbound_connection() && + (_sent_request_method == HTTP_WKSIDX_HEAD || (_sent_request_method == HTTP_WKSIDX_GET && _send_header.presence(mask) && + _receive_header.status_get() == HTTPStatus::NOT_MODIFIED)); if (content_length != 0 && !is_payload_precluded && content_length != data_length) { Warning("Bad payload length content_length=%d data_legnth=%d session_id=%" PRId64, content_length, diff --git a/src/proxy/http2/Http2ConnectionState.cc b/src/proxy/http2/Http2ConnectionState.cc index eff88c388d4..703d3355f21 100644 --- a/src/proxy/http2/Http2ConnectionState.cc +++ b/src/proxy/http2/Http2ConnectionState.cc @@ -2275,6 +2275,7 @@ Http2ConnectionState::send_a_data_frame(Http2Stream *stream, size_t &payload_len uint8_t flags = 0x00; IOBufferReader *resp_reader = stream->get_data_reader_for_send(); + bool last_write_vio_payload{false}; SCOPED_MUTEX_LOCK(stream_lock, stream->mutex, this_ethread()); @@ -2307,6 +2308,16 @@ Http2ConnectionState::send_a_data_frame(Http2Stream *stream, size_t &payload_len } else { payload_length = resp_reader->read_avail(); } + const int64_t remaining_write = stream->write_vio_ntodo(); + if (remaining_write != INT64_MAX) { + if (remaining_write > 0) { + last_write_vio_payload = payload_length >= static_cast(remaining_write); + payload_length = std::min(payload_length, static_cast(remaining_write)); + } else { + last_write_vio_payload = true; + payload_length = 0; + } + } } else { payload_length = 0; } @@ -2334,7 +2345,8 @@ Http2ConnectionState::send_a_data_frame(Http2Stream *stream, size_t &payload_len return Http2SendDataFrameResult::NO_PAYLOAD; } - if (stream->is_write_vio_done() && !resp_reader->is_read_avail_more_than(payload_length) && !stream->expect_send_trailer()) { + if (stream->is_write_vio_done() && (last_write_vio_payload || !resp_reader->is_read_avail_more_than(payload_length)) && + !stream->expect_send_trailer()) { Http2StreamDebug(this->session, stream->get_id(), "End of Data Frame"); flags |= HTTP2_FLAGS_DATA_END_STREAM; } @@ -2456,6 +2468,7 @@ Http2ConnectionState::send_headers_frame(Http2Stream *stream) flags |= HTTP2_FLAGS_HEADERS_END_HEADERS; if (stream->is_outbound_connection()) { // Will be sending a request_header int method = send_hdr->method_get_wksidx(); + stream->set_sent_request_method(method); // Set END_STREAM on request headers for POST, etc. methods combined with // an explicit length 0. Some origins RST on request headers with diff --git a/tests/gold_tests/h2/gold/http-request-method-metrics.gold b/tests/gold_tests/h2/gold/http-request-method-metrics.gold index f949dc4270f..6418e16a565 100644 --- a/tests/gold_tests/h2/gold/http-request-method-metrics.gold +++ b/tests/gold_tests/h2/gold/http-request-method-metrics.gold @@ -1,3 +1,3 @@ proxy.process.http.get_requests 4 proxy.process.http.post_requests 11 -proxy.process.http.put_requests 0 +proxy.process.http.put_requests 1 diff --git a/tests/gold_tests/h2/h2origin.test.py b/tests/gold_tests/h2/h2origin.test.py index 6b0211a8d00..41c9c5bae6b 100644 --- a/tests/gold_tests/h2/h2origin.test.py +++ b/tests/gold_tests/h2/h2origin.test.py @@ -89,7 +89,7 @@ timeout = 30 watcher = tr.Processes.Process("watcher") watcher.Command = f"sleep {timeout}" -watcher.Ready = When.FileContains(ts.Disk.squid_log.Name, r'14 http/1.1 http/2') +watcher.Ready = When.FileContains(ts.Disk.squid_log.Name, r'16 http/2 http/2') watcher.TimeOut = timeout tr.StillRunningAfter = ts tr.StillRunningAfter = server @@ -99,7 +99,7 @@ tr.Processes.Default.ReturnCode = 0 # UUIDs 1-4 should be http/1.1 clients and H2 origin -# UUIDs 5-9 should be http/2 clients and H2 origins +# UUIDs 5-11 and 15-16 should be http/2 clients and H2 origins ts.Disk.squid_log.Content = Testers.ContainsExpression(" [1-4] http/1.1 http/2", "cases 1-4 request http/1.1") ts.Disk.squid_log.Content += Testers.ExcludesExpression(" [1-4] http/2 http/2", "cases 1-4 request http/1.1") ts.Disk.squid_log.Content += Testers.ContainsExpression(" 1[1-4] http/1.1 http/2", "cases 12-14 request http/1.1") @@ -108,6 +108,8 @@ ts.Disk.squid_log.Content += Testers.ExcludesExpression(" [5-9] http/1.1 http/2", "cases 5-11 request http/2") ts.Disk.squid_log.Content += Testers.ContainsExpression(" 1[0-1] http/2 http/2", "cases 5-11 request http/2") ts.Disk.squid_log.Content += Testers.ExcludesExpression(" 1[0-1] http/1.1 http/2", "cases 5-11 request http/2") +ts.Disk.squid_log.Content += Testers.ContainsExpression(" 1[5-6] http/2 http/2", "cases 15-16 request http/2") +ts.Disk.squid_log.Content += Testers.ExcludesExpression(" 1[5-6] http/1.1 http/2", "cases 15-16 request http/2") tr = Test.AddTestRun("Test HTTP method Metrics") tr.Processes.Default.Command = ( diff --git a/tests/gold_tests/h2/replay_h2origin/h2-origin.yaml b/tests/gold_tests/h2/replay_h2origin/h2-origin.yaml index acf7035fcd6..0310eacb347 100644 --- a/tests/gold_tests/h2/replay_h2origin/h2-origin.yaml +++ b/tests/gold_tests/h2/replay_h2origin/h2-origin.yaml @@ -477,3 +477,126 @@ sessions: content: encoding: plain size: 3200 + + # + # Test 8: HEAD response with a non-zero Content-Length and no body. + # + - all: { headers: { fields: [[ uuid, 15 ]]}} + + client-request: + version: '2' + scheme: https + method: HEAD + url: /some/head + headers: + encoding: esc_json + fields: + - [ Host, data.brian.example.com ] + content: + encoding: plain + size: 0 + + proxy-request: + protocol: + stack: http2 + tls: + version: TLSv1.2 + sni: data.brian.example.com + proxy-verify-mode: 1 + proxy-provided-cert: false + version: '2' + scheme: https + method: HEAD + url: /some/head + headers: + encoding: esc_json + fields: + - [ Host, data.brian.example.com ] + - [ Content-Length, 0 ] + content: + encoding: plain + size: 0 + + server-response: + version: '2' + status: 200 + headers: + encoding: esc_json + fields: + - [ Content-Length, 100 ] + content: + encoding: plain + size: 0 + + proxy-response: + version: '2' + status: 200 + headers: + encoding: esc_json + fields: + - [ Content-Length, 100 ] + content: + encoding: plain + size: 0 + + # + # Test 9: large PUT body with a large response. + # + - all: { headers: { fields: [[ uuid, 16 ]]}} + + client-request: + version: '2' + scheme: https + method: PUT + url: /some/large-put + headers: + encoding: esc_json + fields: + - [ Host, data.brian.example.com ] + - [ Content-Length, 300000 ] + content: + encoding: plain + size: 300000 + + proxy-request: + protocol: + stack: http2 + tls: + version: TLSv1.2 + sni: data.brian.example.com + proxy-verify-mode: 1 + proxy-provided-cert: false + version: '2' + scheme: https + method: PUT + url: /some/large-put + headers: + encoding: esc_json + fields: + - [ Host, data.brian.example.com ] + - [ Content-Length, 300000 ] + content: + encoding: plain + size: 300000 + + server-response: + version: '2' + status: 200 + headers: + encoding: esc_json + fields: + - [ Content-Length, 300000 ] + content: + encoding: plain + size: 300000 + + proxy-response: + version: '2' + status: 200 + headers: + encoding: esc_json + fields: + - [ Content-Length, 300000 ] + content: + encoding: plain + size: 300000