In commit "rpc: handle malformed jsonrpc 2.0 requests correctly" (9b21a79587e130f0c51cc0ffc570c1d418258615)
After this push, single requests and batches use different definitions of a notification. {"jsonrpc": "2.0"} gets an error reply with "id": null, as the spec requires, but [{"jsonrpc": "2.0"}] still gets HTTP 204 and no reply, because the batch loop treats any 2.0 entry without an id as a notification. The spec's batch example answers invalid entries with an error too.
I think it would be better not to leave it half-done: either fix batches too, or go back to the previous behavior for single requests (the previous push, or my earlier suggestion, which kept the 204) and change the notification rule in a separate PR. Fixing it everywhere seems simplest. If parse() decides whether a request is a notification, IsNotification() is only true for valid requests, the batch loop needs no changes, and JSONErrorReply() can return 204 for notifications, so #36753 can throw the same error for both versions. Diff from master:
--- a/src/httprpc.cpp
+++ b/src/httprpc.cpp
@@ -40,10 +40,17 @@ static bool g_rpc_whitelist_default = false;
static UniValue JSONErrorReply(UniValue objError, const JSONRPCRequest& jreq, HTTPStatusCode& nStatus)
{
- // HTTP errors should never be returned if JSON-RPC v2 was requested. This
- // function should only be called when a v1 request fails or when a request
- // cannot be parsed, so the version is unknown.
- Assume(jreq.m_json_version != JSONRPCVersion::V2);
+ // A request known to be JSON-RPC 2.0 gets the error in the reply with HTTP
+ // OK, or no reply if it is a notification. HTTP error statuses are only
+ // returned for 1.x requests and requests whose version is unknown.
+ if (jreq.m_json_version == JSONRPCVersion::V2) {
+ if (jreq.IsNotification()) {
+ nStatus = HTTP_NO_CONTENT;
+ return {};
+ }
+ nStatus = HTTP_OK;
+ return JSONRPCReplyObj(NullUniValue, std::move(objError), jreq.id, jreq.m_json_version);
+ }
// Send error reply from json-rpc error object
nStatus = HTTP_INTERNAL_SERVER_ERROR;
--- a/src/rpc/request.cpp
+++ b/src/rpc/request.cpp
@@ -68,7 +68,17 @@ UniValue JSONRPCReplyObj(UniValue result, UniValue error, std::optional<UniValue
if (jsonrpc_version == JSONRPCVersion::V1_LEGACY) reply.pushKV("result", NullUniValue);
reply.pushKV("error", std::move(error));
}
- if (id.has_value()) reply.pushKV("id", std::move(id.value()));
+ if (id.has_value()) {
+ reply.pushKV("id", std::move(id.value()));
+ } else if (jsonrpc_version == JSONRPCVersion::V2) {
+ // The request had no "id" member. This happens for notifications, and
+ // for invalid requests that left out the id (for example with a missing
+ // or non-string method name). The JSON-RPC 2.0 spec requires every
+ // response to have an "id", set to null when the request's id could not
+ // be determined, so add "id": null. For notifications this is harmless,
+ // because their replies are never sent.
+ reply.pushKV("id", NullUniValue);
+ }
return reply;
}
@@ -200,6 +210,13 @@ std::vector<UniValue> JSONRPCProcessBatchReply(const UniValue& in)
void JSONRPCRequest::parse(const UniValue& valRequest)
{
+ // Reset fields set by any previous request in the same batch, so if this
+ // request is invalid, its error reply does not use the previous id or
+ // version.
+ id = UniValue::VNULL;
+ m_json_version = JSONRPCVersion::V1_LEGACY;
+ m_notification = false;
+
// Parse request
if (!valRequest.isObject())
throw JSONRPCError(RPC_INVALID_REQUEST, "Invalid Request object");
@@ -213,7 +230,6 @@ void JSONRPCRequest::parse(const UniValue& valRequest)
}
// Check for JSON-RPC 2.0 (default 1.1)
- m_json_version = JSONRPCVersion::V1_LEGACY;
const UniValue& jsonrpc_version = request.find_value("jsonrpc");
if (!jsonrpc_version.isNull()) {
if (!jsonrpc_version.isStr()) {
@@ -254,4 +270,9 @@ void JSONRPCRequest::parse(const UniValue& valRequest)
params = UniValue(UniValue::VARR);
else
throw JSONRPCError(RPC_INVALID_REQUEST, "Params must be an array or object");
+
+ // A notification is a valid JSON-RPC 2.0 request without an "id" member.
+ // Invalid requests are not notifications, and get an error reply even
+ // without an id.
+ m_notification = m_json_version == JSONRPCVersion::V2 && !id.has_value();
}
--- a/src/rpc/request.h
+++ b/src/rpc/request.h
@@ -63,9 +63,11 @@ public:
std::string peerAddr;
std::any context;
JSONRPCVersion m_json_version = JSONRPCVersion::V1_LEGACY;
+ //! Set by parse() for a valid JSON-RPC 2.0 request without an "id" member.
+ bool m_notification{false};
void parse(const UniValue& valRequest);
- [[nodiscard]] bool IsNotification() const { return !id.has_value() && m_json_version == JSONRPCVersion::V2; };
+ [[nodiscard]] bool IsNotification() const { return m_notification; };
};
#endif // BITCOIN_RPC_REQUEST_H
The HTTP_NO_CONTENT branch in JSONErrorReply() isn't reachable yet, since valid 2.0 notifications never get there on this branch, but it makes the function correct for every 2.0 request. The JSONRPCReplyObj() comment is the one from the separate comment about it.
Resetting the fields at the start of parse() also fixes a bug that is already on master: a batch entry that isn't an object gets an error reply with the id and version of the entry before it. [{"jsonrpc": "2.0", "method": "getblockcount", "id": 5}, 1] gets "id": 5 for the 1, and [{"jsonrpc": "2.0", "method": "getblockcount"}, 1] gets no reply at all.
I tested this with the PR's tests plus the test changes below, which expect an error for the invalid notification in the batch tests and cover the reset. interface_rpc.py, rpc_whitelist.py, interface_http.py and rpc_users.py pass in a debug build.
<details><summary>Test changes</summary>
--- a/test/functional/interface_rpc.py
+++ b/test/functional/interface_rpc.py
@@ -43,12 +43,16 @@ def format_request(options, idx, fields):
return request
-def format_response(options, idx, fields):
- if options.version == 2 and options.notification:
+def format_response(options, idx, fields, invalid=False):
+ # Valid 2.0 notifications get no response. Invalid requests are not
+ # notifications, and get an error response with "id": null.
+ if options.version == 2 and options.notification and not invalid:
return None
response = {}
if not options.notification:
response.update(id=idx)
+ elif options.version == 2:
+ response.update(id=None)
if options.version == 2:
response.update(jsonrpc="2.0")
else:
@@ -130,7 +134,7 @@ class RPCInterfaceTest(BitcoinTestFramework):
if options is None:
continue
request.append(format_request(options, idx, call))
- r = format_response(options, idx, result)
+ r = format_response(options, idx, result, invalid="method" not in call)
if r is not None:
response.append(r)
@@ -176,6 +180,17 @@ class RPCInterfaceTest(BitcoinTestFramework):
request_fields={"jsonrpc": "2.1"},
response_fields={"result": None, "error": {"code": RPC_INVALID_REQUEST, "message": "JSON-RPC version not supported"}}))
+ def test_batch_invalid_entries(self):
+ self.log.info("Testing that invalid batch entries do not reuse the previous entry's id...")
+ valid = {"jsonrpc": "2.0", "method": "getblockcount"}
+ error = {"result": None, "error": {"code": RPC_INVALID_REQUEST, "message": "Invalid Request object"}, "id": None}
+ response, status = send_json_rpc(self.nodes[0], [{**valid, "id": 5}, 1])
+ assert_equal(response, [{"jsonrpc": "2.0", "result": 0, "id": 5}, error])
+ assert_equal(status, 200)
+ response, status = send_json_rpc(self.nodes[0], [valid, 1])
+ assert_equal(response, [error])
+ assert_equal(status, 200)
+
def test_http_status_codes(self):
self.log.info("Testing HTTP status codes for JSON-RPC 1.1 requests...")
# OK
@@ -269,6 +284,7 @@ class RPCInterfaceTest(BitcoinTestFramework):
def run_test(self):
self.test_getrpcinfo()
self.test_batch_requests()
+ self.test_batch_invalid_entries()
self.test_http_status_codes()
self.test_work_queue_exceeded()
</details>
