diff --git a/async-http.gemspec b/async-http.gemspec index 2a698a2f..62ea379b 100644 --- a/async-http.gemspec +++ b/async-http.gemspec @@ -30,7 +30,7 @@ Gem::Specification.new do |spec| spec.add_dependency "async-pool", "~> 0.12" spec.add_dependency "io-endpoint", "~> 0.18" spec.add_dependency "io-stream", "~> 0.14" - spec.add_dependency "protocol-http", "~> 0.66" + spec.add_dependency "protocol-http", "~> 0.72" spec.add_dependency "protocol-http1", "~> 0.41" spec.add_dependency "protocol-http2", "~> 0.28" spec.add_dependency "protocol-url", "~> 0.2" diff --git a/fixtures/async/http/a_protocol.rb b/fixtures/async/http/a_protocol.rb index b14a4bc2..db261e6e 100644 --- a/fixtures/async/http/a_protocol.rb +++ b/fixtures/async/http/a_protocol.rb @@ -44,6 +44,28 @@ module HTTP end end + with "bad requests" do + let(:app) do + ::Protocol::HTTP::Middleware.for do + @app_called = true + + ::Protocol::HTTP::Response[200] + end + end + + it "responds consistently without invoking the application" do + @app_called = false + response = client.get("/", [["range", "bytes=4-1"]]) + + expect(response.status).to be == 400 + expect(response.headers["content-type"]).to be == "text/plain; charset=utf-8" + expect(response.read).to be == "Protocol::HTTP::Header::Range::ParseError" + expect(@app_called).to be == false + ensure + response&.close + end + end + with "interim response" do let(:app) do ::Protocol::HTTP::Middleware.for do |request| diff --git a/lib/async/http/protocol/http1/server.rb b/lib/async/http/protocol/http1/server.rb index 7e343f72..b4a438c1 100644 --- a/lib/async/http/protocol/http1/server.rb +++ b/lib/async/http/protocol/http1/server.rb @@ -12,6 +12,7 @@ require "async/promise" require "console/event/failure" +require "protocol/http/body/buffered" module Async module HTTP @@ -33,12 +34,15 @@ def closed(error = nil) @ready.resolve(nil) end - # Write a failure response with the given status code. + # Write a failure response with the given status code and error class name. # @parameter status [Integer] The HTTP status code to send. - def fail_request(status) + # @parameter error [Exception] The error which caused the request to fail. + def fail_request(status, error) + body = ::Protocol::HTTP::Body::Buffered.wrap(error.class.name) + @persistent = false - write_response(@version, status, {}) - write_body(@version, nil) + write_response(@version, status, {"content-type" => "text/plain; charset=utf-8"}) + write_body(@version, body) rescue => error # At this point, there is very little we can do to recover: Console.debug(self, "Failed to write failure response!", error) @@ -61,8 +65,8 @@ def next_request end return request - rescue ::Protocol::HTTP1::BadRequest - fail_request(400) + rescue ::Protocol::HTTP::BadRequest => error + fail_request(400, error) # Conceivably we could retry here, but we don't really know how bad the error is, so it's better to just fail: raise end diff --git a/lib/async/http/protocol/http2/request.rb b/lib/async/http/protocol/http2/request.rb index 86bd7c6f..05b9fd06 100644 --- a/lib/async/http/protocol/http2/request.rb +++ b/lib/async/http/protocol/http2/request.rb @@ -24,6 +24,29 @@ def initialize(*) attr :request + # Write a failure response with the given status code and error class name. + # @parameter status [Integer] The HTTP status code to send. + # @parameter error [Exception] The error which caused the request to fail. + def fail_request(status, error) + body = error.class.name + headers = [ + [STATUS, status.to_s], + ["content-type", "text/plain; charset=utf-8"], + ] + + if @request.head? + send_headers(headers, ::Protocol::HTTP2::END_STREAM) + else + send_headers(headers) + send_data(body, ::Protocol::HTTP2::END_STREAM) + end + + # The peer may still be sending a request body. The response is complete, so release the stream without reporting a protocol error. + send_reset_stream(::Protocol::HTTP2::Error::NO_ERROR) unless closed? + rescue => error + Console.debug(self, "Failed to write failure response!", error) + end + # Process the initial headers received from the client and construct the request. # @parameter headers [Array] The list of header key-value pairs. # @parameter end_stream [Boolean] Whether the stream is complete after these headers. @@ -67,11 +90,13 @@ def receive_initial_headers(headers, end_stream) end end - @request.headers = @headers - unless @request.valid? raise ::Protocol::HTTP2::HeaderError, "Request is missing required headers!" else + # Validate structured headers before exposing the request to the application: + @headers.to_h + @request.headers = @headers + # We only construct the input/body if data is coming. unless end_stream @request.body = prepare_input(@length) @@ -82,6 +107,8 @@ def receive_initial_headers(headers, end_stream) end return headers + rescue ::Protocol::HTTP::BadRequest => error + fail_request(400, error) end # Called when the stream is closed. diff --git a/lib/async/http/server.rb b/lib/async/http/server.rb index 47594f3f..12a44f07 100755 --- a/lib/async/http/server.rb +++ b/lib/async/http/server.rb @@ -70,6 +70,8 @@ def accept(peer, address, task: Task.current) # If this returns nil, we assume that the connection has been hijacked. self.call(request) end + rescue ::Protocol::HTTP::BadRequest + # Ignore bad requests, just close the connection. ensure connection&.close end diff --git a/releases.md b/releases.md index 25e3402c..114c82c0 100644 --- a/releases.md +++ b/releases.md @@ -1,5 +1,9 @@ # Releases +## Unreleased + + - 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. + ## v0.103.0 - 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. diff --git a/test/async/http/protocol/http11.rb b/test/async/http/protocol/http11.rb index 4aa0f231..a63ddc07 100755 --- a/test/async/http/protocol/http11.rb +++ b/test/async/http/protocol/http11.rb @@ -102,7 +102,9 @@ def around response = client.get("") expect(response.status).to be == 400 + expect(response.read).to be == "Protocol::HTTP1::InvalidRequest" end + end with "head request" do diff --git a/test/async/http/protocol/http2.rb b/test/async/http/protocol/http2.rb index 31b7cbc7..be6d4cf9 100644 --- a/test/async/http/protocol/http2.rb +++ b/test/async/http/protocol/http2.rb @@ -43,6 +43,75 @@ client.post("/", [[":authority", "foo"]]) end.to raise_exception(Protocol::HTTP2::StreamError) end + + with "a bad request header" do + let(:app) do + Protocol::HTTP::Middleware.for do + @app_called = true + Protocol::HTTP::Response[200] + end + end + + it "keeps the connection reusable" do + @app_called = false + response = client.get("/", [["range", "bytes=4-1"]]) + + expect(response.status).to be == 400 + expect(@app_called).to be == false + + response.finish + response = client.get("/") + + expect(response.status).to be == 200 + expect(@app_called).to be == true + ensure + response&.close + end + + it "does not send response data for HEAD" do + response = client.head("/", [["range", "bytes=4-1"]]) + + expect(response.status).to be == 400 + expect(response.body.length).to be_nil + expect(response.read).to be_nil + ensure + response&.close + end + + it "terminates an unfinished request body" do + body = Async::HTTP::Body::Writable.new + response = client.post("/", [["range", "bytes=4-1"]], body) + connection = response.connection + + expect(response.status).to be == 400 + expect(response.read).to be == "Protocol::HTTP::Header::Range::ParseError" + expect(response.stream).to be(:closed?) + expect(connection.streams).to be(:empty?) + expect(connection).to be(:reusable?) + ensure + body&.close + response&.close + end + end + + with "a malformed request" do + it "prioritizes protocol validation over bad request handling" do + client.pool.acquire do |connection| + response = connection.create_response + response.stream.send_headers([ + [":method", "GET"], + [":path", "/"], + ["range", "bytes=4-1"], + ], ::Protocol::HTTP2::END_STREAM) + + expect do + connection.read_response(response) + end.to raise_exception(Protocol::HTTP2::StreamError).and( + have_attributes(code: be == Protocol::HTTP2::Error::STREAM_CLOSED) + ) + end + end + end end with "closed streams" do