Skip to content

Commit 877343a

Browse files
PastaPastaPastaPastaPastaPasta
committed
Merge #6943: fix: don't treat arrays/objects as string literals for composite methods
5978985 fix: don't treat arrays/objects as string literals for composite methods (Kittywhiskers Van Gogh) Pull request description: ## Additional Information thephez identified a bug where attempting to specify arrays as expected by `coreP2PAddrs`, `platformHTTPSAddrs` or `platformP2PAddrs` using `dash-cli` or the Qt wallet resulted in errors (see below) ``` $ ./src/dash-cli --testnet protx register_prepare "6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000" 1 '["1.1.1.1:19999"]' "yhq7ifNCtTKEpY4Yu5XPCcztQco6Fh6JsZ" "93746e8731c57f87f79b3620a7982924e2931717d49540a85864bd543de11c43fb868fd63e501a1db37e19ed59ae6db4" "yTretFTpoi3oQ3maZk5QadGaDWPiKnmDBc" 0 "yNbNZyCiTYSFtDwEXt7jChV7tZVYX862ua" error code: -8 error message: Error setting coreP2PAddrs[0] to '["1.1.1.1:19999"]' (invalid input) ``` This was not caught by functional tests as they send fully formed JSON messages and do not require JSON client interpretation, which both `dash-cli` and Dash Qt require, this pull request adds additional handling to ensure that arrays and objects are interpreted if they are likely to be so (starting and ending with curly or square brackets) or otherwise passed as-is as a string. What this pull request _isn't_ is full fledged conversion support for composite RPC arguments to other non-string types like integers, the scope here is specifically to allow for the backwards compatibility logic for `coreP2PAddrs`, `platformHTTPSAddrs` or `platformP2PAddrs` to work as intended. With this pull request, the following result is expected (see below) ``` $ ./src/dash-cli --testnet protx register_prepare "6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000" 1 '["1.1.1.1:19999"]' "yhq7ifNCtTKEpY4Yu5XPCcztQco6Fh6JsZ" "93746e8731c57f87f79b3620a7982924e2931717d49540a85864bd543de11c43fb868fd63e501a1db37e19ed59ae6db4" "yTretFTpoi3oQ3maZk5QadGaDWPiKnmDBc" 0 "yNbNZyCiTYSFtDwEXt7jChV7tZVYX862ua" error code: -32603 error message: No funds at specified address yNbNZyCiTYSFtDwEXt7jChV7tZVYX862ua ``` ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 5978985 Tree-SHA512: 9271d7a4c5f463611a10c7edcb0cea929d6d7e0c07d6b962f4aca40483f84ff17b29e3d6b84c465c267734ff6e93e2148e11af0ad19ce58585ed3200fc3e3b01
1 parent 00368fb commit 877343a

File tree

4 files changed

+217
-15
lines changed

4 files changed

+217
-15
lines changed

‎src/rpc/client.cpp‎

Lines changed: 78 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -8,16 +8,17 @@
88
#include <tinyformat.h>
99
#include <util/system.h>
1010

11-
#include <set>
11+
#include <map>
1212
#include <string>
1313
#include <string_view>
1414

1515
class CRPCConvertParam
1616
{
1717
public:
18-
std::string methodName; //!< method whose params want conversion
19-
int paramIdx; //!< 0-based idx of param to convert
20-
std::string paramName; //!< parameter name
18+
std::string methodName; //!< method whose params want conversion
19+
int paramIdx; //!< 0-based idx of param to convert
20+
std::string paramName; //!< parameter name
21+
bool preserve_str{false}; //!< only parse if array or object
2122
};
2223

2324
// clang-format off
@@ -253,36 +254,83 @@ static const CRPCConvertParam vRPCConvertParams[] =
253254
{ "verifyislock", 3, "maxHeight" },
254255
{ "submitchainlock", 2, "blockHeight" },
255256
{ "mnauth", 0, "nodeId" },
257+
{ "protx register", 3, "coreP2PAddrs", true },
258+
{ "protx register_legacy", 3, "coreP2PAddrs", true },
259+
{ "protx register_evo", 3, "coreP2PAddrs", true },
260+
{ "protx register_evo", 10, "platformP2PAddrs", true },
261+
{ "protx register_evo", 11, "platformHTTPSAddrs", true },
262+
{ "protx register_fund", 2, "coreP2PAddrs", true },
263+
{ "protx register_fund_legacy", 2, "coreP2PAddrs", true },
264+
{ "protx register_fund_evo", 2, "coreP2PAddrs", true },
265+
{ "protx register_fund_evo", 9, "platformP2PAddrs", true },
266+
{ "protx register_fund_evo", 10, "platformHTTPSAddrs", true },
267+
{ "protx register_prepare", 3, "coreP2PAddrs", true },
268+
{ "protx register_prepare_legacy", 3, "coreP2PAddrs", true },
269+
{ "protx register_prepare_evo", 3, "coreP2PAddrs", true },
270+
{ "protx register_prepare_evo", 10, "platformP2PAddrs", true },
271+
{ "protx register_prepare_evo", 11, "platformHTTPSAddrs", true },
272+
{ "protx update_service", 2, "coreP2PAddrs", true },
273+
{ "protx update_service_evo", 2, "coreP2PAddrs", true },
274+
{ "protx update_service_evo", 5, "platformP2PAddrs", true },
275+
{ "protx update_service_evo", 6, "platformHTTPSAddrs", true },
256276
};
257277
// clang-format on
258278

259279
class CRPCConvertTable
260280
{
261281
private:
262-
std::set<std::pair<std::string, int>> members;
263-
std::set<std::pair<std::string, std::string>> membersByName;
282+
std::map<std::pair<std::string, int>, bool> members;
283+
std::map<std::pair<std::string, std::string>, bool> membersByName;
284+
285+
std::string_view MaybeUnquoteString(std::string_view arg_value)
286+
{
287+
if (arg_value.size() >= 2 && ((arg_value.front() == '\'' && arg_value.back() == '\'') || (arg_value.front() == '\"' && arg_value.back() == '\"'))) {
288+
return arg_value.substr(1, arg_value.size() - 2);
289+
}
290+
return arg_value;
291+
}
292+
293+
bool LikelyJSONType(std::string_view arg_value)
294+
{
295+
arg_value = MaybeUnquoteString(arg_value);
296+
return arg_value.size() >= 2 && ((arg_value.front() == '[' && arg_value.back() == ']') || (arg_value.front() == '{' && arg_value.back() == '}'));
297+
}
264298

265299
public:
266300
CRPCConvertTable();
267301

268302
/** Return arg_value as UniValue, and first parse it if it is a non-string parameter */
269303
UniValue ArgToUniValue(std::string_view arg_value, const std::string& method, int param_idx)
270304
{
271-
return members.count({method, param_idx}) > 0 ? ParseNonRFCJSONValue(arg_value) : arg_value;
305+
if (const auto it = members.find({method, param_idx}); it != members.end() && (!it->second || (it->second && LikelyJSONType(arg_value)))) {
306+
return ParseNonRFCJSONValue(MaybeUnquoteString(arg_value));
307+
}
308+
return arg_value;
272309
}
273310

274311
/** Return arg_value as UniValue, and first parse it if it is a non-string parameter */
275312
UniValue ArgToUniValue(std::string_view arg_value, const std::string& method, const std::string& param_name)
276313
{
277-
return membersByName.count({method, param_name}) > 0 ? ParseNonRFCJSONValue(arg_value) : arg_value;
314+
if (const auto it = membersByName.find({method, param_name}); it != membersByName.end() && (!it->second || (it->second && LikelyJSONType(arg_value)))) {
315+
return ParseNonRFCJSONValue(MaybeUnquoteString(arg_value));
316+
}
317+
return arg_value;
318+
}
319+
320+
/** Check if we have any conversion rules for this method */
321+
bool IsDefined(const std::string& method, bool named) const
322+
{
323+
return named ?
324+
std::find_if(membersByName.begin(), membersByName.end(), [&method](const auto& kv) { return kv.first.first == method; }) != membersByName.end()
325+
: std::find_if(members.begin(), members.end(), [&method](const auto& kv) { return kv.first.first == method; }) != members.end();
278326
}
279327
};
280328

281329
CRPCConvertTable::CRPCConvertTable()
282330
{
283331
for (const auto& cp : vRPCConvertParams) {
284-
members.emplace(cp.methodName, cp.paramIdx);
285-
membersByName.emplace(cp.methodName, cp.paramName);
332+
members.try_emplace({cp.methodName, cp.paramIdx}, cp.preserve_str);
333+
membersByName.try_emplace({cp.methodName, cp.paramName}, cp.preserve_str);
286334
}
287335
}
288336

@@ -298,10 +346,19 @@ UniValue ParseNonRFCJSONValue(std::string_view raw)
298346
return parsed;
299347
}
300348

301-
UniValue RPCConvertValues(const std::string &strMethod, const std::vector<std::string> &strParams)
349+
UniValue RPCConvertValues(std::string strMethod, const std::vector<std::string> &strParams)
302350
{
303351
UniValue params(UniValue::VARR);
304352

353+
// If we are using a subcommand that is in the table, update the method name
354+
strMethod = [&strMethod, &strParams]() {
355+
if (!strParams.empty() && strMethod.find(' ') == std::string::npos) {
356+
std::string candidate{strMethod + " " + strParams[0]};
357+
return rpcCvtTable.IsDefined(candidate, /*named=*/false) ? candidate : strMethod;
358+
}
359+
return strMethod;
360+
}();
361+
305362
for (unsigned int idx = 0; idx < strParams.size(); idx++) {
306363
std::string_view value{strParams[idx]};
307364
params.push_back(rpcCvtTable.ArgToUniValue(value, strMethod, idx));
@@ -310,11 +367,20 @@ UniValue RPCConvertValues(const std::string &strMethod, const std::vector<std::s
310367
return params;
311368
}
312369

313-
UniValue RPCConvertNamedValues(const std::string &strMethod, const std::vector<std::string> &strParams)
370+
UniValue RPCConvertNamedValues(std::string strMethod, const std::vector<std::string> &strParams)
314371
{
315372
UniValue params(UniValue::VOBJ);
316373
UniValue positional_args{UniValue::VARR};
317374

375+
// If we are using a subcommand that is in the table, update the method name
376+
strMethod = [&strMethod, &strParams]() {
377+
if (strMethod.find(' ') == std::string::npos && !strParams.empty() && strParams[0].find('=') == std::string::npos) {
378+
std::string candidate{strMethod + " " + strParams[0]};
379+
return rpcCvtTable.IsDefined(candidate, /*named=*/true) ? candidate : strMethod;
380+
}
381+
return strMethod;
382+
}();
383+
318384
for (std::string_view s: strParams) {
319385
size_t pos = s.find('=');
320386
if (pos == std::string::npos) {

‎src/rpc/client.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,10 @@
1212
#include <univalue.h>
1313

1414
/** Convert positional arguments to command-specific RPC representation */
15-
UniValue RPCConvertValues(const std::string& strMethod, const std::vector<std::string>& strParams);
15+
UniValue RPCConvertValues(std::string strMethod, const std::vector<std::string>& strParams);
1616

1717
/** Convert named arguments to command-specific RPC representation */
18-
UniValue RPCConvertNamedValues(const std::string& strMethod, const std::vector<std::string>& strParams);
18+
UniValue RPCConvertNamedValues(std::string strMethod, const std::vector<std::string>& strParams);
1919

2020
/** Non-RFC4627 JSON parser, accepts internal values (such as numbers, true, false, null)
2121
* as well as objects and arrays.

‎src/test/rpc_tests.cpp‎

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -596,4 +596,138 @@ BOOST_AUTO_TEST_CASE(rpc_bls)
596596
BOOST_CHECK_EQUAL(r.get_obj().find_value("public").get_str(), "b379c28e0f50546906fe733f1222c8f7e39574d513790034f1fec1476286eb652a350c8c0e630cd2cc60d10c26d6f6ee");
597597
}
598598

599+
BOOST_AUTO_TEST_CASE(rpc_convert_composite_commands)
600+
{
601+
UniValue result;
602+
603+
// Validate that array syntax is not interpreted as string literal
604+
BOOST_CHECK_NO_THROW(result = RPCConvertValues("protx", {
605+
"register_prepare",
606+
"6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000",
607+
"1",
608+
"[\"1.1.1.1:19999\",\"1.0.0.1:19999\"]",
609+
"yhq7ifNCtTKEpY4Yu5XPCcztQco6Fh6JsZ"
610+
}));
611+
612+
BOOST_CHECK_EQUAL(result[0].get_str(), "register_prepare");
613+
BOOST_CHECK_EQUAL(result[1].get_str(), "6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000");
614+
BOOST_CHECK_EQUAL(result[2].get_str(), "1");
615+
BOOST_CHECK(result[3].isArray());
616+
BOOST_CHECK_EQUAL(result[3].size(), 2);
617+
BOOST_CHECK_EQUAL(result[3][0].get_str(), "1.1.1.1:19999");
618+
BOOST_CHECK_EQUAL(result[3][1].get_str(), "1.0.0.1:19999");
619+
BOOST_CHECK_EQUAL(result[4].get_str(), "yhq7ifNCtTKEpY4Yu5XPCcztQco6Fh6JsZ");
620+
621+
// Validate that array syntax is not interpreted as string literal (named parameter)
622+
BOOST_CHECK_NO_THROW(result = RPCConvertNamedValues("protx", {
623+
"register_prepare",
624+
"6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000",
625+
"1",
626+
"coreP2PAddrs=[\"1.1.1.1:19999\",\"1.0.0.1:19999\"]",
627+
"ownerAddress=yhq7ifNCtTKEpY4Yu5XPCcztQco6Fh6JsZ"
628+
}));
629+
630+
BOOST_CHECK(result.exists("coreP2PAddrs"));
631+
BOOST_CHECK(result["coreP2PAddrs"].isArray());
632+
BOOST_CHECK_EQUAL(result["coreP2PAddrs"].size(), 2);
633+
BOOST_CHECK_EQUAL(result["coreP2PAddrs"][0].get_str(), "1.1.1.1:19999");
634+
BOOST_CHECK_EQUAL(result["coreP2PAddrs"][1].get_str(), "1.0.0.1:19999");
635+
BOOST_CHECK_EQUAL(result["ownerAddress"].get_str(), "yhq7ifNCtTKEpY4Yu5XPCcztQco6Fh6JsZ");
636+
637+
// Validate that array syntax is parsed for all recognized fields
638+
BOOST_CHECK_NO_THROW(result = RPCConvertValues("protx", {
639+
"register_evo",
640+
"6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000",
641+
"1",
642+
"[\"1.1.1.1:19999\"]",
643+
"yhq7ifNCtTKEpY4Yu5XPCcztQco6Fh6JsZ",
644+
"93746e8731c57f87f79b3620a7982924e2931717d49540a85864bd543de11c43fb868fd63e501a1db37e19ed59ae6db4",
645+
"yTretFTpoi3oQ3maZk5QadGaDWPiKnmDBc",
646+
"0",
647+
"yNbNZyCiTYSFtDwEXt7jChV7tZVYX862ua",
648+
"f2dbd9b0a1f541a7c44d34a58674d0262f5feca5",
649+
"[\"1.1.1.1:22000\"]",
650+
"[\"1.1.1.1:22001\"]",
651+
"yTG8jLL3MvteKXgbEcHyaN7JvTPCejQpSh"
652+
}));
653+
654+
BOOST_CHECK(result[3].isArray());
655+
BOOST_CHECK_EQUAL(result[3][0].get_str(), "1.1.1.1:19999");
656+
BOOST_CHECK(result[10].isArray());
657+
BOOST_CHECK_EQUAL(result[10][0].get_str(), "1.1.1.1:22000");
658+
BOOST_CHECK(result[11].isArray());
659+
BOOST_CHECK_EQUAL(result[11][0].get_str(), "1.1.1.1:22001");
660+
661+
// Validate that extra quotation doesn't cause string literal interpretation
662+
BOOST_CHECK_NO_THROW(result = RPCConvertValues("protx", {
663+
"register_prepare",
664+
"6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000",
665+
"1",
666+
"\'[\"1.1.1.1:19999\",\"1.0.0.1:19999\"]\'",
667+
"yhq7ifNCtTKEpY4Yu5XPCcztQco6Fh6JsZ"
668+
}));
669+
BOOST_CHECK(result[3].isArray());
670+
BOOST_CHECK_EQUAL(result[3].size(), 2);
671+
BOOST_CHECK_EQUAL(result[3][0].get_str(), "1.1.1.1:19999");
672+
BOOST_CHECK_EQUAL(result[3][1].get_str(), "1.0.0.1:19999");
673+
674+
BOOST_CHECK_NO_THROW(result = RPCConvertValues("protx", {
675+
"register_prepare",
676+
"6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000",
677+
"1",
678+
"\"[\"1.1.1.1:19999\",\"1.0.0.1:19999\"]\"",
679+
"yhq7ifNCtTKEpY4Yu5XPCcztQco6Fh6JsZ"
680+
}));
681+
BOOST_CHECK(result[3].isArray());
682+
BOOST_CHECK_EQUAL(result[3].size(), 2);
683+
BOOST_CHECK_EQUAL(result[3][0].get_str(), "1.1.1.1:19999");
684+
BOOST_CHECK_EQUAL(result[3][1].get_str(), "1.0.0.1:19999");
685+
686+
// Validate parsing as string if *not* using array or object syntax
687+
BOOST_CHECK_NO_THROW(result = RPCConvertValues("protx", {
688+
"register_prepare",
689+
"6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000",
690+
"1",
691+
"1.1.1.1:19999",
692+
"yhq7ifNCtTKEpY4Yu5XPCcztQco6Fh6JsZ"
693+
}));
694+
695+
BOOST_CHECK(!result[3].isArray());
696+
BOOST_CHECK_EQUAL(result[3].get_str(), "1.1.1.1:19999");
697+
698+
// Empty arrays should be recognized as arrays
699+
BOOST_CHECK_NO_THROW(result = RPCConvertValues("protx", {
700+
"register_prepare",
701+
"6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000",
702+
"1",
703+
"[]",
704+
"yhq7ifNCtTKEpY4Yu5XPCcztQco6Fh6JsZ"
705+
}));
706+
BOOST_CHECK(result[3].isArray());
707+
BOOST_CHECK(result[3].empty());
708+
709+
// Incomplete syntax should be interpreted as string
710+
BOOST_CHECK_NO_THROW(result = RPCConvertValues("protx", {
711+
"register_prepare",
712+
"6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000",
713+
"1",
714+
"[",
715+
"yhq7ifNCtTKEpY4Yu5XPCcztQco6Fh6JsZ"
716+
}));
717+
BOOST_CHECK(!result[3].isArray());
718+
BOOST_CHECK_EQUAL(result[3].get_str(), "[");
719+
720+
// Sanity check to ensure that regular commands continue to behave as expected
721+
BOOST_CHECK_NO_THROW(result = RPCConvertValues("getblockstats", {
722+
"1000",
723+
"[\"minfeerate\",\"avgfeerate\"]"
724+
}));
725+
726+
BOOST_CHECK_EQUAL(result[0].getInt<int>(), 1000);
727+
BOOST_CHECK(result[1].isArray());
728+
BOOST_CHECK_EQUAL(result[1].size(), 2);
729+
BOOST_CHECK_EQUAL(result[1][0].get_str(), "minfeerate");
730+
BOOST_CHECK_EQUAL(result[1][1].get_str(), "avgfeerate");
731+
}
732+
599733
BOOST_AUTO_TEST_SUITE_END()

‎test/functional/rpc_help.py‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ def process_mapping(fname):
3232
if line.startswith('};'):
3333
in_rpcs = False
3434
elif '{' in line and '"' in line:
35-
m = re.search(r'{ *("[^"]*"), *([0-9]+) *, *("[^"]*") *},', line)
35+
m = re.search(r'{ *("[^"]*"), *([0-9]+) *, *("[^"]*")(?:, *(true|false))? *},', line)
3636
assert m, 'No match to table expression: %s' % line
3737
name = parse_string(m.group(1))
3838
idx = int(m.group(2))
@@ -59,6 +59,8 @@ def test_client_conversion_table(self):
5959
mapping_client = process_mapping(file_conversion_table)
6060
# Ignore echojson in client table
6161
mapping_client = [m for m in mapping_client if m[0] != 'echojson']
62+
# Filter out composite commands
63+
mapping_client = [m for m in mapping_client if ' ' not in m[0]]
6264

6365
mapping_server = self.nodes[0].help("dump_all_command_conversions")
6466
# Filter all RPCs whether they need conversion

0 commit comments

Comments
 (0)