diff --git a/lib/protocol/http1/body/chunked.rb b/lib/protocol/http1/body/chunked.rb index 94b8604..2d9a9d4 100644 --- a/lib/protocol/http1/body/chunked.rb +++ b/lib/protocol/http1/body/chunked.rb @@ -15,6 +15,9 @@ module Body class Chunked < HTTP::Body::Readable CRLF = "\r\n" + # The maximum amount of body data returned by a single read. + BLOCK_SIZE = 1024 * 64 + # Initialize the chunked body. # # @parameter connection [Protocol::HTTP1::Connection] the connection to read the body from. @@ -27,6 +30,7 @@ def initialize(connection, headers) @length = 0 @count = 0 + @remaining = nil end # @attribute [Integer] the number of chunks read so far. @@ -69,15 +73,39 @@ def close(error = nil) # @returns [String | Nil] the next chunk of data, or `nil` if the body is finished. # @raises [EOFError] if the connection is closed before the expected length is read. def read - if !@finished - if @connection + while !@finished + unless @connection + raise EOFError, "connection closed before expected length was read!" + end + + if @remaining + if @remaining > 0 + chunk = @connection.readpartial([@remaining, BLOCK_SIZE].min) + @remaining -= chunk.bytesize + @length += chunk.bytesize + + return chunk + end + + terminator = @connection.read(CRLF.bytesize) + + unless terminator&.bytesize == CRLF.bytesize + raise EOFError, "connection closed before expected length was read!" + end + + unless terminator == CRLF + raise BadRequest, "Invalid chunk terminator: #{terminator.inspect}" + end + + @remaining = nil + @count += 1 + else length, _extensions = @connection.read_line.split(";", 2) unless length =~ VALID_CHUNK_LENGTH raise BadRequest, "Invalid chunk length: #{length.inspect}" end - # It is possible this line contains chunk extension, so we use `to_i` to only consider the initial integral part: length = Integer(length, 16) if length == 0 @@ -91,27 +119,16 @@ def read return nil end - # Read trailing CRLF: - chunk = @connection.read(length + 2) - - if chunk.bytesize == length + 2 - # ...and chomp it off: - chunk.chomp!(CRLF) - - @length += length - @count += 1 - - return chunk - else - # The connection has been closed before we have read the requested length: - @connection.close_read - @connection = nil - end + @remaining = length end - - # If the connection has been closed before we have read the final chunk, raise an error: - raise EOFError, "connection closed before expected length was read!" end + rescue EOFError + if connection = @connection + @connection = nil + connection.close_read + end + + raise EOFError, "connection closed before expected length was read!" end # @returns [String] a human-readable representation of the body. diff --git a/lib/protocol/http1/body/fixed.rb b/lib/protocol/http1/body/fixed.rb index 732b143..1e56df9 100644 --- a/lib/protocol/http1/body/fixed.rb +++ b/lib/protocol/http1/body/fixed.rb @@ -10,6 +10,9 @@ module HTTP1 module Body # Represents a fixed length body. class Fixed < HTTP::Body::Readable + # The maximum amount of body data returned by a single read. + BLOCK_SIZE = 1024 * 64 + # Initialize the body with the given connection and length. # # @parameter connection [Protocol::HTTP1::Connection] the connection to read the body from. @@ -55,7 +58,7 @@ def read if @remaining > 0 if @connection # `readpartial` will raise `EOFError` if the connection is finished, or `IOError` if the connection is closed. - chunk = @connection.readpartial(@remaining) + chunk = @connection.readpartial([@remaining, BLOCK_SIZE].min) @remaining -= chunk.bytesize diff --git a/releases.md b/releases.md index 1b644c0..b6b07d0 100644 --- a/releases.md +++ b/releases.md @@ -1,5 +1,9 @@ # Releases +## Unreleased + + - Bound fixed-length and chunked body reads, and validate chunk terminators. + ## v0.40.0 - Use `Protocol::HTTP::Status` for standard HTTP status descriptions and remove the duplicate `Protocol::HTTP1::Reason` table. diff --git a/test/protocol/http1/body/chunked.rb b/test/protocol/http1/body/chunked.rb index ce49372..03e4e7f 100644 --- a/test/protocol/http1/body/chunked.rb +++ b/test/protocol/http1/body/chunked.rb @@ -94,6 +94,24 @@ expect(connection).to be(:half_closed_remote?) end + it "bounds the amount of data returned" do + content = "x" * (subject::BLOCK_SIZE + 1) + buffer = StringIO.new("#{content.bytesize.to_s(16)}\r\n#{content}\r\n0\r\n\r\n") + body = subject.new(Protocol::HTTP1::Connection.new(buffer, state: :open), headers) + + expect(body.read.bytesize).to be == subject::BLOCK_SIZE + expect(body.read.bytesize).to be == 1 + expect(body.read).to be_nil + end + + it "rejects an invalid chunk terminator" do + buffer = StringIO.new("#{content.bytesize.to_s(16)}\r\n#{content}XX") + body = subject.new(Protocol::HTTP1::Connection.new(buffer, state: :open), headers) + + expect(body.read).to be == content + expect{body.read}.to raise_exception(Protocol::HTTP1::BadRequest) + end + with "trailer" do let(:postfix) {"ETag: abcd\r\n"} @@ -131,6 +149,7 @@ let(:buffer) {StringIO.new("#{(content.bytesize + 1).to_s(16)}\r\n#{content}")} it "raises error" do + expect(body.read).to be == content expect{body.read}.to raise_exception(EOFError) expect(connection).to be(:half_closed_remote?) diff --git a/test/protocol/http1/body/fixed.rb b/test/protocol/http1/body/fixed.rb index 272502e..b2f2aa9 100644 --- a/test/protocol/http1/body/fixed.rb +++ b/test/protocol/http1/body/fixed.rb @@ -85,6 +85,15 @@ expect(body).to be(:empty?) end + it "bounds the amount of data returned" do + content = "x" * (subject::BLOCK_SIZE + 1) + body = subject.new(Protocol::HTTP1::Connection.new(StringIO.new(content), state: :open), content.bytesize) + + expect(body.read.bytesize).to be == subject::BLOCK_SIZE + expect(body.read.bytesize).to be == 1 + expect(body.read).to be_nil + end + with "length smaller than stream size" do let(:body) {subject.new(connection, 5)}