Repository navigation
Keep received HTTP/2 trailers distinct from the header section. - #259
Closed
samuel-williams-shopify wants to merge 2 commits into
Closed
samuel-williams-shopify wants to merge 2 commits into
samuel-williams-shopify wants to merge 2 commits into
Conversation
Contributor
Author
|
Closing after discussion, for the same reason as socketry/protocol-http1#64.
|
This was referenced Nov 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When an HTTP/2 stream received trailing headers, they were appended to the received message's
Protocol::HTTP::Headerswithout marking where the trailer section begins, so they were merged into the header section. This change callsHeaders#trailer!before adding the first received trailer field, keepingheaders.headerandheaders.trailerdistinct.Problem
Async::HTTP::Protocol::HTTP2::Stream#receive_trailing_headerscalled@headers.add(key, value, trailer: true)for each received trailer field, which validates the field against the trailer policy, but never called@headers.trailer!to record the boundary. As a result, after reading the body,headers.trailer?wasfalse,headers.headerincluded the trailer fields, andheaders.trailerwas empty. RFC 9110 §6.5.2 says a recipient must not merge trailer fields into the header section unless the field semantics permit it.Solution
receive_trailing_headersnow calls@headers.trailer!immediately before adding each trailer field.trailer!is idempotent (@tail ||= @fields.size), so this is safe if trailers are delivered in multiple frames, and a trailing HEADERS frame with no fields leavestrailer?false rather than creating an empty trailer section. The sending side already callstrailer!on its own, separate, headers object before writing, so it is unaffected.The same defect in
protocol-http1's chunked body reader is being fixed separately in socketry/protocol-http1#64, which marks the trailer section lazily in the same way; this is the HTTP/2 counterpart. The new tests are HTTP/2-specific rather than in the sharedAProtocolfixture because HTTP/1 will continue to fail them until theprotocol-http1fix is released, at which point the assertions can be moved into the shared fixture to cover every protocol.Behavioural note
Once a received message's headers have a trailer section,
Headers#addwithout an explicittrailer:argument runs in trailer mode and raisesProtocol::HTTP::InvalidTrailerErrorfor fields whose policy forbids trailers. Code that mutates a received message's headers after consuming the body (for example, a buffering proxy addingx-forwarded-forafter reading the request body) must do so before reading the body, or build a freshHeadersinstance. Previously such mutations silently succeeded because the trailer section was never marked.socketry/protocol-http#125 fixes
Headers#deleteandHeaders#extractto preserve the trailer boundary (@tail) when removing fields; it should ship before this change, otherwise deleting a header field from a received message with trailers can shift the boundary and reclassify fields.Not addressed here
A second HEADERS frame without
END_STREAMon a stream that already has headers falls through toreceive_initial_headersrather than being treated as a malformed trailer. If it carries only regular fields, the request still passesvalid?(from the first frame),@headersis replaced with a fresh instance, andprepare_inputraisesArgumentError("Input body already prepared!"), whichprocess_headersdoes not rescue, so the whole connection is torn down instead of the stream being reset withRST_STREAM(PROTOCOL_ERROR). This is pre-existing and orthogonal to the trailer section boundary, so it is left for a separate change.