fix: account_tx limit parameter validation for malformed values (#5891)

This change fixes the `account_tx` RPC method to properly validate malformed limit parameter values. Previously, invalid values like `0`, `1.2`, `"10"`, `true`, `false`, `-1`, `[]`, `{}`, etc. were either accepted without errors or caused internal errors. Now all malformed values correctly return the `invalidParams` error.

Co-authored-by: Bart Thomee <11445373+bthomee@users.noreply.github.com>

src/test/rpc/AccountOffers_test.cpp

@@ -190,12 +190,6 @@ public:
         }
 
         {
-            // now make a limit (= 0) query for the same data
-            // since we operate on the admin port, the limit
-            // value of 0 is not adjusted into tuned ranges for admin requests
-            // so we literally get 0 elements in that case. For non-admin
-            // requests, we get limit defaults applied thus all our results
-            // come back (we are below the min results limit)
             Json::Value jvParams;
             jvParams[jss::account] = bob.human();
             jvParams[jss::limit] = 0u;
@@ -203,18 +197,7 @@ public:
                 "json",
                 "account_offers",
                 jvParams.toStyledString())[jss::result];
-            auto const& jro = jrr[jss::offers];
-            if (asAdmin)
-            {
-                // limit == 0 is invalid
-                BEAST_EXPECT(jrr.isMember(jss::error_message));
-            }
-            else
-            {
-                // Call should enforce min limit of 10
-                BEAST_EXPECT(checkArraySize(jro, 3u));
-                BEAST_EXPECT(!jrr.isMember(jss::marker));
-            }
+            BEAST_EXPECT(jrr.isMember(jss::error_message));
         }
     }
 

src/test/rpc/AccountTx_test.cpp

@@ -193,26 +193,26 @@ class AccountTx_test : public beast::unit_test::suite
                 j[jss::result][jss::error] == RPC::get_error_info(code).token;
         };
 
-        Json::Value jParms;
-        jParms[jss::api_version] = apiVersion;
+        Json::Value jParams;
+        jParams[jss::api_version] = apiVersion;
 
         BEAST_EXPECT(isErr(
-            env.rpc("json", "account_tx", to_string(jParms)),
+            env.rpc("json", "account_tx", to_string(jParams)),
             rpcINVALID_PARAMS));
 
-        jParms[jss::account] = "0xDEADBEEF";
+        jParams[jss::account] = "0xDEADBEEF";
 
         BEAST_EXPECT(isErr(
-            env.rpc("json", "account_tx", to_string(jParms)),
+            env.rpc("json", "account_tx", to_string(jParams)),
             rpcACT_MALFORMED));
 
-        jParms[jss::account] = A1.human();
+        jParams[jss::account] = A1.human();
         BEAST_EXPECT(hasTxs(
-            env.rpc(apiVersion, "json", "account_tx", to_string(jParms))));
+            env.rpc(apiVersion, "json", "account_tx", to_string(jParams))));
 
         // Ledger min/max index
         {
-            Json::Value p{jParms};
+            Json::Value p{jParams};
             p[jss::ledger_index_min] = -1;
             p[jss::ledger_index_max] = -1;
             BEAST_EXPECT(hasTxs(
@@ -247,7 +247,7 @@ class AccountTx_test : public beast::unit_test::suite
         }
         // Ledger index min only
         {
-            Json::Value p{jParms};
+            Json::Value p{jParams};
             p[jss::ledger_index_min] = -1;
             BEAST_EXPECT(hasTxs(
                 env.rpc(apiVersion, "json", "account_tx", to_string(p))));
@@ -270,7 +270,7 @@ class AccountTx_test : public beast::unit_test::suite
 
         // Ledger index max only
         {
-            Json::Value p{jParms};
+            Json::Value p{jParams};
             p[jss::ledger_index_max] = -1;
             BEAST_EXPECT(hasTxs(
                 env.rpc(apiVersion, "json", "account_tx", to_string(p))));
@@ -298,7 +298,7 @@ class AccountTx_test : public beast::unit_test::suite
 
         // Ledger Sequence
         {
-            Json::Value p{jParms};
+            Json::Value p{jParams};
 
             p[jss::ledger_index] = env.closed()->info().seq;
             BEAST_EXPECT(hasTxs(
@@ -319,7 +319,7 @@ class AccountTx_test : public beast::unit_test::suite
 
         // Ledger Hash
         {
-            Json::Value p{jParms};
+            Json::Value p{jParams};
 
             p[jss::ledger_hash] = to_string(env.closed()->info().hash);
             BEAST_EXPECT(hasTxs(
@@ -332,9 +332,9 @@ class AccountTx_test : public beast::unit_test::suite
         // Ledger index max/min/index all specified
         // ERRORS out with invalid Parenthesis
         {
-            jParms[jss::account] = "0xDEADBEEF";
-            jParms[jss::account] = A1.human();
-            Json::Value p{jParms};
+            jParams[jss::account] = "0xDEADBEEF";
+            jParams[jss::account] = A1.human();
+            Json::Value p{jParams};
 
             p[jss::ledger_index_max] = -1;
             p[jss::ledger_index_min] = -1;
@@ -351,7 +351,7 @@ class AccountTx_test : public beast::unit_test::suite
 
         // Ledger index max only
         {
-            Json::Value p{jParms};
+            Json::Value p{jParams};
             p[jss::ledger_index_max] = env.current()->info().seq;
             if (apiVersion < 2u)
                 BEAST_EXPECT(hasTxs(
@@ -382,7 +382,7 @@ class AccountTx_test : public beast::unit_test::suite
         }
         // test binary and forward for bool/non bool values
         {
-            Json::Value p{jParms};
+            Json::Value p{jParams};
             p[jss::binary] = "asdf";
             if (apiVersion < 2u)
             {
@@ -410,6 +410,117 @@ class AccountTx_test : public beast::unit_test::suite
             result = env.rpc("json", "account_tx", to_string(p));
             BEAST_EXPECT(result[jss::result][jss::status] == "success");
         }
+        // test limit with malformed values
+        {
+            Json::Value p{jParams};
+
+            // Test case: limit = 0 should fail (below minimum)
+            p[jss::limit] = 0;
+            BEAST_EXPECT(isErr(
+                env.rpc("json", "account_tx", to_string(p)),
+                rpcINVALID_PARAMS));
+
+            // Test case: limit = 1.2 should fail (not an integer)
+            p[jss::limit] = 1.2;
+            BEAST_EXPECT(
+                env.rpc(
+                    "json",
+                    "account_tx",
+                    to_string(p))[jss::result][jss::error_message] ==
+                RPC::expected_field_message(jss::limit, "unsigned integer"));
+
+            // Test case: limit = "10" should fail (string instead of integer)
+            p[jss::limit] = "10";
+            BEAST_EXPECT(
+                env.rpc(
+                    "json",
+                    "account_tx",
+                    to_string(p))[jss::result][jss::error_message] ==
+                RPC::expected_field_message(jss::limit, "unsigned integer"));
+
+            // Test case: limit = true should fail (boolean instead of integer)
+            p[jss::limit] = true;
+            BEAST_EXPECT(
+                env.rpc(
+                    "json",
+                    "account_tx",
+                    to_string(p))[jss::result][jss::error_message] ==
+                RPC::expected_field_message(jss::limit, "unsigned integer"));
+
+            // Test case: limit = false should fail (boolean instead of integer)
+            p[jss::limit] = false;
+            BEAST_EXPECT(
+                env.rpc(
+                    "json",
+                    "account_tx",
+                    to_string(p))[jss::result][jss::error_message] ==
+                RPC::expected_field_message(jss::limit, "unsigned integer"));
+
+            // Test case: limit = -1 should fail (negative number)
+            p[jss::limit] = -1;
+            BEAST_EXPECT(
+                env.rpc(
+                    "json",
+                    "account_tx",
+                    to_string(p))[jss::result][jss::error_message] ==
+                RPC::expected_field_message(jss::limit, "unsigned integer"));
+
+            // Test case: limit = [] should fail (array instead of integer)
+            p[jss::limit] = Json::Value(Json::arrayValue);
+            BEAST_EXPECT(
+                env.rpc(
+                    "json",
+                    "account_tx",
+                    to_string(p))[jss::result][jss::error_message] ==
+                RPC::expected_field_message(jss::limit, "unsigned integer"));
+
+            // Test case: limit = {} should fail (object instead of integer)
+            p[jss::limit] = Json::Value(Json::objectValue);
+            BEAST_EXPECT(
+                env.rpc(
+                    "json",
+                    "account_tx",
+                    to_string(p))[jss::result][jss::error_message] ==
+                RPC::expected_field_message(jss::limit, "unsigned integer"));
+
+            // Test case: limit = "malformed" should fail (malformed string)
+            p[jss::limit] = "malformed";
+            BEAST_EXPECT(
+                env.rpc(
+                    "json",
+                    "account_tx",
+                    to_string(p))[jss::result][jss::error_message] ==
+                RPC::expected_field_message(jss::limit, "unsigned integer"));
+
+            // Test case: limit = ["limit"] should fail (array with string)
+            p[jss::limit] = Json::Value(Json::arrayValue);
+            p[jss::limit].append("limit");
+            BEAST_EXPECT(
+                env.rpc(
+                    "json",
+                    "account_tx",
+                    to_string(p))[jss::result][jss::error_message] ==
+                RPC::expected_field_message(jss::limit, "unsigned integer"));
+
+            // Test case: limit = {"limit": 10} should fail (object with
+            // property)
+            p[jss::limit] = Json::Value(Json::objectValue);
+            p[jss::limit][jss::limit] = 10;
+            BEAST_EXPECT(
+                env.rpc(
+                    "json",
+                    "account_tx",
+                    to_string(p))[jss::result][jss::error_message] ==
+                RPC::expected_field_message(jss::limit, "unsigned integer"));
+
+            // Test case: limit = 10 should succeed (valid integer)
+            p[jss::limit] = 10;
+            BEAST_EXPECT(
+                env.rpc(
+                    "json",
+                    "account_tx",
+                    to_string(p))[jss::result][jss::status] == "success");
+        }
     }
 
     void

src/test/rpc/Book_test.cpp

@@ -1633,6 +1633,20 @@ public:
                 "Invalid field 'limit', not unsigned integer.");
         }
 
+        {
+            Json::Value jvParams;
+            jvParams[jss::ledger_index] = "validated";
+            jvParams[jss::taker] = env.master.human();
+            jvParams[jss::limit] = 0;  // must be > 0
+            jvParams[jss::taker_pays][jss::currency] = "XRP";
+            jvParams[jss::taker_gets][jss::currency] = "USD";
+            jvParams[jss::taker_gets][jss::issuer] = gw.human();
+            auto const jrr = env.rpc(
+                "json", "book_offers", to_string(jvParams))[jss::result];
+            BEAST_EXPECT(jrr[jss::error] == "invalidParams");
+            BEAST_EXPECT(jrr[jss::error_message] == "Invalid field 'limit'.");
+        }
+
         {
             Json::Value jvParams;
             jvParams[jss::ledger_index] = "validated";
@@ -1710,11 +1724,6 @@ public:
         BEAST_EXPECT(jrr[jss::offers].size() == (asAdmin ? 1u : 0u));
         // NOTE - a marker field is not returned for this method
 
-        jvParams[jss::limit] = 0u;
-        jrr = env.rpc("json", "book_offers", to_string(jvParams))[jss::result];
-        BEAST_EXPECT(jrr[jss::offers].isArray());
-        BEAST_EXPECT(jrr[jss::offers].size() == 0u);
-
         jvParams[jss::limit] = RPC::Tuning::bookOffers.rmax + 1;
         jrr = env.rpc("json", "book_offers", to_string(jvParams))[jss::result];
         BEAST_EXPECT(jrr[jss::offers].isArray());

src/xrpld/rpc/detail/RPCHelpers.cpp

@@ -704,15 +704,21 @@ readLimitField(
     JsonContext const& context)
 {
     limit = range.rdefault;
-    if (auto const& jvLimit = context.params[jss::limit])
-    {
-        if (!(jvLimit.isUInt() || (jvLimit.isInt() && jvLimit.asInt() >= 0)))
-            return RPC::expected_field_error(jss::limit, "unsigned integer");
+    if (!context.params.isMember(jss::limit) ||
+        context.params[jss::limit].isNull())
+        return std::nullopt;
+
+    auto const& jvLimit = context.params[jss::limit];
+    if (!(jvLimit.isUInt() || (jvLimit.isInt() && jvLimit.asInt() >= 0)))
+        return RPC::expected_field_error(jss::limit, "unsigned integer");
+
+    limit = jvLimit.asUInt();
+    if (limit == 0)
+        return RPC::invalid_field_error(jss::limit);
+
+    if (!isUnlimited(context.role))
+        limit = std::max(range.rmin, std::min(range.rmax, limit));
 
-        limit = jvLimit.asUInt();
-        if (!isUnlimited(context.role))
-            limit = std::max(range.rmin, std::min(range.rmax, limit));
-    }
     return std::nullopt;
 }
 

src/xrpld/rpc/detail/Tuning.h

@@ -45,6 +45,9 @@ static LimitRange constexpr accountObjects = {10, 200, 400};
 /** Limits for the account_offers command. */
 static LimitRange constexpr accountOffers = {10, 200, 400};
 
+/** Limits for the account_tx command. */
+static LimitRange constexpr accountTx = {10, 200, 400};
+
 /** Limits for the book_offers command. */
 static LimitRange constexpr bookOffers = {0, 60, 100};
 

src/xrpld/rpc/handlers/AccountChannels.cpp

@@ -103,9 +103,6 @@ doAccountChannels(RPC::JsonContext& context)
     if (auto err = readLimitField(limit, RPC::Tuning::accountChannels, context))
         return *err;
 
-    if (limit == 0u)
-        return rpcError(rpcINVALID_PARAMS);
-
     Json::Value jsonChannels{Json::arrayValue};
     struct VisitData
     {

src/xrpld/rpc/handlers/AccountLines.cpp

@@ -120,9 +120,6 @@ doAccountLines(RPC::JsonContext& context)
     if (auto err = readLimitField(limit, RPC::Tuning::accountLines, context))
         return *err;
 
-    if (limit == 0)
-        return rpcError(rpcINVALID_PARAMS);
-
     // this flag allows the requester to ask incoming trustlines in default
     // state be omitted
     bool ignoreDefault = params.isMember(jss::ignore_default) &&

src/xrpld/rpc/handlers/AccountOffers.cpp

@@ -86,9 +86,6 @@ doAccountOffers(RPC::JsonContext& context)
     if (auto err = readLimitField(limit, RPC::Tuning::accountOffers, context))
         return *err;
 
-    if (limit == 0)
-        return RPC::invalid_field_error(jss::limit);
-
     Json::Value& jsonOffers(result[jss::offers] = Json::arrayValue);
     std::vector<std::shared_ptr<SLE const>> offers;
     uint256 startAfter = beast::zero;

src/xrpld/rpc/handlers/AccountTx.cpp

@@ -26,6 +26,8 @@
 #include <xrpld/rpc/DeliveredAmount.h>
 #include <xrpld/rpc/MPTokenIssuanceID.h>
 #include <xrpld/rpc/Role.h>
+#include <xrpld/rpc/detail/RPCHelpers.h>
+#include <xrpld/rpc/detail/Tuning.h>
 
 #include <xrpl/json/json_value.h>
 #include <xrpl/ledger/ReadView.h>
@@ -429,7 +431,10 @@ doAccountTxJson(RPC::JsonContext& context)
         return RPC::invalid_field_error(jss::forward);
     }
 
-    args.limit = params.isMember(jss::limit) ? params[jss::limit].asUInt() : 0;
+    if (auto const err =
+            RPC::readLimitField(args.limit, RPC::Tuning::accountTx, context))
+        return *err;
+
     args.binary = params.isMember(jss::binary) && params[jss::binary].asBool();
     args.forward =
         params.isMember(jss::forward) && params[jss::forward].asBool();