tests: stop using shlex.split() and use lists instead

Stop using `shlex.split()` in conftest.py as a way to prepare test
commands for subprocess.run() because it does not always work. When we
have quoting issues, just avoid them entirely by passing the test
command as a list.

Replace shlex.split() with a simple str.split() for convenience in
simple cases. This simple split() will immediately fail when trying to
use quote which is working exactly as intended.

This also makes the cmd() interface more similar to subprocess.run() and
its many friends, which is good thing.

This is similar to commit 4100764 ("Project.git(list/str): reduce
reliance on shlex.split()") but applied to tests/

Before commit 624880e, shlex.split() was used unconditionally in
conftest.py. As expected, this eventually broke on Windows: shlex is
not compatible across all operating systems and shells.

https://docs.python.org/3/library/subprocess.html#security-considerations

> If the shell is invoked explicitly, via shell=True, it is the
> application’s responsibility to ensure that all whitespace and
> metacharacters are quoted appropriately to avoid shell injection
> vulnerabilities. On _some_ platforms, it is possible to use shlex.quote()
> for this escaping.

(Emphasis mine)

Then commit 624880e made the "interesting" decision to stop using
shlex.split()... only on Windows. This was discussed in
#266 (comment)

So after this commit, parsing of test commands was delegated to the
shell but... only on Windows! This worked for a long time but eventually
broke testing white spaces for the new `west alias` #716. Rather than
making that Windows-specific hack even more complex with a special case,
finally do the right thing and ask more complex test commands to use a
list. Now we can drop shlex.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>

tests/conftest.py

@@ -5,7 +5,6 @@
 import os
 from pathlib import Path, PurePath
 import platform
-import shlex
 import shutil
 import subprocess
 import sys
@@ -227,7 +226,7 @@ def west_init_tmpdir(repos_tmpdir):
     py.path.local, with the current working directory set there.'''
     west_tmpdir = repos_tmpdir / 'workspace'
     manifest = repos_tmpdir / 'repos' / 'zephyr'
-    cmd(f'init -m "{manifest}" "{west_tmpdir}"')
+    cmd(['init', '-m', str(manifest), str(west_tmpdir)])
     west_tmpdir.chdir()
     config.read_config()
     return west_tmpdir
@@ -327,9 +326,11 @@ def cmd(cmd, cwd=None, stderr=None, env=None):
     # stdout from cmd is captured and returned. The command is run in
     # a python subprocess so that program-level setup and teardown
     # happen fresh.
-    cmd = 'west ' + cmd
-    if not WINDOWS:
-        cmd = shlex.split(cmd)
+
+    # If you have quoting issues: do NOT quote. It's not portable.
+    # Instead, pass `cmd` as a list.
+    cmd = ['west'] + (cmd.split() if isinstance(cmd, str) else cmd)
+
     print('running:', cmd)
     if env:
         print('with non-default environment:')

tests/test_project.py

@@ -77,6 +77,10 @@ def west_update_tmpdir(west_init_tmpdir):
 # Test cases
 #
 
+def _list_f(format):
+    return ['list', '-f', format]
+
+
 def test_workspace(west_update_tmpdir):
     # Basic test that west_update_tmpdir bootstrapped correctly. This
     # is a basic test of west init and west update.
@@ -104,7 +108,7 @@ def test_workspace(west_update_tmpdir):
 def test_list(west_update_tmpdir):
     # Projects shall be listed in the order they appear in the manifest.
     # Check the behavior for some format arguments of interest as well.
-    actual = cmd('list -f "{name} {revision} {path} {cloned} {clone_depth}"')
+    actual = cmd(_list_f('{name} {revision} {path} {cloned} {clone_depth}'))
     expected = ['manifest HEAD zephyr cloned None',
                 'Kconfiglib zephyr subdir/Kconfiglib cloned None',
                 'tagged_repo v1.0 tagged_repo cloned None',
@@ -116,16 +120,16 @@ def test_list(west_update_tmpdir):
     klib_rel = os.path.join('subdir', 'Kconfiglib')
     klib_abs = str(west_update_tmpdir.join('subdir', 'Kconfiglib'))
 
-    rel_outside = cmd(f'list -f "{{name}}" {klib_rel}').strip()
+    rel_outside = cmd(_list_f('{name}') + [klib_rel]).strip()
     assert rel_outside == 'Kconfiglib'
 
-    abs_outside = cmd(f'list -f "{{name}}" {klib_abs}').strip()
+    abs_outside = cmd(_list_f('{name}') + [klib_abs]).strip()
     assert abs_outside == 'Kconfiglib'
 
-    rel_inside = cmd('list -f "{name}" .', cwd=klib_abs).strip()
+    rel_inside = cmd('list -f {name} .', cwd=klib_abs).strip()
     assert rel_inside == 'Kconfiglib'
 
-    abs_inside = cmd(f'list -f "{{name}}" {klib_abs}', cwd=klib_abs).strip()
+    abs_inside = cmd(_list_f('{name}') + [klib_abs], cwd=klib_abs).strip()
     assert abs_inside == 'Kconfiglib'
 
     with pytest.raises(subprocess.CalledProcessError):
@@ -143,19 +147,19 @@ def test_list_manifest(west_update_tmpdir):
     shutil.copy('zephyr/west.yml', 'manifest_moved/west.yml')
     cmd('config manifest.path manifest_moved')
 
-    path = cmd('list -f "{path}" manifest').strip()
-    abspath = cmd('list -f "{abspath}" manifest').strip()
-    posixpath = cmd('list -f "{posixpath}" manifest').strip()
+    path = cmd('list -f {path} manifest').strip()
+    abspath = cmd('list -f {abspath} manifest').strip()
+    posixpath = cmd('list -f {posixpath} manifest').strip()
     assert path == 'manifest_moved'
     assert Path(abspath) == west_update_tmpdir / 'manifest_moved'
     assert posixpath == Path(west_update_tmpdir).as_posix() + '/manifest_moved'
 
     path = cmd('list --manifest-path-from-yaml '
-               '-f "{path}" manifest').strip()
+               '-f {path} manifest').strip()
     abspath = cmd('list --manifest-path-from-yaml '
-                  '-f "{abspath}" manifest').strip()
+                  '-f {abspath} manifest').strip()
     posixpath = cmd('list --manifest-path-from-yaml '
-                    '-f "{posixpath}" manifest').strip()
+                    '-f {posixpath} manifest').strip()
     assert path == 'zephyr'
     assert Path(abspath) == Path(str(west_update_tmpdir / 'zephyr'))
     assert posixpath == Path(west_update_tmpdir).as_posix() + '/zephyr'
@@ -187,29 +191,29 @@ def check(command_string, expected):
         out_lines = cmd(command_string).splitlines()
         assert out_lines == expected
 
-    check('list -f "{name} .{groups}. {path}"',
+    check(_list_f('{name} .{groups}. {path}'),
           ['manifest .. zephyr',
            'bar .. path-for-bar'])
 
-    check('list -f "{name} .{groups}. {path}" foo',
+    check(_list_f('{name} .{groups}. {path}') + ['foo'],
           ['foo .foo-group-1,foo-group-2. foo'])
 
-    check('list -f "{name} .{groups}. {path}" baz',
+    check(_list_f('{name} .{groups}. {path}') + ['baz'],
           ['baz .baz-group. baz'])
 
-    check('list -f "{name} .{groups}. {path}" foo bar baz',
+    check(_list_f("{name} .{groups}. {path}") + 'foo bar baz'.split(),
           ['foo .foo-group-1,foo-group-2. foo',
            'bar .. path-for-bar',
            'baz .baz-group. baz'])
 
-    check('list --all -f "{name} .{groups}. {path}"',
+    check(_list_f('{name} .{groups}. {path}') + ['--all'],
           ['manifest .. zephyr',
            'foo .foo-group-1,foo-group-2. foo',
            'bar .. path-for-bar',
            'baz .baz-group. baz'])
 
     cmd('config manifest.group-filter +foo-group-1')
-    check('list -f "{name} .{groups}. {path}"',
+    check(_list_f('{name} .{groups}. {path}'),
           ['manifest .. zephyr',
            'foo .foo-group-1,foo-group-2. foo',
            'bar .. path-for-bar'])
@@ -219,7 +223,7 @@ def test_list_sha(west_update_tmpdir):
     # Regression test for listing with {sha}. This should print N/A
     # for the first project, which is the ManifestProject.
 
-    assert cmd('list -f "{sha}"').startswith("N/A")
+    assert cmd('list -f {sha}').startswith("N/A")
 
 
 def test_manifest_freeze(west_update_tmpdir):
@@ -370,14 +374,14 @@ def test_forall(west_init_tmpdir):
 
     # 'forall' with no projects cloned shouldn't fail
 
-    assert cmd('forall -c "echo foo"').splitlines() == [
+    assert cmd(['forall', '-c', 'echo foo']).splitlines() == [
         '=== running "echo foo" in manifest (zephyr):',
         'foo']
 
     # Neither should it fail after cloning one or both projects
 
     cmd('update net-tools')
-    assert cmd('forall -c "echo foo"').splitlines() == [
+    assert cmd(['forall', '-c', 'echo foo']).splitlines() == [
         '=== running "echo foo" in manifest (zephyr):',
         'foo',
         '=== running "echo foo" in net-tools (net-tools):',
@@ -387,22 +391,22 @@ def test_forall(west_init_tmpdir):
 
     env_var = "%WEST_PROJECT_NAME%" if WINDOWS else "$WEST_PROJECT_NAME"
 
-    assert cmd(f'forall -c "echo {env_var}"').splitlines() == [
+    assert cmd(['forall', '-c', f'echo {env_var}']).splitlines() == [
         f'=== running "echo {env_var}" in manifest (zephyr):',
         'manifest',
         f'=== running "echo {env_var}" in net-tools (net-tools):',
         'net-tools']
 
     cmd('update Kconfiglib')
-    assert cmd('forall -c "echo foo"').splitlines() == [
+    assert cmd(['forall', '-c', 'echo foo']).splitlines() == [
         '=== running "echo foo" in manifest (zephyr):',
         'foo',
         '=== running "echo foo" in Kconfiglib (subdir/Kconfiglib):',
         'foo',
         '=== running "echo foo" in net-tools (net-tools):',
         'foo']
 
-    assert cmd('forall --group Kconfiglib-group -c "echo foo"'
+    assert cmd('forall --group Kconfiglib-group -c'.split() + ['echo foo']
                ).splitlines() == [
                    '=== running "echo foo" in Kconfiglib (subdir/Kconfiglib):',
                    'foo',
@@ -605,7 +609,7 @@ def test_update_head_0(west_init_tmpdir):
                     - Kconfiglib
         ''')
 
-    cmd(f"config manifest.path {my_manifest_dir}")
+    cmd(["config", "manifest.path", my_manifest_dir])
 
     # Update the upstream repositories, getting an UpdateResults tuple
     # back.
@@ -664,7 +668,7 @@ def test_update_some_with_imports(repos_tmpdir):
                           url: {net_tools}
                       '''})
 
-    cmd(f'init -l {manifest_repo}')
+    cmd(['init', '-l', manifest_repo])
 
     # Updating unknown projects should fail as always.
 
@@ -739,7 +743,7 @@ def test_update_submodules_list(repos_tmpdir):
                             - name: Kconfiglib
                               path: {kconfiglib_submodule}
                        '''})
-    cmd(f'init -l {manifest_repo}')
+    cmd(['init', '-l', manifest_repo])
 
     # Make tagged_repo to be zephyr project submodule.
     subprocess.check_call(SUBMODULE_ADD +
@@ -834,7 +838,7 @@ def test_update_all_submodules(repos_tmpdir):
                           url: {zephyr}
                           submodules: true
                        '''})
-    cmd(f'init -l {manifest_repo}')
+    cmd(['init', '-l', manifest_repo])
 
     # Make tagged_repo to be zephyr project submodule.
     subprocess.check_call(SUBMODULE_ADD +
@@ -922,7 +926,7 @@ def test_update_no_submodules(repos_tmpdir):
                           url: {zephyr}
                           submodules: false
                        '''})
-    cmd(f'init -l {manifest_repo}')
+    cmd(['init', '-l', manifest_repo])
 
     # Make tagged_repo to be zephyr project submodule.
     subprocess.check_call(SUBMODULE_ADD +
@@ -1006,7 +1010,7 @@ def test_update_submodules_strategy(repos_tmpdir):
                             - name: Kconfiglib
                               path: Kconfiglib
                        '''})
-    cmd(f'init -l {manifest_repo}')
+    cmd(['init', '-l', manifest_repo])
 
     # Make tagged_repo to be zephyr project submodule.
     subprocess.check_call(SUBMODULE_ADD +
@@ -1251,7 +1255,7 @@ def test_update_recovery(tmpdir):
         ''')
 
     # Use west init -l + west update to point p's manifest-rev at rbad.
-    cmd(f'init -l {m}', cwd=workspacestr)
+    cmd(['init', '-l', m], cwd=workspacestr)
     with pytest.raises(subprocess.CalledProcessError):
         cmd('update', cwd=workspacestr)
 
@@ -1317,7 +1321,7 @@ def test_update_name_cache(tmpdir):
     bar = workspace / 'bar'
 
     # Test the command line option.
-    cmd(f'update --name-cache {name_cache_dir}')
+    cmd(['update', '--name-cache', name_cache_dir])
     assert foo.check(dir=1)
     assert bar.check(dir=1)
     assert rev_parse(foo, 'HEAD') == foo_head
@@ -1327,7 +1331,7 @@ def test_update_name_cache(tmpdir):
     # (We can't use shutil.rmtree here because Windows.)
     shutil.move(os.fspath(foo), os.fspath(tmpdir))
     shutil.move(os.fspath(bar), os.fspath(tmpdir))
-    cmd(f'config update.name-cache {name_cache_dir}')
+    cmd(['config', 'update.name-cache', name_cache_dir])
     cmd('update')
     assert foo.check(dir=1)
     assert bar.check(dir=1)
@@ -1354,7 +1358,7 @@ def test_update_path_cache(tmpdir):
     bar = workspace / 'bar'
 
     # Test the command line option.
-    cmd(f'update --path-cache {path_cache_dir}')
+    cmd(['update', '--path-cache', path_cache_dir])
     assert foo.check(dir=1)
     assert bar.check(dir=1)
     assert rev_parse(foo, 'HEAD') == foo_head
@@ -1364,7 +1368,7 @@ def test_update_path_cache(tmpdir):
     # (We can't use shutil.rmtree here because Windows.)
     shutil.move(os.fspath(foo), os.fspath(tmpdir))
     shutil.move(os.fspath(bar), os.fspath(tmpdir))
-    cmd(f'config update.path-cache {path_cache_dir}')
+    cmd(['config', 'update.path-cache', path_cache_dir])
     cmd('update')
     assert foo.check(dir=1)
     assert bar.check(dir=1)
@@ -1475,7 +1479,7 @@ def test_init_local_manifest_project(repos_tmpdir):
     clone(str(repos_tmpdir.join('repos', 'zephyr')),
           str(zephyr_install_dir))
 
-    cmd(f'init -l "{zephyr_install_dir}"')
+    cmd(['init', '-l', zephyr_install_dir])
 
     # Verify Zephyr has been installed during init -l, but not projects.
     zid = repos_tmpdir.join('workspace')
@@ -1537,14 +1541,14 @@ def test_init_local_with_manifest_filename(repos_tmpdir):
 
     # fails because west.yml is missing
     with pytest.raises(subprocess.CalledProcessError):
-        cmd(f'init -l "{zephyr_install_dir}"')
+        cmd(['init', '-l', zephyr_install_dir])
 
     # create a manifest with a syntax error so we can test if it's being parsed
     with open(zephyr_install_dir / 'west.yml', 'w') as f:
         f.write('[')
 
     cwd = os.getcwd()
-    cmd(f'init -l "{zephyr_install_dir}"')
+    cmd(['init', '-l', zephyr_install_dir])
 
     # init with a local manifest doesn't parse the file, so let's access it
     workspace.chdir()
@@ -1555,7 +1559,7 @@ def test_init_local_with_manifest_filename(repos_tmpdir):
     shutil.move(workspace / '.west', workspace / '.west-syntaxerror')
 
     # success
-    cmd(f'init --mf project.yml -l "{zephyr_install_dir}"')
+    cmd(['init', '--mf', 'project.yml', '-l', zephyr_install_dir])
     workspace.chdir()
     config.read_config()
     cmd('update')
@@ -1724,11 +1728,11 @@ def test_init_with_manifest_filename(repos_tmpdir):
 
     # syntax error
     with pytest.raises(subprocess.CalledProcessError):
-        cmd(f'init -m "{manifest}" "{west_tmpdir}"')
+        cmd(['init', '-m', manifest, west_tmpdir])
     shutil.move(west_tmpdir, repos_tmpdir / 'workspace-syntaxerror')
 
     # success
-    cmd(f'init -m "{manifest}" --mf project.yml "{west_tmpdir}"')
+    cmd(['init', '-m', manifest, '--mf', 'project.yml', west_tmpdir])
     west_tmpdir.chdir()
     config.read_config()
     cmd('update')
@@ -1748,7 +1752,7 @@ def test_init_with_manifest_in_subdir(repos_tmpdir):
                       ''')})
 
     workspace = repos_tmpdir / 'workspace'
-    cmd(f'init -m "{manifest}" "{workspace}"')
+    cmd(['init', '-m', manifest, workspace])
     assert (workspace / 'nested' / 'subdirectory').check(dir=1)
 
 def test_extension_command_execution(west_init_tmpdir):
@@ -1820,7 +1824,7 @@ def do_run(self, args, ignored):
                       })
     west_tmpdir = repos_tmpdir / 'workspace'
     zephyr = repos_tmpdir / 'repos' / 'zephyr'
-    cmd(f'init -m "{zephyr}" "{west_tmpdir}"')
+    cmd(['init', '-m', zephyr, west_tmpdir])
     west_tmpdir.chdir()
     config.read_config()
     cmd('update')
@@ -1900,7 +1904,7 @@ def do_run(self, args, ignored):
                       })
     west_tmpdir = repos_tmpdir / 'workspace'
     zephyr = repos_tmpdir / 'repos' / 'zephyr'
-    cmd(f'init -m "{zephyr}" "{west_tmpdir}"')
+    cmd(['init', '-m', zephyr, west_tmpdir])
     west_tmpdir.chdir()
     config.read_config()
     cmd('update')
@@ -2128,7 +2132,7 @@ def test_import_project_release(repos_tmpdir):
     # Create the workspace and verify we can't load the manifest yet
     # (because some imported data is missing).
     ws = repos_tmpdir / 'ws'
-    cmd(f'init -m {manifest_remote} {ws}')
+    cmd(['init', '-m', manifest_remote, ws])
     with pytest.raises(ManifestImportFailed):
         # We can't load this yet, because we haven't cloned zephyr.
         Manifest.from_topdir(topdir=ws)
@@ -2199,7 +2203,7 @@ def test_import_project_release_fork(repos_tmpdir):
                           revision: fork-tag
                       '''})
 
-    cmd(f'init -l {manifest_repo}')
+    cmd(['init', '-l', manifest_repo])
     with pytest.raises(ManifestImportFailed):
         Manifest.from_topdir(topdir=ws)
 
@@ -2276,7 +2280,7 @@ def test_import_project_release_dir(tmpdir):
                           import: test.d
                       '''})
 
-    cmd(f'init -l {manifest_repo}')
+    cmd(['init', '-l', manifest_repo])
     with pytest.raises(ManifestImportFailed):
         Manifest.from_topdir(topdir=ws)
 
@@ -2313,7 +2317,7 @@ def test_import_project_rolling(repos_tmpdir):
                           import: true
                       '''})
 
-    cmd(f'init -l {manifest_repo}')
+    cmd(['init', '-l', manifest_repo])
     with pytest.raises(ManifestImportFailed):
         Manifest.from_topdir(topdir=ws)