Skip to content

Commit 3d4aea0

Browse files
authored
Ignore bad requests. (#216)
1 parent f826140 commit 3d4aea0

8 files changed

Lines changed: 139 additions & 9 deletions

File tree

‎async-http.gemspec‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ Gem::Specification.new do |spec|
3030
spec.add_dependency "async-pool", "~> 0.12"
3131
spec.add_dependency "io-endpoint", "~> 0.18"
3232
spec.add_dependency "io-stream", "~> 0.14"
33-
spec.add_dependency "protocol-http", "~> 0.66"
33+
spec.add_dependency "protocol-http", "~> 0.72"
3434
spec.add_dependency "protocol-http1", "~> 0.41"
3535
spec.add_dependency "protocol-http2", "~> 0.28"
3636
spec.add_dependency "protocol-url", "~> 0.2"

‎fixtures/async/http/a_protocol.rb‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,28 @@ module HTTP
4444
end
4545
end
4646

47+
with "bad requests" do
48+
let(:app) do
49+
::Protocol::HTTP::Middleware.for do
50+
@app_called = true
51+
52+
::Protocol::HTTP::Response[200]
53+
end
54+
end
55+
56+
it "responds consistently without invoking the application" do
57+
@app_called = false
58+
response = client.get("/", [["range", "bytes=4-1"]])
59+
60+
expect(response.status).to be == 400
61+
expect(response.headers["content-type"]).to be == "text/plain; charset=utf-8"
62+
expect(response.read).to be == "Protocol::HTTP::Header::Range::ParseError"
63+
expect(@app_called).to be == false
64+
ensure
65+
response&.close
66+
end
67+
end
68+
4769
with "interim response" do
4870
let(:app) do
4971
::Protocol::HTTP::Middleware.for do |request|

‎lib/async/http/protocol/http1/server.rb‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212

1313
require "async/promise"
1414
require "console/event/failure"
15+
require "protocol/http/body/buffered"
1516

1617
module Async
1718
module HTTP
@@ -33,12 +34,15 @@ def closed(error = nil)
3334
@ready.resolve(nil)
3435
end
3536

36-
# Write a failure response with the given status code.
37+
# Write a failure response with the given status code and error class name.
3738
# @parameter status [Integer] The HTTP status code to send.
38-
def fail_request(status)
39+
# @parameter error [Exception] The error which caused the request to fail.
40+
def fail_request(status, error)
41+
body = ::Protocol::HTTP::Body::Buffered.wrap(error.class.name)
42+
3943
@persistent = false
40-
write_response(@version, status, {})
41-
write_body(@version, nil)
44+
write_response(@version, status, {"content-type" => "text/plain; charset=utf-8"})
45+
write_body(@version, body)
4246
rescue => error
4347
# At this point, there is very little we can do to recover:
4448
Console.debug(self, "Failed to write failure response!", error)
@@ -61,8 +65,8 @@ def next_request
6165
end
6266

6367
return request
64-
rescue ::Protocol::HTTP1::BadRequest
65-
fail_request(400)
68+
rescue ::Protocol::HTTP::BadRequest => error
69+
fail_request(400, error)
6670
# Conceivably we could retry here, but we don't really know how bad the error is, so it's better to just fail:
6771
raise
6872
end

‎lib/async/http/protocol/http2/request.rb‎

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,29 @@ def initialize(*)
2424

2525
attr :request
2626

27+
# Write a failure response with the given status code and error class name.
28+
# @parameter status [Integer] The HTTP status code to send.
29+
# @parameter error [Exception] The error which caused the request to fail.
30+
def fail_request(status, error)
31+
body = error.class.name
32+
headers = [
33+
[STATUS, status.to_s],
34+
["content-type", "text/plain; charset=utf-8"],
35+
]
36+
37+
if @request.head?
38+
send_headers(headers, ::Protocol::HTTP2::END_STREAM)
39+
else
40+
send_headers(headers)
41+
send_data(body, ::Protocol::HTTP2::END_STREAM)
42+
end
43+
44+
# The peer may still be sending a request body. The response is complete, so release the stream without reporting a protocol error.
45+
send_reset_stream(::Protocol::HTTP2::Error::NO_ERROR) unless closed?
46+
rescue => error
47+
Console.debug(self, "Failed to write failure response!", error)
48+
end
49+
2750
# Process the initial headers received from the client and construct the request.
2851
# @parameter headers [Array] The list of header key-value pairs.
2952
# @parameter end_stream [Boolean] Whether the stream is complete after these headers.
@@ -67,11 +90,13 @@ def receive_initial_headers(headers, end_stream)
6790
end
6891
end
6992

70-
@request.headers = @headers
71-
7293
unless @request.valid?
7394
raise ::Protocol::HTTP2::HeaderError, "Request is missing required headers!"
7495
else
96+
# Validate structured headers before exposing the request to the application:
97+
@headers.to_h
98+
@request.headers = @headers
99+
75100
# We only construct the input/body if data is coming.
76101
unless end_stream
77102
@request.body = prepare_input(@length)
@@ -82,6 +107,8 @@ def receive_initial_headers(headers, end_stream)
82107
end
83108

84109
return headers
110+
rescue ::Protocol::HTTP::BadRequest => error
111+
fail_request(400, error)
85112
end
86113

87114
# Called when the stream is closed.

‎lib/async/http/server.rb‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,8 @@ def accept(peer, address, task: Task.current)
7070
# If this returns nil, we assume that the connection has been hijacked.
7171
self.call(request)
7272
end
73+
rescue ::Protocol::HTTP::BadRequest
74+
# Ignore bad requests, just close the connection.
7375
ensure
7476
connection&.close
7577
end

‎releases.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
# Releases
22

3+
## Unreleased
4+
5+
- Respond with `400 Bad Request` for any `Protocol::HTTP::BadRequest` raised while parsing HTTP/1 or HTTP/2 requests, include the exception class name without reflecting request data, and avoid reporting them as unhandled server errors.
6+
37
## v0.103.0
48

59
- Handle `RST_STREAM(NO_ERROR)` as an orderly HTTP/2 stream closure while still failing requests whose streams close before any response headers are received.

‎test/async/http/protocol/http11.rb‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,9 @@ def around
102102
response = client.get("")
103103

104104
expect(response.status).to be == 400
105+
expect(response.read).to be == "Protocol::HTTP1::InvalidRequest"
105106
end
107+
106108
end
107109

108110
with "head request" do

‎test/async/http/protocol/http2.rb‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,75 @@
4343
client.post("/", [[":authority", "foo"]])
4444
end.to raise_exception(Protocol::HTTP2::StreamError)
4545
end
46+
47+
with "a bad request header" do
48+
let(:app) do
49+
Protocol::HTTP::Middleware.for do
50+
@app_called = true
51+
Protocol::HTTP::Response[200]
52+
end
53+
end
54+
55+
it "keeps the connection reusable" do
56+
@app_called = false
57+
response = client.get("/", [["range", "bytes=4-1"]])
58+
59+
expect(response.status).to be == 400
60+
expect(@app_called).to be == false
61+
62+
response.finish
63+
response = client.get("/")
64+
65+
expect(response.status).to be == 200
66+
expect(@app_called).to be == true
67+
ensure
68+
response&.close
69+
end
70+
71+
it "does not send response data for HEAD" do
72+
response = client.head("/", [["range", "bytes=4-1"]])
73+
74+
expect(response.status).to be == 400
75+
expect(response.body.length).to be_nil
76+
expect(response.read).to be_nil
77+
ensure
78+
response&.close
79+
end
80+
81+
it "terminates an unfinished request body" do
82+
body = Async::HTTP::Body::Writable.new
83+
response = client.post("/", [["range", "bytes=4-1"]], body)
84+
connection = response.connection
85+
86+
expect(response.status).to be == 400
87+
expect(response.read).to be == "Protocol::HTTP::Header::Range::ParseError"
88+
expect(response.stream).to be(:closed?)
89+
expect(connection.streams).to be(:empty?)
90+
expect(connection).to be(:reusable?)
91+
ensure
92+
body&.close
93+
response&.close
94+
end
95+
end
96+
97+
with "a malformed request" do
98+
it "prioritizes protocol validation over bad request handling" do
99+
client.pool.acquire do |connection|
100+
response = connection.create_response
101+
response.stream.send_headers([
102+
[":method", "GET"],
103+
[":path", "/"],
104+
["range", "bytes=4-1"],
105+
], ::Protocol::HTTP2::END_STREAM)
106+
107+
expect do
108+
connection.read_response(response)
109+
end.to raise_exception(Protocol::HTTP2::StreamError).and(
110+
have_attributes(code: be == Protocol::HTTP2::Error::STREAM_CLOSED)
111+
)
112+
end
113+
end
114+
end
46115
end
47116

48117
with "closed streams" do

0 commit comments

Comments
 (0)