Reject invalid :method and :path in HTTP/2 requests

lib/bandit/headers.ex

@@ -23,6 +23,32 @@ defmodule Bandit.Headers do
 
   def field_value_valid?(_value), do: false
 
+  # RFC9110§5.6.2: tchar = "!" / "#" / "$" / "%" / "&" / "'" / "*" / "+" / "-" / "." /
+  # "^" / "_" / "`" / "|" / "~" / DIGIT / ALPHA
+  @spec is_tchar(integer()) :: Macro.t()
+  defguard is_tchar(char)
+           when char in ?a..?z or char in ?A..?Z or char in ?0..?9 or
+                  char in ~c"!#$%&'*+-.^_`|~"
+
+  # RFC9110§5.6.2: token = 1*tchar
+  @spec token?(binary()) :: boolean()
+  def token?(<<char>>) when is_tchar(char), do: true
+  def token?(<<char, rest::binary>>) when is_tchar(char), do: token?(rest)
+  def token?(_value), do: false
+
+  # RFC9112§3.2 - the request-target may not contain CTLs or whitespace. Shared by the HTTP/1
+  # request line and the HTTP/2 :path pseudo header. Non-ASCII bytes are accepted
+  @spec request_line_component_valid?(term()) :: boolean()
+  def request_line_component_valid?(<<>>), do: true
+
+  def request_line_component_valid?(<<char, rest::binary>>) when char > 0x20 and char != 0x7F,
+    do: request_line_component_valid?(rest)
+
+  def request_line_component_valid?(<<_char, _rest::binary>>), do: false
+
+  # Request line components which are not binaries (ports, the :* path) cannot contain bytes
+  def request_line_component_valid?(_component), do: true
+
   @spec get_header(Plug.Conn.headers(), header :: binary()) :: binary() | nil
   def get_header(headers, header) do
     case List.keyfind(headers, header, 0) do

lib/bandit/http1/socket.ex

@@ -49,15 +49,10 @@ defmodule Bandit.HTTP1.Socket do
 
   defimpl Bandit.HTTPTransport do
     require Logger
+    import Bandit.Headers, only: [is_tchar: 1]
 
     @max_chunk_size_byte_count 16
 
-    # RFC9110§5.6.2: tchar = "!" / "#" / "$" / "%" / "&" / "'" / "*" / "+" / "-" / "." /
-    # "^" / "_" / "`" / "|" / "~" / DIGIT / ALPHA
-    defguardp is_tchar(char)
-              when char in ?a..?z or char in ?A..?Z or char in ?0..?9 or
-                     char in ~c"!#$%&'*+-.^_`|~"
-
     def peer_data(%@for{} = socket), do: Bandit.SocketHelpers.peer_data(socket.socket)
 
     def sock_data(%@for{} = socket), do: Bandit.SocketHelpers.sock_data(socket.socket)
@@ -139,24 +134,14 @@ defmodule Bandit.HTTP1.Socket do
     # and every component of the request-target, since all of them reach the Plug.Conn
     defp validate_request_line!(method, {scheme, host, port, path}) do
       [method, scheme, host, port, path]
-      |> Enum.all?(&request_line_component_valid?/1)
+      |> Enum.all?(&Bandit.Headers.request_line_component_valid?/1)
       |> if do
         :ok
       else
         request_error!("Request line contains invalid characters (RFC9112§3.2)")
       end
     end
 
-    defp request_line_component_valid?(<<>>), do: true
-
-    defp request_line_component_valid?(<<char, rest::binary>>) when char > 0x20 and char != 0x7F,
-      do: request_line_component_valid?(rest)
-
-    defp request_line_component_valid?(<<_char, _rest::binary>>), do: false
-
-    # Request line components which are not binaries (ports, the :* path) cannot contain bytes
-    defp request_line_component_valid?(_component), do: true
-
     # decode_packet reads the version numbers into C integers, which wrap (so 'HTTP/4294967297.1'
     # reads as HTTP/1.1), and it accepts more than the RFC9112§2.3 grammar allows ('HTTP/01.01',
     # 'HTTP/1.1.1', 'HTTP/1.1x'). It also ignores any words after the version. A front end that

lib/bandit/http2/stream.ex

@@ -145,6 +145,7 @@ defmodule Bandit.HTTP2.Stream do
           request_target = build_request_target!(pseudo_headers, stream)
           exactly_one_instance_of!(pseudo_headers, ":scheme", stream)
           exactly_one_instance_of!(pseudo_headers, ":method", stream)
+          valid_method!(method, stream)
           exactly_one_instance_of!(pseudo_headers, ":path", stream)
           at_most_one_instance_of!(pseudo_headers, ":authority", stream)
           at_most_one_instance_of!(headers, "host", stream)
@@ -212,11 +213,25 @@ defmodule Bandit.HTTP2.Stream do
       |> case do
         nil -> stream_error!("Received empty :path", stream)
         "*" -> :*
-        "/" <> _ = path -> path
+        "/" <> _ = path -> valid_path!(path, stream)
         _ -> stream_error!("Path does not start with /", stream)
       end
     end
 
+    # RFC9113§8.3.1 - path must be valid, so it may not contain the CTLs or whitespace that
+    # HTTP/1 rejects in a request-target (RFC9112§3.2)
+    defp valid_path!(path, stream) do
+      if Bandit.Headers.request_line_component_valid?(path),
+        do: path,
+        else: stream_error!("Received invalid :path (RFC9113§8.3.1)", stream)
+    end
+
+    # RFC9113§8.3.1 - method must be valid, and RFC9110§9.1 defines a method as a token
+    defp valid_method!(method, stream) do
+      if not Bandit.Headers.token?(method),
+        do: stream_error!("Received invalid :method (RFC9113§8.3.1)", stream)
+    end
+
     # RFC9113§8.3 - pseudo headers must appear first
     defp split_headers!(headers, stream) do
       {pseudo_headers, headers} =

test/bandit/http2/protocol_test.exs

@@ -3555,7 +3555,7 @@
       assert SimpleH2Client.connection_alive?(socket)
     end
 
     test "does not enforce a limit on decoded header list size when max_header_list_size is overridden to :infinity",
          context do
       context =
         https_server(context,
@@ -4819,6 +4819,105 @@
     end
   end
 
+  describe ":method and :path pseudo headers (RFC9113§8.3.1)" do
+    setup do
+      Process.register(self(), __MODULE__)
+      :ok
+    end
+
+    def echo_method_and_path(conn) do
+      send(__MODULE__, :plug_called)
+      send_resp(conn, 200, "#{conn.method} #{conn.request_path}?#{conn.query_string}")
+    end
+
+    for {description, method} <- [
+          {"contains a space", "GE T"},
+          {"contains a tab", "GE\tT"},
+          {"contains a delimiter", "GET/1"},
+          {"contains a non-ASCII byte", "G\xC3\x89T"},
+          {"is empty", ""}
+        ] do
+      @invalid_method method
+
+      @tag :capture_log
+      test "returns a stream error if :method #{description}", context do
+        socket = SimpleH2Client.setup_connection(context)
+
+        headers = [
+          {":method", @invalid_method},
+          {":path", "/echo_method_and_path"},
+          {":scheme", "https"},
+          {":authority", "localhost:#{context.port}"}
+        ]
+
+        SimpleH2Client.send_headers(socket, 1, true, headers)
+
+        assert SimpleH2Client.recv_rst_stream(socket) == {:ok, 1, 1}
+        assert SimpleH2Client.connection_alive?(socket)
+        refute_received :plug_called
+
+        assert_receive {:log, %{level: :error, msg: {:string, msg}, meta: %{stream_id: 1}}}, 500
+
+        assert msg ==
+                 "** (Bandit.HTTP2.Errors.StreamError) Received invalid :method (RFC9113§8.3.1)"
+      end
+    end
+
+    for {description, path} <- [
+          {"contains a space", "/echo_method_and_path/a b"},
+          {"contains a tab", "/echo_method_and_path/a\tb"}
+        ] do
+      @invalid_path path
+
+      @tag :capture_log
+      test "returns a stream error if :path #{description}", context do
+        socket = SimpleH2Client.setup_connection(context)
+
+        headers = [
+          {":method", "GET"},
+          {":path", @invalid_path},
+          {":scheme", "https"},
+          {":authority", "localhost:#{context.port}"}
+        ]
+
+        SimpleH2Client.send_headers(socket, 1, true, headers)
+
+        assert SimpleH2Client.recv_rst_stream(socket) == {:ok, 1, 1}
+        assert SimpleH2Client.connection_alive?(socket)
+        refute_received :plug_called
+
+        assert_receive {:log, %{level: :error, msg: {:string, msg}, meta: %{stream_id: 1}}}, 500
+
+        assert msg ==
+                 "** (Bandit.HTTP2.Errors.StreamError) Received invalid :path (RFC9113§8.3.1)"
+      end
+    end
+
+    test "accepts any token as :method, and percent-encoded or non-ASCII bytes in :path",
+         context do
+      for method <- ["GET", "PURGE", "M-SEARCH"],
+          path <- [
+            "/echo_method_and_path/a%20b?a=b%20c",
+            "/echo_method_and_path/caf\xC3\xA9?q=\xC3\xA9"
+          ] do
+        socket = SimpleH2Client.setup_connection(context)
+
+        headers = [
+          {":method", method},
+          {":path", path},
+          {":scheme", "https"},
+          {":authority", "localhost:#{context.port}"}
+        ]
+
+        SimpleH2Client.send_headers(socket, 1, true, headers)
+
+        assert SimpleH2Client.successful_response?(socket, 1, false)
+        assert SimpleH2Client.recv_body(socket) == {:ok, 1, true, "#{method} #{path}"}
+        assert_receive :plug_called
+      end
+    end
+  end
+
   describe "max_concurrent_streams flag" do
     test "refuses streams when max_concurrent_streams limit is exceeded", context do
       context =