Skip to content

Validate trailer fields in write_chunked_body regardless of the collection type. - #67

Open
samuel-williams-shopify wants to merge 8 commits into
mainfrom
validate-written-trailers
Open

samuel-williams-shopify wants to merge 8 commits into
mainfrom
validate-written-trailers

Conversation

@samuel-williams-shopify

@samuel-williams-shopify samuel-williams-shopify commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

write_chunked_body now validates every trailer field against Protocol::HTTP::Headers::POLICY before writing the trailer section, so framing fields such as content-length and transfer-encoding raise Protocol::HTTP::InvalidTrailerError instead of being written to the wire. Additionally, a connection whose body write fails part-way is now marked non-persistent so it can never be reused.

Problem

write_chunked_body(body, head, trailer) (reached via write_body) wrote whatever the trailer argument yielded after the terminal 0\r\n chunk. Only fields added through Protocol::HTTP::Headers#add with trailer: true were checked against the trailer policy; a plain Hash or any other enumerable bypassed that check entirely, so a caller could emit:

HTTP/1.1 200 OK\r\ntransfer-encoding: chunked\r\n\r\n1\r\nx\r\n0\r\ncontent-length: 5\r\ntransfer-encoding: chunked\r\n\r\n

Framing fields in a trailer are forbidden by RFC 9110 §6.5.1 and are a request/response smuggling vector when an intermediary honours them.

Separately, when write_chunked_body or write_fixed_length_body raised mid-body (e.g. ContentLengthError, or now InvalidTrailerError), the ensure branch still called send_end_stream!, which on a server in the half-closed remote state calls close! with no error and returns a still-persistent connection to the :idle state. A caller which swallowed the error could therefore reuse a connection with a half-written body on it.

Solution

  • Added Connection#format_trailer(trailer), which iterates the trailer, looks up each name (case-insensitively) in Protocol::HTTP::Headers::POLICY, and raises Protocol::HTTP::InvalidTrailerError when the policy is false (single-valued headers such as content-length) or the header class reports trailer? as false (e.g. transfer-encoding, trailer). Unlisted names fall back to Header::Generic, which permits trailers, matching the behaviour of Headers#add. The same error class is already raised by the read side (Body::Chunked#read_trailer), so callers see consistent errors in both directions.
  • The whole trailer section is validated and formatted into a single binary buffer before the terminal 0\r\n is written, so an invalid trailer leaves the chunked body visibly unterminated rather than half-emitted with a forbidden field. Each formatted line is coerced with .b before being appended, since appending to an empty String adopts the argument's encoding and a UTF-8 value followed by a binary value would otherwise raise Encoding::CompatibilityError. Token/CR/LF/NUL validation is reused by extracting format_header from write_header (which is otherwise unchanged).
  • This only costs a hash lookup and a downcase per field, and only when a trailer is actually present.
  • write_chunked_body and write_fixed_length_body now set @persistent = false in their existing rescue branches, so send_end_stream! transitions to :closed rather than :idle after a failed body write. (write_body_and_close already does this unconditionally.)
  • The trailer parameter is documented as Hash | Enumerable | Nil of [name, value] pairs, matching what is accepted, e.g. the enumerator returned by Protocol::HTTP::Headers#trailer!.

Tests

  • test/protocol/http1/connection.rb: Hash trailers containing content-length / transfer-encoding (including mixed case) and an array-of-pairs trailer raise InvalidTrailerError, and the exact captured wire output confirms neither the forbidden field nor the terminal chunk was written. A trailer produced by Headers#trailer! (as async-http does) continues to round-trip, including when the trailer field is added to the headers while the body is still being written. Mixed UTF-8 and binary trailer values are written byte-for-byte. After a rejected trailer or a ContentLengthError, the connection is non-persistent and does not return to :idle.
  • test/protocol/http1/trailer.rb: the same rejection through write_body, asserting the exact bytes written.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant