From 5be4d97c42d5de33257b8a0c26ceef0b53a7433b Mon Sep 17 00:00:00 2001
From: Zhongwen Deng <zhongwencool@gmail.com>
Date: Mon, 13 Mar 2023 12:08:19 +0800
Subject: [PATCH] fix: don't allow empty string in server_port schema

---
 apps/emqx/src/emqx_schema.erl               | 34 ++++++------
 apps/emqx_statsd/test/emqx_statsd_SUITE.erl | 58 ++++++++++++++++++++-
 2 files changed, 73 insertions(+), 19 deletions(-)

diff --git a/apps/emqx/src/emqx_schema.erl b/apps/emqx/src/emqx_schema.erl
index a673fa898..914ad40a1 100644
--- a/apps/emqx/src/emqx_schema.erl
+++ b/apps/emqx/src/emqx_schema.erl
@@ -2608,7 +2608,7 @@ non_empty_string(_) -> {error, invalid_string}.
 servers_sc(Meta0, ParseOpts) ->
     %% if this filed has a default value
     %% then it is not NOT required
-    %% NOTE: maps:is_key is not the solution beause #{default => undefined} is legit
+    %% NOTE: maps:is_key is not the solution because #{default => undefined} is legit
     HasDefault = (maps:get(default, Meta0, undefined) =/= undefined),
     Required = maps:get(required, Meta0, not HasDefault),
     Meta = #{
@@ -2661,17 +2661,18 @@ normalize_host_port_str(Str) ->
 %% NOTE: Validator is called after converter.
 servers_validator(Opts, Required) ->
     fun(Str0) ->
-        Str = str(Str0),
-        case Str =:= "" orelse Str =:= "undefined" of
-            true when Required ->
-                %% it's a required field
-                %% but value is set to an empty string (from environment override)
-                %% or when the filed is not set in config file
+        case str(Str0) of
+            "" ->
+                %% Empty string is not allowed even if the field is not required
+                %% remove field from config if it's empty
+                throw("cannot_be_empty");
+            "undefined" when Required ->
+                %% when the filed is not set in config file
                 %% NOTE: assuming nobody is going to name their server "undefined"
                 throw("cannot_be_empty");
-            true ->
+            "undefined" ->
                 ok;
-            _ ->
+            Str ->
                 %% it's valid as long as it can be parsed
                 _ = parse_servers(Str, Opts),
                 ok
@@ -2816,20 +2817,17 @@ is_port_number(Port) ->
     end.
 
 parse_port(Port) ->
-    try
-        P = list_to_integer(string:strip(Port)),
-        true = (P > 0),
-        true = (P =< 65535),
-        P
-    catch
-        _:_ ->
-            throw("bad_port_number")
+    case string:to_integer(string:strip(Port)) of
+        {P, ""} when P < 0 -> throw("port_number_too_small");
+        {P, ""} when P > 65535 -> throw("port_number_too_large");
+        {P, ""} -> P;
+        _ -> throw("bad_port_number")
     end.
 
 quic_feature_toggle(Desc) ->
     sc(
         %% true, false are for user facing
-        %% 0, 1 are for internal represtation
+        %% 0, 1 are for internal representation
         typerefl:alias("boolean", typerefl:union([true, false, 0, 1])),
         #{
             desc => Desc,
diff --git a/apps/emqx_statsd/test/emqx_statsd_SUITE.erl b/apps/emqx_statsd/test/emqx_statsd_SUITE.erl
index a203ef7d5..bcc710050 100644
--- a/apps/emqx_statsd/test/emqx_statsd_SUITE.erl
+++ b/apps/emqx_statsd/test/emqx_statsd_SUITE.erl
@@ -33,6 +33,26 @@
     "tags {\"t1\" = \"good\", test = 100}\n"
     "}\n"
 >>).
+-define(BAD_CONF, <<
+    "\n"
+    "statsd {\n"
+    "enable = true\n"
+    "flush_time_interval = 4s\n"
+    "sample_time_interval = 4s\n"
+    "server = \"\"\n"
+    "tags {\"t1\" = \"good\", test = 100}\n"
+    "}\n"
+>>).
+
+-define(DEFAULT_CONF, <<
+    "\n"
+    "statsd {\n"
+    "enable = true\n"
+    "flush_time_interval = 4s\n"
+    "sample_time_interval = 4s\n"
+    "tags {\"t1\" = \"good\", test = 100}\n"
+    "}\n"
+>>).
 
 init_per_suite(Config) ->
     emqx_common_test_helpers:start_apps(
@@ -55,6 +75,33 @@ set_special_configs(_) ->
 all() ->
     emqx_common_test_helpers:all(?MODULE).
 
+t_server_validator(_) ->
+    Server0 = emqx_conf:get_raw([statsd, server]),
+    ?assertThrow(
+        #{
+            kind := validation_error,
+            path := "statsd.server",
+            reason := "cannot_be_empty",
+            value := ""
+        },
+        emqx_common_test_helpers:load_config(emqx_statsd_schema, ?BAD_CONF, #{
+            raw_with_default => true
+        })
+    ),
+    %% default
+    ok = emqx_common_test_helpers:load_config(emqx_statsd_schema, ?DEFAULT_CONF, #{
+        raw_with_default => true
+    }),
+    undefined = emqx_conf:get_raw([statsd, server], undefined),
+    ?assertMatch("127.0.0.1:8125", emqx_conf:get([statsd, server])),
+    %% recover
+    ok = emqx_common_test_helpers:load_config(emqx_statsd_schema, ?BASE_CONF, #{
+        raw_with_default => true
+    }),
+    Server2 = emqx_conf:get_raw([statsd, server]),
+    ?assertMatch(Server0, Server2),
+    ok.
+
 t_statsd(_) ->
     {ok, Socket} = gen_udp:open(8126, [{active, true}]),
     receive
@@ -137,7 +184,16 @@ t_config_update(_) ->
         ?assertNotEqual(OldPid, NewPid)
     after
         {ok, _} = emqx_statsd_config:update(OldRawConf)
-    end.
+    end,
+    %% bad server url
+    BadRawConf = OldRawConf#{<<"server">> := <<"">>},
+    {error, #{
+        kind := validation_error,
+        path := "statsd.server",
+        reason := "cannot_be_empty",
+        value := ""
+    }} = emqx_statsd_config:update(BadRawConf),
+    ok.
 
 request(Method) -> request(Method, []).