Skip to content

A 304 with content-length is accepted or rejected by framing alone #1330

Description

@feiiiiii5

While working on #1328 I found two more holes in the same function, and they turn out to be two halves of one problem. I have not touched anything — the measurements are in a standalone probe, and I would rather agree on the direction first.

A 304 or 204 carrying content-length is accepted or rejected depending on framing

_initialize_content_length (src/h2/stream.py:1369) implements exactly one exemption, self.request_method == b"HEAD" (:1375). RFC 9113 § 8.1.1 names three messages that are "defined as having no content": 204, 304, and the response to HEAD, and it says a message defined to have no content "MAY have a non-zero content-length header field". RFC 9110 § 8.6 repeats it for 304 — that is how a client learns the size of the cached entity. H2Stream never stores :status, so the other two cannot be written as the code stands. Observable today, on master and on #1329 alike:

304 + content-length: 1234 + empty DATA with END_STREAM   -> InvalidBodyLengthError: Expected 1234 bytes, received 0
304 + content-length: 1234 + END_STREAM on the HEADERS    -> accepted ['ResponseReceived', 'StreamEnded']
204 + content-length: 1234 + empty DATA with END_STREAM   -> InvalidBodyLengthError: Expected 1234 bytes, received 0

One message, two verdicts, decided purely by which frame carried END_STREAM. That is an enforcement that rejects something the RFC permits, and it is reachable from a plain H2Connection.receive_data on a legitimate response.

The same hole #1328 fixed, in the other branch of the same if

receive_headers (:1113) — the else branch, i.e. a HEADERS block that is not trailers, calls _initialize_content_length(headers) and nothing else. So a bodyless message that ends on the HEADERS is never policed at all:

POST content-length: 15 + END_STREAM on the HEADERS, no DATA  -> accepted ['RequestReceived', 'StreamEnded']
200  content-length: 1234 + END_STREAM on the HEADERS         -> accepted ['ResponseReceived', 'StreamEnded']

content-length: 15 with nothing sent is the same lie #1328 is about, and _actual_content_length stays 0.

The obvious one-line fix is not safe on its own. Adding if end_stream: self._track_content_length(0, end_stream=True) to that branch, measured:

                                    before        after that line
POST content-length: 15, no body    accepted      InvalidBodyLengthError(15, 0)   <- intended
304  content-length: 1234, no body  accepted      InvalidBodyLengthError(1234, 0) <- new breakage
204  content-length: 1234, no body  accepted      InvalidBodyLengthError(1234, 0) <- new breakage
POST content-length: 0, no body     accepted      accepted                          <- control
GET  no content-length, no body     accepted      accepted                          <- control

So the 204/304 exemption has to land first, or with it. That is why I am asking rather than sending a patch: the exemption is a design call. Setting self._expected_content_length = 0 for 204/304 mirrors the HEAD precedent and additionally makes DATA after a 204 an error, which RFC 9110 § 6.3 implies; setting it to None only skips the comparison. I lean to 0, but that is your call, and H2Stream would need to start recording :status.

Two smaller ones in the same file, and what I am not reporting

Two smaller ones in the same file, both one-line

  • send_headers (:909) assigns self.request_method = extract_method_header(bytes_headers) on every header block, including a trailers block, which has no :method — so outbound trailers overwrite a remembered b"HEAD" with None. H2Connection.send_headers(1, HEAD) then a trailers block, then a 200 + content-length: 1234 + empty DATA + END_STREAM, raises where it should not. Guarding on method is not None fixes it.
  • remotely_pushed (:1052) records self._authority but never the method, so a pushed stream never gets the HEAD exemption at all. A PUSH_PROMISE with :method: HEAD followed by 200 + content-length: 1234 + empty DATA + END_STREAM raises; the identical push with :method: GET raises the same way, which is the tell that the method is simply not consulted.

Not reported

  • RST_STREAM or a local reset mid-body runs no length check, and should not: the message is abandoned and the stream is closed, so nothing later can contradict the sender. Measured, no error, and I read that as specified rather than a bug.
  • A content-length in outbound trailers is a real asymmetry — h2 will send a trailers block carrying content-length: 0, and feeding those exact bytes back into an h2 client raises ProtocolError: Received content-length header in trailer. But fixing it means changing the trailer field in four existing tests in tests/test_basic_logic.py, so I would file that separately rather than widen this.
  • A 1xx carrying content-length is accepted and then overwritten. Inconsistent with the new trailers rule, but the RFC puts that obligation on the sender and no wrong byte is ever compared, so I do not think it is worth an interop risk.

Verification

uv run --frozen pytest tests/ -q is 1670 passed at 100.00% coverage (466 statements, 98 branches in stream.py, 0 missed), on d2b43d0. Every scenario above is a runtime measurement against the real H2Connection, not a reading. I have the probe as a standalone script and the 17 assertions as a pytest file; say the word and I will open a PR with the exemption and the HEADERS-frame check together, at 100% coverage.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions