From e2c3b18e671e347e422d696d1cbdd9f82b2ce468 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Fri, 2 Dec 2022 17:37:08 -0500 Subject: test: Add RPC tests for same named parameter specified more than once Current behavior isn't ideal and will be changed in upcoming commits, but it's useful to have test coverage regardless. MarcoFalke reported the case of bitcoin-cli positional arguments overwriting the named "args" parameter in https://github.com/bitcoin/bitcoin/pull/19762#discussion_r1035761471 --- src/test/rpc_tests.cpp | 3 +++ test/functional/interface_bitcoin_cli.py | 4 ++++ 2 files changed, 7 insertions(+) diff --git a/src/test/rpc_tests.cpp b/src/test/rpc_tests.cpp index 21ccbe9648..23e666bc45 100644 --- a/src/test/rpc_tests.cpp +++ b/src/test/rpc_tests.cpp @@ -89,6 +89,9 @@ BOOST_AUTO_TEST_CASE(rpc_namedparams) // Make sure named arguments are transformed into positional arguments in correct places separated by nulls BOOST_CHECK_EQUAL(TransformParams(JSON(R"({"arg2": 2, "arg4": 4})"), arg_names).write(), "[null,2,null,4]"); + // Make sure later named argument value silently overwrites earlier values + BOOST_CHECK_EQUAL(TransformParams(JSON(R"({"arg2": 2, "arg2": 4})"), arg_names).write(), "[null,4]"); + // Make sure named and positional arguments can be combined. BOOST_CHECK_EQUAL(TransformParams(JSON(R"({"arg5": 5, "args": [1, 2], "arg4": 4})"), arg_names).write(), "[1,2,null,4,5]"); diff --git a/test/functional/interface_bitcoin_cli.py b/test/functional/interface_bitcoin_cli.py index b1369c2615..afba8dba85 100755 --- a/test/functional/interface_bitcoin_cli.py +++ b/test/functional/interface_bitcoin_cli.py @@ -90,6 +90,10 @@ class TestBitcoinCli(BitcoinTestFramework): assert_raises_rpc_error(-8, "Parameter arg1 specified twice both as positional and named argument", self.nodes[0].cli.echo, 0, 1, arg1=1) assert_raises_rpc_error(-8, "Parameter arg1 specified twice both as positional and named argument", self.nodes[0].cli.echo, 0, None, 2, arg1=1) + self.log.info("Test that later cli named arguments values silently overwrite earlier ones") + assert_equal(self.nodes[0].cli("-named", "echo", "arg0=0", "arg1=1", "arg2=2", "arg1=3").send_cli(), ['0', '3', '2']) + assert_equal(self.nodes[0].cli("-named", "echo", "args=[0,1,2,3]", "4", "5", "6", ).send_cli(), ['4', '5', '6']) + user, password = get_auth_cookie(self.nodes[0].datadir, self.chain) self.log.info("Test -stdinrpcpass option") -- cgit v1.2.3 From 6bd1d20b8cf27aa72ec2907342787e6fc9f94c50 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Fri, 2 Dec 2022 17:53:16 -0500 Subject: rpc: Make it an error server-side to specify same named parameter multiple times Specifying same named parameter multiple times is still allowed by bitcoin-cli. The client implementation overwrites earlier option values with later ones before sending to server. This is tested by interface_bitcoin_cli.py Rationale for allowing client parameters to be specified multiple times in bitcoin-cli is that this behavior has been supported for a long time, and that when using the command line interactively, it can be convenient to override earlier option values with new values without having to go back and remove the old value. But for the RPC server, there isn't really a good use-case for earlier values to be discarded if multiple values are specified. JSON keys are generally supposed to be unique and if they aren't it's probably an indication of some problem generating the RPC request. --- doc/release-notes-26628.md | 4 ++++ src/rpc/server.cpp | 5 ++++- src/test/rpc_tests.cpp | 5 +++-- 3 files changed, 11 insertions(+), 3 deletions(-) create mode 100644 doc/release-notes-26628.md diff --git a/doc/release-notes-26628.md b/doc/release-notes-26628.md new file mode 100644 index 0000000000..48a07c1e81 --- /dev/null +++ b/doc/release-notes-26628.md @@ -0,0 +1,4 @@ +JSON-RPC +--- + +The JSON-RPC server now rejects requests where a parameter is specified multiple times with the same name, instead of silently overwriting earlier parameter values with later ones. (#26628) diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp index f57133f75b..232e2119f1 100644 --- a/src/rpc/server.cpp +++ b/src/rpc/server.cpp @@ -399,7 +399,10 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c const std::vector& values = in.params.getValues(); std::unordered_map argsIn; for (size_t i=0; i Date: Fri, 2 Dec 2022 17:53:58 -0500 Subject: bitcoin-cli: Make it an error to specify the "args" parameter two different ways MarcoFalke reported the case of positional arguments silently overwriting the named "args" parameter in bitcoin-cli https://github.com/bitcoin/bitcoin/pull/19762#discussion_r1035761471 and this behavior is confusing and was not intended when support for "args" parameters was added to bitcoin-cli in #19762. Instead of letting one "args" value overwrite the other in the client, just pass the values to the server verbatim, and let the error be handled server side. --- src/rpc/client.cpp | 8 +++++++- test/functional/interface_bitcoin_cli.py | 2 +- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index b3434b80c7..ea094976bf 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -289,6 +289,9 @@ UniValue RPCConvertNamedValues(const std::string &strMethod, const std::vector Date: Fri, 9 Dec 2022 10:34:28 -0500 Subject: test: Suggested cleanups for rpc_namedparams test No changes in behavior, just implements review suggestions from https://github.com/bitcoin/bitcoin/pull/19762#discussion_r1025573943 https://github.com/bitcoin/bitcoin/pull/19762#discussion_r1035955247 https://github.com/bitcoin/bitcoin/pull/26628#discussion_r1038765894 --- src/test/rpc_tests.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/rpc_tests.cpp b/src/test/rpc_tests.cpp index a4d5a4b12c..f9b8a47330 100644 --- a/src/test/rpc_tests.cpp +++ b/src/test/rpc_tests.cpp @@ -84,7 +84,7 @@ BOOST_FIXTURE_TEST_SUITE(rpc_tests, RPCTestingSetup) BOOST_AUTO_TEST_CASE(rpc_namedparams) { - const std::vector arg_names{{"arg1", "arg2", "arg3", "arg4", "arg5"}}; + const std::vector arg_names{"arg1", "arg2", "arg3", "arg4", "arg5"}; // Make sure named arguments are transformed into positional arguments in correct places separated by nulls BOOST_CHECK_EQUAL(TransformParams(JSON(R"({"arg2": 2, "arg4": 4})"), arg_names).write(), "[null,2,null,4]"); @@ -104,7 +104,7 @@ BOOST_AUTO_TEST_CASE(rpc_namedparams) BOOST_CHECK_EXCEPTION(TransformParams(JSON(R"({"args": [1,2,3], "arg4": 4, "arg2": 2})"), arg_names), UniValue, HasJSON(R"({"code":-8,"message":"Parameter arg2 specified twice both as positional and named argument"})")); - // Make sure extra positional arguments can be passed through to the method implemenation, as long as they don't overlap with named arguments. + // Make sure extra positional arguments can be passed through to the method implementation, as long as they don't overlap with named arguments. BOOST_CHECK_EQUAL(TransformParams(JSON(R"({"args": [1,2,3,4,5,6,7,8,9,10]})"), arg_names).write(), "[1,2,3,4,5,6,7,8,9,10]"); BOOST_CHECK_EQUAL(TransformParams(JSON(R"([1,2,3,4,5,6,7,8,9,10])"), arg_names).write(), "[1,2,3,4,5,6,7,8,9,10]"); } -- cgit v1.2.3