From e7ee80dcf2b68684eae96070875ea13a60e3e7b0 Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Fri, 7 Jul 2023 15:06:35 -0400 Subject: [PATCH] rpc: JSON-RPC 2.0 should not respond to "notifications" 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 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. --- src/httprpc.cpp | 31 ++++++++++++++++++--- src/rpc/protocol.h | 1 + src/rpc/request.cpp | 10 +++++-- src/rpc/request.h | 6 ++-- test/functional/interface_rpc.py | 27 ++++++++++++------ test/functional/test_framework/authproxy.py | 9 ++++++ 6 files changed, 67 insertions(+), 17 deletions(-) diff --git a/src/httprpc.cpp b/src/httprpc.cpp index 777ad32bbe2..3eb34dbe6ab 100644 --- 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 index 75e42e4c887..83a90106819 100644 --- 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 index b5bf1e5ffab..ca424b4953a 100644 --- 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 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 index a7c9af2216b..e47f90af86b 100644 --- a/src/rpc/request.h +++ b/src/rpc/request.h @@ -7,6 +7,7 @@ #define BITCOIN_RPC_REQUEST_H #include +#include #include #include @@ -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 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 JSONRPCProcessBatchReply(const UniValue& in); class JSONRPCRequest { public: - UniValue id; + std::optional id = UniValue::VNULL; std::string strMethod; UniValue params; enum Mode { EXECUTE, GET_HELP, GET_ARGS } mode = EXECUTE; @@ -43,6 +44,7 @@ public: 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 index 824a7200cd4..b08ca427965 100755 --- 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 @@ class RPCInterfaceTest(BitcoinTestFramework): 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 @@ class RPCInterfaceTest(BitcoinTestFramework): 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 @@ class RPCInterfaceTest(BitcoinTestFramework): # 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 index 03042877b20..7edf9f36794 100644 --- a/test/functional/test_framework/authproxy.py +++ b/test/functional/test_framework/authproxy.py @@ -160,6 +160,15 @@ class AuthServiceProxy(): 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(