Merge pull request #16946 from rabbitmq/RMQ-2785

[mgt] Require .json extension to upload definitions file

Author: michaelklishin

deps/rabbitmq_management/priv/schema/rabbitmq_management.schema

@@ -487,6 +487,20 @@ end}.
 {mapping, "management.http.hide_allow_header", "rabbitmq_management.hide_http_allow_header",
     [{datatype, boolean}]}.
 
+%% When enabled, multipart definition uploads to the HTTP API are only accepted
+%% when the uploaded file name has a .json extension.
+%%
+%% Files with any other extension, or with no extension at all, are rejected with HTTP 400.
+%%
+%% This setting applies to any client using the HTTP API, not just the management UI.
+%% Definitions posted with a Content-Type of application/json are not affected.
+%% Disabled by default to preserve backwards compatibility.
+%%
+%% {require_definition_json_extension, false},
+
+{mapping, "management.definitions.require_json_extension", "rabbitmq_management.require_definition_json_extension",
+    [{datatype, boolean}]}.
+
 {mapping, "management.disable_basic_auth", "rabbitmq_management.disable_basic_auth",
     [{datatype, boolean}]}.
 

deps/rabbitmq_management/priv/www/js/main.js

@@ -1,4 +1,20 @@
 
+const API_ERROR_REASONS = {
+    'failed_to_parse_json':       'Definitions file could not be parsed. Make sure it is valid JSON.',
+    'unsupported_file_extension': '{filename}Only .json files are accepted for definitions import.',
+};
+
+function format_error_response(response, reason) {
+    var template = API_ERROR_REASONS[reason];
+    if (typeof(template) != 'string') {
+        return reason;
+    }
+    return template.replace(/\{(\w+)\}/g, function(_, key) {
+        var val = response[key];
+        return val !== undefined ? val + ': ' : '';
+    });
+}
+
 $(document).ready(function() {    
   var url_string = window.location.href;
   var url = new URL(url_string);
@@ -1560,7 +1576,7 @@ function check_bad_response(req, full_page_404, on404fun) {
             } else if (on404fun && (typeof on404fun === 'function') && req.status == 404) {
                 on404fun(response);
             } else {
-                show_popup('warn', fmt_escape_html(reason));
+                show_popup('warn', fmt_escape_html(format_error_response(response, reason)));
             }
         } else if (error == 'page_out_of_range') {
             var seconds = 60;

deps/rabbitmq_management/priv/www/js/tmpl/overview.ejs

@@ -347,7 +347,7 @@
         <td>
           <p>
             <label>Definitions file:</label><br/>
-            <input type="file" name="file"/>
+            <input type="file" name="file"<% if (overview.require_definition_json_extension) { %> accept=".json"<% } %>/>
           </p>
         </td>
         <td>

deps/rabbitmq_management/src/rabbit_mgmt_features.erl

@@ -9,7 +9,8 @@
 
 -export([is_op_policy_updating_disabled/0,
          is_qq_replica_operations_disabled/0,
-         are_stats_enabled/0]).
+         are_stats_enabled/0,
+         is_definition_json_extension_required/0]).
 
 is_qq_replica_operations_disabled() ->
     get_restriction([quorum_queue_replica_operations, disabled]).
@@ -28,6 +29,9 @@ are_stats_enabled() ->
         _    -> rabbit_mgmt_agent_config:is_metrics_collector_permitted()
     end.
 
+is_definition_json_extension_required() ->
+    application:get_env(rabbitmq_management, require_definition_json_extension, false).
+
 %% Private
 
 get_restriction(Path) ->

deps/rabbitmq_management/src/rabbit_mgmt_util.erl

@@ -18,7 +18,7 @@
          is_authorized_vhost_visible_for_monitoring/2,
          is_authorized_global_parameters/2]).
 -export([user/1]).
--export([bad_request/3, service_unavailable/3, bad_request_exception/4,
+-export([bad_request/3, bad_request/4, service_unavailable/3, bad_request_exception/4,
          internal_server_error/3, internal_server_error/4, precondition_failed/3,
          id/2, parse_bool/1, parse_int/1, redirect_to_home/3]).
 -export([with_decode/4, with_ids/4, not_found/3]).
@@ -698,6 +698,16 @@ a2b(B)                 -> B.
 bad_request(Reason, ReqData, Context) ->
     halt_response(400, bad_request, Reason, ReqData, Context).
 
+%% Like bad_request/3 but merges ExtraFields into the JSON response body.
+%% Reason must be a binary or string.
+bad_request(Reason, ExtraFields, ReqData, Context) ->
+    ReasonBin = rabbit_data_coercion:to_binary(Reason),
+    Json = maps:merge(#{error => bad_request, reason => ReasonBin}, ExtraFields),
+    ReqData1 = cowboy_req:reply(400,
+        #{<<"content-type">> => <<"application/json">>},
+        rabbit_json:encode(Json), ReqData),
+    {stop, ReqData1, Context}.
+
 service_unavailable(Reason, ReqData, Context) ->
     halt_response(503, service_unavailable, Reason, ReqData, Context).
 

deps/rabbitmq_management/src/rabbit_mgmt_wm_definitions.erl

@@ -11,6 +11,7 @@
 -export([content_types_accepted/2, allowed_methods/2, accept_json/2]).
 -export([accept_multipart/2]).
 -export([variances/2]).
+-export([has_json_extension/1]).
 
 -include("rabbit_mgmt.hrl").
 -include_lib("rabbitmq_management_agent/include/rabbit_mgmt_records.hrl").
@@ -264,14 +265,25 @@ accept_multipart(ReqData0, Context) ->
                              "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);
-        {Parts, ReqData} ->
-            Redirect = get_part(<<"redirect">>, Parts),
-            Payload = get_part(<<"file">>, Parts),
-            Resp = {Res, _, _} = accept(Payload, ReqData, Context),
-            case {Res, Redirect} of
-                {true, unknown} -> {true, ReqData, Context};
-                {true, _}       -> {{true, Redirect}, ReqData, Context};
-                _               -> Resp
+        {Parts, Filename, ReqData} ->
+            case is_acceptable_filename(Filename) of
+                false ->
+                    ExtraFields = case Filename of
+                        unknown -> #{};
+                        _       -> #{filename => Filename}
+                    end,
+                    rabbit_mgmt_util:bad_request(<<"unsupported_file_extension">>,
+                                                 ExtraFields,
+                                                 ReqData, Context);
+                true ->
+                    Redirect = get_part(<<"redirect">>, Parts),
+                    Payload = get_part(<<"file">>, Parts),
+                    Resp = {Res, _, _} = accept(Payload, ReqData, Context),
+                    case {Res, Redirect} of
+                        {true, unknown} -> {true, ReqData, Context};
+                        {true, _}       -> {{true, Redirect}, ReqData, Context};
+                        _               -> Resp
+                    end
             end
     end.
 
@@ -355,28 +367,42 @@ get_all_parts(Req, BodySizeLimit) ->
         N when is_integer(N), N > BodySizeLimit ->
             {error, http_body_limit_exceeded, BodySizeLimit, N};
         _ ->
-            get_all_parts(Req, 0, BodySizeLimit, [])
+            get_all_parts(Req, 0, BodySizeLimit, [], unknown)
     end.
 
-get_all_parts(Req0, BodySize0, BodySizeLimit, Acc) ->
+get_all_parts(Req0, BodySize0, BodySizeLimit, Acc, FileFilename) ->
     case cowboy_req:read_part(Req0) of
         {done, Req1} ->
-            {Acc, Req1};
+            {Acc, FileFilename, Req1};
         {ok, Headers, Req1} ->
             %% Approximate maximum size of part headers.
             BodySize1 = BodySize0 + 2048,
-            Name = case cow_multipart:form_data(Headers) of
-                       {data, N} -> N;
-                       {file, N, _, _} -> N
+            {Name, Filename} = case cow_multipart:form_data(Headers) of
+                       {data, N}          -> {N, unknown};
+                       {file, N, FN, _}   -> {N, FN}
                    end,
             case stream_part_body(Req1, BodySize1, BodySizeLimit, <<>>) of
                 {ok, Body, BodySize, Req2} ->
-                    get_all_parts(Req2, BodySize, BodySizeLimit, [{Name, Body}|Acc]);
+                    %% Track the filename of the "file" field specifically.
+                    UpdatedFileFilename = case Name of
+                        <<"file">> -> Filename;
+                        _          -> FileFilename
+                    end,
+                    get_all_parts(Req2, BodySize, BodySizeLimit, [{Name, Body}|Acc], UpdatedFileFilename);
                 {error, http_body_limit_exceeded, _, _} = Error ->
                     Error
             end
     end.
 
+is_acceptable_filename(Filename) ->
+    not rabbit_mgmt_features:is_definition_json_extension_required()
+        orelse has_json_extension(Filename).
+
+has_json_extension(unknown) ->
+    false;
+has_json_extension(Filename) ->
+    filename:extension(string:lowercase(Filename)) =:= <<".json">>.
+
 stream_part_body(Req0, BodySize, BodySizeLimit, Acc) ->
     %% Pass an explicit length to read_part_body so that Cowboy's internal
     %% buffering (default 8 MiB) respects the configured limit instead of

deps/rabbitmq_management/src/rabbit_mgmt_wm_overview.erl

@@ -55,7 +55,9 @@ to_json(ReqData, Context = #context{user = User = #user{tags = Tags}}) ->
                  {disable_stats,                 rabbit_mgmt_util:disable_stats(ReqData)},
                  {default_queue_type,            rabbit_queue_type:default_alias()},
                  {is_op_policy_updating_enabled, not rabbit_mgmt_features:is_op_policy_updating_disabled()},
-                 {enable_queue_totals,           rabbit_mgmt_util:enable_queue_totals(ReqData)}],
+                 {enable_queue_totals,           rabbit_mgmt_util:enable_queue_totals(ReqData)},
+                 {require_definition_json_extension,
+                  rabbit_mgmt_features:is_definition_json_extension_required()}],
     try
         case rabbit_mgmt_util:disable_stats(ReqData) of
             false ->

deps/rabbitmq_management/test/config_schema_SUITE_data/rabbitmq_management.snippets

@@ -607,6 +607,24 @@
    ], [rabbitmq_management]
   },
 
+  {require_definition_json_extension_disabled,
+   "management.definitions.require_json_extension = false",
+   [
+    {rabbitmq_management, [
+                           {require_definition_json_extension, false}
+                          ]}
+   ], [rabbitmq_management]
+  },
+
+  {require_definition_json_extension_enabled,
+   "management.definitions.require_json_extension = true",
+   [
+    {rabbitmq_management, [
+                           {require_definition_json_extension, true}
+                          ]}
+   ], [rabbitmq_management]
+  },
+
  %%
  %% Exotic options
  %%

deps/rabbitmq_management/test/rabbit_mgmt_http_SUITE.erl

@@ -56,6 +56,7 @@ all() ->
         {group, definitions_group2_without_prefix},
         {group, definitions_group3_without_prefix},
         {group, definitions_group4_without_prefix},
+        {group, definitions_group5_without_prefix},
         {group, default_queue_type_without_prefix}
     ].
 
@@ -71,6 +72,7 @@ groups() ->
         {definitions_group2_without_prefix, [], definitions_group2_tests()},
         {definitions_group3_without_prefix, [], definitions_group3_tests()},
         {definitions_group4_without_prefix, [], definitions_group4_tests()},
+        {definitions_group5_without_prefix, [], definitions_group5_tests()},
         {default_queue_type_without_prefix, [], default_queue_type_group_tests()}
     ].
 
@@ -113,6 +115,14 @@ definitions_group4_tests() ->
         definitions_vhost_test
     ].
 
+definitions_group5_tests() ->
+    [
+        definitions_multipart_non_json_extension_rejected_test,
+        definitions_multipart_non_json_extension_allowed_when_not_enforced_test,
+        require_definition_json_extension_defaults_to_false_in_overview_test,
+        require_definition_json_extension_enabled_in_overview_test
+    ].
+
 default_queue_type_group_tests() ->
     [
         default_queue_type_fallback_in_overview_test,
@@ -2234,7 +2244,7 @@ long_definitions_multipart_test(Config) ->
     Data = binary_to_list(format_for_upload(LongDefs)),
     CodeExp = {group, '2xx'},
     Boundary = "------------long_definitions_multipart_test",
-    Body = format_multipart_filedata(Boundary, [{file, "file", Data}]),
+    Body = format_multipart_filedata(Boundary, [{file, "definitions.json", Data}]),
     ContentType = lists:concat(["multipart/form-data; boundary=", Boundary]),
     MoreHeaders = [{"content-type", ContentType}, {"content-length", integer_to_list(length(Body))}],
     http_upload_raw(Config, post, "/definitions", Body, "guest", "guest", CodeExp, MoreHeaders),
@@ -3492,7 +3502,7 @@ definitions_multipart_body_size_limit_test(Config) ->
         Boundary = "------------definitions_multipart_body_size_limit_test",
         LargeBody = list_to_binary(lists:duplicate(300, $\s)),
         OversizedMultipart = format_multipart_filedata(Boundary,
-            [{file, "file", Data ++ binary_to_list(LargeBody)}]),
+            [{file, "definitions.json", Data ++ binary_to_list(LargeBody)}]),
         ContentType = lists:concat(["multipart/form-data; boundary=", Boundary]),
         MoreHeaders = [{"content-type", ContentType},
                        {"content-length", integer_to_list(length(OversizedMultipart))}],
@@ -3503,6 +3513,64 @@ definitions_multipart_body_size_limit_test(Config) ->
         rpc(Config, application, unset_env, [rabbitmq_management, max_http_body_size])
     end.
 
+definitions_multipart_non_json_extension_rejected_test(Config) ->
+    %% When require_definition_json_extension is enabled, uploads whose filename does not
+    %% end in .json must be rejected with HTTP 400. The JSON response body must
+    %% include the reason code and the offending filename.
+    rpc(Config, application, set_env,
+        [rabbitmq_management, require_definition_json_extension, true]),
+    try
+        SmallDefs = #{users => [], vhosts => [#{name => <<"test">>}]},
+        Data = binary_to_list(format_for_upload(SmallDefs)),
+        Boundary = "------------definitions_multipart_non_json_rejected",
+        Body = format_multipart_filedata(Boundary, [{file, "definitions.txt", Data}]),
+        ContentType = lists:concat(["multipart/form-data; boundary=", Boundary]),
+        MoreHeaders = [{"content-type", ContentType},
+                       {"content-length", integer_to_list(length(Body))},
+                       auth_header("guest", "guest")],
+        {ok, {{_, 400, _}, _, ResBody}} =
+            req(Config, 0, post, "/definitions", MoreHeaders, Body),
+        Decoded = decode_body(ResBody),
+        ?assertEqual(<<"unsupported_file_extension">>, maps:get(reason, Decoded)),
+        ?assertEqual(<<"definitions.txt">>, maps:get(filename, Decoded)),
+        passed
+    after
+        rpc(Config, application, unset_env,
+            [rabbitmq_management, require_definition_json_extension])
+    end.
+
+definitions_multipart_non_json_extension_allowed_when_not_enforced_test(Config) ->
+    %% By default (require_definition_json_extension = false), files with any extension are accepted.
+    SmallDefs = #{users => [], vhosts => [#{name => <<"test">>}]},
+    Data = binary_to_list(format_for_upload(SmallDefs)),
+    Boundary = "------------definitions_multipart_non_json_allowed",
+    Body = format_multipart_filedata(Boundary, [{file, "definitions.txt", Data}]),
+    ContentType = lists:concat(["multipart/form-data; boundary=", Boundary]),
+    MoreHeaders = [{"content-type", ContentType},
+                   {"content-length", integer_to_list(length(Body))}],
+    http_upload_raw(Config, post, "/definitions", Body,
+                    "guest", "guest", {group, '2xx'}, MoreHeaders),
+    passed.
+
+require_definition_json_extension_defaults_to_false_in_overview_test(Config) ->
+    %% By default, require_definition_json_extension is false and must be reflected in the overview.
+    Overview = http_get(Config, "/overview"),
+    ?assertEqual(false, maps:get(require_definition_json_extension, Overview)),
+    passed.
+
+require_definition_json_extension_enabled_in_overview_test(Config) ->
+    %% When enabled, the overview endpoint must report require_definition_json_extension as true.
+    rpc(Config, application, set_env,
+        [rabbitmq_management, require_definition_json_extension, true]),
+    try
+        Overview = http_get(Config, "/overview"),
+        ?assertEqual(true, maps:get(require_definition_json_extension, Overview)),
+        passed
+    after
+        rpc(Config, application, unset_env,
+            [rabbitmq_management, require_definition_json_extension])
+    end.
+
 publish_accept_json_test(Config) ->
     Headers = #{'x-forwarding' => [#{uri => <<"amqp://localhost/%2F/upstream">>}]},
     Msg = msg(<<"publish_accept_json_test">>, Headers, <<"Hello world">>),

deps/rabbitmq_management/test/rabbit_mgmt_test_unit_SUITE.erl

@@ -24,7 +24,8 @@ groups() ->
                                    pack_binding_test,
                                    default_restrictions,
                                    path_prefix_test,
-                                   regex_dos_test
+                                   regex_dos_test,
+                                   has_json_extension_test
                                   ]},
      {sequential_tests, [], [
                               referrer_policy_header_set_when_configured,
@@ -120,6 +121,18 @@ regex_dos_test(_) ->
     ?assertEqual(false, rabbit_mgmt_util:maybe_filter_by_keyword(
         name, EvilRegex, [{name, TargetString}], "true")).
 
+has_json_extension_test(_Config) ->
+    %% Standard .json extension
+    ?assert(rabbit_mgmt_wm_definitions:has_json_extension(<<"definitions.json">>)),
+    %% Case-insensitive
+    ?assert(rabbit_mgmt_wm_definitions:has_json_extension(<<"definitions.JSON">>)),
+    ?assert(rabbit_mgmt_wm_definitions:has_json_extension(<<"definitions.Json">>)),
+    %% Non-json extensions are rejected
+    ?assertNot(rabbit_mgmt_wm_definitions:has_json_extension(<<"definitions.txt">>)),
+    ?assertNot(rabbit_mgmt_wm_definitions:has_json_extension(<<"definitions">>)),
+    %% The atom 'unknown' (no filename provided) is rejected
+    ?assertNot(rabbit_mgmt_wm_definitions:has_json_extension(unknown)).
+
 referrer_policy_header_set_when_configured(_Config) ->
     application:set_env(rabbitmq_management, headers,
                         [{referrer_policy, "no-referrer"}]),