From bf1a1f1662427fbf1a43bb951364eface469bdb7 Mon Sep 17 00:00:00 2001
From: Matthew Zipkin <pinheadmz@gmail.com>
Date: Fri, 7 Jul 2023 14:41:23 -0400
Subject: rpc: Avoid returning HTTP errors for JSON-RPC 2.0 requests

Avoid returning HTTP status errors for non-batch JSON-RPC 2.0 requests if the
RPC method failed but the HTTP request was otherwise valid. Batch requests
already did not return HTTP errors previously.
---
 src/httprpc.cpp    | 15 ++++++++++++---
 src/rpc/server.cpp | 19 ++++++++++++++-----
 src/rpc/server.h   |  2 +-
 3 files changed, 27 insertions(+), 9 deletions(-)

(limited to 'src')

diff --git a/src/httprpc.cpp b/src/httprpc.cpp
index 817c75f3b5..777ad32bbe 100644
--- a/src/httprpc.cpp
+++ b/src/httprpc.cpp
@@ -75,6 +75,9 @@ static bool g_rpc_whitelist_default = false;
 
 static void JSONErrorReply(HTTPRequest* req, UniValue objError, const JSONRPCRequest& jreq)
 {
+    // Sending HTTP errors is a legacy JSON-RPC behavior.
+    Assume(jreq.m_json_version != JSONRPCVersion::V2);
+
     // Send error reply from json-rpc error object
     int nStatus = HTTP_INTERNAL_SERVER_ERROR;
     int code = objError.find_value("code").getInt<int>();
@@ -201,7 +204,12 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req)
                 return false;
             }
 
-            reply = JSONRPCExec(jreq);
+            // Legacy 1.0/1.1 behavior is for failed requests to throw
+            // exceptions which return HTTP errors and RPC errors to the client.
+            // 2.0 behavior is to catch exceptions and return HTTP success with
+            // RPC errors, as long as there is not an actual HTTP server error.
+            const bool catch_errors{jreq.m_json_version == JSONRPCVersion::V2};
+            reply = JSONRPCExec(jreq, catch_errors);
 
         // array of requests
         } else if (valRequest.isArray()) {
@@ -226,10 +234,11 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req)
             // Execute each request
             reply = UniValue::VARR;
             for (size_t i{0}; i < valRequest.size(); ++i) {
-                // Batches include errors in the batch response, they do not throw
+                // Batches never throw HTTP errors, they are always just included
+                // in "HTTP OK" responses.
                 try {
                     jreq.parse(valRequest[i]);
-                    reply.push_back(JSONRPCExec(jreq));
+                    reply.push_back(JSONRPCExec(jreq, /*catch_errors=*/true));
                 } catch (UniValue& e) {
                     reply.push_back(JSONRPCReplyObj(NullUniValue, std::move(e), jreq.id, jreq.m_json_version));
                 } catch (const std::exception& e) {
diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp
index 8e150ef2b7..a30fd3b7d0 100644
--- a/src/rpc/server.cpp
+++ b/src/rpc/server.cpp
@@ -358,11 +358,20 @@ bool IsDeprecatedRPCEnabled(const std::string& method)
     return find(enabled_methods.begin(), enabled_methods.end(), method) != enabled_methods.end();
 }
 
-UniValue JSONRPCExec(const JSONRPCRequest& jreq)
-{
-    // Might throw exception. Single requests will throw and send HTTP error codes
-    // but inside a batch, we just include the error object and return HTTP 200
-    UniValue result = tableRPC.execute(jreq);
+UniValue JSONRPCExec(const JSONRPCRequest& jreq, bool catch_errors)
+{
+    UniValue result;
+    if (catch_errors) {
+        try {
+            result = tableRPC.execute(jreq);
+        } catch (UniValue& e) {
+            return JSONRPCReplyObj(NullUniValue, std::move(e), jreq.id, jreq.m_json_version);
+        } catch (const std::exception& e) {
+            return JSONRPCReplyObj(NullUniValue, JSONRPCError(RPC_MISC_ERROR, e.what()), jreq.id, jreq.m_json_version);
+        }
+    } else {
+        result = tableRPC.execute(jreq);
+    }
 
     return JSONRPCReplyObj(std::move(result), NullUniValue, jreq.id, jreq.m_json_version);
 }
diff --git a/src/rpc/server.h b/src/rpc/server.h
index 2761bcd9db..c0e7bc04ad 100644
--- a/src/rpc/server.h
+++ b/src/rpc/server.h
@@ -181,7 +181,7 @@ extern CRPCTable tableRPC;
 void StartRPC();
 void InterruptRPC();
 void StopRPC();
-UniValue JSONRPCExec(const JSONRPCRequest& jreq);
+UniValue JSONRPCExec(const JSONRPCRequest& jreq, bool catch_errors);
 
 // Drop witness when serializing for RPC?
 bool RPCSerializationWithoutWitness();
-- 
cgit v1.2.3