diff --git a/src/rpc/RPCEngine.hpp b/src/rpc/RPCEngine.hpp index 26cac32d..43e4c815 100644 --- a/src/rpc/RPCEngine.hpp +++ b/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"; diff --git a/src/rpc/RPCHelpers.cpp b/src/rpc/RPCHelpers.cpp index 75f661b8..e4d169af 100644 --- a/src/rpc/RPCHelpers.cpp +++ b/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 getNFTID(boost::json::object const& request) { diff --git a/src/rpc/RPCHelpers.hpp b/src/rpc/RPCHelpers.hpp index fd92f65a..7fe2a5b6 100644 --- a/src/rpc/RPCHelpers.hpp +++ b/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 * diff --git a/src/rpc/common/impl/ForwardingProxy.hpp b/src/rpc/common/impl/ForwardingProxy.hpp index efc3d380..bd3a83b8 100644 --- a/src/rpc/common/impl/ForwardingProxy.hpp +++ b/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; diff --git a/tests/unit/rpc/ForwardingProxyTests.cpp b/tests/unit/rpc/ForwardingProxyTests.cpp index 3a09e4a6..5c37fa3c 100644 --- a/tests/unit/rpc/ForwardingProxyTests.cpp +++ b/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; diff --git a/tests/unit/rpc/RPCHelpersTests.cpp b/tests/unit/rpc/RPCHelpersTests.cpp index 15b32879..85dd4782 100644 --- a/tests/unit/rpc/RPCHelpersTests.cpp +++ b/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 @@ -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 {}; + +static auto +generateTestValuesForParametersTest() +{ + return std::vector{ + {"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); +}