diff --git a/API-CHANGELOG.md b/API-CHANGELOG.md index a04f2653285..665cba64eb0 100644 --- a/API-CHANGELOG.md +++ b/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 diff --git a/src/test/rpc/Connect_test.cpp b/src/test/rpc/Connect_test.cpp index 6f88bbb715c..9276bfb2d14 100644 --- a/src/test/rpc/Connect_test.cpp +++ b/src/test/rpc/Connect_test.cpp @@ -1,30 +1,100 @@ #include +#include +#include + #include +#include #include +#include +#include + 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 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); } } @@ -32,7 +102,9 @@ class Connect_test : public beast::unit_test::Suite void run() override { - testErrors(); + testStandalone(); + testMissingIp(); + testBadIp(); } }; diff --git a/src/xrpld/rpc/handlers/admin/peer/Connect.cpp b/src/xrpld/rpc/handlers/admin/peer/Connect.cpp index 568dcdaa267..6b2ca3f08cf 100644 --- a/src/xrpld/rpc/handlers/admin/peer/Connect.cpp +++ b/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)) {