From 11e17ef77b2d819d0ed3b32a59da4a71cca0fb2c Mon Sep 17 00:00:00 2001 From: Will Greenberg Date: Fri, 13 Oct 2023 12:02:01 -0700 Subject: [PATCH] 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 | 3 +- certbot/certbot/_internal/cli/helpful.py | 60 ++++++++++++++++++--- certbot/certbot/_internal/tests/cli_test.py | 42 +++++++++++++++ 3 files changed, 96 insertions(+), 9 deletions(-) diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md index 28a866731..92ceba3fb 100644 --- a/certbot/CHANGELOG.md +++ b/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. diff --git a/certbot/certbot/_internal/cli/helpful.py b/certbot/certbot/_internal/cli/helpful.py index f42096ded..6fea1b131 100644 --- a/certbot/certbot/_internal/cli/helpful.py +++ b/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|" 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. diff --git a/certbot/certbot/_internal/tests/cli_test.py b/certbot/certbot/_internal/tests/cli_test.py index 446b2252f..e4505dbee 100644 --- a/certbot/certbot/_internal/tests/cli_test.py +++ b/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