From 0f694d68365abe7df27e0bb86e52802eb6705e55 Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Thu, 27 Aug 2026 13:39:48 +0200 Subject: [PATCH] tasks: quote argument tokens for the shell run_ansible_in_environment joins a list of arguments with plain spaces and runs the result through subprocess.Popen(..., shell=True), so any `-e` value containing a shell metacharacter is parsed by /bin/sh. A tempest regex alternation is the motivating case: osism apply tempest -e 'tempest_include_regex=(A|B)' /bin/sh: 1: Syntax error: "(" unexpected The command dies before ansible runs. This affects every `osism apply ... -e key=value` whose value contains a metacharacter, across all workers. Two things constrain the fix. First, shlex.quote() per list element is wrong, and test_run_ansible_list_multitoken_element_word_split_not_quoted exists to prevent it: callers deliberately pack several shell words into ONE element and rely on the outer shell to tokenize them (commands/set.py and commands/noset.py pass ["-e status=True", f"-l {host}"]; commands/validate.py and commands/apply.py prepend "-e kolla_action=..."). The run-.sh scripts forward args via "$@" without re-tokenizing, so that step is load-bearing; quoting whole elements glues "-e status=True" into one token and breaks -e/-l parsing. Second, 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: element /bin/sh today str.split() + quote -e foo='hello world' [-e][foo=hello world] [-e][foo='hello][world'] -e path=a\ b [-e][path=a b] [-e][path=a\][b] So tokenize each element the way the shell would, with shlex.split(), then quote the resulting tokens. That reproduces today's tokenization for quoted and escaped whitespace while making metacharacters safe. shlex.split() raises on unbalanced quoting, where /bin/sh merely fails with its own error. Such elements are emitted verbatim so the failure mode stays a shell error rather than becoming a worker traceback. Tests written first and watched fail. test_run_ansible_multitoken_element_ tokens_quoted_individually pins both properties at once -- a multi-token element still tokenizes AND a metacharacter inside one of its tokens is quoted -- and four more cover single-quoted, double-quoted and backslash-escaped whitespace plus the unbalanced-quote passthrough. Verified: 86 passed in tests/unit/tasks/test_init.py including the existing guard; 3154 passed / 4 pre-existing xfail across tests/unit; and end-to-end on a live OSISM 10.2.0 cluster, where the alternation above previously failed with the syntax error and now selects and runs both tests (Passed: 2, Failed: 0). Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi --- osism/tasks/__init__.py | 36 ++++++++++++++++++++++++- tests/unit/tasks/test_init.py | 50 +++++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 1 deletion(-) 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]