Support JSON-RPC 2.0 when requested by client #27101
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| JSON-RPC | ||
| -------- | ||
|
|
||
| The JSON-RPC server now recognizes JSON-RPC 2.0 requests and responds with | ||
| strict adherence to the specification (https://www.jsonrpc.org/specification): | ||
|
|
||
| - Returning HTTP "204 No Content" responses to JSON-RPC 2.0 notifications instead of full responses. | ||
| - Returning HTTP "200 OK" responses in all other cases, rather than 404 responses for unknown methods, 500 responses for invalid parameters, etc. | ||
| - Returning either "result" fields or "error" fields in JSON-RPC responses, rather than returning both fields with one field set to null. | ||
|
Member
There was a problem hiding this comment. nit instead of duplicating the section from |
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -73,8 +73,11 @@ static std::vector<std::vector<std::string>> g_rpcauth; | |
| static std::map<std::string, std::set<std::string>> g_rpc_whitelist; | ||
| static bool g_rpc_whitelist_default = false; | ||
|
|
||
| static void JSONErrorReply(HTTPRequest* req, UniValue objError, const JSONRPCRequest& jreq) | ||
|
Contributor
There was a problem hiding this comment. nit: |
||
| { | ||
| // 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>(); | ||
|
|
@@ -84,7 +87,7 @@ static void JSONErrorReply(HTTPRequest* req, const UniValue& objError, const Uni | |
| else if (code == RPC_METHOD_NOT_FOUND) | ||
| nStatus = HTTP_NOT_FOUND; | ||
|
|
||
| std::string strReply = JSONRPCReplyObj(NullUniValue, std::move(objError), jreq.id, jreq.m_json_version).write() + "\n"; | ||
|
|
||
| req->WriteHeader("Content-Type", "application/json"); | ||
| req->WriteReply(nStatus, strReply); | ||
|
|
@@ -185,7 +188,7 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req) | |
| // Set the URI | ||
| jreq.URI = req->GetURI(); | ||
|
|
||
| UniValue reply; | ||
| bool user_has_whitelist = g_rpc_whitelist.count(jreq.authUser); | ||
| if (!user_has_whitelist && g_rpc_whitelist_default) { | ||
| LogPrintf("RPC User %s not allowed to call any methods\n", jreq.authUser); | ||
|
|
@@ -200,13 +203,23 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req) | |
| req->WriteReply(HTTP_FORBIDDEN); | ||
| return false; | ||
| } | ||
|
|
||
| // 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); | ||
|
|
||
| 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()) { | ||
ryanofsky marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| // Check authorization for each request's method | ||
| if (user_has_whitelist) { | ||
| for (unsigned int reqIdx = 0; reqIdx < valRequest.size(); reqIdx++) { | ||
| if (!valRequest[reqIdx].isObject()) { | ||
|
|
@@ -223,18 +236,49 @@ 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 never throw HTTP errors, they are always just included | ||
| // in "HTTP OK" responses. Notifications never get any response. | ||
| UniValue response; | ||
| try { | ||
| jreq.parse(valRequest[i]); | ||
| response = JSONRPCExec(jreq, /*catch_errors=*/true); | ||
| } catch (UniValue& e) { | ||
| response = JSONRPCReplyObj(NullUniValue, std::move(e), jreq.id, jreq.m_json_version); | ||
| } catch (const std::exception& e) { | ||
| response = JSONRPCReplyObj(NullUniValue, JSONRPCError(RPC_PARSE_ERROR, e.what()), jreq.id, jreq.m_json_version); | ||
| } | ||
|
Comment on lines
+246
to
+253
Contributor
There was a problem hiding this comment. (Came across this block again while working on something else. It struck me that the exception handling looked redundant as we catch the same exact exception types inside One could move out |
||
| 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 | ||
| throw JSONRPCError(RPC_PARSE_ERROR, "Top-level object parse error"); | ||
|
|
||
| req->WriteHeader("Content-Type", "application/json"); | ||
| req->WriteReply(HTTP_OK, reply.write() + "\n"); | ||
|
||
| } catch (UniValue& e) { | ||
| JSONErrorReply(req, std::move(e), jreq); | ||
| return false; | ||
| } catch (const std::exception& e) { | ||
| JSONErrorReply(req, JSONRPCError(RPC_PARSE_ERROR, e.what()), jreq); | ||
| return false; | ||
| } | ||
| return true; | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,6 +26,17 @@ | |
| * | ||
| * 1.0 spec: http://json-rpc.org/wiki/specification | ||
| * 1.2 spec: http://jsonrpc.org/historical/json-rpc-over-http.html | ||
| * | ||
| * If the server receives a request with the JSON-RPC 2.0 marker `{"jsonrpc": "2.0"}` | ||
| * then Bitcoin will respond with a strictly specified response. | ||
| * It will only return an HTTP error code if an actual HTTP error is encountered | ||
| * such as the endpoint is not found (404) or the request is not formatted correctly (500). | ||
| * Otherwise the HTTP code is always OK (200) and RPC errors will be included in the | ||
| * response body. | ||
| * | ||
| * 2.0 spec: https://www.jsonrpc.org/specification | ||
| * | ||
| * Also see http://www.simple-is-better.org/rpc/#differences-between-1-0-and-2-0 | ||
| */ | ||
|
|
||
| UniValue JSONRPCRequestObj(const std::string& strMethod, const UniValue& params, const UniValue& id) | ||
|
|
@@ -37,24 +48,25 @@ UniValue JSONRPCRequestObj(const std::string& strMethod, const UniValue& params, | |
| return request; | ||
| } | ||
|
|
||
| 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. | ||
| if (jsonrpc_version == JSONRPCVersion::V2) reply.pushKV("jsonrpc", "2.0"); | ||
|
|
||
| // Add both result and error fields in v1, even though one will be null. | ||
| // Omit the null field in v2. | ||
| if (error.isNull()) { | ||
| reply.pushKV("result", std::move(result)); | ||
| if (jsonrpc_version == JSONRPCVersion::V1_LEGACY) reply.pushKV("error", NullUniValue); | ||
| } else { | ||
| 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())); | ||
| return reply; | ||
| } | ||
|
|
||
| UniValue JSONRPCError(int code, const std::string& message) | ||
| { | ||
| UniValue error(UniValue::VOBJ); | ||
|
|
@@ -171,7 +183,30 @@ void JSONRPCRequest::parse(const UniValue& valRequest) | |
| const UniValue& request = valRequest.get_obj(); | ||
|
|
||
| // Parse id now so errors from here on will have the 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; | ||
| const UniValue& jsonrpc_version = request.find_value("jsonrpc"); | ||
| if (!jsonrpc_version.isNull()) { | ||
| if (!jsonrpc_version.isStr()) { | ||
| throw JSONRPCError(RPC_INVALID_REQUEST, "jsonrpc field must be a string"); | ||
| } | ||
| // The "jsonrpc" key was added in the 2.0 spec, but some older documentation | ||
| // incorrectly included {"jsonrpc":"1.0"} in a request object, so we | ||
| // maintain that for backwards compatibility. | ||
|
Contributor
There was a problem hiding this comment. In commit "rpc: identify JSON-RPC 2.0 requests" (2ca1460) I think it would be a little clearer to say "continue to accept that" instead of "maintain that." Otherwise it sounds like we are trying to maintain incorrectly including the field, not just allowing it if it is specified. |
||
| if (jsonrpc_version.get_str() == "1.0") { | ||
| m_json_version = JSONRPCVersion::V1_LEGACY; | ||
| } else if (jsonrpc_version.get_str() == "2.0") { | ||
| m_json_version = JSONRPCVersion::V2; | ||
| } else { | ||
| throw JSONRPCError(RPC_INVALID_REQUEST, "JSON-RPC version not supported"); | ||
ryanofsky marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
|
|
||
| // Parse method | ||
| const UniValue& valMethod{request.find_value("method")}; | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -358,36 +358,22 @@ bool IsDeprecatedRPCEnabled(const std::string& method) | |
| return find(enabled_methods.begin(), enabled_methods.end(), method) != enabled_methods.end(); | ||
| } | ||
|
|
||
| UniValue JSONRPCExec(const JSONRPCRequest& jreq, bool catch_errors) | ||
|
Contributor
There was a problem hiding this comment. Would have gone the opposite way and called it
Contributor
There was a problem hiding this comment. re: #27101 (comment)
I think either way is fine, but catch_errors does seem more literally correct since the argument is just controlling whether the exceptions will be caught. Errors will be still be thrown regardless. |
||
| { | ||
| 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); | ||
| } | ||
|
|
||
| /** | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Now that this is merged, it could say "in 27.0 and prior releases." Otherwise, on 29.x it will read as-if 28.0 had it missing.