diff --git a/samples/configs/parameterized.json b/samples/configs/parameterized.json index b28bf5a3..b3090dbc 100644 --- a/samples/configs/parameterized.json +++ b/samples/configs/parameterized.json @@ -28,14 +28,14 @@ { "name": "Simple List", "param": "--simple_list=", + "same_arg_param": true, "type": "list", "description": "Parameter Five", "values": [ "val1", "val3", "some long value" - ], - "same_arg_param": true + ] }, { "name": "File upload", @@ -48,13 +48,13 @@ "param": "--multiselect", "type": "multiselect", "description": "Multiselect list", - "multiselect_argument_type": "repeat_param_value", "values": [ "Black cat", "Brown dog", "Green parrot", "Red fox" - ] + ], + "multiselect_argument_type": "repeat_param_value" }, { "name": "Required Text", @@ -111,7 +111,7 @@ "type": "list", "description": "List parameter 2", "values": { - "script": "ls /var" + "script": "ls /var | grep -v lock | grep -v backups" } }, { @@ -238,9 +238,9 @@ }, { "name": "Editable list", + "required": true, "param": "--editable_list", "type": "editable_list", - "required": true, "values": [ "Value A", "Value B", diff --git a/src/config/script/list_values.py b/src/config/script/list_values.py index 4a053bf3..b05f2cdd 100644 --- a/src/config/script/list_values.py +++ b/src/config/script/list_values.py @@ -45,8 +45,8 @@ def get_values(self, parameter_values): class ScriptValuesProvider(ValuesProvider): - def __init__(self, script) -> None: - script_output = process_utils.invoke(script) + def __init__(self, script, shell) -> None: + script_output = process_utils.invoke(script, shell=shell) script_output = script_output.rstrip('\n') self._values = [line for line in script_output.split('\n') if not is_empty(line)] @@ -56,7 +56,7 @@ def get_values(self, parameter_values): class DependantScriptValuesProvider(ValuesProvider): - def __init__(self, script, parameters_supplier) -> None: + def __init__(self, script, parameters_supplier, shell) -> None: pattern = re.compile('\${([^}]+)\}') search_start = 0 @@ -80,6 +80,7 @@ def __init__(self, script, parameters_supplier) -> None: self._required_parameters = tuple(required_parameters) self._script_template = script self._parameters_supplier = parameters_supplier + self._shell = shell def get_required_parameters(self): return self._required_parameters @@ -94,7 +95,7 @@ def get_values(self, parameter_values): script = fill_parameter_values(parameters, self._script_template, parameter_values) try: - script_output = process_utils.invoke(script) + script_output = process_utils.invoke(script, shell=self._shell) except Exception as e: LOGGER.warning('Failed to execute script. ' + str(e)) return [] diff --git a/src/model/parameter_config.py b/src/model/parameter_config.py index 24644754..446b114d 100644 --- a/src/model/parameter_config.py +++ b/src/model/parameter_config.py @@ -188,12 +188,16 @@ def _create_values_provider(self, values_config, type, constant): return ConstValuesProvider(values_config) elif 'script' in values_config: - script = replace_auth_vars(values_config['script'], self._username, self._audit_name) + original_script = values_config['script'] + has_variables = ('${' in original_script) + + script = replace_auth_vars(original_script, self._username, self._audit_name) + shell = read_bool_from_config('shell', values_config, default=not has_variables) if '${' not in script: - return ScriptValuesProvider(script) + return ScriptValuesProvider(script, shell) - return DependantScriptValuesProvider(script, self._parameters_supplier) + return DependantScriptValuesProvider(script, self._parameters_supplier, shell) else: message = 'Unsupported "values" format for ' + self.name @@ -439,11 +443,13 @@ def _resolve_default(default, username, audit_name, working_dir): if resolved_string_value == string_value: resolved_string_value = replace_auth_vars(string_value, username, audit_name) - if not script: - return resolved_string_value + if script: + has_variables = string_value != resolved_string_value + shell = read_bool_from_config('shell', default, default=not has_variables) + output = process_utils.invoke(resolved_string_value, working_dir, shell=shell) + return output.strip() - output = process_utils.invoke(resolved_string_value, working_dir) - return output.strip() + return resolved_string_value def _resolve_file_dir(config, key): diff --git a/src/tests/list_values_test.py b/src/tests/list_values_test.py index d70af467..fa7188bb 100644 --- a/src/tests/list_values_test.py +++ b/src/tests/list_values_test.py @@ -1,22 +1,42 @@ import os import unittest +from parameterized import parameterized + from config.script.list_values import DependantScriptValuesProvider, FilesProvider, ScriptValuesProvider from tests import test_utils from tests.test_utils import create_parameter_model from utils import file_utils +from utils.process_utils import ExecutionException class ScriptValuesProviderTest(unittest.TestCase): - def test_ls_3_files(self): + @parameterized.expand([(True,), (False,)]) + def test_ls_3_files(self, shell): test_utils.create_files(['f1', 'f2', 'f3']) - provider = ScriptValuesProvider('ls "' + test_utils.temp_folder + '"') + provider = ScriptValuesProvider('ls "' + test_utils.temp_folder + '"', + shell=shell) self.assertEqual(['f1', 'f2', 'f3'], provider.get_values({})) - def test_ls_no_files(self): - provider = ScriptValuesProvider('ls "' + test_utils.temp_folder + '"') + @parameterized.expand([(True,), (False,)]) + def test_ls_no_files(self, shell): + provider = ScriptValuesProvider('ls "' + test_utils.temp_folder + '"', + shell=shell) self.assertEqual([], provider.get_values({})) + def test_ls_3_files_when_bash_operator(self): + test_utils.create_files(['f1', 'f2', 'f3']) + self.assertRaises(ExecutionException, + ScriptValuesProvider, + 'ls "' + test_utils.temp_folder + '" | grep 2', + shell=False) + + def test_ls_3_files_when_bash_operator_and_shell(self): + test_utils.create_files(['f1', 'f2', 'f3']) + provider = ScriptValuesProvider('ls "' + test_utils.temp_folder + '" | grep 2', + shell=True) + self.assertEqual(['f2'], provider.get_values({})) + def setUp(self) -> None: super().setUp() @@ -29,73 +49,109 @@ def tearDown(self) -> None: class DependantScriptValuesProviderTest(unittest.TestCase): - def test_get_required_parameters_when_single_dependency(self): - values_provider = DependantScriptValuesProvider('ls ${param1}', self.create_parameters_supplier('param1')) + + @parameterized.expand([(True,), (False,)]) + def test_get_required_parameters_when_single_dependency(self, shell): + values_provider = DependantScriptValuesProvider( + 'ls ${param1}', + self.create_parameters_supplier('param1'), + shell=shell) + self.assertCountEqual(['param1'], values_provider.get_required_parameters()) - def test_get_required_parameters_when_single_dependency_and_many_params(self): + @parameterized.expand([(True,), (False,)]) + def test_get_required_parameters_when_single_dependency_and_many_params(self, shell): values_provider = DependantScriptValuesProvider( 'ls ${param1}', - self.create_parameters_supplier('param1', 'param2', 'param3')) + self.create_parameters_supplier('param1', 'param2', 'param3'), + shell=shell) self.assertCountEqual(['param1'], values_provider.get_required_parameters()) - def test_get_required_parameters_when_multiple_dependencies(self): + @parameterized.expand([(True,), (False,)]) + def test_get_required_parameters_when_multiple_dependencies(self, shell): values_provider = DependantScriptValuesProvider( 'ls ${param1}/${param2}', - self.create_parameters_supplier('param1', 'param2', 'param3')) + self.create_parameters_supplier('param1', 'param2', 'param3'), + shell=shell) self.assertCountEqual(['param1', 'param2'], values_provider.get_required_parameters()) - def test_get_values_when_no_values(self): + @parameterized.expand([(True,), (False,)]) + def test_get_values_when_no_values(self, shell): values_provider = DependantScriptValuesProvider( 'ls ${param1}', - self.create_parameters_supplier('param1')) + self.create_parameters_supplier('param1'), + shell=shell) self.assertEqual([], values_provider.get_values({})) - def test_get_values_when_single_parameter(self): + @parameterized.expand([(True,), (False,)]) + def test_get_values_when_single_parameter(self, shell): values_provider = DependantScriptValuesProvider( "echo '_${param1}_'", - self.create_parameters_supplier('param1')) + self.create_parameters_supplier('param1'), + shell=shell) self.assertEqual(['_hello world_'], values_provider.get_values({'param1': 'hello world'})) - def test_get_values_when_multiple_parameters(self): + @parameterized.expand([(True,), (False,)]) + def test_get_values_when_multiple_parameters(self, shell): files_path = os.path.join(test_utils.temp_folder, 'path1', 'path2') for i in range(0, 5): file_utils.write_file(os.path.join(files_path, 'f' + str(i) + '.txt'), 'test') values_provider = DependantScriptValuesProvider( 'ls ' + test_utils.temp_folder + '/${param1}/${param2}', - self.create_parameters_supplier('param1', 'param2')) + self.create_parameters_supplier('param1', 'param2'), + shell=shell) self.assertEqual(['f0.txt', 'f1.txt', 'f2.txt', 'f3.txt', 'f4.txt'], values_provider.get_values({'param1': 'path1', 'param2': 'path2'})) - def test_get_values_when_parameter_repeats(self): + @parameterized.expand([(True,), (False,)]) + def test_get_values_when_parameter_repeats(self, shell): values_provider = DependantScriptValuesProvider( "echo '_${param1}_\n' 'test\n' '+${param1}+'", - self.create_parameters_supplier('param1')) + self.create_parameters_supplier('param1'), + shell=shell) self.assertEqual(['_123_', ' test', ' +123+'], values_provider.get_values({'param1': '123'})) - def test_get_values_when_numeric_parameter(self): + @parameterized.expand([(True,), (False,)]) + def test_get_values_when_numeric_parameter(self, shell): values_provider = DependantScriptValuesProvider( "echo '_${param1}_'", - self.create_parameters_supplier('param1')) + self.create_parameters_supplier('param1'), + shell=shell) self.assertEqual(['_123_'], values_provider.get_values({'param1': 123})) - def test_get_values_when_newline_response(self): + @parameterized.expand([(True,), (False,)]) + def test_get_values_when_newline_response(self, shell): values_provider = DependantScriptValuesProvider( "ls '${param1}'", - self.create_parameters_supplier('param1')) + self.create_parameters_supplier('param1'), + shell=shell) self.assertEqual([], values_provider.get_values({'param1': test_utils.temp_folder})) - def test_no_code_injection(self): + @parameterized.expand([(True, ['1', '2']), (False, ['1 && echo 2'])]) + def test_no_code_injection_for_and_operator(self, shell, expected_values): values_provider = DependantScriptValuesProvider( "echo ${param1}", - self.create_parameters_supplier('param1')) - self.assertEqual(['1 && echo 2'], values_provider.get_values({'param1': '1 && echo 2'})) + self.create_parameters_supplier('param1'), + shell=shell) + self.assertEqual(expected_values, values_provider.get_values({'param1': '1 && echo 2'})) + + @parameterized.expand([(True, ['y2', 'y3']), (False, [])]) + def test_no_code_injection_for_pipe_operator(self, shell, expected_values): + test_utils.create_files(['x1', 'y2', 'y3']) + + values_provider = DependantScriptValuesProvider( + "ls ${param1}", + self.create_parameters_supplier('param1'), + shell=shell) + self.assertEqual(expected_values, values_provider.get_values({'param1': test_utils.temp_folder + ' | grep y'})) - def test_script_fails(self): + @parameterized.expand([(True,), (False,)]) + def test_script_fails(self, shell): values_provider = DependantScriptValuesProvider( "echo2 ${param1}", - self.create_parameters_supplier('param1')) + self.create_parameters_supplier('param1'), + shell=shell) self.assertEqual([], values_provider.get_values({'param1': 'abc'})) def setUp(self): diff --git a/src/tests/parameter_config_test.py b/src/tests/parameter_config_test.py index 58d4eabc..88bbaa21 100644 --- a/src/tests/parameter_config_test.py +++ b/src/tests/parameter_config_test.py @@ -1,13 +1,17 @@ +import inspect import os import unittest from collections import OrderedDict +from parameterized import parameterized + from config.constants import PARAM_TYPE_SERVER_FILE, PARAM_TYPE_MULTISELECT from model import parameter_config from model.parameter_config import get_sorted_config from react.properties import ObservableDict from tests import test_utils from tests.test_utils import create_parameter_model, create_parameter_model_from_config +from utils.process_utils import ExecutionException from utils.string_utils import is_blank DEF_USERNAME = 'my_user' @@ -109,6 +113,25 @@ def test_values_from_script(self): 'values': {'script': 'echo "123\n" "456"'}}) self.assertEqual(['123', ' 456'], parameter_model.values) + @parameterized.expand([ + ('y2', None, ['y2', 'y3']), + ('y2', False, ['x1', 'y2', 'y3 | grep y']), + ('y2', True, ['y2', 'y3']), + ('${auth.username}', None, ['x1', 'my_user', 'y3 | grep y']), + ('${auth.username}', False, ['x1', 'my_user', 'y3 | grep y']), + ('${auth.username}', True, ['my_user', 'y3']), + ]) + def test_values_from_script_with_bash_operator(self, second_value, shell, expected_values): + values = {'script': 'echo "x1\n""%s\n""y3" | grep y' % (second_value,)} + if shell is not None: + values['shell'] = shell + + parameter_model = _create_parameter_model({ + 'name': 'def_param', + 'type': 'list', + 'values': values}) + self.assertEqual(expected_values, parameter_model.values) + def test_values_from_script_win_newline(self): test_utils.set_win() @@ -278,9 +301,26 @@ def test_script_value_when_username(self): username='TONY') self.assertEqual('xTONYx', default) - def test_script_value_with_shell_operators(self): - default = self.resolve_default({'script': 'echo 12345 | grep "1"'}) - self.assertEqual('12345 | grep 1', default) + @parameterized.expand([ + ('y', None, 'my_user\ny1'), + ('y', False, ExecutionException), + ('y', True, 'my_user\ny1'), + ('${auth.username}', None, ExecutionException), + ('${auth.username}', False, ExecutionException), + ('${auth.username}', True, 'my_user'), + ]) + def test_script_value_with_shell_operators(self, grep_pattern, shell, expected_value): + test_utils.create_files(['x1', 'y1', 'my_user']) + config = {'script': 'ls "%s" | grep %s' % (test_utils.temp_folder, grep_pattern)} + if shell is not None: + config['shell'] = shell + + if inspect.isclass(expected_value) and issubclass(expected_value, BaseException): + self.assertRaises(expected_value, self.resolve_default, config, username='my_user') + else: + default = self.resolve_default(config, + username='my_user') + self.assertEqual(expected_value, default) @staticmethod def resolve_default(value, *, username=None, audit_name=None, working_dir=None): @@ -553,7 +593,8 @@ def test_list_with_script_when_matches_and_win_newline(self): error = parameter.validate_value('123') self.assertIsNone(error) - def test_list_with_dependency_when_matches(self): + @parameterized.expand([(False,), (True,), (None,)]) + def test_list_with_dependency_when_matches(self, shell): parameters = [] values = ObservableDict() dep_param = create_parameter_model('dep_param') @@ -561,7 +602,8 @@ def test_list_with_dependency_when_matches(self): type='list', values_script="echo '${dep_param}_\n' '_${dep_param}_'", all_parameters=parameters, - other_param_values=values) + other_param_values=values, + values_script_shell=shell) parameters.extend([dep_param, parameter]) values['dep_param'] = 'abc' diff --git a/src/tests/test_utils.py b/src/tests/test_utils.py index 4917b108..1c2d03c1 100644 --- a/src/tests/test_utils.py +++ b/src/tests/test_utils.py @@ -140,7 +140,8 @@ def create_script_param_config( file_type=None, file_extensions=None, excluded_files=None, - same_arg_param=None): + same_arg_param=None, + values_script_shell=None): conf = {'name': param_name} if type is not None: @@ -148,6 +149,8 @@ def create_script_param_config( if values_script is not None: conf['values'] = {'script': values_script} + if values_script_shell is not None: + conf['values']['shell'] = values_script_shell if default is not None: conf['default'] = default @@ -269,7 +272,8 @@ def create_parameter_model(name=None, all_parameters=None, file_dir=None, file_recursive=None, - other_param_values: ObservableDict = None): + other_param_values: ObservableDict = None, + values_script_shell=None): config = create_script_param_config( name, type=type, @@ -287,7 +291,8 @@ def create_parameter_model(name=None, max=max, allowed_values=allowed_values, file_dir=file_dir, - file_recursive=file_recursive) + file_recursive=file_recursive, + values_script_shell=values_script_shell) if all_parameters is None: all_parameters = [] diff --git a/src/utils/process_utils.py b/src/utils/process_utils.py index 56a52704..1795763b 100644 --- a/src/utils/process_utils.py +++ b/src/utils/process_utils.py @@ -10,8 +10,8 @@ LOGGER = logging.getLogger('script_server.process_utils') -def invoke(command, work_dir='.', *, environment_variables=None, check_stderr=True): - if isinstance(command, str): +def invoke(command, work_dir='.', *, environment_variables=None, check_stderr=True, shell=False): + if isinstance(command, str) and not shell: command = split_command(command, working_directory=work_dir) if environment_variables is not None: @@ -24,7 +24,8 @@ def invoke(command, work_dir='.', *, environment_variables=None, check_stderr=Tr stderr=subprocess.PIPE, cwd=work_dir, env=env, - universal_newlines=True) + universal_newlines=True, + shell=shell) (output, error) = p.communicate() diff --git a/web-src/src/admin/components/scripts-config/ParameterConfigForm.vue b/web-src/src/admin/components/scripts-config/ParameterConfigForm.vue index 8386d182..dbc31370 100644 --- a/web-src/src/admin/components/scripts-config/ParameterConfigForm.vue +++ b/web-src/src/admin/components/scripts-config/ParameterConfigForm.vue @@ -40,12 +40,16 @@ class="row"> - + +
!isEmptyString(s))); @@ -340,7 +352,10 @@ export default { methods: { updateAllowedValues() { if (this.allowedValuesFromScript) { - updateValue(this.value, 'values', {script: this.allowedValuesScript}); + updateValue(this.value, 'values', { + script: this.allowedValuesScript, + shell: this.allowedValuesScriptShellEnabled + }); } else { updateValue(this.value, 'values', this.allowedValues); } diff --git a/web-src/src/admin/components/scripts-config/parameter-fields.js b/web-src/src/admin/components/scripts-config/parameter-fields.js index b7d67f63..a0ab67eb 100644 --- a/web-src/src/admin/components/scripts-config/parameter-fields.js +++ b/web-src/src/admin/components/scripts-config/parameter-fields.js @@ -72,6 +72,13 @@ export const allowedValuesFromScriptField = { description: 'Fill values based on defined script' }; +export const allowedValuesScriptShellEnabledField = { + name: 'Enable bash operators', + withoutValue: true, + description: 'Enables bash operators (e.g. | && ||) in script section. ' + + 'Be careful!! If a script has dependant values, it will be a subject to script injection' +}; + export const defaultValueField = { name: 'Default value' }; diff --git a/web-src/tests/unit/admin/ParameterConfigForm_test.js b/web-src/tests/unit/admin/ParameterConfigForm_test.js index 7fda5a21..5abe5f01 100644 --- a/web-src/tests/unit/admin/ParameterConfigForm_test.js +++ b/web-src/tests/unit/admin/ParameterConfigForm_test.js @@ -309,6 +309,7 @@ describe('Test ParameterConfigForm', function () { assert.isUndefined(_findField('script', false)); assert.deepEqual(['abc', '123', 'xyz'], _findField('allowed values').value); assert.isFalse(_findField('load from script').value); + assert.isUndefined(_findField('enable bash operators', false)); }); it('Test allowed values when script', async function () { @@ -324,6 +325,7 @@ describe('Test ParameterConfigForm', function () { assert.equal('ls ~/', _findField('script').value); assert.isUndefined(_findField('allowed values', false)); assert.isTrue(_findField('load from script').value); + assert.isFalse(_findField('enable bash operators').value); }); it('Test allowed values when array and type editable_list', async function () { @@ -406,7 +408,17 @@ describe('Test ParameterConfigForm', function () { await _setValueByUser('Script', 'ls ~/'); - assertOutputValue('values', {'script': 'ls ~/'}); + assertOutputValue('values', {'script': 'ls ~/', 'shell': false}); + }); + + it('Test update allowed values to script with shell', async function () { + await _setValueByUser('Type', 'list'); + await _setValueByUser('Load from script', true); + + await _setValueByUser('Script', 'ls ~/'); + await _setValueByUser('Enable bash operators', true); + + assertOutputValue('values', {'script': 'ls ~/', 'shell': true}); }); it('Test update allowed values to script and back', async function () {