fix: Validate the connect ip parameter

API-CHANGELOG.md

@@ -53,6 +53,7 @@ This section contains changes targeting a future version.
 - `submit`: The `fail_hard` field now returns an error if the value is not a boolean. [#6529](https://github.com/XRPLF/rippled/pull/6529)
 - `subscribe`: The `taker` field in the `books` array now returns `actMalformed` instead of `badIssuer` if the value is not a valid account. [#6529](https://github.com/XRPLF/rippled/pull/6529)
 - Fixed a bug in `Forwarded` HTTP header parsing where the extracted IP address could be incorrect when no comma or semicolon delimiter follows the address. This could cause the server to misidentify a client's IP address when operating behind a reverse proxy. [#6529](https://github.com/XRPLF/rippled/pull/6529)
+- `connect`: The `ip` field now returns `invalidParams` if the value is not a string. Previously, arrays and objects returned an `internal` error, and numbers, booleans and `null` were coerced into a string that could never parse as an address, so the request reported an attempted connection that never happened. [#7954](https://github.com/XRPLF/rippled/pull/7954)
 
 ## XRP Ledger server version 3.1.0
 

src/test/rpc/Connect_test.cpp

@@ -1,38 +1,110 @@
 
 #include <test/jtx/Env.h>
 
+#include <xrpld/app/main/Application.h>
+#include <xrpld/core/Config.h>
+
 #include <xrpl/beast/unit_test/suite.h>
+#include <xrpl/protocol/ErrorCodes.h>
 #include <xrpl/protocol/jss.h>
 
+#include <array>
+#include <string>
+
 namespace xrpl {
 
 class Connect_test : public beast::unit_test::Suite
 {
+    // doConnect refuses every request in standalone mode before it reads a
+    // parameter, so nothing below the standalone check is reachable from a
+    // default Env. Leave standalone once the application is set up: connect is
+    // a Condition::NoCondition command, so no other part of the dispatch path
+    // reads this flag.
+    static void
+    leaveStandalone(test::jtx::Env& env)
+    {
+        env.app().config().setupControl(true, true, false);
+    }
+
     void
-    testErrors()
+    testStandalone()
     {
-        testcase("Errors");
+        testcase("Standalone");
 
         using namespace test::jtx;
 
+        // standalone mode should fail
+        Env env{*this};
+        BEAST_EXPECT(env.app().config().standalone());
+
+        auto const result = env.rpc("json", "connect", "{}");
+        BEAST_EXPECT(result[jss::result][jss::status] == "error");
+        BEAST_EXPECT(result[jss::result].isMember(jss::error));
+        BEAST_EXPECT(result[jss::result][jss::error] == "notSynced");
+        BEAST_EXPECT(result[jss::result][jss::error_message] == "Not synced to the network.");
+    }
+
+    void
+    testMissingIp()
+    {
+        testcase("Missing ip");
+
+        using namespace test::jtx;
+
+        Env env{*this};
+        leaveStandalone(env);
+
+        auto const result = env.rpc("json", "connect", "{}")[jss::result];
+        BEAST_EXPECT(result[jss::status] == "error");
+        BEAST_EXPECT(result[jss::error] == "invalidParams");
+        // missingFieldMessage, unlike missingFieldError, has no StaticString
+        // overload, so jss::ip has to be converted explicitly.
+        BEAST_EXPECT(result[jss::error_message] == RPC::missingFieldMessage(std::string(jss::ip)));
+    }
+
+    void
+    testBadIp()
+    {
+        testcase("Invalid ip");
+
+        using namespace test::jtx;
+
+        Env env{*this};
+        leaveStandalone(env);
+        BEAST_EXPECT(!env.app().config().standalone());
+
+        // Before the type check the array and the object returned a generic
+        // internal error, and the scalars were stringified into "42",
+        // "1.500000", "true" and "", none of which parse as an address. The
+        // connection was silently skipped, but the reply still claimed one had
+        // been attempted.
+        std::array<char const*, 6> const badIps{
+            R"({"ip": 42})",
+            R"({"ip": 1.5})",
+            R"({"ip": true})",
+            R"({"ip": null})",
+            R"({"ip": {"host": "127.0.0.1"}})",
+            R"({"ip": ["127.0.0.1"]})",
+        };
+
+        for (auto const* badIp : badIps)
         {
-            // standalone mode should fail
-            Env env{*this};
-            BEAST_EXPECT(env.app().config().standalone());
-
-            auto const result = env.rpc("json", "connect", "{}");
-            BEAST_EXPECT(result[jss::result][jss::status] == "error");
-            BEAST_EXPECT(result[jss::result].isMember(jss::error));
-            BEAST_EXPECT(result[jss::result][jss::error] == "notSynced");
-            BEAST_EXPECT(result[jss::result][jss::error_message] == "Not synced to the network.");
+            auto const result = env.rpc("json", "connect", badIp)[jss::result];
+            BEAST_EXPECTS(result[jss::status] == "error", badIp);
+            BEAST_EXPECTS(result[jss::error] == "invalidParams", badIp);
+            BEAST_EXPECTS(
+                result[jss::error_message] == RPC::expectedFieldMessage(jss::ip, "a string"),
+                badIp);
         }
     }
 
 public:
     void
     run() override
     {
-        testErrors();
+        testStandalone();
+        testMissingIp();
+        testBadIp();
     }
 };
 

src/xrpld/rpc/handlers/admin/peer/Connect.cpp

@@ -31,6 +31,12 @@ doConnect(RPC::JsonContext& context)
     if (!context.params.isMember(jss::ip))
         return RPC::missingFieldError(jss::ip);
 
+    // Without this, asString() below throws for an array or an object, and
+    // silently stringifies every scalar into something that cannot parse as an
+    // address.
+    if (!context.params[jss::ip].isString())
+        return RPC::expectedFieldError(jss::ip, "a string");
+
     if (context.params.isMember(jss::port) &&
         !context.params[jss::port].isConvertibleTo(json::ValueType::Int))
     {