Merge pull request #12697 from rabbitmq/mergify/bp/v4.0.x/pr-12696

HTTP API: reduce body size limit for the endpoint used to bind queues/streams/exchanges (backport #12696)
rabbitmq · Nov 9, 2024 · 91c2d15 · 91c2d15
2 parents 073c993 + a66c926
commit 91c2d15
Show file tree

Hide file tree

Showing 3 changed files with 83 additions and 44 deletions.
diff --git a/deps/rabbitmq_management/src/rabbit_mgmt_util.erl b/deps/rabbitmq_management/src/rabbit_mgmt_util.erl
@@ -30,7 +30,7 @@
          list_login_vhosts_names/2]).
 -export([filter_tracked_conn_list/3]).
 -export([with_decode/5, decode/1, decode/2, set_resp_header/3,
-         args/1, read_complete_body/1]).
+         args/1, read_complete_body/1, read_complete_body_with_limit/2]).
 -export([reply_list/3, reply_list/5, reply_list/4,
          sort_list/2, destination_type/1, reply_list_or_paginate/3
          ]).
@@ -703,26 +703,56 @@ halt_response(Code, Type, Reason, ReqData, Context) ->
 id(Key, ReqData) ->
     rabbit_web_dispatch_access_control:id(Key, ReqData).
 
+%% IMPORTANT:
+%% Prefer read_complete_body_with_limit/2 with an explicit limit to make it easier
+%% to reason about what limit will be used.
 read_complete_body(Req) ->
     read_complete_body(Req, <<"">>).
 read_complete_body(Req, Acc) ->
     BodySizeLimit = application:get_env(rabbitmq_management, max_http_body_size, ?MANAGEMENT_DEFAULT_HTTP_MAX_BODY_SIZE),
     read_complete_body(Req, Acc, BodySizeLimit).
 read_complete_body(Req0, Acc, BodySizeLimit) ->
-    case bit_size(Acc) > BodySizeLimit of
+    N = byte_size(Acc),
+    case N > BodySizeLimit of
         true ->
-            {error, "Exceeded HTTP request body size limit"};
+            {error, http_body_limit_exceeded, BodySizeLimit, N};
         false ->
             case cowboy_req:read_body(Req0) of
                 {ok, Data, Req}   -> {ok, <<Acc/binary, Data/binary>>, Req};
                 {more, Data, Req} -> read_complete_body(Req, <<Acc/binary, Data/binary>>)
             end
     end.
 
+read_complete_body_with_limit(Req, BodySizeLimit) when is_integer(BodySizeLimit) ->
+    case cowboy_req:body_length(Req) of
+        N when is_integer(N) ->
+            case N > BodySizeLimit of
+                true ->
+                    {error, http_body_limit_exceeded, BodySizeLimit, N};
+                false ->
+                    do_read_complete_body_with_limit(Req, <<"">>, BodySizeLimit)
+            end;
+        undefined ->
+            do_read_complete_body_with_limit(Req, <<"">>, BodySizeLimit)
+    end.
+
+do_read_complete_body_with_limit(Req0, Acc, BodySizeLimit) ->
+    N = byte_size(Acc),
+    case N > BodySizeLimit of
+        true ->
+            {error, http_body_limit_exceeded, BodySizeLimit, N};
+        false ->
+            case cowboy_req:read_body(Req0, #{length => BodySizeLimit, period => 30000}) of
+                {ok, Data, Req}   -> {ok, <<Acc/binary, Data/binary>>, Req};
+                {more, Data, Req} -> do_read_complete_body_with_limit(Req, <<Acc/binary, Data/binary>>, BodySizeLimit)
+            end
+    end.
+
 with_decode(Keys, ReqData, Context, Fun) ->
     case read_complete_body(ReqData) of
-        {error, Reason} ->
-            bad_request(Reason, ReqData, Context);
+        {error, http_body_limit_exceeded, LimitApplied, BytesRead} ->
+            rabbit_log:warning("HTTP API: request exceeded maximum allowed payload size (limit: ~tp bytes, payload size: ~tp bytes)", [LimitApplied, BytesRead]),
+            bad_request("Exceeded HTTP request body size limit", ReqData, Context);
         {ok, Body, ReqData1} ->
             with_decode(Keys, Body, ReqData1, Context, Fun)
     end.

diff --git a/deps/rabbitmq_management/src/rabbit_mgmt_wm_bindings.erl b/deps/rabbitmq_management/src/rabbit_mgmt_wm_bindings.erl
@@ -16,6 +16,10 @@
 -include_lib("rabbitmq_management_agent/include/rabbit_mgmt_records.hrl").
 -include_lib("amqp_client/include/amqp_client.hrl").
 
+%% Use a much lower limit for creating bindings over the HTTP API.
+%% The payload is not meant to be even 50 KiB in size.
+-define(HTTP_BODY_SIZE_LIMIT, 5000).
+
 %%--------------------------------------------------------------------
 
 init(Req, [Mode]) ->
@@ -64,39 +68,44 @@ to_json(ReqData, {Mode, Context}) ->
       ReqData, {Mode, Context}).
 
 accept_content(ReqData0, {_Mode, Context}) ->
-    {ok, Body, ReqData} = rabbit_mgmt_util:read_complete_body(ReqData0),
-    Source = rabbit_mgmt_util:id(source, ReqData),
-    Dest = rabbit_mgmt_util:id(destination, ReqData),
-    DestType = rabbit_mgmt_util:id(dtype, ReqData),
-    VHost = rabbit_mgmt_util:vhost(ReqData),
-    {ok, Props} = rabbit_mgmt_util:decode(Body),
-    MethodName = case rabbit_mgmt_util:destination_type(ReqData) of
-                     exchange -> 'exchange.bind';
-                     queue    -> 'queue.bind'
-                 end,
-    {Key, Args} = key_args(DestType, Props),
-    case rabbit_mgmt_util:direct_request(
-           MethodName,
-           fun rabbit_mgmt_format:format_accept_content/1,
-           [{queue, Dest},
-            {exchange, Source},
-            {destination, Dest},
-            {source, Source},
-            {routing_key, Key},
-            {arguments, Args}],
-           "Binding error: ~ts", ReqData, Context) of
-        {stop, _, _} = Res ->
-            Res;
-        {true, ReqData, Context2} ->
-            From = binary_to_list(cowboy_req:path(ReqData)),
-            Prefix = rabbit_mgmt_util:get_path_prefix(),
-            BindingProps = rabbit_mgmt_format:pack_binding_props(Key, Args),
-            UrlWithBindings = rabbit_mgmt_format:url("/api/bindings/~ts/e/~ts/~ts/~ts/~ts",
-                                                     [VHost, Source, DestType,
-                                                      Dest, BindingProps]),
-            To = Prefix ++ binary_to_list(UrlWithBindings),
-            Loc = rabbit_web_dispatch_util:relativise(From, To),
-            {{true, Loc}, ReqData, Context2}
+    case rabbit_mgmt_util:read_complete_body_with_limit(ReqData0, ?HTTP_BODY_SIZE_LIMIT) of
+        {ok, Body, ReqData} ->
+            Source = rabbit_mgmt_util:id(source, ReqData),
+            Dest = rabbit_mgmt_util:id(destination, ReqData),
+            DestType = rabbit_mgmt_util:id(dtype, ReqData),
+            VHost = rabbit_mgmt_util:vhost(ReqData),
+            {ok, Props} = rabbit_mgmt_util:decode(Body),
+            MethodName = case rabbit_mgmt_util:destination_type(ReqData) of
+                             exchange -> 'exchange.bind';
+                             queue    -> 'queue.bind'
+                         end,
+            {Key, Args} = key_args(DestType, Props),
+            case rabbit_mgmt_util:direct_request(
+                MethodName,
+                fun rabbit_mgmt_format:format_accept_content/1,
+                [{queue, Dest},
+                 {exchange, Source},
+                 {destination, Dest},
+                 {source, Source},
+                 {routing_key, Key},
+                 {arguments, Args}],
+                "Binding error: ~ts", ReqData, Context) of
+                {stop, _, _} = Res ->
+                    Res;
+                {true, ReqData, Context2} ->
+                    From = binary_to_list(cowboy_req:path(ReqData)),
+                    Prefix = rabbit_mgmt_util:get_path_prefix(),
+                    BindingProps = rabbit_mgmt_format:pack_binding_props(Key, Args),
+                    UrlWithBindings = rabbit_mgmt_format:url("/api/bindings/~ts/e/~ts/~ts/~ts/~ts",
+                        [VHost, Source, DestType,
+                            Dest, BindingProps]),
+                    To = Prefix ++ binary_to_list(UrlWithBindings),
+                    Loc = rabbit_web_dispatch_util:relativise(From, To),
+                    {{true, Loc}, ReqData, Context2}
+            end;
+        {error, http_body_limit_exceeded, LimitApplied, BytesRead} ->
+            rabbit_log:warning("HTTP API: binding creation request exceeded maximum allowed payload size (limit: ~tp bytes, payload size: ~tp bytes)", [LimitApplied, BytesRead]),
+            rabbit_mgmt_util:bad_request("Payload size limit exceeded", ReqData0, Context)
     end.
 
 is_authorized(ReqData, {Mode, Context}) ->

diff --git a/deps/rabbitmq_management/src/rabbit_mgmt_wm_definitions.erl b/deps/rabbitmq_management/src/rabbit_mgmt_wm_definitions.erl
@@ -84,12 +84,12 @@ all_definitions(ReqData, Context) ->
       Context).
 
 accept_json(ReqData0, Context) ->
-    case rabbit_mgmt_util:read_complete_body(ReqData0) of
-        {error, Reason} ->
-            BodySizeLimit = application:get_env(rabbitmq_management, max_http_body_size, ?MANAGEMENT_DEFAULT_HTTP_MAX_BODY_SIZE),
-            _ = rabbit_log:warning("HTTP API: uploaded definition file exceeded the maximum request body limit of ~p bytes. "
-                                   "Use the 'management.http.max_body_size' key in rabbitmq.conf to increase the limit if necessary", [BodySizeLimit]),
-            rabbit_mgmt_util:bad_request(Reason, ReqData0, Context);
+    BodySizeLimit = application:get_env(rabbitmq_management, max_http_body_size, ?MANAGEMENT_DEFAULT_HTTP_MAX_BODY_SIZE),
+    case rabbit_mgmt_util:read_complete_body_with_limit(ReqData0, BodySizeLimit) of
+        {error, http_body_limit_exceeded, LimitApplied, BytesRead} ->
+            _ = rabbit_log:warning("HTTP API: uploaded definition file size (~tp) exceeded the maximum request body limit of ~tp bytes. "
+                                   "Use the 'management.http.max_body_size' key in rabbitmq.conf to increase the limit if necessary", [BytesRead, LimitApplied]),
+            rabbit_mgmt_util:bad_request("Exceeded HTTP request body size limit", ReqData0, Context);
         {ok, Body, ReqData} ->
             accept(Body, ReqData, Context)
     end.