helpful: fix handling of abbreviated ConfigArgparse arguments (#9796)

* helpful: fix handling of abbreviated ConfigArgparse arguments

ConfigArgparse allows for "abbreviated" arguments, i.e. just the prefix
of an argument, but it doesn't set the argument sources in these cases.
This commit checks for those cases and sets the sources appropriately.

* failing to find an action raises an error instead of logging

* Update changelog

* Add handling for short arguments, fix equals sign handling

These were silently being dropped before, possibly leading to instances
of `NamespaceConfig.set_by_user()` returning false negatives.

certbot/CHANGELOG.md

@@ -14,7 +14,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
 
 ### Fixed
 
-*
+* Fixed a bug where argument sources weren't correctly detected in abbreviated
+  arguments, short arguments, and some other circumstances
 
 More details about these changes can be found on our GitHub repo.
 

certbot/certbot/_internal/cli/helpful.py

@@ -187,10 +187,12 @@ class HelpfulArgumentParser:
         #   2. config files
         #   3. env vars (shouldn't be any)
         #   4. command line
+
         def update_result(settings_dict: Dict[str, Tuple[configargparse.Action, str]],
                           source: ArgumentSource) -> None:
-            actions = [action for _, (action, _) in settings_dict.items()]
-            result.update({ action.dest: source for action in actions})
+            actions = [self._find_action_for_arg(arg) if action is None else action
+                       for arg, (action, _) in settings_dict.items()]
+            result.update({ action.dest: source for action in actions })
 
         # config file sources look like "config_file|<name of file>"
         for source_key in source_to_settings_dict:
@@ -203,17 +205,59 @@ class HelpfulArgumentParser:
         if 'command_line' in source_to_settings_dict:
             settings_dict: Dict[str, Tuple[None, List[str]]]
             settings_dict = source_to_settings_dict['command_line'] # type: ignore
-            (_, args) = settings_dict['']
-            args = [arg for arg in args if arg.startswith('-')]
+            (_, unprocessed_args) = settings_dict['']
+            args = []
+            for arg in unprocessed_args:
+                # ignore non-arguments
+                if not arg.startswith('-'):
+                    continue
+
+                # special case for config file argument, which we don't have an action for
+                if arg in ['-c', '--config']:
+                    result['config_dir'] = ArgumentSource.COMMAND_LINE
+                    continue
+
+                if '=' in arg:
+                    arg = arg.split('=')[0]
+
+                if arg.startswith('--'):
+                    args.append(arg)
+                # for short args (ones that start with a single hyphen), handle
+                # the case of multiple short args together, e.g. "-tvm"
+                else:
+                    for short_arg in arg[1:]:
+                        args.append(f"-{short_arg}")
+
             for arg in args:
                 # find the action corresponding to this arg
-                for action in self.actions:
-                    if arg in action.option_strings:
-                        result[action.dest] = ArgumentSource.COMMAND_LINE
-                        continue
+                action = self._find_action_for_arg(arg)
+                result[action.dest] = ArgumentSource.COMMAND_LINE
 
         return result
 
+    def _find_action_for_arg(self, arg: str) -> configargparse.Action:
+        # Finds a configargparse Action which matches the given arg, where arg
+        # can either be preceded by hyphens (as on the command line) or not (as
+        # in config files)
+
+        # if the argument doesn't have leading hypens, prefix it so it can be
+        # compared directly w/ action option strings
+        if arg[0] != '-':
+            arg = '--' + arg
+
+        # first, check for exact matches
+        for action in self.actions:
+            if arg in action.option_strings:
+                return action
+
+        # now check for abbreviated (i.e. prefix) matches
+        for action in self.actions:
+            for option_string in action.option_strings:
+                if option_string.startswith(arg):
+                    return action
+
+        raise AssertionError(f"Action corresponding to argument {arg} is None")
+
     def parse_args(self) -> NamespaceConfig:
         """Parses command line arguments and returns the result.
 

certbot/certbot/_internal/tests/cli_test.py

@@ -552,6 +552,48 @@ class ParseTest(unittest.TestCase):
             ])
             assert_value_and_source(namespace, 'server', COMMAND_LINE_VALUE, ArgumentSource.COMMAND_LINE)
 
+    def test_abbreviated_arguments(self):
+        # Argparse's "allow_abbrev" option (which is True by default) allows
+        # for unambiguous partial arguments (e.g. "--preferred-chal dns" will be
+        # interepreted the same as "--preferred-challenges dns")
+        namespace = self.parse('--preferred-chal dns --no-dir')
+        assert_set_by_user_with_value(namespace, 'pref_challs', ['dns-01'])
+        assert_set_by_user_with_value(namespace, 'directory_hooks', False)
+
+        with tempfile.NamedTemporaryFile() as tmp_config:
+            tmp_config.close()  # close now because of compatibility issues on Windows
+            with open(tmp_config.name, 'w') as file_h:
+                file_h.write('preferred-chal = dns')
+
+            namespace = self.parse([
+                'certonly',
+                '--config', tmp_config.name,
+            ])
+            assert_set_by_user_with_value(namespace, 'pref_challs', ['dns-01'])
+
+    @mock.patch('certbot._internal.hooks.validate_hooks')
+    def test_argument_with_equals(self, unsused_mock_validate_hooks):
+        namespace = self.parse('-d=example.com')
+        assert_set_by_user_with_value(namespace, 'domains', ['example.com'])
+
+        # make sure it doesn't choke on equals signs being present in the argument value
+        plugins = disco.PluginsRegistry.find_all()
+        namespace = cli.prepare_and_parse_args(plugins, ['run', '--pre-hook="foo=bar"'])
+        assert_set_by_user_with_value(namespace, 'pre_hook', '"foo=bar"')
+
+    def test_adjacent_short_args(self):
+        namespace = self.parse('-tv')
+        assert_set_by_user_with_value(namespace, 'text_mode', True)
+        assert_set_by_user_with_value(namespace, 'verbose_count', 1)
+
+        namespace = self.parse('-tvvv')
+        assert_set_by_user_with_value(namespace, 'text_mode', True)
+        assert_set_by_user_with_value(namespace, 'verbose_count', 3)
+
+        namespace = self.parse('-tvm foo@example.com')
+        assert_set_by_user_with_value(namespace, 'text_mode', True)
+        assert_set_by_user_with_value(namespace, 'verbose_count', 1)
+        assert_set_by_user_with_value(namespace, 'email', 'foo@example.com')
 
 if __name__ == '__main__':
     sys.exit(pytest.main(sys.argv[1:] + [__file__]))  # pragma: no cover