diff --git a/lib/protocol/http1/body/chunked.rb b/lib/protocol/http1/body/chunked.rb index 2c8b19e..975d6c9 100644 --- a/lib/protocol/http1/body/chunked.rb +++ b/lib/protocol/http1/body/chunked.rb @@ -148,12 +148,16 @@ def as_json(...) private # Read the trailer from the connection, and add any headers to the trailer. + # + # The trailer section is marked before the first field is added, so that `headers.header` and `headers.trailer` remain distinct. A recipient must not merge trailer fields into the header section (RFC 9110, Section 6.5.2). If no trailer fields are received, which is the common case, the headers are left untouched, so an empty trailer section does not change `headers.trailer?` and does not fail on frozen headers. If the trailer section was already marked, marking it again is a no-op. def read_trailer while line = @connection.read_line # Empty line indicates end of trailer: break if line.empty? if match = line.match(HEADER) + # Mark the start of the trailer section (no-op if already marked): + @headers.trailer! @headers.add(match[1], match[2], trailer: true) else raise BadHeader, "Could not parse header: #{line.inspect}" diff --git a/releases.md b/releases.md index db77727..5f97747 100644 --- a/releases.md +++ b/releases.md @@ -1,5 +1,9 @@ # Releases +## Unreleased + + - Keep received chunked trailers in the trailer section of the headers, rather than merging them into the header section. The trailer section is only marked when at least one trailer field is received, so messages with an empty trailer section leave the headers unchanged. + ## v0.41.0 - Map remote socket failures to `Protocol::HTTP::RemoteError` and prevent connection reuse. diff --git a/test/protocol/http1/body/chunked.rb b/test/protocol/http1/body/chunked.rb index 3d4580f..98baa31 100644 --- a/test/protocol/http1/body/chunked.rb +++ b/test/protocol/http1/body/chunked.rb @@ -112,6 +112,36 @@ expect{body.read}.to raise_exception(Protocol::HTTP1::BadRequest) end + with "empty trailer section" do + it "does not mark the trailer section" do + headers.add("etag", "abcd") + + body.join + + expect(headers).not.to be(:trailer?) + expect(headers.header.to_a).to be == [["etag", "abcd"]] + expect(headers.trailer.to_a).to be == [] + end + + it "does not modify frozen headers" do + headers.add("etag", "abcd") + headers.freeze + + expect(body.join).to be == "Hello World" + + expect(headers).not.to be(:trailer?) + expect(connection).to be(:half_closed_remote?) + end + + it "allows headers to be added after the body is read" do + body.join + + headers.add("content-type", "text/plain") + + expect(headers.header.to_a).to be == [["content-type", "text/plain"]] + end + end + with "trailer" do let(:postfix) {"ETag: abcd\r\n"} @@ -126,6 +156,43 @@ expect(connection).to be(:half_closed_remote?) end + + it "does not merge trailers into the header section" do + headers.add("trailer", "etag") + + expect(headers).not.to be(:trailer?) + + body.join + + expect(headers).to be(:trailer?) + expect(headers.header.to_a).to be == [["trailer", "etag"]] + expect(headers.trailer.to_a).to be == [["ETag", "abcd"]] + end + + it "preserves the trailer section if it was already marked" do + headers.add("trailer", "etag") + headers.trailer! + headers.add("x-sent", "1", trailer: true) + + body.join + + # The existing trailer field is retained, so the trailer section was not re-marked: + expect(headers.header.to_a).to be == [["trailer", "etag"]] + expect(headers.trailer.to_a).to be == [["x-sent", "1"], ["ETag", "abcd"]] + end + end + + with "trailer not announced in header section" do + let(:postfix) {"x-forwarded-host: evil\r\n"} + + it "does not merge trailers into the header section" do + headers.add("etag", "abcd") + + body.join + + expect(headers.header.to_a).to be == [["etag", "abcd"]] + expect(headers.trailer.to_a).to be == [["x-forwarded-host", "evil"]] + end end with "incomplete trailer" do diff --git a/test/protocol/http1/trailer.rb b/test/protocol/http1/trailer.rb index ad08c4d..ec45732 100644 --- a/test/protocol/http1/trailer.rb +++ b/test/protocol/http1/trailer.rb @@ -51,5 +51,61 @@ # Headers are updated: expect(headers).to be == {"foo" => ["bar"]} end + + it "keeps received trailers separate from the header section" do + expect(body).to receive(:length).and_return(nil) + trailer["foo"] = "bar" + + server.write_response("HTTP/1.1", 200, {"etag" => "abcd"}) + server.write_body("HTTP/1.1", body, false, trailer) + + version, status, reason, headers, body = client.read_response("GET") + + expect(headers).not.to be(:trailer?) + expect(headers.header.to_a).to be == [["etag", "abcd"]] + + body.join + + expect(headers).to be(:trailer?) + expect(headers.header.to_a).to be == [["etag", "abcd"]] + expect(headers.trailer.to_a).to be == [["foo", "bar"]] + end + end + + with "request trailers" do + it "keeps received request trailers separate from the header section" do + expect(body).to receive(:length).and_return(nil) + trailer["x-forwarded-host"] = "evil" + + client.write_request("localhost", "POST", "/", "HTTP/1.1", {"content-type" => "text/plain"}) + client.write_body("HTTP/1.1", body, false, trailer) + + authority, method, target, version, headers, body = server.read_request + + expect(method).to be == "POST" + expect(headers).not.to be(:trailer?) + expect(headers.header.to_a).to be == [["content-type", "text/plain"]] + + expect(body.join).to be == "HelloWorld" + + # Request trailers are controlled by the client, so they must not appear in the header section: + expect(headers).to be(:trailer?) + expect(headers.header.to_a).to be == [["content-type", "text/plain"]] + expect(headers.trailer.to_a).to be == [["x-forwarded-host", "evil"]] + end + + it "does not mark the trailer section when no request trailers are received" do + expect(body).to receive(:length).and_return(nil) + + client.write_request("localhost", "POST", "/", "HTTP/1.1", {"content-type" => "text/plain"}) + client.write_body("HTTP/1.1", body) + + authority, method, target, version, headers, body = server.read_request + + expect(body.join).to be == "HelloWorld" + + expect(headers).not.to be(:trailer?) + expect(headers.header.to_a).to be == [["content-type", "text/plain"]] + end end end