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, a content-length received in a trailers section is now rejected whatever its value, in the same validate_headers pipeline that already rejects a pseudo-header there: RFC 9110 § 6.5.1 keeps fields that describe message framing out of trailer sections, because they have to be evaluated before the content is received. Trailers that end a stream run the same length check as any other end of stream, so they can neither reset nor redefine the expectation.

Test plan

h2 4.4.1 installed editable from this branch, Python 3.11.15 on macOS arm64. master is measured from its own source at bc239af1d1b85bc70482804f30a0e0e587d90a08, under the same interpreter and the same hpack/hyperframe, so only h2's source differs. 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 13, 15, 0 and banana, 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

This branch's tests run against master's source (PYTHONPATH=<master src>), i.e. before the fix:

$ pytest tests/test_invalid_content_lengths.py::TestContentLengthEnforcedAtTrailers -q
6 failed, 2 passed in 0.11s
E  Failed: DID NOT RAISE <class 'h2.exceptions.InvalidBodyLengthError'>  (insufficient_data, no_data)
E  Failed: DID NOT RAISE <class 'h2.exceptions.ProtocolError'>           ([13], [15], [0])
E  assert 'content-length header in trailer' in "Invalid content-length header: b'banana'"  ([banana])

The two that pass either way pin that valid trailers and a stream with no content-length are still accepted. Note the banana case: master does refuse a non-numeric trailer content-length, through the ordinary parser and with different wording, so it fails on the message rather than on "did not raise". The numeric values are accepted on master and redefine _expected_content_length.

After, on this branch:

$ python -bb -m pytest --cov-report=term-missing --cov=h2 -q
1670 passed in 8.73s
src/h2/stream.py           463      0     96      0   100%
src/h2/utilities.py        272      0    146      0   100%
TOTAL                     1923      0    472      0   100%
Required test coverage of 100.0% reached. Total coverage: 100.00%

$ ruff check src/
All checks passed!

$ mypy --strict-bytes src/ tests/typing/strict_bytes.py
Success: no issues found in 13 source files

Unwiring the new check from validate_headers, with the tests left in place, fails exactly the four test_content_length_rejected_in_trailers[...] cases and nothing else.

Not run: the h2spec tox env (needs a downloaded Go binary and a live TLS server) and the packaging/docs envs; and no CI has executed on this PR, since the check suite sits at action_required for fork branches. Everything above is local.

[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.

[feiiiiii5]

Same rule as the other thread, detail there: 28cc3a5 rejects a content-length received in a trailers section whatever its value, via _reject_content_length_in_trailers in utilities.validate_headers, with the test parametrized over 13, 15, 0 and banana.

[Kriechi]

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

[feiiiiii5]

Done in 28cc3a5, and my earlier "base already refuses it" was wrong for numeric values — correction posted in the conversation.

_reject_content_length_in_trailers now sits in the validate_headers pipeline next to _reject_connection_header, keys off the same hdr_validation_flags.is_trailer flag as the pseudo-header-in-trailer rule, and raises ProtocolError("Received content-length header in trailer") whatever the value:

for header in headers:
    if hdr_validation_flags.is_trailer and header[0] == b"content-length":
        msg = "Received content-length header in trailer"
        raise ProtocolError(msg)
    yield header

test_content_length_rejected_in_trailers is parametrized over 13, 15, 0, banana, each also asserting the PROTOCOL_ERROR GOAWAY. stream.py stays at the one-line change you asked for.

One question rather than an assumption: the send path is untouched, so send_headers still emits a trailer content-length if a caller passes one, and four existing tests in test_basic_logic.py do exactly that with content-length: 0. Do you want the outbound side symmetric, or was the receive-side rule the one you meant?

[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.

[feiiiiii5]

Correcting something I published here, because it changed what this branch did.

In an earlier comment I wrote that base already refuses a content-length in a trailers section and that "the numeric cases are rejected too", and commit 90409750 dropped the check on that premise. Measured against master at bc239af1, that is only half right: a non-numeric value is refused by the ordinary content-length parser (Invalid content-length header: b'banana'), but 13, 15 and 0 are accepted and redefine _expected_content_length. So at 90409750 a trailer content-length of any value was accepted, which is worse than master on the non-numeric case and contrary to the rule @Kriechi stated.

28cc3a5 fixes it: _reject_content_length_in_trailers in utilities.validate_headers raises ProtocolError("Received content-length header in trailer") whatever the value, next to the existing pseudo-header-in-trailer rule that keys off the same is_trailer flag. test_content_length_rejected_in_trailers is parametrized over 13, 15, 0 and banana; the whole suite is 1670 passed at 100% coverage, ruff check src/ and mypy --strict-bytes src/ tests/typing/strict_bytes.py are clean, and unwiring the check fails exactly those four cases. The description's verification numbers are updated to match — the previous ones described a state of the branch that no longer existed.