From ce86572274500c909c9390e64b6e66687ab47166 Mon Sep 17 00:00:00 2001
From: Alex Kremer <akremer@ripple.com>
Date: Fri, 12 Jan 2024 14:02:50 +0000
Subject: [PATCH] Fix forwarded flag placement (#1101)

Fixes #1091
---
 src/web/RPCServerHandler.h              |  6 ++
 unittests/web/RPCServerHandlerTests.cpp | 92 +++++++++++++++++++++++++
 2 files changed, 98 insertions(+)

diff --git a/src/web/RPCServerHandler.h b/src/web/RPCServerHandler.h
index 0dbc8000..6e56f267 100644
--- a/src/web/RPCServerHandler.h
+++ b/src/web/RPCServerHandler.h
@@ -211,6 +211,9 @@ private:
                 auto const isForwarded =
                     json.contains("forwarded") && json.at("forwarded").is_bool() && json.at("forwarded").as_bool();
 
+                if (isForwarded)
+                    json.erase("forwarded");
+
                 // if the result is forwarded - just use it as is
                 // if forwarded request has error, for http, error should be in "result"; for ws, error should
                 // be at top
@@ -221,6 +224,9 @@ private:
                     response[JS(result)] = json;
                 }
 
+                if (isForwarded)
+                    response["forwarded"] = true;
+
                 // for ws there is an additional field "status" in the response,
                 // otherwise the "status" is in the "result" field
                 if (connection->upgraded) {
diff --git a/unittests/web/RPCServerHandlerTests.cpp b/unittests/web/RPCServerHandlerTests.cpp
index 4a864c93..7042a1d9 100644
--- a/unittests/web/RPCServerHandlerTests.cpp
+++ b/unittests/web/RPCServerHandlerTests.cpp
@@ -168,6 +168,7 @@ TEST_F(WebRPCServerHandlerTest, HTTPForwardedPath)
 
     backend->setRange(MINSEQ, MAXSEQ);
 
+    // Note: forwarding always goes thru WS API
     static auto constexpr result = R"({
                                         "result": {
                                             "index": 1
@@ -197,6 +198,50 @@ TEST_F(WebRPCServerHandlerTest, HTTPForwardedPath)
     EXPECT_EQ(boost::json::parse(session->message), boost::json::parse(response));
 }
 
+TEST_F(WebRPCServerHandlerTest, HTTPForwardedErrorPath)
+{
+    static auto constexpr request = R"({
+                                        "method": "server_info",
+                                        "params": [{}]
+                                    })";
+
+    backend->setRange(MINSEQ, MAXSEQ);
+
+    // Note: forwarding always goes thru WS API
+    static auto constexpr result = R"({
+                                        "error": "error",
+                                        "error_code": 123,
+                                        "error_message": "error message",
+                                        "status": "error",
+                                        "type": "response",
+                                        "forwarded": true
+                                    })";
+    static auto constexpr response = R"({
+                                        "result":{
+                                            "error": "error",
+                                            "error_code": 123,
+                                            "error_message": "error message",
+                                            "status": "error",
+                                            "type": "response"
+                                        },
+                                        "forwarded": true,
+                                        "warnings":[
+                                            {
+                                                "id": 2001,
+                                                "message": "This is a clio server. clio only serves validated data. If you want to talk to rippled, include 'ledger_index':'current' in your request"
+                                            }
+                                        ]
+                                    })";
+    EXPECT_CALL(*rpcEngine, buildResponse(testing::_))
+        .WillOnce(testing::Return(boost::json::parse(result).as_object()));
+    EXPECT_CALL(*rpcEngine, notifyComplete("server_info", testing::_)).Times(1);
+
+    EXPECT_CALL(*etl, lastCloseAgeSeconds()).WillOnce(testing::Return(45));
+
+    (*handler)(request, session);
+    EXPECT_EQ(boost::json::parse(session->message), boost::json::parse(response));
+}
+
 TEST_F(WebRPCServerHandlerTest, WsForwardedPath)
 {
     session->upgraded = true;
@@ -207,6 +252,7 @@ TEST_F(WebRPCServerHandlerTest, WsForwardedPath)
 
     backend->setRange(MINSEQ, MAXSEQ);
 
+    // Note: forwarding always goes thru WS API
     static auto constexpr result = R"({
                                         "result": {
                                             "index": 1
@@ -238,6 +284,52 @@ TEST_F(WebRPCServerHandlerTest, WsForwardedPath)
     EXPECT_EQ(boost::json::parse(session->message), boost::json::parse(response));
 }
 
+TEST_F(WebRPCServerHandlerTest, WsForwardedErrorPath)
+{
+    session->upgraded = true;
+    static auto constexpr request = R"({
+                                        "command": "server_info",
+                                        "id": 99
+                                    })";
+
+    backend->setRange(MINSEQ, MAXSEQ);
+
+    // Note: forwarding always goes thru WS API
+    static auto constexpr result = R"({
+                                        "error": "error",
+                                        "error_code": 123,
+                                        "error_message": "error message",
+                                        "status": "error",
+                                        "type": "response",
+                                        "forwarded": true
+                                   })";
+    // WS error responses, unlike their successful counterpart, contain everything on top level without "result"
+    static auto constexpr response = R"({
+                                        "error": "error",
+                                        "error_code": 123,
+                                        "error_message": "error message",
+                                        "status": "error",
+                                        "type": "response",
+                                        "forwarded": true,
+                                        "id": 99,
+                                        "warnings": [
+                                            {
+                                                "id": 2001,
+                                                "message": "This is a clio server. clio only serves validated data. If you want to talk to rippled, include 'ledger_index':'current' in your request"
+                                            }
+                                        ]
+                                    })";
+    EXPECT_CALL(*rpcEngine, buildResponse(testing::_))
+        .WillOnce(testing::Return(boost::json::parse(result).as_object()));
+
+    // Forwarded errors counted as successful:
+    EXPECT_CALL(*rpcEngine, notifyComplete("server_info", testing::_)).Times(1);
+    EXPECT_CALL(*etl, lastCloseAgeSeconds()).WillOnce(testing::Return(45));
+
+    (*handler)(request, session);
+    EXPECT_EQ(boost::json::parse(session->message), boost::json::parse(response));
+}
+
 TEST_F(WebRPCServerHandlerTest, HTTPErrorPath)
 {
     static auto constexpr response = R"({