From 900d8f6f70859f528e84c5c38d0332f81d19df55 Mon Sep 17 00:00:00 2001
From: Russell Yanofsky <russ@yanofsky.org>
Date: Thu, 19 Dec 2019 16:27:15 -0500
Subject: util: Disallow network-qualified command line options

Previously these were allowed but ignored.
---
 doc/release-notes.md                   |  7 +++++++
 src/test/util_tests.cpp                | 26 ++++++++++++++++++++++++--
 src/util/system.cpp                    | 23 ++++++++++-------------
 test/functional/feature_config_args.py |  2 +-
 4 files changed, 42 insertions(+), 16 deletions(-)

diff --git a/doc/release-notes.md b/doc/release-notes.md
index eec1ef9c46..99ca53c597 100644
--- a/doc/release-notes.md
+++ b/doc/release-notes.md
@@ -104,6 +104,13 @@ Wallet
 Low-level changes
 =================
 
+Command line
+------------
+
+Command line options prefixed with main/test/regtest network names like
+`-main.port=8333` `-test.server=1` previously were allowed but ignored. Now
+they trigger "Invalid parameter" errors on startup.
+
 Tests
 -----
 
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 86ea56ff36..7b024ba75f 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -340,6 +340,27 @@ BOOST_AUTO_TEST_CASE(util_ParseParameters)
     BOOST_CHECK(testArgs.GetArgs("-ccc").size() == 2);
 }
 
+BOOST_AUTO_TEST_CASE(util_ParseInvalidParameters)
+{
+    TestArgsManager test;
+    test.SetupArgs({{"-registered", ArgsManager::ALLOW_ANY}});
+
+    const char* argv[] = {"ignored", "-registered"};
+    std::string error;
+    BOOST_CHECK(test.ParseParameters(2, (char**)argv, error));
+    BOOST_CHECK_EQUAL(error, "");
+
+    argv[1] = "-unregistered";
+    BOOST_CHECK(!test.ParseParameters(2, (char**)argv, error));
+    BOOST_CHECK_EQUAL(error, "Invalid parameter -unregistered");
+
+    // Make sure registered parameters prefixed with a chain name trigger errors.
+    // (Previously, they were accepted and ignored.)
+    argv[1] = "-test.registered";
+    BOOST_CHECK(!test.ParseParameters(2, (char**)argv, error));
+    BOOST_CHECK_EQUAL(error, "Invalid parameter -test.registered");
+}
+
 static void TestParse(const std::string& str, bool expected_bool, int64_t expected_int)
 {
     TestArgsManager test;
@@ -835,7 +856,8 @@ struct ArgsMergeTestingSetup : public BasicTestingSetup {
     void ForEachMergeSetup(Fn&& fn)
     {
         ActionList arg_actions = {};
-        ForEachNoDup(arg_actions, SET, SECTION_NEGATE, [&] {
+        // command_line_options do not have sections. Only iterate over SET and NEGATE
+        ForEachNoDup(arg_actions, SET, NEGATE, [&] {
             ActionList conf_actions = {};
             ForEachNoDup(conf_actions, SET, SECTION_NEGATE, [&] {
                 for (bool soft_set : {false, true}) {
@@ -995,7 +1017,7 @@ BOOST_FIXTURE_TEST_CASE(util_ArgsMerge, ArgsMergeTestingSetup)
     // Results file is formatted like:
     //
     //   <input> || <IsArgSet/IsArgNegated/GetArg output> | <GetArgs output> | <GetUnsuitable output>
-    BOOST_CHECK_EQUAL(out_sha_hex, "b835eef5977d69114eb039a976201f8c7121f34fe2b7ea2b73cafb516e5c9dc8");
+    BOOST_CHECK_EQUAL(out_sha_hex, "8fd4877bb8bf337badca950ede6c917441901962f160e52514e06a60dea46cde");
 }
 
 // Similar test as above, but for ArgsManager::GetChainName function.
diff --git a/src/util/system.cpp b/src/util/system.cpp
index d99a87a9f2..5587764c58 100644
--- a/src/util/system.cpp
+++ b/src/util/system.cpp
@@ -311,21 +311,18 @@ bool ArgsManager::ParseParameters(int argc, const char* const argv[], std::strin
         std::string section;
         util::SettingsValue value = InterpretOption(section, key, val);
         Optional<unsigned int> flags = GetArgFlags('-' + key);
-        if (flags) {
-            if (!CheckValid(key, value, *flags, error)) {
-                return false;
-            }
-            // Weird behavior preserved for backwards compatibility: command
-            // line options with section prefixes are allowed but ignored. It
-            // would be better if these options triggered the Invalid parameter
-            // error below.
-            if (section.empty()) {
-                m_settings.command_line_options[key].push_back(value);
-            }
-        } else {
-            error = strprintf("Invalid parameter -%s", key);
+
+        // Unknown command line options and command line options with dot
+        // characters (which are returned from InterpretOption with nonempty
+        // section strings) are not valid.
+        if (!flags || !section.empty()) {
+            error = strprintf("Invalid parameter %s", argv[i]);
             return false;
         }
+
+        if (!CheckValid(key, value, *flags, error)) return false;
+
+        m_settings.command_line_options[key].push_back(value);
     }
 
     // we do not allow -includeconf from command line
diff --git a/test/functional/feature_config_args.py b/test/functional/feature_config_args.py
index 801209b1c0..8397eb9f59 100755
--- a/test/functional/feature_config_args.py
+++ b/test/functional/feature_config_args.py
@@ -23,7 +23,7 @@ class ConfArgsTest(BitcoinTestFramework):
             conf.write('includeconf={}\n'.format(inc_conf_file_path))
 
         self.nodes[0].assert_start_raises_init_error(
-            expected_msg='Error: Error parsing command line arguments: Invalid parameter -dash_cli',
+            expected_msg='Error: Error parsing command line arguments: Invalid parameter -dash_cli=1',
             extra_args=['-dash_cli=1'],
         )
         with open(inc_conf_file_path, 'w', encoding='utf-8') as conf:
-- 
cgit v1.2.3