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

8c3ff7d52ae3314959e1e66da8718a3f0d30abaa test: Suggested cleanups for rpc_namedparams test (Ryan Ofsky)
d1ca56382512df3084fce7353bf1e8b66cae61bc bitcoin-cli: Make it an error to specify the "args" parameter two different ways (Ryan Ofsky)
6bd1d20b8cf27aa72ec2907342787e6fc9f94c50 rpc: Make it an error server-side to specify same named parameter multiple times (Ryan Ofsky)
e2c3b18e671e347e422d696d1cbdd9f82b2ce468 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 8c3ff7d52ae3314959e1e66da8718a3f0d30abaa 🗂
  kristapsk:
    ACK 8c3ff7d52ae3314959e1e66da8718a3f0d30abaa
  stickies-v:
    ACK 8c3ff7d52

Tree-SHA512: 2d1357dcc2c171da287aeefc7b333ba4e67babfb64fc14d7fa0940256e18010a2a65054f3bf7fa1571b144d2de8b82d53076111b5f97ba29320cfe84b6ed986f

5 files changed, 25 insertions, 4 deletions

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/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<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);
@@ -299,7 +302,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;
diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp
index 80a22ff8ca..a026b7adfa 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<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
diff --git a/src/test/rpc_tests.cpp b/src/test/rpc_tests.cpp
index 21ccbe9648..f9b8a47330 100644
--- a/src/test/rpc_tests.cpp
+++ b/src/test/rpc_tests.cpp
@@ -84,11 +84,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]");
 
@@ -100,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]");
 }
diff --git a/test/functional/interface_bitcoin_cli.py b/test/functional/interface_bitcoin_cli.py
index b1369c2615..90a543b51b 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_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")