Validate content-length when a stream ends with trailers

[feiiiiii5]

Fixes #1328

Description

A stream that ends with a trailers section never had its content-length policed, and a content-length in the trailers was accepted as if it were the one from the header section.

H2Stream.receive_headers handles every received HEADERS block, trailers included, and called _initialize_content_length(headers) unconditionally. _track_content_length, where the comparison actually happens, was only ever called from receive_data, so end_stream was never True for a body that ends with trailers. A peer declaring content-length: 15 could send 13 bytes and then a trailers section, and the short body was accepted.

Per @Kriechi's review, content-length is now rejected in a trailers section outright: RFC 9110 § 6.5.1 only allows trailer fields whose definition permits them there, and content-length has to be evaluated before the content is received. Trailers that end a stream run the same length check, so they can neither reset nor redefine the expectation.

Test plan

master at bc239af1d1b85bc70482804f30a0e0e587d90a08 (4.4.1), Python 3.14, macOS. Unit level with the existing frame_factory fixture, in-memory bytes, no network.

TestContentLengthEnforcedAtTrailers in tests/test_invalid_content_lengths.py: insufficient data ended by trailers, no data at all ended by trailers, content-length in trailers rejected for several values, a matching body ended by trailers still accepted with TrailersReceived, and a request with no content-length unaffected. Four receive-side tests in tests/test_basic_logic.py used content-length: 0 as their trailer field and now use x-checksum.

Command output

Before the fix, with only the tests added:

$ pytest tests/test_invalid_content_lengths.py::TestContentLengthEnforcedAtTrailers -q
FAILED ...::test_insufficient_data_ended_by_trailers[request_headers0]
FAILED ...::test_no_data_ended_by_trailers
FAILED ...::test_content_length_rejected_in_trailers[13]
FAILED ...::test_content_length_rejected_in_trailers[15]
FAILED ...::test_content_length_rejected_in_trailers[0]
FAILED ...::test_content_length_rejected_in_trailers[banana]
6 failed, 2 passed in 0.40s

The two that passed before the fix as well are the ones pinning that valid trailers and a stream with no content-length are still accepted.

After:

$ pytest -q
1670 passed in 4.26s

$ coverage report | grep stream.py
src/h2/stream.py           466      0     98      0   100%
TOTAL                     1919      0    470      0   100%

$ ruff check src/
All checks passed!

$ mypy --strict src/h2/stream.py
Success: no issues found in 1 source file

[Kriechi]

I am not sure if I understand the problem stated here. To my understanding, a content-length header is not allowed in Trailers anyway, see https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Trailer#directives

If a HEADERS frame with END_STREAM set is received after DATA, the content length is already known or final, so there is no need to parse or validate it again.

So the only missing call is possibly _track_content_length once we receive Trailers - this would reduce the PR to a single line of code change at the right place?

[feiiiiii5]

Thanks for the review. The length check now runs before _initialize_content_length(headers) when a trailer block sets END_STREAM, so it checks against the original header value. Trailer parsing still follows, preserving the existing rejection of malformed values. I removed the save/restore path in 753a5f3. The regression cases fail 3/6 on the PR base and pass 6/6 here; pytest -q passes (1668), as do ruff check src/ and the configured strict-byte mypy command.

[Kriechi]

Trailers are not allowed to have content-length headers at all - no matter their value.

[Kriechi]

Trailers are not allowed to have content-length headers at all - no matter their value.

[feiiiiii5]

@Kriechi good catch — trailers must not carry content-length at all, so rejecting it there is the right shape. Done in d2b43d0: a trailers section containing content-length is now a ProtocolError regardless of value (RFC 9110 § 6.5.1), instead of being parsed and then ignored, and the trailers no longer reach _initialize_content_length at all, so they can neither reset nor redefine the expectation.

That made the receive-side trailer fixtures in tests/test_basic_logic.py invalid, so the four of them now use x-checksum: 0 instead of content-length: 0. The send-side ones are untouched.

Still the same fix underneath: a body that ends with trailers is policed where the stream actually ends. pytest -q is 1670 passed with coverage at 100%, ruff check src/ and mypy --strict src/h2/stream.py are clean. I also reworded the PR description to match.

[feiiiiii5]

Both of these are addressed in d2b43d0, the current head — the comments are on 753a5f3.

test_content_length_rejected_in_trailers now parametrizes over ["13", "15", "0", "banana"] and asserts ProtocolError for each, so a trailers section cannot carry a content-length at any value, not just one that disagrees with the body. That is the rule you stated: the field is not allowed there in the first place, so there is nothing to compare it against.

The rest of the change is separate from that rule and is what the earlier commits do: a trailers section that ends a stream without a content-length still runs the body-length check, so trailers can neither satisfy nor reset the expectation set by the header section. test_matching_body_ended_by_trailers_is_accepted covers the case where they agree, and still asserts TrailersReceived is emitted.

If the RFC 9110 § 6.5.1 reading is not what you want for the mismatch cases — i.e. if you would rather a content-length in trailers be ignored outright than be a protocol error — say so and I will change the assertion.

[feiiiiii5]

You were right on both counts, and I checked rather than argued. Narrowed to your one-liner.

On content-length in trailers being redundant — it is. Base already refuses it: the field goes through the regular content-length parser, so content-length: banana in a trailer section fails with Invalid content-length header: b'banana', and the numeric cases are rejected too. My explicit check was a second, differently-worded rejection for input that was already refused. I've dropped it and the test that asserted my custom message.

On HEADERS-with-END_STREAM after DATA — agreed. Once DATA has arrived the body length is already known, and a trailers block is not the place to re-initialise it. That is now the only structural change:

if isinstance(headers_event, TrailersReceived):
    # Trailers are not part of the content, but the stream ends here,
    # so this is the only point at which the body length can be policed.
    self._track_content_length(0, end_stream=True)
else:
    self._initialize_content_length(headers)

_initialize_content_length stays on the non-trailers path, and the pre-existing "Trailers must have END_STREAM set" check is untouched — I moved it back out rather than nesting it, since it reads better flat.

So src/h2/stream.py is now +9/-1, of which one line is the call and the rest is the comment saying why that line is there. The PR as a whole is +127/-6 across three files, and 116 of those insertions are tests — two in test_basic_logic.py and the TestContentLengthEnforcedAtTrailers cases. I kept the coverage rather than shrinking it to match the line count, but if you would rather the test class live in its own file than be appended to test_invalid_content_lengths.py, say so and I will move it.

Full suite on the narrowed branch: 1666 passed (master is 1662 here, so the four added cases are the difference).

One thing I did not do, so it is not silently assumed: I did not add a content-length-in-trailers test, because base already covers that path and a new test would only pin my own error message.