Update websocket/rpc mappings. #502
Conversation
|
Depends on libbitcoin/libbitcoin-protocol#199 |
|
This looks unsafe. Client input can crash debug server and is not handled in a non-debug server. |
| @@ -63,6 +72,17 @@ query_socket::query_socket(zmq::context& context, server_node& node, | |||
| request.enqueue(to_chunk(hash)); | |||
| }; | |||
|
|
|||
| const auto encode_hash_or_height = [&](zmq::message& request, | |||
There was a problem hiding this comment.
Avoid the conditionality here, create independent handlers.
There was a problem hiding this comment.
The method calls the independent handlers, the issue is that arguments can contain either a hash or a height. I agree that the decode isn't the safest way to determine which it is though. A lexical cast for height's type should work (which should throw if given a hash as input). UPDATE: That last part was sort of a question ...
There was a problem hiding this comment.
Do you mean that there is a single RPC method call (same name) on the wire that allows either parameter type?
There was a problem hiding this comment.
Yes, there is, but there doesn't have to be. We can support some calls by either (e.g. getblock/getblockheader). Not required, but I thought it would be nice :-) I see why it's dangerous too though, so I'll separate these properly.
There was a problem hiding this comment.
encode_hash_or_height is not yet removed in this revision. It's marginally safer (and can properly error now), but can still be removed.
There was a problem hiding this comment.
Looks entirely safe (the domain of a base 10 encoded uint32_t is less than required for a valid base 16 hash encoding).
|
Use .enqueue() here. |
src/web/query_socket.cpp
Outdated
| { | ||
| request.enqueue(command); | ||
| request.enqueue_little_endian(id); | ||
| const uint32_t height = std::strtoul(arguments.c_str(), nullptr, 0); |
There was a problem hiding this comment.
I don't understand the multi-argument encoding within the string here. But std::strtoul should be avoided here. Passing the c string directly is Unicode unsafe.
There was a problem hiding this comment.
There is an implicit cast here from unsigned long to uint32_t. These aren't the same size, making this a bug unless the argument is first validated.
There was a problem hiding this comment.
It doesn't appear that there's any way using these handlers to return an error for invalid parameterization.
There was a problem hiding this comment.
It doesn't appear that there's any way using these handlers to return an error for invalid parameterization.
I'm adding that, there's no reason there shouldn't be error handing on failed encodings.
src/web/query_socket.cpp
Outdated
| const std::string& command, const std::string& arguments, uint32_t id) | ||
| { | ||
| data_chunk decoded; | ||
| if (decode_base16(decoded, arguments) && |
There was a problem hiding this comment.
Use bool decode_hash() here and handle the error (false).
Agreed, will handle properly. |
src/web/query_socket.cpp
Outdated
| request.enqueue_little_endian(height); | ||
| try | ||
| { | ||
| request.enqueue_little_endian(boost::lexical_cast<uint32_t>( |
There was a problem hiding this comment.
I never like to see try/catch. This should use system::deserialize(...). [In locating the proper deserializer in libbitcoin-system I found and fixed an issue.]
uint32_t value;
if (!deserialize(value, arguments, false))
return false;
request.enqueue_little_endian(value);
return true;
src/web/query_socket.cpp
Outdated
| else | ||
| encode_height(request, command, arguments, id); | ||
| return encode_hash(request, command, arguments, id) ? | ||
| true : encode_height(request, command, arguments, id); |
There was a problem hiding this comment.
A Boolean literal should never be included in a conditional expression...
return encode_hash(request, command, arguments, id) ||
encode_height(request, command, arguments, id);| @@ -63,6 +72,17 @@ query_socket::query_socket(zmq::context& context, server_node& node, | |||
| request.enqueue(to_chunk(hash)); | |||
| }; | |||
|
|
|||
| const auto encode_hash_or_height = [&](zmq::message& request, | |||
There was a problem hiding this comment.
Looks entirely safe (the domain of a base 10 encoded uint32_t is less than required for a valid base 16 hash encoding).
|
Updated (as well as protocol). Also just noticed our HTTP(s) origins were incorrect in the config, so fixed that here (though not particularly related). |
src/web/query_socket.cpp
Outdated
| const auto height = source.read_4_bytes_little_endian(); | ||
| decode_send(connection, http::to_json(height, id)); | ||
| const auto height = static_cast<uint64_t>( | ||
| source.read_4_bytes_little_endian()); |
There was a problem hiding this comment.
No need for an explicit upcast for an integer (just downcast).
src/web/query_socket.cpp
Outdated
| const auto decode_block = [this, &node, decode_send]( | ||
| const data_chunk& data, const uint32_t id, connection_ptr connection) | ||
| const auto decode_block_raw = [this, &node, decode_send]( | ||
| const data_chunk& data, const uint32_t id, connection_ptr connection, |
There was a problem hiding this comment.
Don't use const for non-ref parameters (see other places too).
thecodefactory
force-pushed
the
rpc
branch
2 times, most recently
from
1c48779
to
1d091a5
Compare
Pass through when an rpc request is incoming so that the output can be formatted appropriately.
Updates based on libbitcoin-protocol changes.
Add support for both core and native websocket/rpc mappings.
Pass through when an rpc request is incoming so that the output can be
formatted appropriately.