rpc: JSON-RPC 2.0 should not respond to "notifications"

pinheadmz · pinheadmz · commit e7ee80dcf2b6 · 2024-05-14T11:28:43.000-04:00
For JSON-RPC 2.0 requests we need to distinguish between
a missing "id" field and "id":null. This is accomplished
by making the JSONRPCRequest id property a
std::optional&lt;UniValue&gt; with a default value of
UniValue::VNULL.

A side-effect of this change for non-2.0 requests is that request which do not
specify an "id" field will no longer return "id": null in the response.
diff --git a/src/httprpc.cpp b/src/httprpc.cpp
@@ -211,6 +211,12 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req)
             const bool catch_errors{jreq.m_json_version == JSONRPCVersion::V2};
             reply = JSONRPCExec(jreq, catch_errors);
 
+            if (jreq.IsNotification()) {
+                // Even though we do execute notifications, we do not respond to them
+                req->WriteReply(HTTP_NO_CONTENT);
+                return true;
+            }
+
         // array of requests
         } else if (valRequest.isArray()) {
             // Check authorization for each request's method
@@ -235,15 +241,32 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req)
             reply = UniValue::VARR;
             for (size_t i{0}; i < valRequest.size(); ++i) {
                 // Batches never throw HTTP errors, they are always just included
-                // in "HTTP OK" responses.
+                // in "HTTP OK" responses. Notifications never get any response.
+                UniValue response;
                 try {
                     jreq.parse(valRequest[i]);
-                    reply.push_back(JSONRPCExec(jreq, /*catch_errors=*/true));
+                    response = JSONRPCExec(jreq, /*catch_errors=*/true);
                 } catch (UniValue& e) {
-                    reply.push_back(JSONRPCReplyObj(NullUniValue, std::move(e), jreq.id, jreq.m_json_version));
+                    response = JSONRPCReplyObj(NullUniValue, std::move(e), jreq.id, jreq.m_json_version);
                 } catch (const std::exception& e) {
-                    reply.push_back(JSONRPCReplyObj(NullUniValue, JSONRPCError(RPC_PARSE_ERROR, e.what()), jreq.id, jreq.m_json_version));
+                    response = JSONRPCReplyObj(NullUniValue, JSONRPCError(RPC_PARSE_ERROR, e.what()), jreq.id, jreq.m_json_version);
                 }
+                if (!jreq.IsNotification()) {
+                    reply.push_back(std::move(response));
+                }
+            }
+            // Return no response for an all-notification batch, but only if the
+            // batch request is non-empty. Technically according to the JSON-RPC
+            // 2.0 spec, an empty batch request should also return no response,
+            // However, if the batch request is empty, it means the request did
+            // not contain any JSON-RPC version numbers, so returning an empty
+            // response could break backwards compatibility with old RPC clients
+            // relying on previous behavior. Return an empty array instead of an
+            // empty response in this case to favor being backwards compatible
+            // over complying with the JSON-RPC 2.0 spec in this case.
+            if (reply.size() == 0 && valRequest.size() > 0) {
+                req->WriteReply(HTTP_NO_CONTENT);
+                return true;
             }
         }
         else
diff --git a/src/rpc/protocol.h b/src/rpc/protocol.h
@@ -10,6 +10,7 @@
 enum HTTPStatusCode
 {
     HTTP_OK                    = 200,
+    HTTP_NO_CONTENT            = 204,
     HTTP_BAD_REQUEST           = 400,
     HTTP_UNAUTHORIZED          = 401,
     HTTP_FORBIDDEN             = 403,
diff --git a/src/rpc/request.cpp b/src/rpc/request.cpp
@@ -37,7 +37,7 @@ UniValue JSONRPCRequestObj(const std::string& strMethod, const UniValue& params,
     return request;
 }
 
-UniValue JSONRPCReplyObj(UniValue result, UniValue error, UniValue id, JSONRPCVersion jsonrpc_version)
+UniValue JSONRPCReplyObj(UniValue result, UniValue error, std::optional<UniValue> id, JSONRPCVersion jsonrpc_version)
 {
     UniValue reply(UniValue::VOBJ);
     // Add JSON-RPC version number field in v2 only.
@@ -52,7 +52,7 @@ UniValue JSONRPCReplyObj(UniValue result, UniValue error, UniValue id, JSONRPCVe
         if (jsonrpc_version == JSONRPCVersion::V1_LEGACY) reply.pushKV("result", NullUniValue);
         reply.pushKV("error", std::move(error));
     }
-    reply.pushKV("id", std::move(id));
+    if (id.has_value()) reply.pushKV("id", std::move(id.value()));
     return reply;
 }
 
@@ -172,7 +172,11 @@ void JSONRPCRequest::parse(const UniValue& valRequest)
     const UniValue& request = valRequest.get_obj();
 
     // Parse id now so errors from here on will have the id
-    id = request.find_value("id");
+    if (request.exists("id")) {
+        id = request.find_value("id");
+    } else {
+        id = std::nullopt;
+    }
 
     // Check for JSON-RPC 2.0 (default 1.1)
     m_json_version = JSONRPCVersion::V1_LEGACY;
diff --git a/src/rpc/request.h b/src/rpc/request.h
@@ -7,6 +7,7 @@
 #define BITCOIN_RPC_REQUEST_H
 
 #include <any>
+#include <optional>
 #include <string>
 
 #include <univalue.h>
@@ -17,7 +18,7 @@ enum class JSONRPCVersion {
 };
 
 UniValue JSONRPCRequestObj(const std::string& strMethod, const UniValue& params, const UniValue& id);
-UniValue JSONRPCReplyObj(UniValue result, UniValue error, UniValue id, JSONRPCVersion jsonrpc_version);
+UniValue JSONRPCReplyObj(UniValue result, UniValue error, std::optional<UniValue> id, JSONRPCVersion jsonrpc_version);
 UniValue JSONRPCError(int code, const std::string& message);
 
 /** Generate a new RPC authentication cookie and write it to disk */
@@ -32,7 +33,7 @@ std::vector<UniValue> JSONRPCProcessBatchReply(const UniValue& in);
 class JSONRPCRequest
 {
 public:
-    UniValue id;
+    std::optional<UniValue> id = UniValue::VNULL;
     std::string strMethod;
     UniValue params;
     enum Mode { EXECUTE, GET_HELP, GET_ARGS } mode = EXECUTE;
@@ -43,6 +44,7 @@ class JSONRPCRequest
     JSONRPCVersion m_json_version = JSONRPCVersion::V1_LEGACY;
 
     void parse(const UniValue& valRequest);
+    [[nodiscard]] bool IsNotification() const { return !id.has_value() && m_json_version == JSONRPCVersion::V2; };
 };
 
 #endif // BITCOIN_RPC_REQUEST_H
diff --git a/test/functional/interface_rpc.py b/test/functional/interface_rpc.py
@@ -46,8 +46,11 @@ def format_request(options, idx, fields):
 
 
 def format_response(options, idx, fields):
+    if options.version == 2 and options.notification:
+        return None
     response = {}
-    response.update(id=None if options.notification else idx)
+    if not options.notification:
+        response.update(id=idx)
     if options.version == 2:
         response.update(jsonrpc="2.0")
     else:
@@ -129,11 +132,17 @@ def test_batch_request(self, call_options):
             if options is None:
                 continue
             request.append(format_request(options, idx, call))
-            response.append(format_response(options, idx, result))
+            r = format_response(options, idx, result)
+            if r is not None:
+                response.append(r)
 
         rpc_response, http_status = send_json_rpc(self.nodes[0], request)
-        assert_equal(http_status, 200)
-        assert_equal(rpc_response, response)
+        if len(response) == 0 and len(request) > 0:
+            assert_equal(http_status, 204)
+            assert_equal(rpc_response, None)
+        else:
+            assert_equal(http_status, 200)
+            assert_equal(rpc_response, response)
 
     def test_batch_requests(self):
         self.log.info("Testing empty batch request...")
@@ -193,10 +202,10 @@ def test_http_status_codes(self):
         expect_http_rpc_status(200, RPC_INVALID_PARAMETER,  self.nodes[0], "getblockhash", [42], 2, False)
         # force-send invalidly formatted requests
         response, status = send_json_rpc(self.nodes[0], {"jsonrpc": 2, "method": "getblockcount"})
-        assert_equal(response, {"id": None, "result": None, "error": {"code": RPC_INVALID_REQUEST, "message": "jsonrpc field must be a string"}})
+        assert_equal(response, {"result": None, "error": {"code": RPC_INVALID_REQUEST, "message": "jsonrpc field must be a string"}})
         assert_equal(status, 400)
         response, status = send_json_rpc(self.nodes[0], {"jsonrpc": "3.0", "method": "getblockcount"})
-        assert_equal(response, {"id": None, "result": None, "error": {"code": RPC_INVALID_REQUEST, "message": "JSON-RPC version not supported"}})
+        assert_equal(response, {"result": None, "error": {"code": RPC_INVALID_REQUEST, "message": "JSON-RPC version not supported"}})
         assert_equal(status, 400)
 
         self.log.info("Testing HTTP status codes for JSON-RPC 2.0 notifications...")
@@ -209,10 +218,12 @@ def test_http_status_codes(self):
         # Not notification: has "id" field
         expect_http_rpc_status(200, None,                   self.nodes[0], "getblockcount", [],  2, False)
         block_count = self.nodes[0].getblockcount()
-        expect_http_rpc_status(200, None,                   self.nodes[0], "generatetoaddress", [1, "bcrt1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqdku202"],  2, True)
+        # Notification response status code: HTTP_NO_CONTENT
+        expect_http_rpc_status(204, None,                   self.nodes[0], "generatetoaddress", [1, "bcrt1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqdku202"],  2, True)
         # The command worked even though there was no response
         assert_equal(block_count + 1, self.nodes[0].getblockcount())
-        expect_http_rpc_status(200, RPC_INVALID_ADDRESS_OR_KEY, self.nodes[0], "generatetoaddress", [1, "invalid_address"], 2, True)
+        # No error response for notifications even if they are invalid
+        expect_http_rpc_status(204, None, self.nodes[0], "generatetoaddress", [1, "invalid_address"], 2, True)
         # Sanity check: command was not executed
         assert_equal(block_count + 1, self.nodes[0].getblockcount())
 
diff --git a/test/functional/test_framework/authproxy.py b/test/functional/test_framework/authproxy.py
@@ -160,6 +160,15 @@ def _get_response(self):
             raise JSONRPCException({
                 'code': -342, 'message': 'missing HTTP response from server'})
 
+        # Check for no-content HTTP status code, which can be returned when an
+        # RPC client requests a JSON-RPC 2.0 "notification" with no response.
+        # Currently this is only possible if clients call the _request() method
+        # directly to send a raw request.
+        if http_response.status == HTTPStatus.NO_CONTENT:
+            if len(http_response.read()) != 0:
+                raise JSONRPCException({'code': -342, 'message': 'Content received with NO CONTENT status code'})
+            return None, http_response.status
+
         content_type = http_response.getheader('Content-Type')
         if content_type != 'application/json':
             raise JSONRPCException(

Original file line number	Diff line number	Diff line change
`@@ -10,6 +10,7 @@`
`10`	`10`	`enum HTTPStatusCode`
`11`	`11`	`{`
`12`	`12`	`HTTP_OK = 200,`
	`13`	`+ HTTP_NO_CONTENT = 204,`
`13`	`14`	`HTTP_BAD_REQUEST = 400,`
`14`	`15`	`HTTP_UNAUTHORIZED = 401,`
`15`	`16`	`HTTP_FORBIDDEN = 403,`