Reject transfer-encoding in HTTP/1.0 messages and non-chunked request bodies.

Author: samuel-williams-shopify

lib/protocol/http1/connection.rb

@@ -451,7 +451,7 @@ def read_request
 					@persistent = persistent?(version, method, headers)
 				end
 
-				body = read_request_body(method, headers)
+				body = read_request_body(method, headers, version)
 
 				unless body
 					self.receive_end_stream!
@@ -507,7 +507,7 @@ def read_response(method)
 				end
 
 				unless interim_status?(status)
-					body = read_response_body(method, status, headers)
+					body = read_response_body(method, status, headers, version)
 
 					unless body
 						self.receive_end_stream!
@@ -928,7 +928,8 @@ def extract_content_length(headers)
 			# @parameter method [String] the HTTP method.
 			# @parameter status [Integer] the HTTP status code.
 			# @parameter headers [Hash] the headers of the response.
-			def read_response_body(method, status, headers)
+			# @parameter version [String | Nil] the HTTP version of the response, if known.
+			def read_response_body(method, status, headers, version = nil)
 				# RFC 7230 3.3.3
 				# 1.  Any response to a HEAD request and any response with a 1xx
 				# (Informational), 204 (No Content), or 304 (Not Modified) status
@@ -965,7 +966,7 @@ def read_response_body(method, status, headers)
 					return read_tunnel_body
 				end
 
-				return read_body(headers, true)
+				return read_body(headers, true, version)
 			end
 
 			# Read the body of the request.
@@ -976,7 +977,8 @@ def read_response_body(method, status, headers)
 			#
 			# @parameter method [String] the HTTP method.
 			# @parameter headers [Hash] the headers of the request.
-			def read_request_body(method, headers)
+			# @parameter version [String | Nil] the HTTP version of the request, if known.
+			def read_request_body(method, headers, version = nil)
 				# 2.  Any 2xx (Successful) response to a CONNECT request implies that
 				# the connection will become a tunnel immediately after the empty
 				# line that concludes the header fields.  A client MUST ignore any
@@ -993,7 +995,7 @@ def read_request_body(method, headers)
 
 				# 6.  If this is a request message and none of the above are true, then
 				# the message body length is zero (no message body is present).
-				return read_body(headers)
+				return read_body(headers, false, version)
 			end
 
 			# Read the body of the message.
@@ -1002,15 +1004,28 @@ def read_request_body(method, headers)
 			# - Otherwise, if the `content-length` is present, the body is read until the content length is reached.
 			# - Otherwise, if `remainder` is true, the body is read until the connection is closed.
 			#
+			# Framing errors (e.g. `transfer-encoding` in an HTTP/1.0 message, or a request whose final transfer coding is not `chunked`) are rejected and mark the connection as non-persistent, since the remainder of the stream can no longer be delimited reliably.
+			#
 			# @parameter headers [Hash] the headers of the message.
-			# @parameter remainder [Boolean] whether to read the remainder of the body.
+			# @parameter remainder [Boolean] whether to read the remainder of the body, i.e. the message is a response which may be delimited by closing the connection.
+			# @parameter version [String | Nil] the HTTP version of the message, if known.
 			# @returns [Object] the body.
-			def read_body(headers, remainder = false)
+			# @raises [BadRequest] if the message framing is invalid.
+			def read_body(headers, remainder = false, version = nil)
 				# 3.  If a Transfer-Encoding header field is present and the chunked
 				# transfer coding (Section 4.1) is the final encoding, the message
 				# body length is determined by reading and decoding the chunked
 				# data until the transfer coding indicates the data is complete.
 				if transfer_encoding = headers.delete(TRANSFER_ENCODING)
+					# A server or client that receives an HTTP/1.0 message containing a
+					# Transfer-Encoding header field MUST treat the message as if the
+					# framing is faulty, even if a Content-Length is present, and close
+					# the connection after processing the message.
+					if version == HTTP10
+						@persistent = false
+						raise BadRequest, "HTTP/1.0 message contains transfer encoding!"
+					end
+					
 					# If a message is received with both a Transfer-Encoding and a
 					# Content-Length header field, the Transfer-Encoding overrides the
 					# Content-Length.  Such a message might indicate an attempt to
@@ -1019,21 +1034,26 @@ def read_body(headers, remainder = false)
 					# remove the received Content-Length field prior to forwarding such
 					# a message downstream.
 					if headers[CONTENT_LENGTH]
+						@persistent = false
 						raise BadRequest, "Message contains both transfer encoding and content length!"
 					end
 
 					if transfer_encoding.last == CHUNKED
 						return read_chunked_body(headers)
-					else
+					elsif remainder
 						# If a Transfer-Encoding header field is present in a response and
 						# the chunked transfer coding is not the final encoding, the
 						# message body length is determined by reading the connection until
-						# it is closed by the server.  If a Transfer-Encoding header field
-						# is present in a request and the chunked transfer coding is not
-						# the final encoding, the message body length cannot be determined
-						# reliably; the server MUST respond with the 400 (Bad Request)
-						# status code and then close the connection.
+						# it is closed by the server.
 						return read_remainder_body
+					else
+						# If a Transfer-Encoding header field is present in a request and
+						# the chunked transfer coding is not the final encoding, the
+						# message body length cannot be determined reliably; the server
+						# MUST respond with the 400 (Bad Request) status code and then
+						# close the connection.
+						@persistent = false
+						raise BadRequest, "Request transfer encoding does not end with chunked: #{transfer_encoding.inspect}"
 					end
 				end
 

releases.md

@@ -1,5 +1,9 @@
 # Releases
 
+## Unreleased
+
+  - Reject `transfer-encoding` in HTTP/1.0 messages and requests whose final transfer coding is not `chunked` with `BadRequest`, and mark the connection as non-persistent, as required by RFC 9112 for reliable message framing.
+
 ## v0.41.0
 
   - Map remote socket failures to `Protocol::HTTP::RemoteError` and prevent connection reuse.

test/protocol/http1/connection.rb

@@ -156,6 +156,75 @@
 			expect(server).to be(:persistent)
 		end
 
+		it "reads request with chunked body and trailer" do
+			client.stream.write "GET / HTTP/1.1\r\nHost: localhost\r\nTransfer-Encoding: chunked\r\nTrailer: etag\r\n\r\nb\r\nHello World\r\n0\r\nETag: abcd\r\n\r\n"
+			client.stream.close
+			
+			authority, method, target, version, headers, body = server.read_request
+			
+			expect(body.join).to be == "Hello World"
+			expect(headers).to be == {"trailer" => ["etag"], "etag" => "abcd"}
+			
+			expect(server).to be(:persistent)
+		end
+		
+		it "fails with HTTP/1.0 request with transfer-encoding" do
+			# RFC 9112 Section 6.1: a recipient MUST treat an HTTP/1.0 message containing Transfer-Encoding as faulty framing.
+			client.stream.write "GET / HTTP/1.0\r\nHost: localhost\r\nConnection: keep-alive\r\nTransfer-Encoding: chunked\r\n\r\nb\r\nHello World\r\n0\r\n\r\n"
+			client.stream.close
+			
+			expect do
+				server.read_request
+			end.to raise_exception(Protocol::HTTP1::BadRequest, message: be =~ /HTTP\/1.0/)
+			
+			expect(server).not.to be(:persistent)
+		end
+		
+		it "fails with HTTP/1.0 request with transfer-encoding and content-length" do
+			client.stream.write "GET / HTTP/1.0\r\nHost: localhost\r\nContent-Length: 11\r\nTransfer-Encoding: chunked\r\n\r\nHello World"
+			client.stream.close
+			
+			expect do
+				server.read_request
+			end.to raise_exception(Protocol::HTTP1::BadRequest)
+			
+			expect(server).not.to be(:persistent)
+		end
+		
+		it "fails with request whose final transfer-encoding is not chunked" do
+			# RFC 9112 Section 6.3: the server MUST respond with 400 and close the connection, since the body length cannot be determined reliably.
+			client.stream.write "GET / HTTP/1.1\r\nHost: localhost\r\nTransfer-Encoding: gzip\r\n\r\nHello World"
+			client.stream.close
+			
+			expect do
+				server.read_request
+			end.to raise_exception(Protocol::HTTP1::BadRequest, message: be =~ /chunked/)
+			
+			expect(server).not.to be(:persistent)
+		end
+		
+		it "fails with request where chunked is not the final transfer-encoding" do
+			client.stream.write "GET / HTTP/1.1\r\nHost: localhost\r\nTransfer-Encoding: chunked, gzip\r\n\r\nb\r\nHello World\r\n0\r\n\r\n"
+			client.stream.close
+			
+			expect do
+				server.read_request
+			end.to raise_exception(Protocol::HTTP1::BadRequest)
+			
+			expect(server).not.to be(:persistent)
+		end
+		
+		it "fails with request containing both transfer-encoding and content-length" do
+			client.stream.write "GET / HTTP/1.1\r\nHost: localhost\r\nContent-Length: 11\r\nTransfer-Encoding: chunked\r\n\r\nb\r\nHello World\r\n0\r\n\r\n"
+			client.stream.close
+			
+			expect do
+				server.read_request
+			end.to raise_exception(Protocol::HTTP1::BadRequest)
+			
+			expect(server).not.to be(:persistent)
+		end
+		
 		it "reads request with CONNECT method" do
 			client.stream.write "CONNECT localhost:443 HTTP/1.1\r\nHost: localhost\r\n\r\n"
 			client.stream.close
@@ -333,6 +402,46 @@
 
 			expect(result).to be == :yielded
 		end
+		
+		it "should read chunked response" do
+			client.open!
+			
+			server.stream.write("HTTP/1.1 200 OK\r\nTransfer-Encoding: chunked\r\n\r\nb\r\nHello World\r\n0\r\n\r\n")
+			server.stream.close
+			
+			version, status, reason, headers, body = client.read_response("GET")
+			
+			expect(headers).to be == {}
+			expect(body.join).to be == "Hello World"
+			expect(client).to be(:persistent)
+		end
+		
+		it "should read response with non-chunked transfer-encoding until closed" do
+			client.open!
+			
+			# RFC 9112 Section 6.3: for a response, the body is delimited by closing the connection.
+			server.stream.write("HTTP/1.1 200 OK\r\nTransfer-Encoding: gzip\r\n\r\nHello World")
+			server.stream.close
+			
+			version, status, reason, headers, body = client.read_response("GET")
+			
+			expect(body).to be_a(Protocol::HTTP1::Body::Remainder)
+			expect(body.join).to be == "Hello World"
+			expect(client).not.to be(:persistent)
+		end
+		
+		it "should fail with HTTP/1.0 response with transfer-encoding" do
+			client.open!
+			
+			server.stream.write("HTTP/1.0 200 OK\r\nConnection: keep-alive\r\nTransfer-Encoding: chunked\r\n\r\nb\r\nHello World\r\n0\r\n\r\n")
+			server.stream.close
+			
+			expect do
+				client.read_response("GET")
+			end.to raise_exception(Protocol::HTTP1::BadRequest, message: be =~ /HTTP\/1.0/)
+			
+			expect(client).not.to be(:persistent)
+		end
 	end
 
 	with "#read_response_body" do