Skip to content

Validate trailers when assigning headers with Headers#[]=. - #126

Open
samuel-williams-shopify wants to merge 7 commits into
mainfrom
validate-trailer-assignment
Open

samuel-williams-shopify wants to merge 7 commits into
mainfrom
validate-trailer-assignment

Conversation

@samuel-williams-shopify

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

Copy link
Copy Markdown
Contributor

Protocol::HTTP::Headers#[]= did not perform trailer validation, so forbidden headers such as content-length, transfer-encoding and authorization could be appended to the trailer section after trailer! had been called and subsequently written to the wire. This change routes []= through the same validation as add, so it now raises InvalidTrailerError in that case, and makes set validate before it deletes the existing header.

Problem

Once trailer! has been called, add checks the header policy's trailer? and raises InvalidTrailerError for fields which must not appear in trailers (framing, hop-by-hop, routing and credential headers). However, []= appended directly to @fields without that check, so:

headers.trailer!
headers["content-length"] = "999" # Landed in the trailer section.
headers["transfer-encoding"] = "gzip" # Landed in the trailer section.

merge! was already safe, since it delegates to add. set also delegates to add, so it did raise, but only after calling delete(key), so a rejected set("content-length", "2") in trailer mode destroyed the existing content-length header and then raised.

Separately, InvalidTrailerError#key was always nil, because initialize never assigned @key.

Solution

  • Extract the trailer check into a protected validate_trailer!(key, policy) helper, shared by add, set and []=.
  • In []= and set, validate when trailer? is true, before the existing delete(key) call, so that a rejected assignment leaves the headers (including any existing header of the same name) unchanged.
  • []= passes the key as given to validate_trailer! (the downcased key is still used for the policy lookup), so its error message and InvalidTrailerError#key match the caller's spelling, consistent with add.
  • InvalidTrailerError#initialize now assigns @key.
  • Behaviour when not in trailer mode is unchanged; the existing policy lookup is simply hoisted so it can be reused for validation.
  • Tests cover: []= with forbidden fields (content-length, transfer-encoding, authorization) after trailer! raises with the matching key and leaves the trailer empty; []= with a permitted field (etag) appears in trailer; a rejected []= or set does not delete an existing header; []= reports the key as given; []= with content-length before trailer! still works; and regression guards for set/merge! in trailer mode.
  • The headers guide incorrectly stated that forbidden trailers are "silently ignored"; it now states that add, set, []= and merge! validate before modifying the headers and raise InvalidTrailerError. context/headers.md is the generated copy of that guide and has been updated to match.

This deliberately does not touch delete/extract index handling, which is being addressed separately.

Not addressed here

In trailer mode, []= or set on a key which already exists in the header section deletes it from the header section and appends the new value to the trailer section. With #125 (which adjusts @tail on delete), the appended value becomes a real trailer; however, replacing a header which has conceptually already been sent is arguably meaningless and should probably raise instead. That is left for a follow-up discussion.

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