APIv2(account_tx, noripple_check): return error on invalid input (#4620)

For the `account_tx` and `noripple_check` methods, perform input
validation for optional parameters such as "binary", "forward",
"strict", "transactions". Previously, when these parameters had invalid
values (e.g. not a bool), no error would be returned. Now, it returns an
`invalidParams` error.

* This updates the behavior to match Clio
  (https://github.com/XRPLF/clio).
* Since this is potentially a breaking change, it only applies to
  requests specifying api_version: 2.
* Fix #4543.

src/ripple/rpc/handlers/AccountInfo.cpp

@@ -134,10 +134,12 @@ doAccountInfo(RPC::JsonContext& context)
 
         result[jss::account_flags] = std::move(acctFlags);
 
-        // The document states that signer_lists is a bool, however
+        // The document[https://xrpl.org/account_info.html#account_info] states
-        // assigning any string value works. Do not allow this.
+        // that signer_lists is a bool, however assigning any string value
-        // This check is for api Version 2 onwards only
+        // works. Do not allow this. This check is for api Version 2 onwards
-        if (!params[jss::signer_lists].isBool() && context.apiVersion > 1)
+        // only
+        if (context.apiVersion > 1u && params.isMember(jss::signer_lists) &&
+            !params[jss::signer_lists].isBool())
         {
             RPC::inject_error(rpcINVALID_PARAMS, result);
             return result;

src/ripple/rpc/handlers/AccountTx.cpp

@@ -58,7 +58,7 @@ parseLedgerArgs(RPC::Context& context, Json::Value const& params)
     Json::Value response;
     // if ledger_index_min or max is specified, then ledger_hash or ledger_index
     // should not be specified. Error out if it is
-    if (context.apiVersion > 1)
+    if (context.apiVersion > 1u)
     {
         if ((params.isMember(jss::ledger_index_min) ||
              params.isMember(jss::ledger_index_max)) &&
@@ -162,7 +162,7 @@ getLedgerRange(
                     // if ledger_index_min or ledger_index_max is out of
                     // valid ledger range, error out. exclude -1 as
                     // it is a valid input
-                    if (context.apiVersion > 1)
+                    if (context.apiVersion > 1u)
                     {
                         if ((ls.max > uValidatedMax && ls.max != -1) ||
                             (ls.min < uValidatedMin && ls.min != 0))
@@ -389,6 +389,21 @@ doAccountTxJson(RPC::JsonContext& context)
     AccountTxArgs args;
     Json::Value response;
 
+    // The document[https://xrpl.org/account_tx.html#account_tx] states that
+    // binary and forward params are both boolean values, however, assigning any
+    // string value works. Do not allow this. This check is for api Version 2
+    // onwards only
+    if (context.apiVersion > 1u && params.isMember(jss::binary) &&
+        !params[jss::binary].isBool())
+    {
+        return rpcError(rpcINVALID_PARAMS);
+    }
+    if (context.apiVersion > 1u && params.isMember(jss::forward) &&
+        !params[jss::forward].isBool())
+    {
+        return rpcError(rpcINVALID_PARAMS);
+    }
+
     args.limit = params.isMember(jss::limit) ? params[jss::limit].asUInt() : 0;
     args.binary = params.isMember(jss::binary) && params[jss::binary].asBool();
     args.forward =

src/ripple/rpc/handlers/NoRippleCheck.cpp

@@ -83,6 +83,16 @@ doNoRippleCheck(RPC::JsonContext& context)
     if (params.isMember(jss::transactions))
         transactions = params["transactions"].asBool();
 
+    // The document[https://xrpl.org/noripple_check.html#noripple_check] states
+    // that transactions params is a boolean value, however, assigning any
+    // string value works. Do not allow this. This check is for api Version 2
+    // onwards only
+    if (context.apiVersion > 1u && params.isMember(jss::transactions) &&
+        !params[jss::transactions].isBool())
+    {
+        return rpcError(rpcINVALID_PARAMS);
+    }
+
     std::shared_ptr<ReadView const> ledger;
     auto result = RPC::lookupLedger(ledger, context);
     if (!ledger)

src/test/rpc/AccountTx_test.cpp

@@ -144,8 +144,6 @@ class AccountTx_test : public beast::unit_test::suite
         Json::Value jParms;
         jParms[jss::api_version] = apiVersion;
 
-        if (apiVersion < 2)
-        {
         BEAST_EXPECT(isErr(
             env.rpc("json", "account_tx", to_string(jParms)),
             rpcINVALID_PARAMS));
@@ -157,52 +155,61 @@ class AccountTx_test : public beast::unit_test::suite
             rpcACT_MALFORMED));
 
         jParms[jss::account] = A1.human();
-            BEAST_EXPECT(
+        BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(jParms))));
-                hasTxs(env.rpc("json", "account_tx", to_string(jParms))));
 
         // Ledger min/max index
         {
             Json::Value p{jParms};
             p[jss::ledger_index_min] = -1;
             p[jss::ledger_index_max] = -1;
-                BEAST_EXPECT(
+            BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(p))));
-                    hasTxs(env.rpc("json", "account_tx", to_string(p))));
 
             p[jss::ledger_index_min] = 0;
             p[jss::ledger_index_max] = 100;
+            if (apiVersion < 2u)
                 BEAST_EXPECT(
                     hasTxs(env.rpc("json", "account_tx", to_string(p))));
+            else
+                BEAST_EXPECT(isErr(
+                    env.rpc("json", "account_tx", to_string(p)),
+                    rpcLGR_IDX_MALFORMED));
 
             p[jss::ledger_index_min] = 1;
             p[jss::ledger_index_max] = 2;
+            if (apiVersion < 2u)
                 BEAST_EXPECT(
                     noTxs(env.rpc("json", "account_tx", to_string(p))));
+            else
+                BEAST_EXPECT(isErr(
+                    env.rpc("json", "account_tx", to_string(p)),
+                    rpcLGR_IDX_MALFORMED));
 
             p[jss::ledger_index_min] = 2;
             p[jss::ledger_index_max] = 1;
             BEAST_EXPECT(isErr(
                 env.rpc("json", "account_tx", to_string(p)),
-                    (RPC::apiMaximumSupportedVersion == 1
+                (apiVersion == 1 ? rpcLGR_IDXS_INVALID
-                         ? rpcLGR_IDXS_INVALID
                                  : rpcINVALID_LGR_RANGE)));
         }
-
         // Ledger index min only
         {
             Json::Value p{jParms};
             p[jss::ledger_index_min] = -1;
-                BEAST_EXPECT(
+            BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(p))));
-                    hasTxs(env.rpc("json", "account_tx", to_string(p))));
 
             p[jss::ledger_index_min] = 1;
+            if (apiVersion < 2u)
                 BEAST_EXPECT(
                     hasTxs(env.rpc("json", "account_tx", to_string(p))));
+            else
+                BEAST_EXPECT(isErr(
+                    env.rpc("json", "account_tx", to_string(p)),
+                    rpcLGR_IDX_MALFORMED));
 
             p[jss::ledger_index_min] = env.current()->info().seq;
             BEAST_EXPECT(isErr(
                 env.rpc("json", "account_tx", to_string(p)),
-                    (RPC::apiMaximumSupportedVersion == 1
+                (apiVersion == 1 ? rpcLGR_IDXS_INVALID
-                         ? rpcLGR_IDXS_INVALID
                                  : rpcINVALID_LGR_RANGE)));
         }
 
@@ -210,24 +217,25 @@ class AccountTx_test : public beast::unit_test::suite
         {
             Json::Value p{jParms};
             p[jss::ledger_index_max] = -1;
-                BEAST_EXPECT(
+            BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(p))));
-                    hasTxs(env.rpc("json", "account_tx", to_string(p))));
 
             p[jss::ledger_index_max] = env.current()->info().seq;
+            if (apiVersion < 2u)
                 BEAST_EXPECT(
                     hasTxs(env.rpc("json", "account_tx", to_string(p))));
+            else
+                BEAST_EXPECT(isErr(
+                    env.rpc("json", "account_tx", to_string(p)),
+                    rpcLGR_IDX_MALFORMED));
 
             p[jss::ledger_index_max] = 3;
-                BEAST_EXPECT(
+            BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(p))));
-                    hasTxs(env.rpc("json", "account_tx", to_string(p))));
 
             p[jss::ledger_index_max] = env.closed()->info().seq;
-                BEAST_EXPECT(
+            BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(p))));
-                    hasTxs(env.rpc("json", "account_tx", to_string(p))));
 
             p[jss::ledger_index_max] = env.closed()->info().seq - 1;
-                BEAST_EXPECT(
+            BEAST_EXPECT(noTxs(env.rpc("json", "account_tx", to_string(p))));
-                    noTxs(env.rpc("json", "account_tx", to_string(p))));
         }
 
         // Ledger Sequence
@@ -235,12 +243,10 @@ class AccountTx_test : public beast::unit_test::suite
             Json::Value p{jParms};
 
             p[jss::ledger_index] = env.closed()->info().seq;
-                BEAST_EXPECT(
+            BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(p))));
-                    hasTxs(env.rpc("json", "account_tx", to_string(p))));
 
             p[jss::ledger_index] = env.closed()->info().seq - 1;
-                BEAST_EXPECT(
+            BEAST_EXPECT(noTxs(env.rpc("json", "account_tx", to_string(p))));
-                    noTxs(env.rpc("json", "account_tx", to_string(p))));
 
             p[jss::ledger_index] = env.current()->info().seq;
             BEAST_EXPECT(isErr(
@@ -249,8 +255,7 @@ class AccountTx_test : public beast::unit_test::suite
 
             p[jss::ledger_index] = env.current()->info().seq + 1;
             BEAST_EXPECT(isErr(
-                    env.rpc("json", "account_tx", to_string(p)),
+                env.rpc("json", "account_tx", to_string(p)), rpcLGR_NOT_FOUND));
-                    rpcLGR_NOT_FOUND));
         }
 
         // Ledger Hash
@@ -258,17 +263,12 @@ class AccountTx_test : public beast::unit_test::suite
             Json::Value p{jParms};
 
             p[jss::ledger_hash] = to_string(env.closed()->info().hash);
-                BEAST_EXPECT(
+            BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(p))));
-                    hasTxs(env.rpc("json", "account_tx", to_string(p))));
 
-                p[jss::ledger_hash] =
+            p[jss::ledger_hash] = to_string(env.closed()->info().parentHash);
-                    to_string(env.closed()->info().parentHash);
+            BEAST_EXPECT(noTxs(env.rpc("json", "account_tx", to_string(p))));
-                BEAST_EXPECT(
-                    noTxs(env.rpc("json", "account_tx", to_string(p))));
         }
-        }
+
-        else
-        {
         // Ledger index max/min/index all specified
         // ERRORS out with invalid Parenthesis
         {
@@ -280,40 +280,56 @@ class AccountTx_test : public beast::unit_test::suite
             p[jss::ledger_index_min] = -1;
             p[jss::ledger_index] = -1;
 
+            if (apiVersion < 2u)
+                BEAST_EXPECT(
+                    hasTxs(env.rpc("json", "account_tx", to_string(p))));
+            else
                 BEAST_EXPECT(isErr(
                     env.rpc("json", "account_tx", to_string(p)),
                     rpcINVALID_PARAMS));
         }
 
-            // Ledger index min/max only
-            {
-                Json::Value p{jParms};
-                p[jss::ledger_index_max] = 100;
-                p[jss::ledger_index_min] = 0;
-                BEAST_EXPECT(isErr(
-                    env.rpc("json", "account_tx", to_string(p)),
-                    rpcLGR_IDX_MALFORMED));
-
-                p[jss::ledger_index_max] = -1;
-                p[jss::ledger_index_min] = -1;
-                BEAST_EXPECT(
-                    hasTxs(env.rpc("json", "account_tx", to_string(p))));
-
-                p[jss::ledger_index_min] = 2;
-                p[jss::ledger_index_max] = 1;
-                BEAST_EXPECT(isErr(
-                    env.rpc("json", "account_tx", to_string(p)),
-                    rpcINVALID_LGR_RANGE));
-            }
-
         // Ledger index max only
         {
             Json::Value p{jParms};
             p[jss::ledger_index_max] = env.current()->info().seq;
+            if (apiVersion < 2u)
+                BEAST_EXPECT(
+                    hasTxs(env.rpc("json", "account_tx", to_string(p))));
+            else
                 BEAST_EXPECT(isErr(
                     env.rpc("json", "account_tx", to_string(p)),
                     rpcLGR_IDX_MALFORMED));
         }
+        // test binary and forward for bool/non bool values
+        {
+            Json::Value p{jParms};
+            p[jss::binary] = "asdf";
+            if (apiVersion < 2u)
+            {
+                Json::Value result{env.rpc("json", "account_tx", to_string(p))};
+                BEAST_EXPECT(result[jss::result][jss::status] == "success");
+            }
+            else
+                BEAST_EXPECT(isErr(
+                    env.rpc("json", "account_tx", to_string(p)),
+                    rpcINVALID_PARAMS));
+
+            p[jss::binary] = true;
+            Json::Value result{env.rpc("json", "account_tx", to_string(p))};
+            BEAST_EXPECT(result[jss::result][jss::status] == "success");
+
+            p[jss::forward] = "true";
+            if (apiVersion < 2u)
+                BEAST_EXPECT(result[jss::result][jss::status] == "success");
+            else
+                BEAST_EXPECT(isErr(
+                    env.rpc("json", "account_tx", to_string(p)),
+                    rpcINVALID_PARAMS));
+
+            p[jss::forward] = false;
+            result = env.rpc("json", "account_tx", to_string(p));
+            BEAST_EXPECT(result[jss::result][jss::status] == "success");
         }
     }
 

src/test/rpc/NoRipple_test.cpp

@@ -19,6 +19,7 @@
 
 #include <ripple/protocol/Feature.h>
 #include <ripple/protocol/jss.h>
+#include <ripple/rpc/impl/RPCHelpers.h>
 #include <test/jtx.h>
 
 namespace ripple {
@@ -202,9 +203,12 @@ public:
     }
 
     void
-    testDefaultRipple(FeatureBitset features)
+    testDefaultRipple(FeatureBitset features, unsigned int apiVersion)
     {
-        testcase("Set default ripple on an account and check new trustlines");
+        testcase(
+            "Set default ripple on an account and check new trustlines "
+            "Version " +
+            std::to_string(apiVersion));
 
         using namespace jtx;
         Env env(*this, features);
@@ -221,9 +225,10 @@ public:
 
         env(trust(gw, USD(100), alice, 0));
         env(trust(gw, USD(100), bob, 0));
+        Json::Value params;
+        params[jss::api_version] = apiVersion;
 
         {
-            Json::Value params;
             params[jss::account] = gw.human();
             params[jss::peer] = alice.human();
 
@@ -232,7 +237,6 @@ public:
             BEAST_EXPECT(line0[jss::no_ripple_peer].asBool() == true);
         }
         {
-            Json::Value params;
             params[jss::account] = alice.human();
             params[jss::peer] = gw.human();
 
@@ -241,7 +245,6 @@ public:
             BEAST_EXPECT(line0[jss::no_ripple].asBool() == true);
         }
         {
-            Json::Value params;
             params[jss::account] = gw.human();
             params[jss::peer] = bob.human();
 
@@ -250,7 +253,6 @@ public:
             BEAST_EXPECT(line0[jss::no_ripple].asBool() == false);
         }
         {
-            Json::Value params;
             params[jss::account] = bob.human();
             params[jss::peer] = gw.human();
 
@@ -258,6 +260,22 @@ public:
             auto const& line0 = lines[jss::result][jss::lines][0u];
             BEAST_EXPECT(line0[jss::no_ripple_peer].asBool() == false);
         }
+        {
+            // test for transactions
+            {
+                params[jss::account] = bob.human();
+                params[jss::role] = "gateway";
+                params[jss::transactions] = "asdf";
+
+                auto lines =
+                    env.rpc("json", "noripple_check", to_string(params));
+                if (apiVersion < 2u)
+                    BEAST_EXPECT(lines[jss::result][jss::status] == "success");
+                else
+                    BEAST_EXPECT(
+                        lines[jss::result][jss::error] == "invalidParams");
+            }
+        }
     }
 
     void
@@ -266,9 +284,14 @@ public:
         testSetAndClear();
 
         auto withFeatsTests = [this](FeatureBitset features) {
+            for (auto testVersion = RPC::apiMinimumSupportedVersion;
+                 testVersion <= RPC::apiBetaVersion;
+                 ++testVersion)
+            {
+                testDefaultRipple(features, testVersion);
+            }
             testNegativeBalance(features);
             testPairwise(features);
-            testDefaultRipple(features);
         };
         using namespace jtx;
         auto const sa = supported_amendments();