diff --git a/lib/protocol/http1/connection.rb b/lib/protocol/http1/connection.rb index 0ec263e..4100ace 100644 --- a/lib/protocol/http1/connection.rb +++ b/lib/protocol/http1/connection.rb @@ -791,7 +791,9 @@ def close!(error = nil) # @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. - def write_body(version, body, head = false, trailer = nil) + # @parameter request [Boolean] whether the body belongs to a request, which cannot be delimited by closing the connection. + # @raises [ProtocolError] if a request body has unknown length and the version does not support chunked encoding. + def write_body(version, body, head = false, trailer = nil, request = false) # HTTP/1.0 cannot in any case handle trailers. if version == HTTP10 # or te: trailers was not present (strictly speaking not required.) trailer = nil @@ -815,7 +817,13 @@ def write_body(version, body, head = false, trailer = nil) write_connection_header(version) # We specifically ensure that non-persistent connections do not use chunked response, so that hijacking works as expected. write_chunked_body(body, head, trailer) + elsif request + # A request body cannot be delimited by closing the connection, as the server would read it as the next request: + error = ProtocolError.new("Cannot write #{version} request body of unknown length!") + body.close(error) + raise error else + # Only a response body may be delimited by closing the connection: @persistent = false write_connection_header(version) write_body_and_close(body, head) diff --git a/lib/traces/provider/protocol/http1/connection.rb b/lib/traces/provider/protocol/http1/connection.rb index 80dc396..d745bc9 100644 --- a/lib/traces/provider/protocol/http1/connection.rb +++ b/lib/traces/provider/protocol/http1/connection.rb @@ -45,11 +45,12 @@ def write_interim_response(version, status, headers, reason = nil) end end - def write_body(version, body, head = false, trailer = nil) + def write_body(version, body, head = false, trailer = nil, request = false) attributes = { version: version, head: head, trailer: trailer, + request: request, body: body&.as_json, } diff --git a/releases.md b/releases.md index db77727..41917f8 100644 --- a/releases.md +++ b/releases.md @@ -1,5 +1,9 @@ # Releases +## Unreleased + + - `Protocol::HTTP1::Connection#write_body` accepts a trailing `request` argument. When true, an HTTP/1.0 body of unknown length raises `ProtocolError` instead of being written close-delimited, which the server would read as the next request. Clients should pass it when writing request bodies. + ## 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..cde4fad 100644 --- a/test/protocol/http1/connection.rb +++ b/test/protocol/http1/connection.rb @@ -632,9 +632,33 @@ expect(body).to receive(:empty?).and_return(false) expect(body).to receive(:length).and_return(nil) + # Only a response body may be delimited by closing the connection: expect(server).to receive(:write_body_and_close) server.write_body("HTTP/1.0", body) end + + it "refuses to write closed body for a request" do + expect(body).to receive(:empty?).and_return(false) + expect(body).to receive(:length).and_return(nil) + expect(body).to receive(:close) + + # A request body cannot be delimited by closing the connection, so nothing should be written: + expect do + server.write_body("HTTP/1.0", body, false, nil, true) + end.to raise_exception(Protocol::HTTP1::ProtocolError, message: be =~ /unknown length/) + + expect(server).to be(:open?) + server.close + expect(client.stream.read).to be == "" + end + + it "can write chunked body for a request" do + expect(body).to receive(:empty?).and_return(false) + expect(body).to receive(:length).and_return(nil) + + expect(server).to receive(:write_chunked_body) + server.write_body("HTTP/1.1", body, false, nil, true) + end end with "bad requests" do @@ -739,6 +763,58 @@ expect(client).to be(:closed?) end + with "HTTP/1.0 request body" do + it "can write a request body of known length" do + client.write_request("localhost", "POST", "/", "HTTP/1.0", {"connection" => "keep-alive"}) + + body = Protocol::HTTP::Body::Buffered.new(["Hello World"]) + client.write_body("HTTP/1.0", body, false, nil, true) + expect(client).to be(:half_closed_local?) + + authority, method, path, version, headers, body = server.read_request + expect(method).to be == "POST" + expect(body).to be_a(Protocol::HTTP1::Body::Fixed) + expect(body.join).to be == "Hello World" + expect(server).to be(:persistent) + end + + it "can write an empty request body of unknown length" do + client.write_request("localhost", "POST", "/", "HTTP/1.0", {}) + + body = Protocol::HTTP::Body::Writable.new + body.close_write + + client.write_body("HTTP/1.0", body, false, nil, true) + expect(client).to be(:half_closed_local?) + + authority, method, path, version, headers, body = server.read_request + expect(body).to be_nil + expect(server).to be(:persistent) + end + + it "refuses to write a request body of unknown length" do + client.write_request("localhost", "POST", "/", "HTTP/1.0", {"connection" => "keep-alive"}) + + # Without a content length, the server would treat these bytes as the start of the next request: + body = Protocol::HTTP::Body::Writable.new + body.write "GET /other HTTP/1.1\r\nhost: localhost\r\n\r\n" + body.close_write + + expect do + client.write_body("HTTP/1.0", body, false, nil, true) + end.to raise_exception(Protocol::HTTP1::ProtocolError, message: be =~ /unknown length/) + + expect(body).to be(:closed?) + + # The connection is still open, so the caller is responsible for closing it: + expect(client).to be(:open?) + client.close + + # Only the request head was written; none of the body bytes reached the server: + expect(server.stream.read).to be == "POST / HTTP/1.0\r\nhost: localhost\r\nconnection: keep-alive\r\n" + end + end + it "raises RefusedError when the stream is broken" do client.stream.close