diff --git a/lib/protocol/http1/connection.rb b/lib/protocol/http1/connection.rb index 0ec263e..b61f432 100644 --- a/lib/protocol/http1/connection.rb +++ b/lib/protocol/http1/connection.rb @@ -332,12 +332,13 @@ def write_interim_response(version, status, headers, reason = nil) flush_stream end - # Write a header to the connection, rejecting CR, LF, and NUL in its value. + # Format a header field as a single line, rejecting CR, LF, and NUL in its value. # # @parameter name [String] the header name. # @parameter value [String] the header value. + # @returns [String] the formatted header line, including the trailing CRLF. # @raises [BadHeader] if the header name or value is invalid. - def write_header(name, value) + def format_header(name, value) # Convert it to a string: name = name.to_s value = value.to_s @@ -351,8 +352,16 @@ def write_header(name, value) raise BadHeader, "Invalid header value for #{name}: #{value.inspect}" end - # Write it: - write_stream("#{name}: #{value}\r\n") + return "#{name}: #{value}\r\n" + end + + # Write a header to the connection, rejecting CR, LF, and NUL in its value. + # + # @parameter name [String] the header name. + # @parameter value [String] the header value. + # @raises [BadHeader] if the header name or value is invalid. + def write_header(name, value) + write_stream(format_header(name, value)) end # Write headers to the connection. @@ -365,6 +374,34 @@ def write_headers(headers) end end + # Format the trailer section of a chunked body, i.e. the fields following the terminal chunk, including the final empty line. + # + # Fields are validated against {Protocol::HTTP::Headers::POLICY} before any are written, since a plain `Hash` does not enforce the trailer policy. + # + # @parameter trailer [Hash | Enumerable] the trailer fields to format, as `[name, value]` pairs, e.g. a `Hash` or the enumerator returned by `Protocol::HTTP::Headers#trailer!`. + # @returns [String] the formatted trailer section. + # @raises [Protocol::HTTP::InvalidTrailerError] if a field is not permitted in a trailer. + # @raises [BadHeader] if a header name or value is invalid. + def format_trailer(trailer) + buffer = String.new(encoding: Encoding::BINARY) + + trailer.each do |name, value| + # A policy of `false` indicates a single-valued header, which is never permitted as a trailer: + policy = HTTP::Headers::POLICY[name.to_s.downcase] + + unless policy and policy.trailer? + raise HTTP::InvalidTrailerError, name + end + + # Appending to an empty string adopts the encoding of the argument, so each line is coerced to binary to avoid incompatible encodings between fields: + buffer << format_header(name, value).b + end + + buffer << CRLF + + return buffer + end + # Read some data from the connection. # # @parameter length [Integer] the maximum number of bytes to read. @@ -673,6 +710,9 @@ def write_fixed_length_body(body, length, head) end end rescue => error + # The body may have been partially written, so the connection can't be reused, even if the caller swallows the error: + @persistent = false + raise ensure # Close the body after the response is fully flushed, so that completion callbacks run after the client has received the response: @@ -689,7 +729,8 @@ def write_fixed_length_body(body, length, head) # # @parameter body [Object] the body to write. # @parameter head [Boolean] whether the request was a `HEAD` request. - # @parameter trailer [Hash | Nil] the trailers to write. + # @parameter trailer [Hash | Enumerable | Nil] the trailers to write, as `[name, value]` pairs. + # @raises [Protocol::HTTP::InvalidTrailerError] if a trailer field is not permitted in a trailer. def write_chunked_body(body, head, trailer = nil) write_stream("transfer-encoding: chunked\r\n\r\n") @@ -710,9 +751,11 @@ def write_chunked_body(body, head, trailer = nil) end if trailer&.any? + # Validate and format the entire trailer section before writing the terminal chunk, so that an invalid trailer is rejected rather than partially written: + trailer = format_trailer(trailer) + write_stream("0\r\n") - write_headers(trailer) - write_stream("\r\n") + write_stream(trailer) else write_stream("0\r\n\r\n") end @@ -720,6 +763,9 @@ def write_chunked_body(body, head, trailer = nil) flush_stream end rescue => error + # The body may have been partially written, so the connection can't be reused, even if the caller swallows the error: + @persistent = false + raise ensure # Close the body after the complete chunked response (including terminal chunk) is flushed, so that completion callbacks don't block the client from seeing the response as complete: @@ -790,7 +836,7 @@ def close!(error = nil) # @parameter version [String] the HTTP version. # @parameter body [Object] the body to write. # @parameter head [Boolean] whether the request was a `HEAD` request. - # @parameter trailer [Hash | Nil] the trailers to write. + # @parameter trailer [Hash | Enumerable | Nil] the trailers to write, as `[name, value]` pairs. def write_body(version, body, head = false, trailer = nil) # HTTP/1.0 cannot in any case handle trailers. if version == HTTP10 # or te: trailers was not present (strictly speaking not required.) diff --git a/releases.md b/releases.md index db77727..878d4ac 100644 --- a/releases.md +++ b/releases.md @@ -1,5 +1,10 @@ # Releases +## Unreleased + + - Validate trailer fields in `write_chunked_body` against the header policy, so framing fields such as `content-length` and `transfer-encoding` are rejected with `Protocol::HTTP::InvalidTrailerError` rather than written to the wire, regardless of the trailer collection type. + - Mark the connection as non-persistent when `write_chunked_body` or `write_fixed_length_body` fails, so a connection with a partially written body is never reused, even if the caller swallows the error. + ## v0.41.0 - Map remote socket failures to `Protocol::HTTP::RemoteError` and prevent connection reuse. diff --git a/test/protocol/http1/connection.rb b/test/protocol/http1/connection.rb index 701ea88..8c3e6ce 100644 --- a/test/protocol/http1/connection.rb +++ b/test/protocol/http1/connection.rb @@ -427,6 +427,118 @@ expect(headers).to have_keys("etag") end + it "can generate and read trailer from headers trailer enumerator" do + response_headers = Protocol::HTTP::Headers.new([["trailer", "etag"]]) + server.write_headers(response_headers) + + # Mark the start of the trailer, and add a field to it, as a server would while generating the body: + trailer = response_headers.trailer! + response_headers.add("etag", "abcd") + + server.write_chunked_body(body, false, trailer) + server.close + + headers = client.read_headers + body = client.read_body(headers, false) + expect(body.join).to be == chunks.join + + expect(headers["etag"]).to be == "abcd" + end + + it "can generate and read trailer added while the body is being written" do + response_headers = Protocol::HTTP::Headers.new([["trailer", "etag"]]) + server.write_headers(response_headers) + + # As a server does, capture the trailer enumerator before writing the body, and add the field while the body is still being generated: + trailer = response_headers.trailer! + + body = Protocol::HTTP::Body::Writable.new + + writer = Thread.new do + chunks.each do |chunk| + body.write(chunk) + end + + response_headers.add("etag", "late") + body.close_write + end + + server.write_chunked_body(body, false, trailer) + server.close + writer.join + + expect(sockets.first.read).to be == "trailer: etag\r\ntransfer-encoding: chunked\r\n\r\n5\r\nHello\r\n5\r\nWorld\r\n0\r\netag: late\r\n\r\n" + end + + it "can write trailer values with mixed encodings" do + # A UTF-8 value with a non-ASCII character followed by a binary value with a high byte, which cannot be concatenated without coercion: + trailer = [["x-utf8", "héllo"], ["x-binary", "\xFF".b]] + + server.write_chunked_body(body, false, trailer) + server.close + + expect(sockets.first.read).to be == "transfer-encoding: chunked\r\n\r\n5\r\nHello\r\n5\r\nWorld\r\n0\r\nx-utf8: h\xC3\xA9llo\r\nx-binary: \xFF\r\n\r\n".b + end + + with "invalid trailer" do + # Everything the server wrote, so we can confirm forbidden fields never reach the wire: + let(:written) {sockets.first.read} + + it "rejects content-length in a hash trailer" do + expect do + server.write_chunked_body(body, false, {"content-length" => "5"}) + end.to raise_exception(Protocol::HTTP::InvalidTrailerError, message: be =~ /content-length/) + + server.close + + # Neither the terminal chunk nor the forbidden field is written: + expect(written).to be == "transfer-encoding: chunked\r\n\r\n5\r\nHello\r\n5\r\nWorld\r\n" + end + + it "rejects transfer-encoding in a hash trailer" do + expect do + server.write_chunked_body(body, false, {"transfer-encoding" => "chunked"}) + end.to raise_exception(Protocol::HTTP::InvalidTrailerError, message: be =~ /transfer-encoding/) + + server.close + + expect(written).to be == "transfer-encoding: chunked\r\n\r\n5\r\nHello\r\n5\r\nWorld\r\n" + end + + it "rejects forbidden fields regardless of case" do + expect do + server.write_chunked_body(body, false, {"Content-Length" => "5"}) + end.to raise_exception(Protocol::HTTP::InvalidTrailerError) + end + + it "rejects forbidden fields from an arbitrary enumerable" do + trailer = [["etag", "abcd"], ["content-length", "5"]] + + expect do + server.write_chunked_body(body, false, trailer) + end.to raise_exception(Protocol::HTTP::InvalidTrailerError) + + server.close + + # The valid field preceding the invalid one must not have been written either: + expect(written).to be == "transfer-encoding: chunked\r\n\r\n5\r\nHello\r\n5\r\nWorld\r\n" + end + + it "prevents the connection from being reused" do + # As a server which has finished reading the request: + server.receive_end_stream! + expect(server).to be(:half_closed_remote?) + + expect do + server.write_chunked_body(body, false, {"content-length" => "5"}) + end.to raise_exception(Protocol::HTTP::InvalidTrailerError) + + # The body was partially written, so even if the error is swallowed, the connection must not return to the idle state: + expect(server.persistent).to be == false + expect(server).to be(:closed?) + end + end + with "HEAD request" do it "can generate and read chunked response" do server.write_chunked_body(body, true) @@ -466,6 +578,9 @@ expect do server.write_fixed_length_body(body, 100, false) end.to raise_exception(Protocol::HTTP1::ContentLengthError) + + # The body was partially written, so the connection can't be reused: + expect(server.persistent).to be == false end end @@ -474,6 +589,9 @@ expect do server.write_fixed_length_body(body, 1, false) end.to raise_exception(Protocol::HTTP1::ContentLengthError) + + # The body was partially written, so the connection can't be reused: + expect(server.persistent).to be == false end end diff --git a/test/protocol/http1/trailer.rb b/test/protocol/http1/trailer.rb index ad08c4d..fee3c9b 100644 --- a/test/protocol/http1/trailer.rb +++ b/test/protocol/http1/trailer.rb @@ -51,5 +51,21 @@ # Headers are updated: expect(headers).to be == {"foo" => ["bar"]} end + + it "rejects trailers which are not permitted by the header policy" do + expect(body).to receive(:length).and_return(nil) + trailer["content-length"] = "5" + + server.write_response("HTTP/1.1", 200, {}) + + expect do + server.write_body("HTTP/1.1", body, false, trailer) + end.to raise_exception(Protocol::HTTP::InvalidTrailerError) + + server.close + + # The forbidden trailer must never reach the client, and neither must the terminal chunk: + expect(sockets.first.read).to be == "HTTP/1.1 200 OK\r\ntransfer-encoding: chunked\r\n\r\n5\r\nHello\r\n5\r\nWorld\r\n" + end end end