From aa8870e793215c9849d314ebf5aa7c0f883c0a58 Mon Sep 17 00:00:00 2001 From: Peter Conrad Date: Fri, 24 May 2019 14:33:46 +0200 Subject: [PATCH 1/2] Fixed JSONRPC error handling --- include/fc/exception/exception.hpp | 4 +- include/fc/network/http/connection.hpp | 4 +- include/fc/network/http/websocket.hpp | 9 +- include/fc/rpc/api_connection.hpp | 8 +- include/fc/rpc/state.hpp | 27 ++-- include/fc/rpc/websocket_api.hpp | 10 +- include/fc/variant.hpp | 10 +- src/network/http/websocket.cpp | 15 +- src/rpc/state.cpp | 5 +- src/rpc/websocket_api.cpp | 186 ++++++++++++++++--------- src/variant.cpp | 12 +- tests/api_tests.cpp | 1 + 12 files changed, 184 insertions(+), 107 deletions(-) diff --git a/include/fc/exception/exception.hpp b/include/fc/exception/exception.hpp index b9b6d8e..159ee1a 100644 --- a/include/fc/exception/exception.hpp +++ b/include/fc/exception/exception.hpp @@ -35,7 +35,8 @@ namespace fc aes_error_code = 18, overflow_code = 19, underflow_code = 20, - divide_by_zero_code = 21 + divide_by_zero_code = 21, + method_not_found_exception_code = 22 }; /** @@ -273,6 +274,7 @@ namespace fc FC_DECLARE_EXCEPTION( key_not_found_exception, key_not_found_exception_code, "Key Not Found" ); FC_DECLARE_EXCEPTION( bad_cast_exception, bad_cast_exception_code, "Bad Cast" ); FC_DECLARE_EXCEPTION( out_of_range_exception, out_of_range_exception_code, "Out of Range" ); + FC_DECLARE_EXCEPTION( method_not_found_exception, method_not_found_exception_code, "Method Not Found" ); /** @brief if an operation is unsupported or not valid this may be thrown */ FC_DECLARE_EXCEPTION( invalid_operation_exception, diff --git a/include/fc/network/http/connection.hpp b/include/fc/network/http/connection.hpp index 466d111..fc8156b 100644 --- a/include/fc/network/http/connection.hpp +++ b/include/fc/network/http/connection.hpp @@ -25,6 +25,7 @@ namespace fc { enum status_code { OK = 200, RecordCreated = 201, + NoContent = 204, BadRequest = 400, NotAuthorized = 401, NotFound = 404, @@ -35,6 +36,7 @@ namespace fc { int status; std::vector
headers; std::vector body; + std::string body_as_string; }; struct request @@ -79,4 +81,4 @@ namespace fc { #include FC_REFLECT( fc::http::header, (key)(val) ) - +FC_REFLECT( fc::http::reply, (status)(headers)(body)(body_as_string) ) diff --git a/include/fc/network/http/websocket.hpp b/include/fc/network/http/websocket.hpp index 11db65c..9a4f974 100644 --- a/include/fc/network/http/websocket.hpp +++ b/include/fc/network/http/websocket.hpp @@ -4,6 +4,7 @@ #include #include #include +#include #include namespace fc { namespace http { @@ -12,7 +13,7 @@ namespace fc { namespace http { class websocket_tls_server_impl; class websocket_client_impl; class websocket_tls_client_impl; - } // namespace detail; + } // namespace detail class websocket_connection { @@ -21,10 +22,10 @@ namespace fc { namespace http { virtual void send_message( const std::string& message ) = 0; virtual void close( int64_t code, const std::string& reason ){}; void on_message( const std::string& message ) { _on_message(message); } - string on_http( const std::string& message ) { return _on_http(message); } + fc::http::reply on_http( const std::string& message ) { return _on_http(message); } void on_message_handler( const std::function& h ) { _on_message = h; } - void on_http_handler( const std::function& h ) { _on_http = h; } + void on_http_handler( const std::function& h ) { _on_http = h; } void set_session_data( boost::any d ){ _session_data = std::move(d); } boost::any& get_session_data() { return _session_data; } @@ -35,7 +36,7 @@ namespace fc { namespace http { private: boost::any _session_data; std::function _on_message; - std::function _on_http; + std::function _on_http; }; typedef std::shared_ptr websocket_connection_ptr; diff --git a/include/fc/rpc/api_connection.hpp b/include/fc/rpc/api_connection.hpp index 8e1df24..71eaf80 100644 --- a/include/fc/rpc/api_connection.hpp +++ b/include/fc/rpc/api_connection.hpp @@ -112,13 +112,17 @@ namespace fc { variant call( const string& name, const variants& args ) { auto itr = _by_name.find(name); - FC_ASSERT( itr != _by_name.end(), "no method with name '${name}'", ("name",name)("api",_by_name) ); + if( itr == _by_name.end() ) + FC_THROW_EXCEPTION( method_not_found_exception, "No method with name '${name}'", + ("name",name)("api",_by_name) ); return call( itr->second, args ); } variant call( uint32_t method_id, const variants& args ) { - FC_ASSERT( method_id < _methods.size() ); + if( method_id >= _methods.size() ) + FC_THROW_EXCEPTION( method_not_found_exception, "No method with id '${id}'", + ("id",method_id)("api",_by_name) ); return _methods[method_id](args); } diff --git a/include/fc/rpc/state.hpp b/include/fc/rpc/state.hpp index 5d6f618..32211c6 100644 --- a/include/fc/rpc/state.hpp +++ b/include/fc/rpc/state.hpp @@ -6,9 +6,10 @@ namespace fc { namespace rpc { struct request { - optional id; + optional id; std::string method; variants params; + optional jsonrpc; }; struct error_object @@ -20,13 +21,15 @@ namespace fc { namespace rpc { struct response { - response(){} - response( int64_t i, fc::variant r ):id(i),result(r){} - response( int64_t i, error_object r ):id(i),error(r){} - response( int64_t i, fc::variant r, string j ):id(i),jsonrpc(j),result(r){} - response( int64_t i, error_object r, string j ):id(i),jsonrpc(j),error(r){} - int64_t id = 0; - optional jsonrpc; + response() {} + response( const optional& _id, const variant& _result, + const optional& version = optional() ) + : id(_id), jsonrpc(version), result(_result) {} + response( const optional& _id, const error_object& error, + const optional& version = optional() ) + : id(_id), jsonrpc(version), error(error) {} + optional id; + optional jsonrpc; optional result; optional error; }; @@ -44,7 +47,7 @@ namespace fc { namespace rpc { void handle_reply( const response& response ); request start_remote_call( const string& method_name, variants args ); - variant wait_for_response( uint64_t request_id ); + variant wait_for_response( const variant& request_id ); void close(); @@ -52,12 +55,12 @@ namespace fc { namespace rpc { private: uint64_t _next_id = 1; - std::unordered_map::ptr> _awaiting; + std::map::ptr> _awaiting; std::unordered_map _methods; - std::function _unhandled; + std::function _unhandled; }; } } // namespace fc::rpc -FC_REFLECT( fc::rpc::request, (id)(method)(params) ); +FC_REFLECT( fc::rpc::request, (id)(method)(params)(jsonrpc) ); FC_REFLECT( fc::rpc::error_object, (code)(message)(data) ) FC_REFLECT( fc::rpc::response, (id)(jsonrpc)(result)(error) ) diff --git a/include/fc/rpc/websocket_api.hpp b/include/fc/rpc/websocket_api.hpp index 8f06a19..5064499 100644 --- a/include/fc/rpc/websocket_api.hpp +++ b/include/fc/rpc/websocket_api.hpp @@ -1,9 +1,7 @@ #pragma once +#include #include #include -#include -#include -#include namespace fc { namespace rpc { @@ -26,9 +24,9 @@ namespace fc { namespace rpc { variants args = variants() ) override; protected: - std::string on_message( - const std::string& message, - bool send_message = true ); + response on_message( const std::string& message ); + response on_request( const variant& message ); + void on_response( const variant& message ); std::shared_ptr _connection; fc::rpc::state _rpc_state; diff --git a/include/fc/variant.hpp b/include/fc/variant.hpp index 8da0b2a..2e08740 100644 --- a/include/fc/variant.hpp +++ b/include/fc/variant.hpp @@ -658,11 +658,11 @@ namespace fc variant operator - ( const variant& a, const variant& b ); variant operator * ( const variant& a, const variant& b ); variant operator / ( const variant& a, const variant& b ); - variant operator == ( const variant& a, const variant& b ); - variant operator != ( const variant& a, const variant& b ); - variant operator < ( const variant& a, const variant& b ); - variant operator > ( const variant& a, const variant& b ); - variant operator ! ( const variant& a ); + bool operator == ( const variant& a, const variant& b ); + bool operator != ( const variant& a, const variant& b ); + bool operator < ( const variant& a, const variant& b ); + bool operator > ( const variant& a, const variant& b ); + bool operator ! ( const variant& a ); } // namespace fc #include diff --git a/src/network/http/websocket.cpp b/src/network/http/websocket.cpp index 6d479bf..64caf51 100644 --- a/src/network/http/websocket.cpp +++ b/src/network/http/websocket.cpp @@ -12,7 +12,10 @@ #include #endif +#include #include +#include +#include #include #include #include @@ -239,10 +242,10 @@ namespace fc { namespace http { wdump(("server")(request_body)); fc::async([current_con, request_body, con] { - std::string response = current_con->on_http(request_body); - idump((response)); - con->set_body( response ); - con->set_status( websocketpp::http::status_code::ok ); + fc::http::reply response = current_con->on_http(request_body); + idump( (response) ); + con->set_body( std::move( response.body_as_string ) ); + con->set_status( websocketpp::http::status_code::value(response.status) ); con->send_http_response(); current_con->closed(); }, "call on_http"); @@ -364,8 +367,8 @@ namespace fc { namespace http { wdump(("server")(con->get_request_body())); auto response = current_con->on_http( con->get_request_body() ); idump((response)); - con->set_body( response ); - con->set_status( websocketpp::http::status_code::ok ); + con->set_body( std::move( response.body_as_string ) ); + con->set_status( websocketpp::http::status_code::value( response.status ) ); } catch ( const fc::exception& e ) { edump((e.to_detail_string())); diff --git a/src/rpc/state.cpp b/src/rpc/state.cpp index 20823e3..97c9ba1 100644 --- a/src/rpc/state.cpp +++ b/src/rpc/state.cpp @@ -29,7 +29,8 @@ variant state::local_call( const string& method_name, const variants& args ) void state::handle_reply( const response& response ) { - auto await = _awaiting.find( response.id ); + FC_ASSERT( response.id, "Response without ID: ${response}", ("response",response) ); + auto await = _awaiting.find( *response.id ); FC_ASSERT( await != _awaiting.end(), "Unknown Response ID: ${id}", ("id",response.id)("response",response) ); if( response.result ) await->second->set_value( *response.result ); @@ -48,7 +49,7 @@ request state::start_remote_call( const string& method_name, variants args ) _awaiting[*request.id] = fc::promise::ptr( new fc::promise("json_connection::async_call") ); return request; } -variant state::wait_for_response( uint64_t request_id ) +variant state::wait_for_response( const variant& request_id ) { auto itr = _awaiting.find(request_id); FC_ASSERT( itr != _awaiting.end() ); diff --git a/src/rpc/websocket_api.cpp b/src/rpc/websocket_api.cpp index d1dd997..f69dca6 100644 --- a/src/rpc/websocket_api.cpp +++ b/src/rpc/websocket_api.cpp @@ -1,5 +1,6 @@ - +#include #include +#include namespace fc { namespace rpc { @@ -49,8 +50,30 @@ websocket_api_connection::websocket_api_connection( const std::shared_ptrreceive_call( 0, method_name, args ); } ); - _connection->on_message_handler( [this]( const std::string& msg ){ on_message(msg,true); } ); - _connection->on_http_handler( [this]( const std::string& msg ){ return on_message(msg,false); } ); + _connection->on_message_handler( [this]( const std::string& msg ){ + response reply = on_message(msg); + if( _connection && ( reply.id || reply.result || reply.error || reply.jsonrpc ) ) + _connection->send_message( fc::json::to_string( reply, fc::json::stringify_large_ints_and_doubles, + _max_conversion_depth ) ); + } ); + _connection->on_http_handler( [this]( const std::string& msg ){ + response reply = on_message(msg); + fc::http::reply result; + if( reply.error ) + { + if( reply.error->code == -32603 ) + result.status = fc::http::reply::InternalServerError; + else if( reply.error->code <= -32600 ) + result.status = fc::http::reply::BadRequest; + } + if( reply.id || reply.result || reply.error || reply.jsonrpc ) + result.body_as_string = fc::json::to_string( reply, fc::json::stringify_large_ints_and_doubles, + _max_conversion_depth ); + else + result.status = fc::http::reply::NoContent; + + return result; + } ); _connection->closed.connect( [this](){ closed(); } ); } @@ -96,81 +119,120 @@ void websocket_api_connection::send_notice( _max_conversion_depth ) ); } -std::string websocket_api_connection::on_message( - const std::string& message, - bool send_message /* = true */ ) +response websocket_api_connection::on_message( const std::string& message ) { + variant var; try { - auto var = fc::json::from_string(message, fc::json::legacy_parser, _max_conversion_depth); - const auto& var_obj = var.get_object(); + var = fc::json::from_string( message, fc::json::legacy_parser, _max_conversion_depth ); + } + catch( const fc::exception& e ) + { + return response( variant(), { -32700, "Invalid JSON message", variant( e, _max_conversion_depth ) }, "2.0" ); + } - if( var_obj.contains( "method" ) ) - { - auto call = var.as(_max_conversion_depth); - exception_ptr optexcept; - try - { - try - { + if( var.is_array() ) + return response( variant(), { -32600, "Batch requests not supported" }, "2.0" ); + + if( !var.is_object() ) + return response( variant(), { -32600, "Invalid JSON request" }, "2.0" ); + + variant_object var_obj = var.get_object(); + + if( var_obj.contains( "id" ) + && !var_obj["id"].is_string() && !var_obj["id"].is_numeric() && !var_obj["id"].is_null() ) + return response( variant(), { -32600, "Invalid id" }, "2.0" ); + + if( var_obj.contains( "method" ) && ( !var_obj["method"].is_string() || var_obj["method"].get_string() == "" ) ) + return response( variant(), { -32602, "Missing or invalid method" }, "2.0" ); + + if( var_obj.contains( "jsonrpc" ) && ( !var_obj["jsonrpc"].is_string() || var_obj["jsonrpc"] != "2.0" ) ) + return response( variant(), { -32600, "Unsupported JSON-RPC version" }, "2.0" ); + + if( var_obj.contains( "method" ) ) + { + if( var_obj.contains( "params" ) && var_obj["params"].is_object() ) + return response( variant(), { -32600, "Named parameters not supported" }, "2.0" ); + + if( var_obj.contains( "params" ) && !var_obj["params"].is_array() ) + return response( variant(), { -32600, "Invalid parameters" }, "2.0" ); + + return on_request( std::move( var ) ); + } + + if( var_obj.contains( "result" ) || var_obj.contains("error") ) + { + if( !var_obj.contains( "id" ) || ( var_obj["id"].is_null() && !var_obj.contains( "jsonrpc" ) ) ) + return response( variant(), { -32600, "Missing or invalid id" }, "2.0" ); + + on_response( std::move( var ) ); + + return response(); + } + + return response( variant(), { -32602, "Missing method or result or error" }, "2.0" ); +} + +void websocket_api_connection::on_response( const variant& var ) +{ + _rpc_state.handle_reply( var.as(_max_conversion_depth) ); +} + +response websocket_api_connection::on_request( const variant& var ) +{ + request call = var.as( _max_conversion_depth ); + if( var.get_object().contains( "id" ) ) + call.id = var.get_object()["id"]; // special handling for null id + + // null ID is valid in JSONRPC-2.0 but signals "no id" in JSONRPC-1.0 + bool has_id = call.id.valid() && ( call.jsonrpc.valid() || !call.id->is_null() ); + + try + { #ifdef LOG_LONG_API - auto start = time_point::now(); + auto start = time_point::now(); #endif - auto result = _rpc_state.local_call( call.method, call.params ); + auto result = _rpc_state.local_call( call.method, call.params ); #ifdef LOG_LONG_API - auto end = time_point::now(); + auto end = time_point::now(); - if( end - start > fc::milliseconds( LOG_LONG_API_MAX_MS ) ) - elog( "API call execution time limit exceeded. method: ${m} params: ${p} time: ${t}", - ("m",call.method)("p",call.params)("t", end - start) ); - else if( end - start > fc::milliseconds( LOG_LONG_API_WARN_MS ) ) - wlog( "API call execution time nearing limit. method: ${m} params: ${p} time: ${t}", - ("m",call.method)("p",call.params)("t", end - start) ); + if( end - start > fc::milliseconds( LOG_LONG_API_MAX_MS ) ) + elog( "API call execution time limit exceeded. method: ${m} params: ${p} time: ${t}", + ("m",call.method)("p",call.params)("t", end - start) ); + else if( end - start > fc::milliseconds( LOG_LONG_API_WARN_MS ) ) + wlog( "API call execution time nearing limit. method: ${m} params: ${p} time: ${t}", + ("m",call.method)("p",call.params)("t", end - start) ); #endif - if( call.id ) - { - auto reply = fc::json::to_string( response( *call.id, result, "2.0" ), - fc::json::stringify_large_ints_and_doubles, - _max_conversion_depth ); - if( send_message && _connection ) - _connection->send_message( reply ); - return reply; - } - } - FC_CAPTURE_AND_RETHROW( (call.method)(call.params) ) - } - catch ( const fc::exception& e ) - { - if( call.id ) - { - optexcept = e.dynamic_copy_exception(); - } - } - if( optexcept ) { - - auto reply = fc::json::to_string( variant(response( *call.id, error_object{ 1, optexcept->to_string(), fc::variant(*optexcept, _max_conversion_depth)}, "2.0" ), _max_conversion_depth ), - fc::json::stringify_large_ints_and_doubles, _max_conversion_depth ); - if( send_message && _connection ) - _connection->send_message( reply ); - - return reply; - } - } - else - { - auto reply = var.as(_max_conversion_depth); - _rpc_state.handle_reply( reply ); - } + if( has_id ) + return response( call.id, result, call.jsonrpc ); + } + catch ( const fc::method_not_found_exception& e ) + { + if( has_id ) + return response( call.id, error_object{ -32601, "Method not found", + variant( (fc::exception) e, _max_conversion_depth ) }, call.jsonrpc ); } catch ( const fc::exception& e ) { - wdump((e.to_detail_string())); - return e.to_detail_string(); + if( has_id ) + return response( call.id, error_object{ e.code(), "Execution error", variant( e, _max_conversion_depth ) }, + call.jsonrpc ); } - return string(); + catch ( const std::exception& e ) + { + elog( "Internal error - ${e}", ("e",e.what()) ); + return response( call.id, error_object{ -32603, "Internal error", variant( e.what(), _max_conversion_depth ) }, + call.jsonrpc ); + } + catch ( ... ) + { + elog( "Internal error while processing RPC request" ); + throw; + } + return response(); } } } // namespace fc::rpc diff --git a/src/variant.cpp b/src/variant.cpp index 74afb1d..4e52ec9 100644 --- a/src/variant.cpp +++ b/src/variant.cpp @@ -678,7 +678,7 @@ void from_variant( const variant& var, std::vector& vo, uint32_t max_depth void to_variant( unsigned long long int s, variant& v, uint32_t max_depth ) { v = variant( uint64_t(s)); } #endif - variant operator == ( const variant& a, const variant& b ) + bool operator == ( const variant& a, const variant& b ) { if( a.is_string() || b.is_string() ) return a.as_string() == b.as_string(); if( a.is_double() || b.is_double() ) return a.as_double() == b.as_double(); @@ -687,7 +687,7 @@ void from_variant( const variant& var, std::vector& vo, uint32_t max_depth return false; } - variant operator != ( const variant& a, const variant& b ) + bool operator != ( const variant& a, const variant& b ) { if( a.is_string() || b.is_string() ) return a.as_string() != b.as_string(); if( a.is_double() || b.is_double() ) return a.as_double() != b.as_double(); @@ -696,12 +696,12 @@ void from_variant( const variant& var, std::vector& vo, uint32_t max_depth return false; } - variant operator ! ( const variant& a ) + bool operator ! ( const variant& a ) { return !a.as_bool(); } - variant operator < ( const variant& a, const variant& b ) + bool operator < ( const variant& a, const variant& b ) { if( a.is_string() || b.is_string() ) return a.as_string() < b.as_string(); if( a.is_double() || b.is_double() ) return a.as_double() < b.as_double(); @@ -710,7 +710,7 @@ void from_variant( const variant& var, std::vector& vo, uint32_t max_depth FC_ASSERT( false, "Invalid operation" ); } - variant operator > ( const variant& a, const variant& b ) + bool operator > ( const variant& a, const variant& b ) { if( a.is_string() || b.is_string() ) return a.as_string() > b.as_string(); if( a.is_double() || b.is_double() ) return a.as_double() > b.as_double(); @@ -719,7 +719,7 @@ void from_variant( const variant& var, std::vector& vo, uint32_t max_depth FC_ASSERT( false, "Invalid operation" ); } - variant operator <= ( const variant& a, const variant& b ) + bool operator <= ( const variant& a, const variant& b ) { if( a.is_string() || b.is_string() ) return a.as_string() <= b.as_string(); if( a.is_double() || b.is_double() ) return a.as_double() <= b.as_double(); diff --git a/tests/api_tests.cpp b/tests/api_tests.cpp index 0902492..dff5350 100644 --- a/tests/api_tests.cpp +++ b/tests/api_tests.cpp @@ -1,6 +1,7 @@ #include #include +#include #include #include #include From e5869b280055614b7e4f73beba5f6e135aaae48c Mon Sep 17 00:00:00 2001 From: Peter Conrad Date: Mon, 27 May 2019 11:55:03 +0200 Subject: [PATCH 2/2] Amended some error codes --- src/rpc/websocket_api.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/rpc/websocket_api.cpp b/src/rpc/websocket_api.cpp index f69dca6..0798f8f 100644 --- a/src/rpc/websocket_api.cpp +++ b/src/rpc/websocket_api.cpp @@ -144,7 +144,7 @@ response websocket_api_connection::on_message( const std::string& message ) return response( variant(), { -32600, "Invalid id" }, "2.0" ); if( var_obj.contains( "method" ) && ( !var_obj["method"].is_string() || var_obj["method"].get_string() == "" ) ) - return response( variant(), { -32602, "Missing or invalid method" }, "2.0" ); + return response( variant(), { -32600, "Missing or invalid method" }, "2.0" ); if( var_obj.contains( "jsonrpc" ) && ( !var_obj["jsonrpc"].is_string() || var_obj["jsonrpc"] != "2.0" ) ) return response( variant(), { -32600, "Unsupported JSON-RPC version" }, "2.0" ); @@ -152,7 +152,7 @@ response websocket_api_connection::on_message( const std::string& message ) if( var_obj.contains( "method" ) ) { if( var_obj.contains( "params" ) && var_obj["params"].is_object() ) - return response( variant(), { -32600, "Named parameters not supported" }, "2.0" ); + return response( variant(), { -32602, "Named parameters not supported" }, "2.0" ); if( var_obj.contains( "params" ) && !var_obj["params"].is_array() ) return response( variant(), { -32600, "Invalid parameters" }, "2.0" ); @@ -170,7 +170,7 @@ response websocket_api_connection::on_message( const std::string& message ) return response(); } - return response( variant(), { -32602, "Missing method or result or error" }, "2.0" ); + return response( variant(), { -32600, "Missing method or result or error" }, "2.0" ); } void websocket_api_connection::on_response( const variant& var )