fix: not forward admin API (#1628)

src/rpc/RPCEngine.hpp

@@ -22,6 +22,7 @@
 #include "data/BackendInterface.hpp"
 #include "rpc/Counters.hpp"
 #include "rpc/Errors.hpp"
+#include "rpc/RPCHelpers.hpp"
 #include "rpc/WorkQueue.hpp"
 #include "rpc/common/HandlerProvider.hpp"
 #include "rpc/common/Types.hpp"
@@ -131,8 +132,13 @@ public:
     Result
     buildResponse(web::Context const& ctx)
     {
-        if (forwardingProxy_.shouldForward(ctx))
+        if (forwardingProxy_.shouldForward(ctx)) {
+            // Disallow forwarding of the admin api, only user api is allowed for security reasons.
+            if (isAdminCmd(ctx.method, ctx.params))
+                return Result{Status{RippledError::rpcNO_PERMISSION}};
+
             return forwardingProxy_.forward(ctx);
+        }
 
         if (backend_->isTooBusy()) {
             LOG(log_.error()) << "Database is too busy. Rejecting request";

src/rpc/RPCHelpers.cpp

@@ -1273,6 +1273,23 @@ specifiesCurrentOrClosedLedger(boost::json::object const& request)
     return false;
 }
 
+bool
+isAdminCmd(std::string const& method, boost::json::object const& request)
+{
+    auto const isFieldSet = [&request](auto const field) {
+        return request.contains(field) and request.at(field).is_bool() and request.at(field).as_bool();
+    };
+
+    if (method == JS(ledger)) {
+        if (isFieldSet(JS(full)) or isFieldSet(JS(accounts)) or isFieldSet(JS(type)))
+            return true;
+    }
+
+    if (method == JS(feature) and request.contains(JS(vetoed)))
+        return true;
+    return false;
+}
+
 std::variant<ripple::uint256, Status>
 getNFTID(boost::json::object const& request)
 {

src/rpc/RPCHelpers.hpp

@@ -557,6 +557,16 @@ parseIssue(boost::json::object const& issue);
 bool
 specifiesCurrentOrClosedLedger(boost::json::object const& request);
 
+/**
+ * @brief Check whether a request requires administrative privileges on rippled side.
+ *
+ * @param method The method name to check
+ * @param request The request to check
+ * @return true if the request requires ADMIN role
+ */
+bool
+isAdminCmd(std::string const& method, boost::json::object const& request);
+
 /**
  * @brief Get the NFTID from the request
  *

src/rpc/common/impl/ForwardingProxy.hpp

@@ -60,10 +60,6 @@ public:
         if (ctx.method == "subscribe" || ctx.method == "unsubscribe")
             return false;
 
-        // Disallow forwarding of the admin api, only user api is allowed for security reasons.
-        if (ctx.method == "feature" and request.contains("vetoed"))
-            return false;
-
         if (handlerProvider_->isClioOnly(ctx.method))
             return false;
 

tests/unit/rpc/ForwardingProxyTests.cpp

@@ -259,22 +259,6 @@ TEST_F(RPCForwardingProxyTest, ShouldForwardReturnsFalseIfAPIVersionIsV2)
     });
 }
 
-TEST_F(RPCForwardingProxyTest, ShouldNeverForwardFeatureWithVetoedFlag)
-{
-    auto const apiVersion = 1u;
-    auto const method = "feature";
-    auto const params = json::parse(R"({"vetoed": true, "feature": "foo"})");
-
-    runSpawn([&](auto yield) {
-        auto const range = backend->fetchLedgerRange();
-        auto const ctx =
-            web::Context(yield, method, apiVersion, params.as_object(), nullptr, tagFactory, *range, CLIENT_IP, true);
-
-        auto const res = proxy.shouldForward(ctx);
-        ASSERT_FALSE(res);
-    });
-}
-
 TEST_F(RPCForwardingProxyTest, ShouldNeverForwardSubscribe)
 {
     auto const apiVersion = 1u;

tests/unit/rpc/RPCHelpersTests.cpp

@@ -25,6 +25,7 @@
 #include "util/AsioContextTestFixture.hpp"
 #include "util/MockBackendTestFixture.hpp"
 #include "util/MockPrometheus.hpp"
+#include "util/NameGenerator.hpp"
 #include "util/TestObject.hpp"
 
 #include <boost/asio/impl/spawn.hpp>
@@ -539,3 +540,41 @@ TEST_F(RPCHelpersTest, ParseIssue)
         std::runtime_error
     );
 }
+
+struct IsAdminCmdParamTestCaseBundle {
+    std::string testName;
+    std::string method;
+    std::string testJson;
+    bool expected;
+};
+
+struct IsAdminCmdParameterTest : public TestWithParam<IsAdminCmdParamTestCaseBundle> {};
+
+static auto
+generateTestValuesForParametersTest()
+{
+    return std::vector<IsAdminCmdParamTestCaseBundle>{
+        {"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}
+    };
+}
+
+INSTANTIATE_TEST_CASE_P(
+    IsAdminCmdTest,
+    IsAdminCmdParameterTest,
+    ValuesIn(generateTestValuesForParametersTest()),
+    tests::util::NameGenerator
+);
+
+TEST_P(IsAdminCmdParameterTest, Test)
+{
+    auto const testBundle = GetParam();
+    EXPECT_EQ(isAdminCmd(testBundle.method, boost::json::parse(testBundle.testJson).as_object()), testBundle.expected);
+}