Fix http2 content-length header check (#12747)

[floom4]

HEAD method with "Content-Length" header was returning 502 code because header differs from payload length as there is no payload in this case.

See https://datatracker.ietf.org/doc/html/rfc7540#section-8.1.2.6

A response that is defined to have no payload, as described in [RFC7230], Section 3.3.2, can have a non-zero content-length header field, even though no content is included in DATA frames.

Fixes #12747

The change fixes the behavior by having payload_length_is_valid() return true if data_length is 0 meaning that the response has no payload, allowing for non-zero 'Content-Length' in this case as specified in the rfc.

[maskit]

Good catch. However, as the RFC says, "A response that is defined to have no payload" is the condition. It looks like your change allows arbitrarily response that have no payload.

[floom4]

My understanding of the RFC is that the condition defining a valid response without payload is the stream ending at header or continuation frame.
Considering that data_length is only incremented inside rcv_data_frame, I assumed that having data_length == 0 means that there was no data frame.

However, if you prefer we could skip this check in rcv_header_frame and rcv_continuation_frame if and only if they are not trailing headers (since trailing headers implies that data frames where received)

Here is an alternative condition that could be used:

if (stream->receive_end_stream && stream->trailing_header_is_possible() && !stream->payload_length_is_valid()) {

In both:

• rcv_header_frame

  trafficserver/src/proxy/http2/Http2ConnectionState.cc

  Line 505 in 0e42870

  if (stream->receive_end_stream && !stream->payload_length_is_valid()) {

• rcv_continuation_frame

  trafficserver/src/proxy/http2/Http2ConnectionState.cc

  Line 1117 in 0e42870

  if (stream->receive_end_stream && !stream->payload_length_is_valid()) {

[maskit]

I don't think what frame carries END_STREAM flag matters. A DATA frame that carries just END_STREAM flag (no payload) is valid.

If a request is GET and the response has content-length: 123, data_length must be 123, right? Because a response for a GET request is not defined to have no payload. A response for a HEAD request is.

[Copilot]

Pull request overview

This PR fixes HTTP/2 HEAD method responses that include Content-Length headers. Previously, these responses incorrectly returned 502 errors when the Content-Length didn't match the (zero) payload length. Per RFC 7540 section 8.1.2.6, HTTP/2 responses defined to have no payload (such as HEAD responses) can legitimately have non-zero Content-Length headers even though no DATA frames are included.

Key Changes:

• Modified payload_length_is_valid() to allow responses with zero-length payloads regardless of Content-Length value
• Updated warning condition to only trigger when both content_length and data_length are non-zero but mismatched

---

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

[Copilot]

This fix for RFC 7540 section 8.1.2.6 compliance (allowing non-zero Content-Length with zero-length payloads) lacks test coverage. Consider adding an HTTP/2 test case that verifies HEAD responses with Content-Length headers are handled correctly, specifically testing that a HEAD response with Content-Length greater than 0 but data_length of 0 does not return a 502 error.

[floom4]

You're right I misunderstood the RFC, I went back to it and in my understanding, the only cases where a non-zero 'Content-Lenght' header without payload is allowed are:

• HEAD response
• 304 response to conditional GET

From Section 3.3.2 of [RFC7230]

A server MAY send a Content-Length header field in a response to a
   HEAD request (Section 4.3.2 of [RFC7231]); ...

A server MAY send a Content-Length header field in a 304 (Not
   Modified) response to a conditional GET request (Section 4.1 of
   [RFC7232]); ...

It indeed means that, while other types of request and response may have an empty payload (or data_length == 0), they must not have a Content-Length header with a value different from 0.

Here is a proposal to check that the current frame is part of an HEAD response that we could use in rcv_headers_frame and rcv_continuation_frame:

stream->is_outbound_connection() && stream->get_sm()->t_state.hdr_info.server_request.method_get_wksidx() == HTTP_WKSIDX_HEAD

My understanding is that is_outbound_connection represents communication with origin which in case of receiving frame means response from origin
I assumed that server_request field would be set since the previous condition ensure that we are at the response step and consequently, the request was created and sent.

I think the same fields can be used to know if the frame is part of a response to a conditional GET request, however to find the http status code seems trickier since the response headers are not parsed yet as we just received the frame and are still validating it.

Maybe we could only fix HEAD request for the moment, does the above conditional statement work for you ?

[maskit]

My understanding is that is_outbound_connection represents communication with origin which in case of receiving frame means response from origin
I assumed that server_request field would be set since the previous condition ensure that we are at the response step and consequently, the request was created and sent.

Correct.

I think the same fields can be used to know if the frame is part of a response to a conditional GET request, however to find the http status code seems trickier since the response headers are not parsed yet as we just received the frame and are still validating it.

stream->decode_header_blocks is called before !stream->payload_length_is_valid() in rcv_headers_frame and rcv_continuation_frame. The status code should be available. But if I'm missing something and it's tricky, I'm fine with fixing just the HEAD request case.

The proposed condition looks fine. Since those are accessible from stream, I think you can use the condition in payload_length_is_valid. And then it should work for rcv_data_frame as well.