From 0d7e52cfd76c3638f51e1ef0c15d70b4edf5716b Mon Sep 17 00:00:00 2001 From: deacon Date: Sun, 15 Mar 2026 13:18:40 -0400 Subject: [PATCH 1/2] fix: use shlex.split() in base_world check_requirement() Replace command.split(' ') with shlex.split(command) to properly handle commands with quoted arguments and spaces in paths. --- app/utility/base_world.py | 3 ++- tests/test_shlex_split.py | 28 ++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) create mode 100644 tests/test_shlex_split.py diff --git a/app/utility/base_world.py b/app/utility/base_world.py index f86de97c2..021db7666 100644 --- a/app/utility/base_world.py +++ b/app/utility/base_world.py @@ -1,4 +1,5 @@ import binascii +import shlex import string import re import yaml @@ -125,7 +126,7 @@ def check_module_version(module, version, attr=None, **kwargs): return compare_versions(mod_version, version) def check_program_version(command, version, **kwargs): - output = subprocess.check_output(command.split(' '), stderr=subprocess.STDOUT, shell=False, timeout=10) + output = subprocess.check_output(shlex.split(command), stderr=subprocess.STDOUT, shell=False, timeout=10) return compare_versions(output.decode('utf-8'), version) def compare_versions(version_string, minimum_version): diff --git a/tests/test_shlex_split.py b/tests/test_shlex_split.py new file mode 100644 index 000000000..1e9d30a6e --- /dev/null +++ b/tests/test_shlex_split.py @@ -0,0 +1,28 @@ +import pytest +from unittest.mock import patch +from app.utility.base_world import BaseWorld + + +class TestShlexSplit: + def test_simple_command(self): + params = {'type': 'installed_program', 'command': 'python3 --version', 'version': '3.0'} + with patch('subprocess.check_output', return_value=b'Python 3.12.0') as mock: + result = BaseWorld.check_requirement(params) + mock.assert_called_once() + args = mock.call_args[0][0] + assert args == ['python3', '--version'] + assert result is True + + def test_command_with_quotes(self): + params = {'type': 'installed_program', 'command': 'echo "hello world"', 'version': '0.0.0'} + with patch('subprocess.check_output', return_value=b'1.0.0') as mock: + BaseWorld.check_requirement(params) + args = mock.call_args[0][0] + assert args == ['echo', 'hello world'] + + def test_command_with_spaces_in_path(self): + params = {'type': 'installed_program', 'command': "'/path/to/my program' --version", 'version': '1.0'} + with patch('subprocess.check_output', return_value=b'1.5.0') as mock: + BaseWorld.check_requirement(params) + args = mock.call_args[0][0] + assert args == ['/path/to/my program', '--version'] From 0dede84413d01eeecb4a6773989a0aaeead9f765 Mon Sep 17 00:00:00 2001 From: deacon Date: Mon, 16 Mar 2026 00:57:47 -0400 Subject: [PATCH 2/2] fix: address Copilot review feedback on shlex-split-check-requirement - Catch ValueError from shlex.split() (unmatched quotes) and return False with a log message instead of propagating an unhandled exception - Patch app.utility.base_world.subprocess.check_output in tests (not the global subprocess) so patches are resilient to import order - Add return-value assertions to all three existing test cases - Add test for unmatched-quote ValueError returning False without calling subprocess - Add test for version below minimum returning False --- app/utility/base_world.py | 11 ++++++++++- tests/test_shlex_split.py | 31 ++++++++++++++++++++++++++----- 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/app/utility/base_world.py b/app/utility/base_world.py index 021db7666..3c95a2756 100644 --- a/app/utility/base_world.py +++ b/app/utility/base_world.py @@ -126,7 +126,16 @@ def check_module_version(module, version, attr=None, **kwargs): return compare_versions(mod_version, version) def check_program_version(command, version, **kwargs): - output = subprocess.check_output(shlex.split(command), stderr=subprocess.STDOUT, shell=False, timeout=10) + try: + args = shlex.split(command) + except ValueError as e: + # Unmatched quotes or other shlex parse errors – treat as a + # failed requirement rather than propagating an unhandled error. + logging.getLogger('check_requirement').error( + 'shlex.split failed for command %r: %s', command, e + ) + return False + output = subprocess.check_output(args, stderr=subprocess.STDOUT, shell=False, timeout=10) return compare_versions(output.decode('utf-8'), version) def compare_versions(version_string, minimum_version): diff --git a/tests/test_shlex_split.py b/tests/test_shlex_split.py index 1e9d30a6e..17d93a129 100644 --- a/tests/test_shlex_split.py +++ b/tests/test_shlex_split.py @@ -2,11 +2,15 @@ from unittest.mock import patch from app.utility.base_world import BaseWorld +# Patch the subprocess reference inside base_world to avoid import-order +# dependent behavior when subprocess is imported differently in other modules. +_PATCH_TARGET = 'app.utility.base_world.subprocess.check_output' + class TestShlexSplit: def test_simple_command(self): params = {'type': 'installed_program', 'command': 'python3 --version', 'version': '3.0'} - with patch('subprocess.check_output', return_value=b'Python 3.12.0') as mock: + with patch(_PATCH_TARGET, return_value=b'Python 3.12.0') as mock: result = BaseWorld.check_requirement(params) mock.assert_called_once() args = mock.call_args[0][0] @@ -15,14 +19,31 @@ def test_simple_command(self): def test_command_with_quotes(self): params = {'type': 'installed_program', 'command': 'echo "hello world"', 'version': '0.0.0'} - with patch('subprocess.check_output', return_value=b'1.0.0') as mock: - BaseWorld.check_requirement(params) + with patch(_PATCH_TARGET, return_value=b'1.0.0') as mock: + result = BaseWorld.check_requirement(params) args = mock.call_args[0][0] assert args == ['echo', 'hello world'] + assert result is True def test_command_with_spaces_in_path(self): params = {'type': 'installed_program', 'command': "'/path/to/my program' --version", 'version': '1.0'} - with patch('subprocess.check_output', return_value=b'1.5.0') as mock: - BaseWorld.check_requirement(params) + with patch(_PATCH_TARGET, return_value=b'1.5.0') as mock: + result = BaseWorld.check_requirement(params) args = mock.call_args[0][0] assert args == ['/path/to/my program', '--version'] + assert result is True + + def test_unmatched_quote_returns_false(self): + """shlex.split raises ValueError on unmatched quotes; must return False, not raise.""" + params = {'type': 'installed_program', 'command': "python3 --flag 'unterminated", 'version': '3.0'} + with patch(_PATCH_TARGET) as mock: + result = BaseWorld.check_requirement(params) + mock.assert_not_called() + assert result is False + + def test_version_below_minimum_returns_false(self): + """check_requirement must return False when the installed version is too old.""" + params = {'type': 'installed_program', 'command': 'python3 --version', 'version': '99.0'} + with patch(_PATCH_TARGET, return_value=b'Python 3.12.0'): + result = BaseWorld.check_requirement(params) + assert result is False