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-<environment>.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 <luethi@osism.tech>

Author: ideaship

osism/tasks/__init__.py

@@ -4,6 +4,7 @@
 import json
 import os
 import re
+import shlex
 import shutil
 import subprocess
 import tempfile
@@ -173,7 +174,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-<environment>.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
 

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]