diff --git a/lib/protocol/http1/connection.rb b/lib/protocol/http1/connection.rb index 61ce0cd..596a39d 100644 --- a/lib/protocol/http1/connection.rb +++ b/lib/protocol/http1/connection.rb @@ -349,18 +349,25 @@ def read(length) # Read a line from the connection. # - # @returns [String | Nil] the line read, or nil if the connection is closed. - # @raises [EOFError] if the connection is closed. + # @returns [String | Nil] the line read, or nil if the connection is gracefully closed. # @raises [LineLengthError] if the line is too long. + # @raises [ProtocolError] if the line is not terminated properly. def read_line? if line = @stream.gets(CRLF, @maximum_line_length) unless line.chomp!(CRLF) - # This basically means that the request line, response line, header, or chunked length line is too long. - raise LineLengthError, "Line too long!" + if line.bytesize == @maximum_line_length + # This basically means that the request line, response line, header, or chunked length line is too long: + raise LineLengthError, "Line too long!" + else + # This means the line was not terminated properly, which is a protocol violation: + raise ProtocolError, "Line not terminated properly!" + end end end return line + + # I considered rescuing Errno::ECONNRESET here, but it seems like that would be ignoring a potentially serious error. end # Read a line from the connection. diff --git a/releases.md b/releases.md index 85951e8..ac83d5a 100644 --- a/releases.md +++ b/releases.md @@ -1,5 +1,9 @@ # Releases +## Unreleased + + - Tidy up implementation of `read_line?` to handle line length errors and protocol violations more clearly. + ## v0.35.0 - Add traces provider for `Protocol::HTTP1::Connection`. diff --git a/test/protocol/http1/connection.rb b/test/protocol/http1/connection.rb index 12dde1a..f6fa283 100644 --- a/test/protocol/http1/connection.rb +++ b/test/protocol/http1/connection.rb @@ -15,6 +15,41 @@ describe Protocol::HTTP1::Connection do include_context ConnectionContext + with "#read_line?" do + it "reads a line from the stream" do + client.stream.write "GET / HTTP/1.1\r\nHost: localhost\r\n\r\n" + client.stream.close + + expect(server.read_line?).to be == "GET / HTTP/1.1" + expect(server.read_line?).to be == "Host: localhost" + expect(server.read_line?).to be == "" + expect(server.read_line?).to be_nil + end + + it "raises LineLengthError if line is too long" do + # We create a thread since the write is liable to block until we try to read: + thread = Thread.new do + client.stream.write "GET / HTTP/1.#{"1" * 10000}" + client.stream.close + end + + expect do + server.read_line? + end.to raise_exception(Protocol::HTTP1::LineLengthError) + ensure + thread.join + end + + it "raises ProtocolError if line is not terminated properly" do + client.stream.write "GET / HTTP/1.1\r" + client.stream.close + + expect do + server.read_line? + end.to raise_exception(Protocol::HTTP1::ProtocolError) + end + end + with "#read_request" do it "reads request without body" do client.stream.write "GET / HTTP/1.1\r\nHost: localhost\r\nContent-Length: 0\r\n\r\n"