fix: Don't cache requests with params (#3113)

`ResponseExpirationCache` is using only `method` (or `command`) as a
key, but responses might be different with different params. This may
lead to a client getting incorrect response because of caching. This PR
fixes it by caching only requests without any additional params.

src/etl/LoadBalancer.cpp

@@ -277,7 +277,7 @@ LoadBalancer::forwardToRippled(
     auto const cmd = boost::json::value_to<std::string>(request.at("command"));
     if (shouldUseCache(isAdmin)) {
         // NOLINTNEXTLINE(bugprone-unchecked-optional-access)
-        if (auto cachedResponse = forwardingCache_->get(cmd); cachedResponse) {
+        if (auto cachedResponse = forwardingCache_->get(cmd, request); cachedResponse) {
             forwardingCounters_.cacheHit.get() += 1;
             return *std::move(cachedResponse);
         }
@@ -311,8 +311,10 @@ LoadBalancer::forwardToRippled(
     }
 
     if (response) {
-        if (shouldUseCache(isAdmin) and not response->contains("error"))
-            forwardingCache_->put(cmd, *response);  // NOLINT(bugprone-unchecked-optional-access)
+        if (shouldUseCache(isAdmin) and not response->contains("error")) {
+            // NOLINTNEXTLINE(bugprone-unchecked-optional-access)
+            forwardingCache_->put(cmd, request, *response);
+        }
         return *std::move(response);
     }
 

src/rpc/RPCEngine.hpp

@@ -141,7 +141,7 @@ public:
         }
 
         if (not ctx.isAdmin and responseCache_) {
-            if (auto res = responseCache_->get(ctx.method); res.has_value())
+            if (auto res = responseCache_->get(ctx.method, ctx.params); res.has_value())
                 return Result{*std::move(res)};
         }
 
@@ -174,7 +174,7 @@ public:
             if (not v) {
                 notifyErrored(ctx.method);
             } else if (not ctx.isAdmin and responseCache_) {
-                responseCache_->put(ctx.method, v.result->as_object());
+                responseCache_->put(ctx.method, ctx.params, v.result->as_object());
             }
 
             return Result{std::move(v)};

src/util/ResponseExpirationCache.cpp

@@ -4,11 +4,14 @@
 
 #include <boost/json/object.hpp>
 
+#include <algorithm>
+#include <array>
 #include <chrono>
 #include <mutex>
 #include <optional>
 #include <shared_mutex>
 #include <string>
+#include <string_view>
 #include <utility>
 
 namespace util {
@@ -44,9 +47,37 @@ ResponseExpirationCache::shouldCache(std::string const& cmd)
     return cache_.contains(cmd);
 }
 
-std::optional<boost::json::object>
-ResponseExpirationCache::get(std::string const& cmd) const
+bool
+ResponseExpirationCache::isBareRequest(boost::json::object const& request)
 {
+    // Keys that identify the command or are safe to ignore for caching purposes.
+    //
+    // "command" and "method" merely name the RPC command — they do not affect response content.
+    //
+    // "id" is safe to ignore because the web layer re-applies the current request's id to the
+    // response AFTER the cache lookup (see src/web/RPCServerHandler.hpp), so a cached body never
+    // leaks a stale id back to the client.  Ignoring "id" is also necessary for WebSocket
+    // requests, which almost always carry an id and would otherwise never benefit from caching.
+    //
+    // "api_version" is intentionally NOT in this ignore-set: it can change the response body
+    // content (not just an echoed field), so requests carrying it must bypass the cache entirely.
+    static constexpr auto kIgnoredKeys =
+        std::to_array<std::string_view>({"command", "method", "id"});
+
+    for (auto const& kv : request) {
+        std::string_view const key{kv.key()};
+        if (not std::ranges::contains(kIgnoredKeys, key))
+            return false;
+    }
+    return true;
+}
+
+std::optional<boost::json::object>
+ResponseExpirationCache::get(std::string const& cmd, boost::json::object const& request) const
+{
+    if (not isBareRequest(request))
+        return std::nullopt;
+
     auto it = cache_.find(cmd);
     if (it == cache_.end())
         return std::nullopt;
@@ -59,8 +90,15 @@ ResponseExpirationCache::get(std::string const& cmd) const
 }
 
 void
-ResponseExpirationCache::put(std::string const& cmd, boost::json::object const& response)
+ResponseExpirationCache::put(
+    std::string const& cmd,
+    boost::json::object const& request,
+    boost::json::object const& response
+)
 {
+    if (not isBareRequest(request))
+        return;
+
     if (not shouldCache(cmd))
         return;
 

src/util/ResponseExpirationCache.hpp

@@ -62,6 +62,19 @@ class ResponseExpirationCache {
     bool
     shouldCache(std::string const& cmd);
 
+    /**
+     * @brief Check whether a request is "bare" (carries no response-customizing params).
+     *
+     * A request is bare iff every key it contains is one of: "command", "method", "id".
+     * Any other key (e.g. "api_version", "hash", "counters", "ledger_index") makes the request
+     * non-bare, and the cache must be bypassed for it.
+     *
+     * @param request The request object to inspect
+     * @return true if the request is bare; false otherwise
+     */
+    static bool
+    isBareRequest(boost::json::object const& request);
+
 public:
     /**
      * @brief Construct a new Cache object
@@ -83,20 +96,30 @@ public:
     /**
      * @brief Get a response from the cache
      *
+     * Only bare requests (those carrying no response-customizing params beyond "command", "method",
+     * and "id") are served from the cache. Non-bare requests always return std::nullopt.
+     *
      * @param cmd The command to get the response for
-     * @return The response if it exists or std::nullopt otherwise
+     * @param request The full request object; used to determine whether the request is bare
+     * @return The cached response if available and not expired, or std::nullopt otherwise
      */
     [[nodiscard]] std::optional<boost::json::object>
-    get(std::string const& cmd) const;
+    get(std::string const& cmd, boost::json::object const& request) const;
 
     /**
      * @brief Put a response into the cache if the request should be cached
      *
+     * Only bare requests (those carrying no response-customizing params beyond "command", "method",
+     * and "id") are stored in the cache. Non-bare requests are silently ignored.
+     *
      * @param cmd The command to store the response for
+     * @param request The full request object; used to determine whether the request is bare
      * @param response The response to store
      */
     void
-    put(std::string const& cmd, boost::json::object const& response);
+    put(std::string const& cmd,
+        boost::json::object const& request,
+        boost::json::object const& response);
 
     /**
      * @brief Invalidate all entries in the cache

tests/unit/etl/LoadBalancerTests.cpp

@@ -762,6 +762,42 @@ TEST_F(LoadBalancerForwardToRippledPrometheusTests, forwardingCacheEnabled)
     });
 }
 
+TEST_F(LoadBalancerForwardToRippledPrometheusTests, forwardingCacheBypassedForNonBareRequest)
+{
+    configJson_.as_object()["forwarding"] = boost::json::object{{"cache_timeout", 10.}};
+    EXPECT_CALL(sourceFactory_, makeSource).Times(2);
+    auto loadBalancer = makeLoadBalancer();
+
+    auto const nonBareRequest = boost::json::object{{"command", "server_info"}, {"counters", true}};
+
+    auto& cacheHitCounter = makeMock<CounterInt>("forwarding_cache_hit_counter", "");
+    auto& cacheMissCounter = makeMock<CounterInt>("forwarding_cache_miss_counter", "");
+    auto& successDurationCounter =
+        makeMock<CounterInt>("forwarding_duration_milliseconds_counter", "{status=\"success\"}");
+
+    EXPECT_CALL(cacheMissCounter, add(1)).Times(2);
+    EXPECT_CALL(cacheHitCounter, add(testing::_)).Times(0);
+    EXPECT_CALL(successDurationCounter, add(testing::_)).Times(2);
+
+    EXPECT_CALL(
+        sourceFactory_.sourceAt(0),
+        forwardToRippled(
+            nonBareRequest, clientIP_, LoadBalancer::kUserForwardingXUserValue, testing::_
+        )
+    )
+        .Times(2)
+        .WillRepeatedly(Return(response_));
+
+    runSpawn([&](boost::asio::yield_context yield) {
+        EXPECT_EQ(
+            loadBalancer->forwardToRippled(nonBareRequest, clientIP_, false, yield), response_
+        );
+        EXPECT_EQ(
+            loadBalancer->forwardToRippled(nonBareRequest, clientIP_, false, yield), response_
+        );
+    });
+}
+
 TEST_F(LoadBalancerForwardToRippledPrometheusTests, source0Fails)
 {
     EXPECT_CALL(sourceFactory_, makeSource).Times(2);

tests/unit/rpc/RPCEngineTests.cpp

@@ -417,17 +417,19 @@ TEST_P(RPCEngineCacheParameterTest, Test)
         handlerProvider
     );
     int callTime = 2;
+    auto const bareParams = boost::json::object{};
     EXPECT_CALL(*handlerProvider, isClioOnly).Times(callTime).WillRepeatedly(Return(false));
     if (testParam.expectedCacheEnabled) {
+        // Cache hit on second call: handler only invoked once.
         EXPECT_CALL(*backend_, isTooBusy).WillOnce(Return(false));
         EXPECT_CALL(*handlerProvider, getHandler)
-            .WillOnce(Return(AnyHandler{tests::common::HandlerFake{}}));
+            .WillOnce(Return(AnyHandler{tests::common::NoInputHandlerFake{}}));
 
     } else {
         EXPECT_CALL(*backend_, isTooBusy).Times(callTime).WillRepeatedly(Return(false));
         EXPECT_CALL(*handlerProvider, getHandler)
             .Times(callTime)
-            .WillRepeatedly(Return(AnyHandler{tests::common::HandlerFake{}}));
+            .WillRepeatedly(Return(AnyHandler{tests::common::NoInputHandlerFake{}}));
     }
 
     while (callTime-- != 0) {
@@ -436,7 +438,7 @@ TEST_P(RPCEngineCacheParameterTest, Test)
                 yield,
                 method,
                 1,
-                boost::json::parse(R"JSON({"hello": "world", "limit": 50})JSON").as_object(),
+                bareParams,
                 nullptr,
                 tagFactory,
                 LedgerRange{.minSequence = 0, .maxSequence = 30},
@@ -448,7 +450,63 @@ TEST_P(RPCEngineCacheParameterTest, Test)
             ASSERT_TRUE(res.response.has_value());
             EXPECT_EQ(
                 res.response.value(),
-                boost::json::parse(R"JSON({ "computed": "world_50"})JSON").as_object()
+                boost::json::parse(R"JSON({"computed": "test"})JSON").as_object()
+            );
+        });
+    }
+}
+
+TEST_F(RPCEngineTest, NonBareRequestBypassesCache)
+{
+    auto const cfgCache = ClioConfigDefinition{
+        {"server.max_queue_size", ConfigValue{ConfigType::Integer}.defaultValue(2)},
+        {"workers",
+         ConfigValue{ConfigType::Integer}.defaultValue(4).withConstraint(gValidateUint16)},
+        {"rpc.cache_timeout",
+         ConfigValue{ConfigType::Double}.defaultValue(10.0).withConstraint(gValidatePositiveDouble)}
+    };
+
+    auto const notAdmin = false;
+    auto const method = "server_info";
+    std::shared_ptr<RPCEngine<MockCounters>> engine = RPCEngine<MockCounters>::makeRPCEngine(
+        cfgCache,
+        backend_,
+        mockLoadBalancerPtr_,
+        dosGuard,
+        queue,
+        *mockCountersPtr_,
+        handlerProvider
+    );
+
+    auto const nonBareParams =
+        boost::json::parse(R"JSON({"hello": "world", "limit": 50})JSON").as_object();
+
+    int callTime = 2;
+    EXPECT_CALL(*handlerProvider, isClioOnly).Times(callTime).WillRepeatedly(Return(false));
+    EXPECT_CALL(*backend_, isTooBusy).Times(callTime).WillRepeatedly(Return(false));
+    EXPECT_CALL(*handlerProvider, getHandler)
+        .Times(callTime)
+        .WillRepeatedly(Return(AnyHandler{tests::common::HandlerFake{}}));
+
+    while (callTime-- != 0) {
+        runSpawn([&](auto yield) {
+            auto const ctx = web::Context(
+                yield,
+                method,
+                1,
+                nonBareParams,
+                nullptr,
+                tagFactory,
+                LedgerRange{.minSequence = 0, .maxSequence = 30},
+                "127.0.0.2",
+                notAdmin
+            );
+
+            auto const res = engine->buildResponse(ctx);
+            ASSERT_TRUE(res.response.has_value());
+            EXPECT_EQ(
+                res.response.value(),
+                boost::json::parse(R"JSON({"computed": "world_50"})JSON").as_object()
             );
         });
     }

tests/unit/util/ResponseExpirationCacheTests.cpp

@@ -11,40 +11,104 @@ using namespace util;
 struct ResponseExpirationCacheTests : public ::testing::Test {
 protected:
     ResponseExpirationCache cache_{std::chrono::seconds{100}, {"key"}};
+    boost::json::object bareRequest_{{"command", "key"}};
     boost::json::object object_{{"key", "value"}};
 };
 
 TEST_F(ResponseExpirationCacheTests, PutAndGetNotExpired)
 {
-    EXPECT_FALSE(cache_.get("key").has_value());
+    EXPECT_FALSE(cache_.get("key", bareRequest_).has_value());
 
-    cache_.put("key", object_);
-    auto result = cache_.get("key");
+    cache_.put("key", bareRequest_, object_);
+    auto result = cache_.get("key", bareRequest_);
     ASSERT_TRUE(result.has_value());
     EXPECT_EQ(*result, object_);  // NOLINT(bugprone-unchecked-optional-access)
-    result = cache_.get("key2");
+    result = cache_.get("key2", bareRequest_);
     ASSERT_FALSE(result.has_value());
 
-    cache_.put("key2", object_);
-    result = cache_.get("key2");
+    cache_.put("key2", bareRequest_, object_);
+    result = cache_.get("key2", bareRequest_);
     ASSERT_FALSE(result.has_value());
 }
 
 TEST_F(ResponseExpirationCacheTests, Invalidate)
 {
-    cache_.put("key", object_);
+    cache_.put("key", bareRequest_, object_);
     cache_.invalidate();
-    EXPECT_FALSE(cache_.get("key").has_value());
+    EXPECT_FALSE(cache_.get("key", bareRequest_).has_value());
 }
 
 TEST_F(ResponseExpirationCacheTests, GetExpired)
 {
     ResponseExpirationCache cache{std::chrono::milliseconds{1}, {"key"}};
     auto const response = boost::json::object{{"key", "value"}};
+    auto const req = boost::json::object{{"command", "key"}};
 
-    cache.put("key", response);
+    cache.put("key", req, response);
     std::this_thread::sleep_for(std::chrono::milliseconds{2});
 
-    auto const result = cache.get("key");
+    auto const result = cache.get("key", req);
     EXPECT_FALSE(result);
 }
+
+TEST_F(ResponseExpirationCacheTests, BareRequestCommandOnlyIsCached)
+{
+    boost::json::object const req{{"command", "key"}};
+    cache_.put("key", req, object_);
+    auto const result = cache_.get("key", req);
+    ASSERT_TRUE(result.has_value());
+    EXPECT_EQ(*result, object_);  // NOLINT(bugprone-unchecked-optional-access)
+}
+
+TEST_F(ResponseExpirationCacheTests, BareRequestWithIdIsCached)
+{
+    boost::json::object const req{{"command", "key"}, {"id", 42}};
+    cache_.put("key", req, object_);
+    auto const result = cache_.get("key", req);
+    ASSERT_TRUE(result.has_value());
+    EXPECT_EQ(*result, object_);  // NOLINT(bugprone-unchecked-optional-access)
+}
+
+TEST_F(ResponseExpirationCacheTests, BareRequestMethodAndIdIsCached)
+{
+    boost::json::object const req{{"method", "key"}, {"id", "req-1"}};
+    cache_.put("key", req, object_);
+    auto const result = cache_.get("key", req);
+    ASSERT_TRUE(result.has_value());
+    EXPECT_EQ(*result, object_);  // NOLINT(bugprone-unchecked-optional-access)
+}
+
+TEST_F(ResponseExpirationCacheTests, NonBareGetReturnNulloptAfterBarePut)
+{
+    boost::json::object const bareReq{{"command", "key"}};
+    boost::json::object const nonBareReq{{"command", "key"}, {"limit", 50}};
+
+    cache_.put("key", bareReq, object_);
+    ASSERT_TRUE(cache_.get("key", bareReq).has_value());
+    EXPECT_FALSE(cache_.get("key", nonBareReq).has_value());
+}
+
+TEST_F(ResponseExpirationCacheTests, NonBarePutDoesNotStore)
+{
+    boost::json::object const nonBareReq{{"command", "key"}, {"limit", 50}};
+    boost::json::object const bareReq{{"command", "key"}};
+
+    cache_.put("key", nonBareReq, object_);
+    EXPECT_FALSE(cache_.get("key", bareReq).has_value());
+}
+
+TEST_F(ResponseExpirationCacheTests, ApiVersionMakesRequestNonBare)
+{
+    boost::json::object const req{{"command", "key"}, {"api_version", 1}};
+    cache_.put("key", req, object_);
+    EXPECT_FALSE(cache_.get("key", req).has_value());
+}
+
+TEST_F(ResponseExpirationCacheTests, BareRequestIdOnlyIsCached)
+{
+    boost::json::object const req{{"id", 7}};
+    cache_.put("key", req, object_);
+    auto const result = cache_.get("key", req);
+    ASSERT_TRUE(result.has_value());
+    EXPECT_EQ(*result, object_);  // NOLINT(bugprone-unchecked-optional-access)
+}