Validate trailer fields in write_chunked_body regardless of the collection type.

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,16 +751,21 @@ 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
 
 					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.)

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.

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
 

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