Enforce the chunk-size grammar in a single pass (#661)

#631 validated the chunk-size line twice: a hand-rolled byte walk
(hex_digits?/1, all_hex_digits?/1) followed by Integer.parse/2. The
overlap between the two is total once the first check passes: RFC9112
section 7.1 defines chunk-size as 1*HEXDIG, and Integer.parse/2's only
divergence from that grammar is that it additionally accepts a single
leading '+' or '-'. A sign can only ever appear in the first byte, so
once hex_digits?/1 confirms every byte is a hex digit, Integer.parse/2
on that string cannot return anything but {n, ""} - the with clause's
second branch was unreachable as a failure path.

Collapse the validator into one function with two clauses: a guard on
the leading byte closes the sign gap directly, and the existing
{chunk_size, ""} match still rules out trailing junk. The accepted
language is unchanged.

Verified byte-for-byte equivalence between the old and new validators
across 41 handpicked adversarial inputs, 200,000 randomized strings
over an adversarial alphabet, and 500,000 raw random byte strings
(not restricted to valid UTF-8) - zero divergences. All four of #631's
regression tests pass unchanged. One new test pins a case that was an
explicit clause before (empty chunk-size) but is now implicit in the
pattern match: a chunk-ext with no preceding size digits.

Credit: this refactor was proposed in a review comment on #631 by
@mpettibone (#631 (comment)).
I independently re-verified the equivalence claims (including with a
separate implementation and additional raw-byte fuzzing) before
applying it here.

Author: mtrudel

lib/bandit/http1/socket.ex

@@ -358,36 +358,28 @@ defmodule Bandit.HTTP1.Socket do
       {parse_chunk_size!(chunk_size), rest}
     end
 
-    # RFC9112§7.1 defines chunk-size as 1*HEXDIG. Integer.parse/2 additionally
-    # accepts a leading '+' or '-', so the grammar has to be checked separately
-    # rather than inferred from the parse succeeding. Without this, '+10' reads
-    # as a sixteen byte chunk, and '-0' parses as zero and is taken for the
-    # terminating chunk, ending the body at a position no conforming parser
-    # would end it at. A negative size reaches read_exactly!/5, which is
-    # guarded on a non-negative length and so raises FunctionClauseError rather
-    # than returning a 400.
+    # RFC9112§7.1 defines chunk-size as 1*HEXDIG. Integer.parse/2 is close to that grammar but
+    # not equal to it: the only extra thing it will consume is a single leading '+' or '-'.
+    # Without ruling that out, '+10' reads as a sixteen byte chunk, and '-0' parses as zero and
+    # is taken for the terminating chunk, ending the body at a position no conforming parser
+    # would end it at. A negative size reaches read_exactly!/5, which is guarded on a
+    # non-negative length and so raises FunctionClauseError rather than returning a 400.
+    #
+    # A sign can only ever appear as the first byte, so anchoring the guard there closes the
+    # gap: once the first byte is a hex digit, the only remaining way Integer.parse/2 can
+    # disagree with the grammar is trailing junk (e.g. "5g"), which the {chunk_size, ""} match
+    # already rules out by requiring the parse to consume every byte. The empty string can't
+    # match the leading-byte pattern either, so it falls through to the catch-all clause.
     @spec parse_chunk_size!(binary()) :: non_neg_integer()
-    defp parse_chunk_size!(chunk_size) do
-      with true <- hex_digits?(chunk_size),
-           {chunk_size, ""} <- Integer.parse(chunk_size, 16) do
-        chunk_size
-      else
+    defp parse_chunk_size!(<<digit, _::binary>> = chunk_size)
+         when digit in ?0..?9 or digit in ?a..?f or digit in ?A..?F do
+      case Integer.parse(chunk_size, 16) do
+        {chunk_size, ""} -> chunk_size
         _ -> request_error!("Unable to parse chunk size")
       end
     end
 
-    @spec hex_digits?(binary()) :: boolean()
-    defp hex_digits?(<<>>), do: false
-    defp hex_digits?(chunk_size), do: all_hex_digits?(chunk_size)
-
-    @spec all_hex_digits?(binary()) :: boolean()
-    defp all_hex_digits?(<<>>), do: true
-
-    defp all_hex_digits?(<<digit, rest::binary>>)
-         when digit in ?0..?9 or digit in ?a..?f or digit in ?A..?F,
-         do: all_hex_digits?(rest)
-
-    defp all_hex_digits?(_chunk_size), do: false
+    defp parse_chunk_size!(_chunk_size), do: request_error!("Unable to parse chunk size")
 
     ##################
     # Internal Reading

test/bandit/http1/protocol_test.exs

@@ -1350,6 +1350,25 @@ defmodule HTTP1ProtocolTest do
       assert msg == "** (Bandit.HTTPError) Unable to parse chunk size"
     end
 
+    # RFC9112§7.1.1 chunk extensions come after the chunk-size, not in place of it. A line
+    # whose size portion is empty once extensions are split off (chunk-ext starting at the very
+    # first byte) is still required to satisfy chunk-size = 1*HEXDIG.
+    @tag :capture_log
+    test "rejects a chunk size that is empty once extensions are split off", context do
+      client = SimpleHTTP1Client.tcp_client(context)
+
+      SimpleHTTP1Client.send(client, "POST", "/expect_chunked_body", [
+        "Host: localhost",
+        "Transfer-encoding: chunked"
+      ])
+
+      Transport.send(client, ";ext=value\r\n")
+      assert SimpleHTTP1Client.recv_reply(client) ~> {:ok, "400 Bad Request", list(), ""}
+
+      assert_receive {:log, %{level: :error, msg: {:string, msg}}}, 500
+      assert msg == "** (Bandit.HTTPError) Unable to parse chunk size"
+    end
+
     # RFC9112§6.1 combines repeated transfer-encoding headers into one
     # comma-separated value; RFC9112§6.3 then requires rejecting the request,
     # since chunked is no longer the final encoding.