Skip to content

Commit

Permalink
Improve request upgrade body handling. (#35)
Browse files Browse the repository at this point in the history
  • Loading branch information
ioquatix authored Sep 2, 2024
1 parent 7863730 commit 55c516a
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 9 deletions.
20 changes: 14 additions & 6 deletions lib/protocol/http1/connection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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

Expand All @@ -503,6 +506,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_upgrade_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)
Expand Down
4 changes: 2 additions & 2 deletions test/protocol/http1/connection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion test/protocol/http1/upgrade.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down

0 comments on commit 55c516a

Please sign in to comment.