diff --git a/lib/protocol/http/headers.rb b/lib/protocol/http/headers.rb index ddccf02..4175fbe 100644 --- a/lib/protocol/http/headers.rb +++ b/lib/protocol/http/headers.rb @@ -231,9 +231,9 @@ def keys # Extract the specified keys from the headers. # - # @parameter keys [Array] The keys to extract. + # @parameter keys [Array] The keys to extract, which must be lowercase. def extract(keys) - deleted, @fields = @fields.partition do |field| + deleted = remove_fields do |field| keys.include?(field.first.downcase) end @@ -420,12 +420,14 @@ def merge(headers) # @parameter key [String] The header key. # @returns [String | Array | Object] The merged header value. def delete(key) + key = key.downcase + # If we've indexed the headers, we can bail out early if the key is not present: - if @indexed && !@indexed.key?(key.downcase) + if @indexed && !@indexed.key?(key) return nil end - deleted, @fields = @fields.partition do |field| + deleted = remove_fields do |field| field.first.downcase == key end @@ -448,6 +450,40 @@ def delete(key) end end + # Remove all fields for which the block returns true, preserving the trailer boundary. + # + # Removing a field before the trailer boundary `@tail` must shift the boundary down, otherwise the first trailer would be treated as a header. + # + # Most headers never have a trailer boundary, so that case is kept on the fast `partition` path and the indexed loop only runs when a boundary exists. + # + # @yields {|field| ...} Each `[key, value]` pair; return true to remove it. + # @returns [Array] The removed fields, in their original order. + private def remove_fields(&block) + if tail = @tail + deleted = [] + kept = [] + + @fields.each_with_index do |field, index| + if yield(field) + deleted << field + + if index < @tail + tail -= 1 + end + else + kept << field + end + end + + @fields = kept + @tail = tail + else + deleted, @fields = @fields.partition(&block) + end + + return deleted + end + # Merge the value into the hash according to the policy for the given key. # # @parameter hash [Hash] The hash to merge into. diff --git a/releases.md b/releases.md index 9480d0a..26fa117 100644 --- a/releases.md +++ b/releases.md @@ -1,5 +1,10 @@ # Releases +## Unreleased + + - Preserve the trailer boundary when `Protocol::HTTP::Headers#delete` or `#extract` removes fields from the header section, so existing trailers are no longer sent as headers. + - `Protocol::HTTP::Headers#delete` (and therefore `#set`) now normalizes the key to lowercase, so mixed case keys remove the matching fields instead of leaving them in place. + ## v0.72.0 - Clarified body stream lifecycle and ownership, including the directional semantics of `Protocol::HTTP::Body::Stream#close_read`, `#close_write`, and `#close`, how premature input closure affects the associated HTTP exchange, and ownership of streams passed to `Streamable#call`. diff --git a/test/protocol/http/headers.rb b/test/protocol/http/headers.rb index 9b0c504..98e24b3 100644 --- a/test/protocol/http/headers.rb +++ b/test/protocol/http/headers.rb @@ -293,6 +293,13 @@ expect(headers["accept-encoding"]).to be == ["gzip"] end + + it "can replace an existing field using a mixed case key" do + headers.set("Content-Type", "text/plain") + + expect(headers.fields.count{|key, value| key.downcase == "content-type"}).to be == 1 + expect(headers["content-type"]).to be == "text/plain" + end end with "#extract" do @@ -329,6 +336,22 @@ it "can delete non-existant fields" do expect(headers.delete("transfer-encoding")).to be_nil end + + it "can delete fields using a mixed case key" do + expect(headers.delete("Content-Type")).to be == "text/html" + + expect(headers.fields).to be == fields[1..-1] + end + + it "can delete indexed fields using a mixed case key" do + # Force the headers to be indexed: + headers.to_h + + expect(headers.delete("Content-Type")).to be == "text/html" + + expect(headers.fields).to be == fields[1..-1] + expect(headers["content-type"]).to be_nil + end end with "#merge" do @@ -512,6 +535,164 @@ end end + with "trailer boundary" do + let(:header_fields) {fields[1..-1]} + + before do + headers.trailer! + headers.add("etag", "abcd") + headers.add("digest", "sha-256=xyz") + end + + it "preserves trailers when deleting a header" do + expect(headers.delete("content-type")).to be == "text/html" + + expect(headers).to be(:trailer?) + expect(headers.tail).to be == 4 + expect(headers.header.to_a).to be == header_fields + expect(headers.trailer.to_a).to be == [["etag", "abcd"], ["digest", "sha-256=xyz"]] + end + + it "preserves trailers when deleting multiple fields with the same key" do + headers.delete("set-cookie") + + expect(headers.tail).to be == 3 + expect(headers.header.to_a).to be == [ + ["Content-Type", "text/html"], + ["connection", "Keep-Alive"], + ["Accept", "*/*"], + ] + expect(headers.trailer.to_a).to be == [["etag", "abcd"], ["digest", "sha-256=xyz"]] + end + + it "preserves trailers when deleting a header with an index" do + # Force the headers to be indexed: + headers.to_h + + headers.delete("content-type") + + expect(headers.tail).to be == 4 + expect(headers.header.to_a).to be == header_fields + expect(headers.trailer.to_a).to be == [["etag", "abcd"], ["digest", "sha-256=xyz"]] + expect(headers["etag"]).to be == "abcd" + end + + it "preserves trailers when extracting headers" do + expect(headers.extract(["content-type", "set-cookie"])).to be == [ + ["Content-Type", "text/html"], + ["Set-Cookie", "hello=world"], + ["set-cookie", "foo=bar"], + ] + + expect(headers).to be(:trailer?) + expect(headers.tail).to be == 2 + expect(headers.header.to_a).to be == [ + ["connection", "Keep-Alive"], + ["Accept", "*/*"], + ] + expect(headers.trailer.to_a).to be == [["etag", "abcd"], ["digest", "sha-256=xyz"]] + end + + it "preserves trailers when extracting all headers" do + headers.extract(["content-type", "connection", "set-cookie", "accept"]) + + expect(headers).to be(:trailer?) + expect(headers.tail).to be == 0 + expect(headers.header.to_a).to be == [] + expect(headers.trailer.to_a).to be == [["etag", "abcd"], ["digest", "sha-256=xyz"]] + end + + it "does not move the boundary when deleting a trailer" do + expect(headers.delete("etag")).to be == "abcd" + + expect(headers).to be(:trailer?) + expect(headers.tail).to be == 5 + expect(headers.header.to_a).to be == fields + expect(headers.trailer.to_a).to be == [["digest", "sha-256=xyz"]] + end + + it "does not move the boundary when extracting trailers" do + expect(headers.extract(["etag", "digest"])).to be == [["etag", "abcd"], ["digest", "sha-256=xyz"]] + + expect(headers).to be(:trailer?) + expect(headers.tail).to be == 5 + expect(headers.header.to_a).to be == fields + expect(headers.trailer.to_a).to be == [] + end + + it "preserves the boundary when deleting a missing header" do + expect(headers.delete("transfer-encoding")).to be_nil + + expect(headers.tail).to be == 5 + expect(headers.header.to_a).to be == fields + expect(headers.trailer.to_a).to be == [["etag", "abcd"], ["digest", "sha-256=xyz"]] + end + + with "a header that is permitted as a trailer" do + let(:fields) do + [ + ["Content-Type", "text/html"], + ["Server-Timing", "db;dur=1"], + ["Accept", "*/*"], + ] + end + + it "preserves trailers when replacing the header" do + headers["server-timing"] = "db;dur=2" + + expect(headers).to be(:trailer?) + expect(headers.tail).to be == 2 + expect(headers.header.to_a).to be == [ + ["Content-Type", "text/html"], + ["Accept", "*/*"], + ] + expect(headers.trailer.to_a).to be == [ + ["etag", "abcd"], + ["digest", "sha-256=xyz"], + ["server-timing", "db;dur=2"], + ] + end + end + + with "a key that appears in both the header and trailer sections" do + let(:fields) do + [ + ["Content-Type", "text/html"], + ["ETag", "wxyz"], + ["Accept", "*/*"], + ] + end + + it "only moves the boundary for the field in the header section" do + expect(headers.delete("etag")).to be == "abcd" + + expect(headers.tail).to be == 2 + expect(headers.header.to_a).to be == [ + ["Content-Type", "text/html"], + ["Accept", "*/*"], + ] + expect(headers.trailer.to_a).to be == [["digest", "sha-256=xyz"]] + end + end + end + + with "trailer boundary and no trailers" do + before do + headers.trailer! + end + + it "keeps the boundary at the end of the fields when deleting a header" do + expect(headers.tail).to be == fields.size + + headers.delete("content-type") + + expect(headers).to be(:trailer?) + expect(headers.tail).to be == headers.fields.size + expect(headers.header.to_a).to be == fields[1..-1] + expect(headers.trailer.to_a).to be == [] + end + end + with "#add with trailer: keyword" do let(:headers) {subject.new}