fix: Add upper bound to limit

'type' should not matter if 'full' or 'accounts' is false. Relax the
restriction for 'type'

src/rpc/RPCHelpers.cpp

@@ -36,6 +36,7 @@
 #include <boost/json/array.hpp>
 #include <boost/json/object.hpp>
 #include <boost/json/parse.hpp>
+#include <boost/json/serialize.hpp>
 #include <boost/json/string.hpp>
 #include <boost/json/value.hpp>
 #include <boost/json/value_to.hpp>
@@ -49,6 +50,7 @@
 #include <ripple/basics/chrono.h>
 #include <ripple/basics/strHex.h>
 #include <ripple/beast/utility/Zero.h>
+#include <ripple/json/json_reader.h>
 #include <ripple/json/json_value.h>
 #include <ripple/protocol/AccountID.h>
 #include <ripple/protocol/Book.h>
@@ -1276,14 +1278,20 @@ specifiesCurrentOrClosedLedger(boost::json::object const& request)
 bool
 isAdminCmd(std::string const& method, boost::json::object const& request)
 {
-    // rippled considers the string as true: https://github.com/XRPLF/rippled/issues/5119
-    auto const isFieldSet = [&request](auto const field) {
-        return request.contains(field) and
-            ((request.at(field).is_bool() and request.at(field).as_bool()) or request.at(field).is_string());
-    };
-
     if (method == JS(ledger)) {
-        if (isFieldSet(JS(full)) or isFieldSet(JS(accounts)) or isFieldSet(JS(type)))
+        auto const requestStr = boost::json::serialize(request);
+        Json::Value jv;
+        Json::Reader{}.parse(requestStr, jv);
+        // rippled considers string/non-zero int/non-empty array/ non-empty json as true.
+        // Use rippled's API asBool to get the same result.
+        // https://github.com/XRPLF/rippled/issues/5119
+        auto const isFieldSet = [&jv](auto const field) { return jv.isMember(field) and jv[field].asBool(); };
+
+        // According to doc
+        // https://xrpl.org/docs/references/http-websocket-apis/public-api-methods/ledger-methods/ledger,
+        // full/accounts/type are admin only, but type only works when full/accounts are set, so we don't need to check
+        // type.
+        if (isFieldSet(JS(full)) or isFieldSet(JS(accounts)))
             return true;
     }
 

src/rpc/handlers/AccountTx.hpp

@@ -39,7 +39,6 @@
 #include <ripple/protocol/jss.h>
 
 #include <cstdint>
-#include <limits>
 #include <memory>
 #include <optional>
 #include <string>
@@ -57,8 +56,8 @@ class AccountTxHandler {
     std::shared_ptr<BackendInterface> sharedPtrBackend_;
 
 public:
-    // no max limit
     static auto constexpr LIMIT_MIN = 1;
+    static auto constexpr LIMIT_MAX = 1000;
     static auto constexpr LIMIT_DEFAULT = 200;
 
     /**
@@ -133,7 +132,7 @@ public:
             {JS(limit),
              validation::Type<uint32_t>{},
              validation::Min(1u),
-             modifiers::Clamp<int32_t>{LIMIT_MIN, std::numeric_limits<int32_t>::max()}},
+             modifiers::Clamp<int32_t>{LIMIT_MIN, LIMIT_MAX}},
             {JS(marker),
              meta::WithCustomError{
                  validation::Type<boost::json::object>{},

tests/unit/rpc/RPCHelpersTests.cpp

@@ -553,19 +553,41 @@ static auto
 generateTestValuesForParametersTest()
 {
     return std::vector<IsAdminCmdParamTestCaseBundle>{
+        {"ledgerEntry", "ledger_entry", R"({"type": false})", false},
+
         {"featureVetoedTrue", "feature", R"({"vetoed": true, "feature": "foo"})", true},
         {"featureVetoedFalse", "feature", R"({"vetoed": false, "feature": "foo"})", true},
-        {"ledgerFullTrue", "ledger", R"({"full": true})", true},
-        {"ledgerAccountsTrue", "ledger", R"({"accounts": true})", true},
-        {"ledgerTypeTrue", "ledger", R"({"type": true})", true},
-        {"ledgerFullFalse", "ledger", R"({"full": false})", false},
-        {"ledgerAccountsFalse", "ledger", R"({"accounts": false})", false},
-        {"ledgerTypeFalse", "ledger", R"({"type": false})", false},
-        {"ledgerEntry", "ledger_entry", R"({"type": false})", false},
-        {"ledgerFullIsStr", "ledger", R"({"full": "String"})", true},
-        {"ledgerAccoutsIsStr", "ledger", R"({"accounts": "String"})", true},
-        {"ledgerTypeIsStr", "ledger", R"({"type": "String"})", true},
         {"featureVetoedIsStr", "feature", R"({"vetoed": "String"})", true},
+
+        {"ledger", "ledger", R"({})", false},
+        {"ledgerWithType", "ledger", R"({"type": "fee"})", false},
+        {"ledgerFullTrue", "ledger", R"({"full": true})", true},
+        {"ledgerFullFalse", "ledger", R"({"full": false})", false},
+        {"ledgerFullIsStr", "ledger", R"({"full": "String"})", true},
+        {"ledgerFullIsEmptyStr", "ledger", R"({"full": ""})", false},
+        {"ledgerFullIsNumber1", "ledger", R"({"full": 1})", true},
+        {"ledgerFullIsNumber0", "ledger", R"({"full": 0})", false},
+        {"ledgerFullIsNull", "ledger", R"({"full": null})", false},
+        {"ledgerFullIsFloat0", "ledger", R"({"full": 0.0})", false},
+        {"ledgerFullIsFloat1", "ledger", R"({"full": 0.1})", true},
+        {"ledgerFullIsArray", "ledger", R"({"full": [1]})", true},
+        {"ledgerFullIsEmptyArray", "ledger", R"({"full": []})", false},
+        {"ledgerFullIsObject", "ledger", R"({"full": {"key": 1}})", true},
+        {"ledgerFullIsEmptyObject", "ledger", R"({"full": {}})", false},
+
+        {"ledgerAccountsTrue", "ledger", R"({"accounts": true})", true},
+        {"ledgerAccountsFalse", "ledger", R"({"accounts": false})", false},
+        {"ledgerAccountsIsStr", "ledger", R"({"accounts": "String"})", true},
+        {"ledgerAccountsIsEmptyStr", "ledger", R"({"accounts": ""})", false},
+        {"ledgerAccountsIsNumber1", "ledger", R"({"accounts": 1})", true},
+        {"ledgerAccountsIsNumber0", "ledger", R"({"accounts": 0})", false},
+        {"ledgerAccountsIsNull", "ledger", R"({"accounts": null})", false},
+        {"ledgerAccountsIsFloat0", "ledger", R"({"accounts": 0.0})", false},
+        {"ledgerAccountsIsFloat1", "ledger", R"({"accounts": 0.1})", true},
+        {"ledgerAccountsIsArray", "ledger", R"({"accounts": [1]})", true},
+        {"ledgerAccountsIsEmptyArray", "ledger", R"({"accounts": []})", false},
+        {"ledgerAccountsIsObject", "ledger", R"({"accounts": {"key": 1}})", true},
+        {"ledgerAccountsIsEmptyObject", "ledger", R"({"accounts": {}})", false},
     };
 }
 

tests/unit/rpc/handlers/AccountTxTests.cpp

@@ -769,14 +769,13 @@ TEST_F(RPCAccountTxHandlerTest, LimitAndMarker)
 
     auto const transactions = genTransactions(MINSEQ + 1, MAXSEQ - 1);
     auto const transCursor = TransactionsAndCursor{transactions, TransactionsCursor{12, 34}};
-    ON_CALL(*backend, fetchAccountTransactions).WillByDefault(Return(transCursor));
     EXPECT_CALL(
         *backend,
         fetchAccountTransactions(
             testing::_, testing::_, false, testing::Optional(testing::Eq(TransactionsCursor{10, 11})), testing::_
         )
     )
-        .Times(1);
+        .WillOnce(Return(transCursor));
 
     runSpawn([&, this](auto yield) {
         auto const handler = AnyHandler{AccountTxHandler{backend}};
@@ -804,6 +803,73 @@ TEST_F(RPCAccountTxHandlerTest, LimitAndMarker)
     });
 }
 
+TEST_F(RPCAccountTxHandlerTest, LimitIsCapped)
+{
+    backend->setRange(MINSEQ, MAXSEQ);
+
+    auto const transactions = genTransactions(MINSEQ + 1, MAXSEQ - 1);
+    auto const transCursor = TransactionsAndCursor{transactions, TransactionsCursor{12, 34}};
+    EXPECT_CALL(*backend, fetchAccountTransactions(testing::_, testing::_, false, testing::_, testing::_))
+        .WillOnce(Return(transCursor));
+
+    runSpawn([&, this](auto yield) {
+        auto const handler = AnyHandler{AccountTxHandler{backend}};
+        auto static const input = json::parse(fmt::format(
+            R"({{
+                "account": "{}",
+                "ledger_index_min": {},
+                "ledger_index_max": {},
+                "limit": 100000,
+                "forward": false
+            }})",
+            ACCOUNT,
+            -1,
+            -1
+        ));
+        auto const output = handler.process(input, Context{yield});
+        ASSERT_TRUE(output);
+        EXPECT_EQ(output.result->at("account").as_string(), ACCOUNT);
+        EXPECT_EQ(output.result->at("ledger_index_min").as_uint64(), MINSEQ);
+        EXPECT_EQ(output.result->at("ledger_index_max").as_uint64(), MAXSEQ);
+        EXPECT_EQ(output.result->at("limit").as_uint64(), AccountTxHandler::LIMIT_MAX);
+        EXPECT_EQ(output.result->at("transactions").as_array().size(), 2);
+    });
+}
+
+TEST_F(RPCAccountTxHandlerTest, LimitAllowedUpToCap)
+{
+    backend->setRange(MINSEQ, MAXSEQ);
+
+    auto const transactions = genTransactions(MINSEQ + 1, MAXSEQ - 1);
+    auto const transCursor = TransactionsAndCursor{transactions, TransactionsCursor{12, 34}};
+    EXPECT_CALL(*backend, fetchAccountTransactions(testing::_, testing::_, false, testing::_, testing::_))
+        .WillOnce(Return(transCursor));
+
+    runSpawn([&, this](auto yield) {
+        auto const handler = AnyHandler{AccountTxHandler{backend}};
+        auto static const input = json::parse(fmt::format(
+            R"({{
+                "account": "{}",
+                "ledger_index_min": {},
+                "ledger_index_max": {},
+                "limit": {},
+                "forward": false
+            }})",
+            ACCOUNT,
+            -1,
+            -1,
+            AccountTxHandler::LIMIT_MAX - 1
+        ));
+        auto const output = handler.process(input, Context{yield});
+        ASSERT_TRUE(output);
+        EXPECT_EQ(output.result->at("account").as_string(), ACCOUNT);
+        EXPECT_EQ(output.result->at("ledger_index_min").as_uint64(), MINSEQ);
+        EXPECT_EQ(output.result->at("ledger_index_max").as_uint64(), MAXSEQ);
+        EXPECT_EQ(output.result->at("limit").as_uint64(), AccountTxHandler::LIMIT_MAX - 1);
+        EXPECT_EQ(output.result->at("transactions").as_array().size(), 2);
+    });
+}
+
 TEST_F(RPCAccountTxHandlerTest, SpecificLedgerIndex)
 {
     backend->setRange(MINSEQ, MAXSEQ);