Merge bitcoin#26628: RPC: Reject RPC requests with same named parameter specified multiple times

8c3ff7d test: Suggested cleanups for rpc_namedparams test (Ryan Ofsky)
d1ca563 bitcoin-cli: Make it an error to specify the "args" parameter two different ways (Ryan Ofsky)
6bd1d20 rpc: Make it an error server-side to specify same named parameter multiple times (Ryan Ofsky)
e2c3b18 test: Add RPC tests for same named parameter specified more than once (Ryan Ofsky)

Pull request description:

  Make the JSON-RPC server reject requests with the same named parameter specified multiple times, instead of silently overwriting earlier parameter values with later ones.

  Generally JSON keys are supposed to unique, and their order isn't supposed to be significant, so having the server silently discard duplicate keys is error-prone. Most likely if an RPC client is sending a request with duplicate keys it means something is wrong with the request and there should be an error.

  After this change, named parameters are still allowed to specified multiple times on the `bitcoin-cli` command line, since `bitcoin-cli` automatically replaces earlier values with later values before sending the JSON-RPC request. This makes sense, since it's not unusual for the order of command line options to be significant or for later command line options to override earlier ones.

ACKs for top commit:
  MarcoFalke:
    review ACK 8c3ff7d 🗂
  kristapsk:
    ACK 8c3ff7d
  stickies-v:
    ACK 8c3ff7d

Tree-SHA512: 2d1357dcc2c171da287aeefc7b333ba4e67babfb64fc14d7fa0940256e18010a2a65054f3bf7fa1571b144d2de8b82d53076111b5f97ba29320cfe84b6ed986f

Author: MarcoFalke

doc/release-notes-6050.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. (dash#6050)

src/rpc/client.cpp

@@ -305,6 +305,9 @@ UniValue RPCConvertNamedValues(const std::string &strMethod, const std::vector<s
         std::string name = s.substr(0, pos);
         std::string value = s.substr(pos+1);
 
+        // Intentionally overwrite earlier named values with later ones as a
+        // convenience for scripts and command line users that want to merge
+        // options.
         if (!rpcCvtTable.convert(strMethod, name)) {
             // insert string value directly
             params.pushKV(name, value);
@@ -315,7 +318,10 @@ UniValue RPCConvertNamedValues(const std::string &strMethod, const std::vector<s
     }
 
     if (!positional_args.empty()) {
-        params.pushKV("args", positional_args);
+        // Use __pushKV instead of pushKV to avoid overwriting an explicit
+        // "args" value with an implicit one. Let the RPC server handle the
+        // request as given.
+        params.__pushKV("args", positional_args);
     }
 
     return params;

src/rpc/server.cpp

@@ -428,7 +428,10 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c
     const std::vector<UniValue>& values = in.params.getValues();
     std::unordered_map<std::string, const UniValue*> argsIn;
     for (size_t i=0; i<keys.size(); ++i) {
-        argsIn[keys[i]] = &values[i];
+        auto [_, inserted] = argsIn.emplace(keys[i], &values[i]);
+        if (!inserted) {
+            throw JSONRPCError(RPC_INVALID_PARAMETER, "Parameter " + keys[i] + " specified multiple times");
+        }
     }
     // Process expected parameters. If any parameters were left unspecified in
     // the request before a parameter that was specified, null values need to be

src/test/rpc_tests.cpp

@@ -85,11 +85,15 @@ BOOST_FIXTURE_TEST_SUITE(rpc_tests, RPCTestingSetup)
 
 BOOST_AUTO_TEST_CASE(rpc_namedparams)
 {
-    const std::vector<std::string> arg_names{{"arg1", "arg2", "arg3", "arg4", "arg5"}};
+    const std::vector<std::string> 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]");
 
+    // Make sure named argument specified multiple times raises an exception
+    BOOST_CHECK_EXCEPTION(TransformParams(JSON(R"({"arg2": 2, "arg2": 4})"), arg_names), UniValue,
+                          HasJSON(R"({"code":-8,"message":"Parameter arg2 specified multiple times"})"));
+
     // 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]");
 
@@ -101,7 +105,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]");
 }

test/functional/interface_bitcoin_cli.py

@@ -89,6 +89,10 @@ def run_test(self):
         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_raises_rpc_error(-8, "Parameter args specified multiple times", self.nodes[0].cli("-named", "echo", "args=[0,1,2,3]", "4", "5", "6", ).send_cli)
+
         user, password = get_auth_cookie(self.nodes[0].datadir, self.chain)
 
         self.log.info("Test -stdinrpcpass option")