Fix chunked read_body(length: 0) falsely completing the body (#691)

Author: tomciopp

lib/bandit/http1/socket.ex

@@ -280,16 +280,26 @@ defmodule Bandit.HTTP1.Socket do
     # do_read_chunked_data! reads up to the configured length, reading multiple
     # chunks to do so. It accumulates data in the 'body' list, adding to it
     # chunk by (possibly partial) chunk until either the end of the body is reached
-    # or the configured length is exceeded
-    @dialyzer {:no_improper_lists, do_read_chunked_data!: 5}
+    # or the configured length is exceeded. Note that an exhausted read budget must
+    # be checked before attempting to read a chunk, since a zero-byte chunk read is
+    # otherwise indistinguishable from the terminal chunk
     defp do_read_chunked_data!(socket, buffer, body, body_length, opts) do
       max_to_read = Keyword.get(opts, :length, 8_000_000) - body_length
 
+      if max_to_read <= 0 do
+        {:more, body, buffer}
+      else
+        do_read_next_chunk!(socket, buffer, body, body_length, max_to_read, opts)
+      end
+    end
+
+    @dialyzer {:no_improper_lists, do_read_next_chunk!: 6}
+    defp do_read_next_chunk!(socket, buffer, body, body_length, max_to_read, opts) do
       case do_read_chunk!(socket, buffer, max_to_read, opts) do
-        {<<>>, rest} ->
+        {:done, rest} ->
           {:ok, body, rest}
 
-        {chunk, rest} ->
+        {:chunk, chunk, rest} ->
           length = IO.iodata_length(chunk)
 
           if length < max_to_read do
@@ -319,7 +329,7 @@ defmodule Bandit.HTTP1.Socket do
           if trailers != [],
             do: Logger.warning("Encountered trailers in chunked request; ignoring")
 
-          {<<>>, fake_socket.buffer}
+          {:done, fake_socket.buffer}
 
         {chunk_size, rest} ->
           to_read = min(chunk_size, max_to_read)
@@ -335,11 +345,11 @@ defmodule Bandit.HTTP1.Socket do
               if IO.iodata_to_binary(newline) != "\r\n",
                 do: request_error!("Malformed chunked encoding request body")
 
-              {to_return, rest}
+              {:chunk, to_return, rest}
 
             remaining when remaining > 0 ->
               # Build a binary since we'll be passing it to read_chunk_size! which expects a binary
-              {to_return, Integer.to_string(remaining, 16) <> "\r\n" <> rest}
+              {:chunk, to_return, Integer.to_string(remaining, 16) <> "\r\n" <> rest}
           end
       end
     end

test/bandit/http1/protocol_test.exs

@@ -1346,6 +1346,28 @@ defmodule HTTP1ProtocolTest do
       send_resp(conn, 200, "OK")
     end
 
+    test "handles a read_body call with length: 0 without falsely completing the body", context do
+      client = SimpleHTTP1Client.tcp_client(context)
+
+      SimpleHTTP1Client.send(client, "POST", "/zero_length_chunked_read", [
+        "Host: localhost",
+        "Transfer-encoding: chunked"
+      ])
+
+      Transport.send(client, "5\r\nhello\r\n0\r\n\r\n")
+      assert {:ok, "200 OK", _headers, "hello"} = SimpleHTTP1Client.recv_reply(client)
+
+      # Make sure the connection is still in sync by trying another request
+      SimpleHTTP1Client.send(client, "GET", "/echo_method", ["host: localhost"])
+      assert {:ok, "200 OK", _headers, "GET"} = SimpleHTTP1Client.recv_reply(client)
+    end
+
+    def zero_length_chunked_read(conn) do
+      {:more, "", conn} = Plug.Conn.read_body(conn, length: 0)
+      {:ok, body, conn} = Plug.Conn.read_body(conn)
+      send_resp(conn, 200, body)
+    end
+
     test "reads a chunked body properly", context do
       stream =
         Stream.repeatedly(fn -> String.duplicate("0123456789", 100_000) end)