From feb73251a549a68172e88972bf91a32ce49b4051 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Wed, 28 Aug 2024 20:55:33 +1200 Subject: [PATCH 1/2] Return body on upgrade. --- lib/protocol/http1/connection.rb | 5 +++++ test/protocol/http1/upgrade.rb | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/protocol/http1/connection.rb b/lib/protocol/http1/connection.rb index 814b1c9..c814d4b 100644 --- a/lib/protocol/http1/connection.rb +++ b/lib/protocol/http1/connection.rb @@ -503,6 +503,11 @@ def read_request_body(method, headers) return read_tunnel_body end + # A successful upgrade response implies that the connection will become a tunnel immediately after the empty line that concludes the header fields. + if headers[UPGRADE] + return read_tunnel_body + end + # 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) diff --git a/test/protocol/http1/upgrade.rb b/test/protocol/http1/upgrade.rb index 8c34a7c..0d9ec8b 100644 --- a/test/protocol/http1/upgrade.rb +++ b/test/protocol/http1/upgrade.rb @@ -24,7 +24,7 @@ expect(version).to be == request_version expect(headers['upgrade']).to be == [protocol] - expect(body).to be_nil + expect(body).to be_a(Protocol::HTTP1::Body::Remainder) stream = server.hijack! expect(stream.read).to be == "Hello World" From 9dda3704205403f7647690d7cd1e76fb937553ef Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Fri, 30 Aug 2024 13:29:46 +1200 Subject: [PATCH 2/2] Explicit handling of upgrade body. --- lib/protocol/http1/connection.rb | 17 ++++++++++------- test/protocol/http1/connection.rb | 4 ++-- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/lib/protocol/http1/connection.rb b/lib/protocol/http1/connection.rb index c814d4b..183356a 100644 --- a/lib/protocol/http1/connection.rb +++ b/lib/protocol/http1/connection.rb @@ -435,6 +435,12 @@ def read_tunnel_body read_remainder_body end + def read_upgrade_body + # When you have an incoming upgrade request body, we must be extremely careful not to start reading it until the upgrade has been confirmed, otherwise if the upgrade was rejected and we started forwarding the incoming request body, it would desynchronize the connection (potential security issue). + # We mitigate this issue by setting @persistent to false, which will prevent the connection from being reused, even if the upgrade fails (potential performance issue). + read_remainder_body + end + HEAD = "HEAD" CONNECT = "CONNECT" @@ -470,14 +476,11 @@ def read_response_body(method, status, headers) return nil end - if status >= 100 and status < 200 - # At the moment this is returned, the Remainder represents any - # future response on the stream. The Remainder may be used directly - # or discarded, or read_response may be called again. - return read_remainder_body + if status == 101 + return read_upgrade_body end - if status == 204 or status == 304 + if (status >= 100 and status < 200) or status == 204 or status == 304 return nil end @@ -505,7 +508,7 @@ def read_request_body(method, headers) # A successful upgrade response implies that the connection will become a tunnel immediately after the empty line that concludes the header fields. if headers[UPGRADE] - return read_tunnel_body + return read_upgrade_body end # 6. If this is a request message and none of the above are true, then diff --git a/test/protocol/http1/connection.rb b/test/protocol/http1/connection.rb index 732143e..c7c851c 100644 --- a/test/protocol/http1/connection.rb +++ b/test/protocol/http1/connection.rb @@ -205,8 +205,8 @@ with "GET" do it "should ignore body for informational responses" do body = client.read_response_body("GET", 100, {'content-length' => '10'}) - expect(body).to be_a(::Protocol::HTTP1::Body::Remainder) - expect(client.persistent).to be == false + expect(body).to be_nil + expect(client.persistent).to be == true end it "should ignore body for no content responses" do