RFC Conformance: Enforce HTTP/2 trailer field section requirements

lib/bandit/http2/stream.ex

@@ -219,15 +219,25 @@ defmodule Bandit.HTTP2.Stream do
     # specific cases by RFC9113§8.2.2. We check those cases in a separate filter
     defp no_connection_headers!(headers, stream) do
       connection_headers =
-        ~w[connection keep-alive proxy-authenticate proxy-authorization proxy-connection trailers transfer-encoding upgrade]
+        ~w[connection keep-alive proxy-connection transfer-encoding upgrade]
 
       if Enum.any?(headers, fn {key, _value} -> key in connection_headers end),
         do: stream_error!("Received connection-specific header", stream)
     end
 
     # RFC9113§8.2.2 - TE header may be present if it contains exactly 'trailers'
     defp valid_te_header!(headers, stream) do
-      if Bandit.Headers.get_header(headers, "te") not in [nil, "trailers"],
+      invalid? =
+        headers
+        |> Enum.filter(fn {name, _value} -> name == "te" end)
+        |> Enum.any?(fn {"te", value} ->
+          case Plug.Conn.Utils.list(value) do
+            [] -> true
+            members -> Enum.any?(members, &(String.downcase(&1, :ascii) != "trailers"))
+          end
+        end)
+
+      if invalid?,
         do: stream_error!("Received invalid TE header", stream)
     end
 
@@ -266,7 +276,6 @@ defmodule Bandit.HTTP2.Stream do
          when state in [:open, :local_closed] do
       case do_recv(stream, timeout) do
         {:headers, trailers, stream} ->
-          no_pseudo_headers!(trailers, stream)
           Logger.warning("Ignoring trailers #{inspect(trailers)}", domain: [:bandit])
           do_read_data(stream, max_bytes, timeout, acc)
 
@@ -297,6 +306,56 @@ defmodule Bandit.HTTP2.Stream do
         do: stream_error!("Received trailers with pseudo headers", stream)
     end
 
+    defp validate_trailers!(_headers, false, stream) do
+      stream_error!("Received trailers without END_STREAM", stream)
+    end
+
+    # RFC9113§8.1 - a trailer field section terminates the stream and is subject
+    # to the same field-name and field-value requirements as other field sections
+    defp validate_trailers!(headers, true, stream) do
+      # HPAX decodes an indexed static-table entry with no value to a nil value; treat it
+      # as the empty binary it represents (elixir-mint/hpax#27)
+      headers =
+        Enum.map(headers, fn
+          {name, nil} -> {name, ""}
+          header -> header
+        end)
+
+      no_pseudo_headers!(headers, stream)
+      headers_all_lowercase!(headers, stream)
+      valid_trailer_field_names!(headers, stream)
+      no_connection_headers!(headers, stream)
+      valid_te_header!(headers, stream)
+      valid_field_values!(headers, stream)
+      valid_trailer_edge_whitespace!(headers, stream)
+    end
+
+    defp valid_trailer_field_names!(headers, stream) do
+      if Enum.any?(headers, fn {key, _value} -> not valid_trailer_field_name?(key) end),
+        do: stream_error!("Received invalid trailer field name (RFC9113§8.2.1)", stream)
+    end
+
+    defp valid_trailer_field_name?(<<>>), do: false
+    defp valid_trailer_field_name?(name), do: valid_trailer_field_name_bytes?(name)
+
+    defp valid_trailer_field_name_bytes?(<<char, rest::binary>>)
+         when char in ?a..?z or char in ?0..?9 or char in ~c"!#$%&'*+-.^_`|~",
+         do: valid_trailer_field_name_bytes?(rest)
+
+    defp valid_trailer_field_name_bytes?(<<_char, _rest::binary>>), do: false
+    defp valid_trailer_field_name_bytes?(<<>>), do: true
+
+    defp valid_trailer_edge_whitespace!(headers, stream) do
+      if Enum.any?(headers, fn
+           {_key, <<>>} ->
+             false
+
+           {_key, value} ->
+             :binary.first(value) in [0x09, 0x20] or :binary.last(value) in [0x09, 0x20]
+         end),
+         do: stream_error!("Field value contains invalid characters (RFC9113§8.2.1)", stream)
+    end
+
     defp do_recv(%@for{state: :idle} = stream, timeout) do
       receive do
         {:bandit, {:headers, headers, end_stream}} ->
@@ -319,6 +378,7 @@ defmodule Bandit.HTTP2.Stream do
          when state in [:open, :local_closed] do
       receive do
         {:bandit, {:headers, headers, end_stream}} ->
+          validate_trailers!(headers, end_stream, stream)
           {:headers, headers, stream |> do_recv_headers() |> do_recv_end_stream(end_stream)}
 
         {:bandit, {:data, data, end_stream}} ->
@@ -560,8 +620,9 @@ defmodule Bandit.HTTP2.Stream do
 
     def ensure_completed(%@for{state: :local_closed} = stream) do
       receive do
-        {:bandit, {:headers, _headers, true}} ->
-          do_recv_end_stream(stream, true)
+        {:bandit, {:headers, headers, end_stream}} ->
+          validate_trailers!(headers, end_stream, stream)
+          do_recv_end_stream(stream, end_stream)
 
         {:bandit, {:data, data, true}} ->
           do_recv_data(stream, data, true) |> do_recv_end_stream(true)

test/bandit/http2/protocol_test.exs

@@ -2073,7 +2073,9 @@ defmodule HTTP2ProtocolTest do
 
       {:ok, ctx} = SimpleH2Client.send_simple_headers(socket, 1, :post, "/echo", context.port)
       SimpleH2Client.send_body(socket, 1, false, "OK")
-      SimpleH2Client.send_headers(socket, 1, true, [{"x-trailer", "trailer"}], ctx)
+      # An extension field named "trailers" is not one of RFC9113§8.2.2's
+      # connection-specific fields and must not be rejected merely by name.
+      SimpleH2Client.send_headers(socket, 1, true, [{"trailers", "extension-value"}], ctx)
 
       {:ok, 0, _} = SimpleH2Client.recv_window_update(socket)
       {:ok, 1, _} = SimpleH2Client.recv_window_update(socket)
@@ -2084,6 +2086,136 @@ defmodule HTTP2ProtocolTest do
       assert SimpleH2Client.connection_alive?(socket)
     end
 
+    @tag :capture_log
+    test "accepts a trailer field encoded as an indexed entry with no value", context do
+      socket = SimpleH2Client.setup_connection(context)
+
+      {:ok, _ctx} = SimpleH2Client.send_simple_headers(socket, 1, :post, "/echo", context.port)
+      SimpleH2Client.send_body(socket, 1, false, "OK")
+
+      # Static-table index 58 (`user-agent`) as an indexed field, which HPAX decodes with
+      # a nil value (elixir-mint/hpax#27); as a trailer it must validate as an empty value
+      SimpleH2Client.send_frame(socket, 1, 0x05, 1, <<0xBA>>)
+
+      {:ok, 0, _} = SimpleH2Client.recv_window_update(socket)
+      {:ok, 1, _} = SimpleH2Client.recv_window_update(socket)
+
+      assert SimpleH2Client.successful_response?(socket, 1, false)
+      assert SimpleH2Client.recv_body(socket) == {:ok, 1, true, "OK"}
+
+      assert SimpleH2Client.connection_alive?(socket)
+    end
+
+    @tag :capture_log
+    test "rejects trailer HEADERS without END_STREAM", context do
+      socket = SimpleH2Client.setup_connection(context)
+
+      {:ok, ctx} = SimpleH2Client.send_simple_headers(socket, 1, :post, "/echo", context.port)
+      SimpleH2Client.send_body(socket, 1, false, "OK")
+
+      {:ok, 0, _} = SimpleH2Client.recv_window_update(socket)
+      {:ok, 1, _} = SimpleH2Client.recv_window_update(socket)
+
+      SimpleH2Client.send_headers(socket, 1, false, [{"x-trailer", "trailer"}], ctx)
+
+      assert SimpleH2Client.recv_rst_stream(socket) == {:ok, 1, 1}
+      assert SimpleH2Client.connection_alive?(socket)
+
+      assert_receive {:log, %{level: :error, msg: {:string, msg}, meta: %{stream_id: 1}}}, 500
+      assert msg == "** (Bandit.HTTP2.Errors.StreamError) Received trailers without END_STREAM"
+    end
+
+    @tag :capture_log
+    test "validates connection-specific fields in trailers", context do
+      socket = SimpleH2Client.setup_connection(context)
+
+      {:ok, ctx} = SimpleH2Client.send_simple_headers(socket, 1, :post, "/echo", context.port)
+      SimpleH2Client.send_body(socket, 1, false, "OK")
+
+      {:ok, 0, _} = SimpleH2Client.recv_window_update(socket)
+      {:ok, 1, _} = SimpleH2Client.recv_window_update(socket)
+
+      SimpleH2Client.send_headers(socket, 1, true, [{"connection", "close"}], ctx)
+
+      assert SimpleH2Client.recv_rst_stream(socket) == {:ok, 1, 1}
+      assert SimpleH2Client.connection_alive?(socket)
+
+      assert_receive {:log, %{level: :error, msg: {:string, msg}, meta: %{stream_id: 1}}}, 500
+      assert msg == "** (Bandit.HTTP2.Errors.StreamError) Received connection-specific header"
+    end
+
+    @tag :capture_log
+    test "validates every TE field line in trailers", context do
+      socket = SimpleH2Client.setup_connection(context)
+
+      {:ok, ctx} = SimpleH2Client.send_simple_headers(socket, 1, :post, "/echo", context.port)
+      SimpleH2Client.send_body(socket, 1, false, "OK")
+
+      {:ok, 0, _} = SimpleH2Client.recv_window_update(socket)
+      {:ok, 1, _} = SimpleH2Client.recv_window_update(socket)
+
+      SimpleH2Client.send_headers(socket, 1, true, [{"te", "trailers"}, {"te", "gzip"}], ctx)
+
+      assert SimpleH2Client.recv_rst_stream(socket) == {:ok, 1, 1}
+      assert SimpleH2Client.connection_alive?(socket)
+
+      assert_receive {:log, %{level: :error, msg: {:string, msg}, meta: %{stream_id: 1}}}, 500
+      assert msg == "** (Bandit.HTTP2.Errors.StreamError) Received invalid TE header"
+    end
+
+    @tag :capture_log
+    test "validates field values in trailers", context do
+      socket = SimpleH2Client.setup_connection(context)
+
+      {:ok, ctx} = SimpleH2Client.send_simple_headers(socket, 1, :post, "/echo", context.port)
+      SimpleH2Client.send_body(socket, 1, false, "OK")
+
+      {:ok, 0, _} = SimpleH2Client.recv_window_update(socket)
+      {:ok, 1, _} = SimpleH2Client.recv_window_update(socket)
+
+      SimpleH2Client.send_headers(socket, 1, true, [{"x-trailer", "bad\rvalue"}], ctx)
+
+      assert SimpleH2Client.recv_rst_stream(socket) == {:ok, 1, 1}
+      assert SimpleH2Client.connection_alive?(socket)
+
+      assert_receive {:log, %{level: :error, msg: {:string, msg}, meta: %{stream_id: 1}}}, 500
+
+      assert msg ==
+               "** (Bandit.HTTP2.Errors.StreamError) Field value contains invalid characters (RFC9113§8.2.1)"
+    end
+
+    @tag :capture_log
+    test "validates regular field-name grammar in trailers", context do
+      socket = SimpleH2Client.setup_connection(context)
+
+      {:ok, ctx} = SimpleH2Client.send_simple_headers(socket, 1, :post, "/echo", context.port)
+      SimpleH2Client.send_body(socket, 1, false, "OK")
+
+      {:ok, 0, _} = SimpleH2Client.recv_window_update(socket)
+      {:ok, 1, _} = SimpleH2Client.recv_window_update(socket)
+
+      SimpleH2Client.send_headers(socket, 1, true, [{"bad/name", "value"}], ctx)
+
+      assert SimpleH2Client.recv_rst_stream(socket) == {:ok, 1, 1}
+      assert SimpleH2Client.connection_alive?(socket)
+    end
+
+    @tag :capture_log
+    test "rejects edge whitespace in trailer field values", context do
+      socket = SimpleH2Client.setup_connection(context)
+
+      {:ok, ctx} = SimpleH2Client.send_simple_headers(socket, 1, :post, "/echo", context.port)
+      SimpleH2Client.send_body(socket, 1, false, "OK")
+
+      {:ok, 0, _} = SimpleH2Client.recv_window_update(socket)
+      {:ok, 1, _} = SimpleH2Client.recv_window_update(socket)
+
+      SimpleH2Client.send_headers(socket, 1, true, [{"x-trailer", "trailing "}], ctx)
+
+      assert SimpleH2Client.recv_rst_stream(socket) == {:ok, 1, 1}
+      assert SimpleH2Client.connection_alive?(socket)
+    end
+
     @tag :capture_log
     test "rejects HEADER frames sent as trailers that contain pseudo headers", context do
       socket = SimpleH2Client.setup_connection(context)