diff --git a/osism/tasks/__init__.py b/osism/tasks/__init__.py index 47e327aa5..650acff8b 100644 --- a/osism/tasks/__init__.py +++ b/osism/tasks/__init__.py @@ -4,6 +4,7 @@ import json import os import re +import shlex import shutil import subprocess import tempfile @@ -161,7 +162,40 @@ def run_ansible_in_environment( extracted_hosts = set() # Local set for host deduplication if type(arguments) == list: - joined_arguments = " ".join(arguments) + # The command below runs through `/bin/sh -c`, and callers rely on that + # shell to tokenize list elements: several deliberately pack more than + # one shell word into ONE element, e.g. commands/set.py and + # commands/noset.py pass ["-e status=True", f"-l {host}"], and + # commands/apply.py prepends f"-e kolla_action={action}". The + # run-.sh scripts forward args via "$@" without + # re-splitting, so that tokenization is load-bearing. + # + # Tokenize each element the way the shell would, then quote the + # resulting tokens. Quoting whole elements instead would glue + # "-e status=True" into one token and break -e/-l parsing (guarded by + # test_run_ansible_list_multitoken_element_word_split_not_quoted). + # str.split() is not sufficient either: an element may use quoting or a + # backslash to hold whitespace inside a single value, and splitting on + # raw whitespace cuts that value into malformed arguments. shlex.split + # honours both, matching what /bin/sh does today. + # + # The gain is that a value containing shell metacharacters becomes safe. + # Without it an `-e` value such as the regex alternation "(A|B)" reaches + # /bin/sh unquoted and the command dies with `Syntax error: "(" + # unexpected` before ansible runs at all. + quoted_arguments = [] + for argument in arguments: + try: + quoted_arguments.extend( + shlex.quote(token) for token in shlex.split(argument) + ) + except ValueError: + # Unbalanced quoting: shlex cannot tokenize it. Emit the element + # verbatim so /bin/sh reports the same error it reports today, + # rather than raising here and turning a shell-level error into + # a worker traceback. + quoted_arguments.append(argument) + joined_arguments = " ".join(quoted_arguments) else: joined_arguments = arguments diff --git a/tests/unit/tasks/test_init.py b/tests/unit/tasks/test_init.py index 584d03ec5..1a58305dd 100644 --- a/tests/unit/tasks/test_init.py +++ b/tests/unit/tasks/test_init.py @@ -385,6 +385,56 @@ def test_run_ansible_list_multitoken_element_word_split_not_quoted(runner_mocks) assert "'-e status=True'" not in command +def test_run_ansible_list_element_with_metacharacters_is_quoted(runner_mocks): + # A value containing shell metacharacters must reach /bin/sh quoted. The + # motivating case is a tempest regex alternation: unquoted, `/bin/sh -c` + # parses the parens and the command dies with + # `Syntax error: "(" unexpected` before ansible ever runs. + run_ansible(arguments=["-e", "tempest_include_regex=(A|B)"]) + command = runner_mocks.popen.call_args.args[0] + assert "'tempest_include_regex=(A|B)'" in command + + +def test_run_ansible_multitoken_element_tokens_quoted_individually(runner_mocks): + # The two requirements together: a multi-token element still word-splits + # (see the regression guard above), AND a metacharacter inside one of its + # tokens is still quoted -- so the split happens before quoting, not after. + run_ansible(arguments=["-e status=True", "-e regex=(A|B)"]) + command = runner_mocks.popen.call_args.args[0] + assert command.endswith("-e status=True -e 'regex=(A|B)'") + assert "'-e status=True'" not in command + + +def test_run_ansible_quoted_whitespace_stays_one_value(runner_mocks): + # A packed element may use shell quoting to hold whitespace inside ONE + # value. Tokenizing must respect that quoting: naive str.split() cuts + # inside the quotes and yields several malformed arguments. + run_ansible(arguments=["-e foo='hello world'"]) + command = runner_mocks.popen.call_args.args[0] + assert command.endswith("-e 'foo=hello world'") + + +def test_run_ansible_double_quoted_whitespace_stays_one_value(runner_mocks): + run_ansible(arguments=['-e foo="hello world"']) + command = runner_mocks.popen.call_args.args[0] + assert command.endswith("-e 'foo=hello world'") + + +def test_run_ansible_backslash_escaped_whitespace_stays_one_value(runner_mocks): + run_ansible(arguments=["-e path=a\\ b"]) + command = runner_mocks.popen.call_args.args[0] + assert command.endswith("-e 'path=a b'") + + +def test_run_ansible_unbalanced_quote_passed_through_verbatim(runner_mocks): + # shlex cannot tokenize unbalanced quoting. Emit the element unchanged so + # /bin/sh reports the same error it does today, rather than raising here + # and turning a shell error into a worker traceback. + run_ansible(arguments=["-e foo='unbalanced"]) + command = runner_mocks.popen.call_args.args[0] + assert command.endswith("-e foo='unbalanced") + + def test_run_ansible_string_arguments_passed_through(runner_mocks): run_ansible(arguments="-e a=b") command = runner_mocks.popen.call_args.args[0]