From 28f171d1bbf79e6bba05779daa13d5cbb8deed52 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Mon, 15 Aug 2022 16:57:46 +0200 Subject: config: Properly convert values for list/dict commands Those commands use the config interface expecting Python objects (via update_mutables), but we always used the raw user-supplied string as input. Thus far, it was very hard to trigger this bug: It would only trigger with a List or Dict config option, with a value type which does *not* accept a string type in to_py(). That means: - List / FlagList / ConfirmQuit / ShellCommand - Bool / BoolAsk - Int - Float - Dict / Padding (Notably, Perc, PercOrInt and Regex all *do* accept a string.) That leaves only a couple of candidates: - hints.selectors, but that's "no_autoconfig: true" - bindings.default, ditto - bindings.commands, but no reason to use :config-dict-* on it, especially with fixed keys Therefore, this got only uncovered now, after adding content.javascript.log_message.levels, which is a Dict with FlagList values. Note that we still don't have any config definition with an affected List type, thus currently making it impossible to test the changes for :config-list-{add,remove}. Also note the adjusted test_{dict,list}_add_invalid_value tests - they were just plain wrong. The command functions are never going to be called by a Python object from the user (only with a str). Finally, test_list_remove_no_value also needed an adjustment because the value validation now happens *before* the other validation done by the command. --- qutebrowser/config/configcommands.py | 15 +++++++------ tests/unit/config/test_configcommands.py | 38 +++++++++++++++++++++----------- 2 files changed, 33 insertions(+), 20 deletions(-) diff --git a/qutebrowser/config/configcommands.py b/qutebrowser/config/configcommands.py index 407c74214..b85031818 100644 --- a/qutebrowser/config/configcommands.py +++ b/qutebrowser/config/configcommands.py @@ -308,7 +308,7 @@ class ConfigCommands: with self._handle_config_error(): option_value = self._config.get_mutable_obj(option) - option_value.append(value) + option_value.append(opt.typ.valtype.from_str(value)) self._config.update_mutables(save_yaml=not temp) @cmdutils.register(instance='config-commands') @@ -327,6 +327,7 @@ class ConfigCommands: """ with self._handle_config_error(): opt = self._config.get_opt(option) + if not isinstance(opt.typ, configtypes.Dict): raise cmdutils.CommandError(":config-dict-add can only be used " "for dicts") @@ -339,7 +340,7 @@ class ConfigCommands: "--replace to overwrite!" .format(key, option)) - option_value[key] = value + option_value[key] = opt.typ.valtype.from_str(value) self._config.update_mutables(save_yaml=not temp) @cmdutils.register(instance='config-commands') @@ -360,15 +361,15 @@ class ConfigCommands: raise cmdutils.CommandError(":config-list-remove can only be used " "for lists") + converted = opt.typ.valtype.from_str(value) + with self._handle_config_error(): option_value = self._config.get_mutable_obj(option) - if value not in option_value: - raise cmdutils.CommandError("{} is not in {}!".format( - value, option)) - - option_value.remove(value) + if converted not in option_value: + raise cmdutils.CommandError(f"{value} is not in {option}!") + option_value.remove(converted) self._config.update_mutables(save_yaml=not temp) @cmdutils.register(instance='config-commands') diff --git a/tests/unit/config/test_configcommands.py b/tests/unit/config/test_configcommands.py index 6dd09cee3..d7031de94 100644 --- a/tests/unit/config/test_configcommands.py +++ b/tests/unit/config/test_configcommands.py @@ -319,12 +319,13 @@ class TestAdd: match=":config-list-add can only be used for lists"): commands.config_list_add('history_gap_interval', 'value') - @pytest.mark.parametrize('value', ['', None, 42]) - def test_list_add_invalid_values(self, commands, value): - with pytest.raises( - cmdutils.CommandError, - match="Invalid value '{}'".format(value)): - commands.config_list_add('content.blocking.whitelist', value) + def test_list_add_invalid_value(self, commands): + with pytest.raises(cmdutils.CommandError, match="Invalid value ''"): + commands.config_list_add('content.blocking.whitelist', '') + + # FIXME test value conversion for :list-add like in test_dict_add_value_type + # (once we have a List config option using a non-str type, or a way to + # dynamically add new option definitions). @pytest.mark.parametrize('value', ['test1', 'test2']) @pytest.mark.parametrize('temp', [True, False]) @@ -368,11 +369,18 @@ class TestAdd: match=":config-dict-add can only be used for dicts"): commands.config_dict_add('history_gap_interval', 'key', 'value') - @pytest.mark.parametrize('value', ['', None, 42]) - def test_dict_add_invalid_values(self, commands, value): - with pytest.raises(cmdutils.CommandError, - match="Invalid value '{}'".format(value)): - commands.config_dict_add('aliases', 'missingkey', value) + def test_dict_add_invalid_value(self, commands): + with pytest.raises(cmdutils.CommandError, match="Invalid value ''"): + commands.config_dict_add('aliases', 'missingkey', '') + + def test_dict_add_value_type(self, commands, config_stub): + commands.config_dict_add( + "content.javascript.log_message.levels", + "example", + "['error']", + ) + value = config_stub.val.content.javascript.log_message.levels["example"] + assert value == ['error'] class TestRemove: @@ -407,8 +415,12 @@ class TestRemove: def test_list_remove_no_value(self, commands): with pytest.raises( cmdutils.CommandError, - match="never is not in colors.completion.fg!"): - commands.config_list_remove('colors.completion.fg', 'never') + match="#133742 is not in colors.completion.fg!"): + commands.config_list_remove('colors.completion.fg', '#133742') + + # FIXME test value conversion for :list-remove like in test_dict_add_value_type + # (once we have a List config option using a non-str type, or a way to + # dynamically add new option definitions). @pytest.mark.parametrize('key', ['w', 'q']) @pytest.mark.parametrize('temp', [True, False]) -- cgit v1.2.3-54-g00ecf