Repository navigation
Keep received chunked trailers in the trailer section rather than merging them into the header section. - #64
samuel-williams-shopify wants to merge 4 commits into
Conversation
…ging them into the header section.
…t can distinguish re-marking from preserving the trailer section.
|
Closing after discussion: this PR models the wrong thing.
The remaining question is whether the policy is right — in particular |
When reading a chunked body, trailer fields were added to the message headers without marking where the trailer section begins, so they ended up indistinguishable from header fields. This change marks the trailer section when the first trailer field is received, so
headers.headerandheaders.trailerremain distinct.Problem
Protocol::HTTP1::Body::Chunked#read_trailercalled@headers.add(name, value, trailer: true)for each received trailer field, but never calledHeaders#trailer!to record where the trailer section starts. After the body was read,headers.trailer?wasfalse,headers.trailerwas empty, andheaders.headerincluded the trailer fields.Anything inspecting or forwarding the headers after the body has been read would therefore treat trailer fields as header fields. RFC 9110, Section 6.5.2 says a recipient MUST NOT merge trailer fields into the header section unless it knows the field semantics permit it. This is more significant because the default
Protocol::HTTP::Header::Genericpolicy permits arbitrary unlisted fields (e.g.x-forwarded-host) in trailers, and request trailers are controlled by the client.For example, a chunked message with trailer
x-forwarded-host: evilpreviously yieldedheaders.trailer? => falseandheaders.header.to_a => [["etag", "\"t\""], ["x-forwarded-host", "evil"]].Solution
read_trailernow calls@headers.trailer!immediately before adding the first trailer field. Sincetrailer!is@tail ||= @fields.size, this is a no-op if the trailer section was already marked, so existing callers which mark the trailer section themselves continue to work. Validation is unchanged, asaddwas already invoked withtrailer: true.Marking is deliberately lazy: the overwhelmingly common case is an empty trailer section (
0\r\n\r\n), and in that case the headers are left untouched. This meansheaders.trailer?staysfalse, later calls toheaders.addare not switched into trailer validation (which would otherwise reject e.g.content-type), frozen headers do not raiseFrozenError, and the behaviour matches socketry/async-http#259, which only marks the trailer section when a trailingHEADERSframe actually arrives.After this change, the same message yields
headers.trailer? => true,headers.header.to_a => [["etag", "\"t\""]]andheaders.trailer.to_a => [["x-forwarded-host", "evil"]].Residual exposure
Only the
headerandtrailerenumerators are separated. The indexed lookup API, includingheaders["x-forwarded-host"],headers.include?, andheaders.to_h, still covers all fields, so code which looks up a field by name after the body has been read will still see trailer values. Callers which need to ignore trailers for a given field should consultheaders.header, andheaders.flattenremains available for callers which explicitly want to merge trailers. Separately,Headers#deleteand#extractdo not adjust@tailwhen removing fields from the header section, which is addressed in socketry/protocol-http#125.Tests
Tests cover the body reader directly (announced and unannounced trailers, an empty trailer section including frozen headers and subsequent
add, and the case where the trailer section was already marked and contained a field), and end-to-end cases through bothConnection#read_responseandConnection#read_request.