From 75fe9a40e7911ba8123ed8bfb7b89a8b7cb74f29 Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Mon, 18 May 2026 20:24:18 +0200 Subject: [PATCH 1/3] vault: avoid shell=True in subprocess calls Both View.take_action and Decrypt.take_action passed user-supplied file paths into a shell command string with shell=True. A path containing shell metacharacters (e.g. a semicolon) could cause arbitrary command execution. Replace the string form with a list, which bypasses the shell entirely and passes the path as a literal argument: subprocess.call(["/usr/local/bin/ansible-vault", "view", path]) Apply the same fix to Decrypt.take_action, which had the identical pattern but was not part of the original PR that introduced View. Update existing tests to assert the list form and add a new test for Decrypt, which previously had no subprocess coverage. SecurityImpact: eliminates shell injection via crafted filenames AI-assisted: Claude Code Signed-off-by: Roger Luethi --- osism/commands/vault.py | 6 ++---- tests/unit/commands/test_vault.py | 24 +++++++++++++++++++++--- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/osism/commands/vault.py b/osism/commands/vault.py index 3557a0985..533ed71dd 100644 --- a/osism/commands/vault.py +++ b/osism/commands/vault.py @@ -84,9 +84,7 @@ def take_action(self, parsed_args): return 1 if content.startswith(b"$ANSIBLE_VAULT"): - return subprocess.call( - f"/usr/local/bin/ansible-vault view {path}", shell=True - ) + return subprocess.call(["/usr/local/bin/ansible-vault", "view", path]) logger.warning(f"File is not vault-encrypted, showing plain content: {path}") sys.stdout.write(content.decode("utf-8", errors="replace")) @@ -104,7 +102,7 @@ def take_action(self, parsed_args): path = parsed_args.path if not os.path.isabs(path): path = os.path.join("/opt/configuration", path) - subprocess.call(f"/usr/local/bin/ansible-vault decrypt {path}", shell=True) + subprocess.call(["/usr/local/bin/ansible-vault", "decrypt", path]) # Well-known paths where secrets.yml files are typically found diff --git a/tests/unit/commands/test_vault.py b/tests/unit/commands/test_vault.py index c97caf825..24d563ca5 100644 --- a/tests/unit/commands/test_vault.py +++ b/tests/unit/commands/test_vault.py @@ -26,7 +26,7 @@ def test_view_invokes_ansible_vault_for_encrypted_file(tmp_path): _make_view().take_action(parsed_args) mock_call.assert_called_once_with( - f"/usr/local/bin/ansible-vault view {path}", shell=True + ["/usr/local/bin/ansible-vault", "view", str(path)] ) @@ -59,7 +59,7 @@ def test_view_resolves_relative_path_against_opt_configuration(): expected = "/opt/configuration/environments/openstack/secure.yml" open_mock.assert_called_once_with(expected, "rb") mock_call.assert_called_once_with( - f"/usr/local/bin/ansible-vault view {expected}", shell=True + ["/usr/local/bin/ansible-vault", "view", expected] ) @@ -108,5 +108,23 @@ def test_view_invokes_ansible_vault_for_vault_variants(tmp_path, header): _make_view().take_action(parsed_args) mock_call.assert_called_once_with( - f"/usr/local/bin/ansible-vault view {path}", shell=True + ["/usr/local/bin/ansible-vault", "view", str(path)] + ) + + +# --- Decrypt.take_action --- + + +def test_decrypt_invokes_ansible_vault_without_shell(tmp_path): + path = tmp_path / "secrets.yml" + path.write_bytes(b"$ANSIBLE_VAULT;1.1;AES256\nciphertext\n") + cmd = vault.Decrypt(MagicMock(), MagicMock()) + parser = cmd.get_parser("test") + parsed_args = parser.parse_args([str(path)]) + + with patch("osism.commands.vault.subprocess.call") as mock_call: + cmd.take_action(parsed_args) + + mock_call.assert_called_once_with( + ["/usr/local/bin/ansible-vault", "decrypt", str(path)] ) From 1de7ee2282ddbb217acb15cd35004655d4782262 Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Mon, 18 May 2026 21:07:15 +0200 Subject: [PATCH 2/3] vault decrypt: propagate ansible-vault exit code Decrypt.take_action() discarded the subprocess return code, while View.take_action() already returned it (added in d9384be). Callers that check $? or inspect the cliff return value would silently see success even when ansible-vault decrypt failed. Add 'return' so the exit code is propagated to the caller, matching the behaviour of View. Add a test that verifies a non-zero exit code from subprocess.call() is returned by take_action(). AI-assisted: Claude Code Signed-off-by: Roger Luethi --- osism/commands/vault.py | 2 +- tests/unit/commands/test_vault.py | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/osism/commands/vault.py b/osism/commands/vault.py index 533ed71dd..5d70c3fc7 100644 --- a/osism/commands/vault.py +++ b/osism/commands/vault.py @@ -102,7 +102,7 @@ def take_action(self, parsed_args): path = parsed_args.path if not os.path.isabs(path): path = os.path.join("/opt/configuration", path) - subprocess.call(["/usr/local/bin/ansible-vault", "decrypt", path]) + return subprocess.call(["/usr/local/bin/ansible-vault", "decrypt", path]) # Well-known paths where secrets.yml files are typically found diff --git a/tests/unit/commands/test_vault.py b/tests/unit/commands/test_vault.py index 24d563ca5..5cdfbe20f 100644 --- a/tests/unit/commands/test_vault.py +++ b/tests/unit/commands/test_vault.py @@ -128,3 +128,16 @@ def test_decrypt_invokes_ansible_vault_without_shell(tmp_path): mock_call.assert_called_once_with( ["/usr/local/bin/ansible-vault", "decrypt", str(path)] ) + + +def test_decrypt_propagates_exit_code(tmp_path): + path = tmp_path / "secrets.yml" + path.write_bytes(b"$ANSIBLE_VAULT;1.1;AES256\nciphertext\n") + cmd = vault.Decrypt(MagicMock(), MagicMock()) + parser = cmd.get_parser("test") + parsed_args = parser.parse_args([str(path)]) + + with patch("osism.commands.vault.subprocess.call", return_value=1): + rc = cmd.take_action(parsed_args) + + assert rc == 1 From c71178a716478756288e800e1771b343e55eb29f Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Mon, 18 May 2026 21:07:53 +0200 Subject: [PATCH 3/3] vault: handle missing path argument in View and Decrypt Both View and Decrypt define 'path' with nargs="?", making it optional. When omitted, parsed_args.path is None, causing os.path.isabs(None) to raise a TypeError before any useful error message reaches the user. Add an explicit None check at the top of each take_action() that logs a clear error and returns 1, matching the error-handling style already used for FileNotFoundError and PermissionError in View. Add tests for both commands that confirm the exit code and error log when path is omitted. AI-assisted: Claude Code Signed-off-by: Roger Luethi --- osism/commands/vault.py | 6 ++++++ tests/unit/commands/test_vault.py | 27 +++++++++++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/osism/commands/vault.py b/osism/commands/vault.py index 5d70c3fc7..57d7b2ec1 100644 --- a/osism/commands/vault.py +++ b/osism/commands/vault.py @@ -67,6 +67,9 @@ def get_parser(self, prog_name): def take_action(self, parsed_args): path = parsed_args.path + if path is None: + logger.error("No path specified") + return 1 if not os.path.isabs(path): path = os.path.join("/opt/configuration", path) @@ -100,6 +103,9 @@ def get_parser(self, prog_name): def take_action(self, parsed_args): path = parsed_args.path + if path is None: + logger.error("No path specified") + return 1 if not os.path.isabs(path): path = os.path.join("/opt/configuration", path) return subprocess.call(["/usr/local/bin/ansible-vault", "decrypt", path]) diff --git a/tests/unit/commands/test_vault.py b/tests/unit/commands/test_vault.py index 5cdfbe20f..de50a09e7 100644 --- a/tests/unit/commands/test_vault.py +++ b/tests/unit/commands/test_vault.py @@ -112,9 +112,36 @@ def test_view_invokes_ansible_vault_for_vault_variants(tmp_path, header): ) +def test_view_requires_path(loguru_logs): + parser = _make_view().get_parser("test") + parsed_args = parser.parse_args([]) + + with patch("osism.commands.vault.subprocess.call") as mock_call: + rc = _make_view().take_action(parsed_args) + + assert rc == 1 + mock_call.assert_not_called() + errors = [r for r in loguru_logs if r["level"] == "ERROR"] + assert any("No path" in r["message"] for r in errors) + + # --- Decrypt.take_action --- +def test_decrypt_requires_path(loguru_logs): + cmd = vault.Decrypt(MagicMock(), MagicMock()) + parser = cmd.get_parser("test") + parsed_args = parser.parse_args([]) + + with patch("osism.commands.vault.subprocess.call") as mock_call: + rc = cmd.take_action(parsed_args) + + assert rc == 1 + mock_call.assert_not_called() + errors = [r for r in loguru_logs if r["level"] == "ERROR"] + assert any("No path" in r["message"] for r in errors) + + def test_decrypt_invokes_ansible_vault_without_shell(tmp_path): path = tmp_path / "secrets.yml" path.write_bytes(b"$ANSIBLE_VAULT;1.1;AES256\nciphertext\n")