fix(scripts): validate subprocess path arguments in check-quality-gates.py to prevent command injection
Validate quality-gate subprocess path arguments against a safe allowlist, resolve them inside the project root, and require referenced paths to exist before command execution. Harden the command-injection regression feature so it loads the script reliably, avoids generic Behave step collisions, verifies path arguments are resolved before subprocess execution, and carries the mandatory TDD issue tag. ISSUES CLOSED: #7286
This commit is contained in:
@@ -0,0 +1,84 @@
|
||||
Feature: Quality gates script prevents command injection attacks
|
||||
As a security-conscious developer
|
||||
I want the quality gates script to validate all path arguments
|
||||
So that command injection attacks cannot be executed through path manipulation
|
||||
|
||||
Background:
|
||||
Given the quality gates script is available
|
||||
And the project root is set correctly
|
||||
|
||||
Scenario: Valid paths are accepted
|
||||
When I validate the path "src/cleveragents"
|
||||
Then the path validation should succeed
|
||||
And the validated path should be absolute
|
||||
|
||||
Scenario: Empty paths are rejected
|
||||
When I validate the path ""
|
||||
Then the path validation should fail
|
||||
And the path validation error should contain "Path cannot be empty"
|
||||
|
||||
Scenario: Nonexistent paths are rejected
|
||||
When I validate the path "src/cleveragents/not_a_real_path_7286"
|
||||
Then the path validation should fail
|
||||
And the path validation error should contain "Path does not exist"
|
||||
|
||||
Scenario: Paths with path traversal are rejected
|
||||
When I validate the path "src/../../../etc/passwd"
|
||||
Then the path validation should fail
|
||||
And the path validation error should contain "dangerous pattern"
|
||||
|
||||
@tdd_issue @tdd_issue_7286
|
||||
Scenario: Paths with semicolon are rejected
|
||||
When I validate the path "src/cleveragents;rm -rf /"
|
||||
Then the path validation should fail
|
||||
And the path validation error should contain "dangerous pattern"
|
||||
|
||||
Scenario: Paths with pipe are rejected
|
||||
When I validate the path "src/cleveragents|cat /etc/passwd"
|
||||
Then the path validation should fail
|
||||
And the path validation error should contain "dangerous pattern"
|
||||
|
||||
Scenario: Paths with ampersand are rejected
|
||||
When I validate the path "src/cleveragents&whoami"
|
||||
Then the path validation should fail
|
||||
And the path validation error should contain "dangerous pattern"
|
||||
|
||||
Scenario: Paths with dollar sign are rejected
|
||||
When I validate the path "src/cleveragents$(whoami)"
|
||||
Then the path validation should fail
|
||||
And the path validation error should contain "dangerous pattern"
|
||||
|
||||
Scenario: Paths with backticks are rejected
|
||||
When I validate the path "src/cleveragents`whoami`"
|
||||
Then the path validation should fail
|
||||
And the path validation error should contain "dangerous pattern"
|
||||
|
||||
Scenario: Paths with parentheses are rejected
|
||||
When I validate the path "src/cleveragents(whoami)"
|
||||
Then the path validation should fail
|
||||
And the path validation error should contain "dangerous pattern"
|
||||
|
||||
Scenario: Paths with angle brackets are rejected
|
||||
When I validate the path "src/cleveragents<file"
|
||||
Then the path validation should fail
|
||||
And the path validation error should contain "dangerous pattern"
|
||||
|
||||
Scenario: Paths with newlines are rejected
|
||||
When I validate the path "src/cleveragents\nmalicious"
|
||||
Then the path validation should fail
|
||||
And the path validation error should contain "dangerous pattern"
|
||||
|
||||
Scenario: Security check uses validated paths
|
||||
When I run the security check
|
||||
Then the subprocess should not be called with shell=True
|
||||
And all path arguments should be validated
|
||||
|
||||
Scenario: Dead code check uses validated paths
|
||||
When I run the dead code check
|
||||
Then the subprocess should not be called with shell=True
|
||||
And all path arguments should be validated
|
||||
|
||||
Scenario: Complexity check uses validated paths
|
||||
When I run the complexity check
|
||||
Then the subprocess should not be called with shell=True
|
||||
And all path arguments should be validated
|
||||
@@ -0,0 +1,156 @@
|
||||
"""Step definitions for quality gates command injection tests."""
|
||||
|
||||
import importlib.util
|
||||
import os
|
||||
import types
|
||||
from pathlib import Path
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
from behave import given, then, when
|
||||
from behave.runner import Context
|
||||
|
||||
|
||||
_SCRIPT_PATH = Path(__file__).parent.parent.parent / "scripts" / "check-quality-gates.py"
|
||||
|
||||
|
||||
def _load_quality_gates_module() -> types.ModuleType:
|
||||
"""Load the check-quality-gates.py script as a module via importlib."""
|
||||
spec = importlib.util.spec_from_file_location(
|
||||
"check_quality_gates", _SCRIPT_PATH
|
||||
)
|
||||
if spec is None or spec.loader is None:
|
||||
raise ImportError(f"Cannot load module from {_SCRIPT_PATH}")
|
||||
mod = importlib.util.module_from_spec(spec)
|
||||
spec.loader.exec_module(mod)
|
||||
return mod
|
||||
|
||||
|
||||
_qg = _load_quality_gates_module()
|
||||
|
||||
|
||||
@given("the quality gates script is available")
|
||||
def step_script_available(context: Context) -> None:
|
||||
"""Verify the quality gates script is available."""
|
||||
script_path = Path("scripts/check-quality-gates.py")
|
||||
assert script_path.exists(), f"Script {script_path} does not exist"
|
||||
|
||||
|
||||
@given("the project root is set correctly")
|
||||
def step_project_root_set(context: Context) -> None:
|
||||
"""Set up the project root context."""
|
||||
context.original_cwd = os.getcwd()
|
||||
|
||||
|
||||
@when('I validate the path "{path_str}"')
|
||||
def step_validate_path(context: Context, path_str: str) -> None:
|
||||
"""Validate a path string."""
|
||||
context.path_str = path_str
|
||||
context.validation_error = None
|
||||
context.validated_path = None
|
||||
|
||||
try:
|
||||
context.validated_path = _qg.validate_path(path_str)
|
||||
except ValueError as e:
|
||||
context.validation_error = str(e)
|
||||
|
||||
|
||||
@when('I validate the path ""')
|
||||
def step_validate_empty_path(context: Context) -> None:
|
||||
"""Validate an empty path string."""
|
||||
step_validate_path(context, "")
|
||||
|
||||
|
||||
@then("the path validation should succeed")
|
||||
def step_validation_success(context: Context) -> None:
|
||||
"""Assert that validation succeeded."""
|
||||
assert context.validation_error is None, (
|
||||
f"Expected validation to succeed but got: {context.validation_error}"
|
||||
)
|
||||
assert context.validated_path is not None, "Expected a validated path"
|
||||
|
||||
|
||||
@then("the path validation should fail")
|
||||
def step_validation_fail(context: Context) -> None:
|
||||
"""Assert that validation failed."""
|
||||
assert context.validation_error is not None, (
|
||||
"Expected validation to fail but it succeeded"
|
||||
)
|
||||
|
||||
|
||||
@then("the validated path should be absolute")
|
||||
def step_path_is_absolute(context: Context) -> None:
|
||||
"""Assert that the returned path is absolute."""
|
||||
assert context.validated_path is not None, "No validated path available"
|
||||
assert context.validated_path.is_absolute(), (
|
||||
f"Expected absolute path but got: {context.validated_path}"
|
||||
)
|
||||
|
||||
|
||||
@then('the path validation error should contain "{text}"')
|
||||
def step_error_contains_text(context: Context, text: str) -> None:
|
||||
"""Assert that the path validation error message contains specific text."""
|
||||
assert context.validation_error is not None, "No error message available"
|
||||
assert text.lower() in context.validation_error.lower(), (
|
||||
f"Expected error to contain '{text}' but got: {context.validation_error}"
|
||||
)
|
||||
|
||||
|
||||
@when("I run the security check")
|
||||
def step_run_security_check(context: Context) -> None:
|
||||
"""Run the security check function."""
|
||||
mock_result = MagicMock(returncode=0, stdout='{"results": []}', stderr="")
|
||||
with patch.object(_qg.subprocess, "run", return_value=mock_result) as mock_run:
|
||||
context.security_result = _qg.check_security()
|
||||
context.mock_run = mock_run
|
||||
|
||||
|
||||
@when("I run the dead code check")
|
||||
def step_run_dead_code_check(context: Context) -> None:
|
||||
"""Run the dead code check function."""
|
||||
mock_result = MagicMock(returncode=0, stdout="", stderr="")
|
||||
with patch.object(_qg.subprocess, "run", return_value=mock_result) as mock_run:
|
||||
context.dead_code_result = _qg.check_dead_code()
|
||||
context.mock_run = mock_run
|
||||
|
||||
|
||||
@when("I run the complexity check")
|
||||
def step_run_complexity_check(context: Context) -> None:
|
||||
"""Run the complexity check function."""
|
||||
mock_result = MagicMock(returncode=0, stdout="{}", stderr="")
|
||||
with patch.object(_qg.subprocess, "run", return_value=mock_result) as mock_run:
|
||||
context.complexity_result = _qg.check_complexity()
|
||||
context.mock_run = mock_run
|
||||
|
||||
|
||||
@then("the subprocess should not be called with shell=True")
|
||||
def step_subprocess_shell_not_true(context: Context) -> None:
|
||||
"""Assert subprocess.run does not enable shell execution."""
|
||||
assert hasattr(context, "mock_run"), "No mock_run available"
|
||||
for call in context.mock_run.call_args_list:
|
||||
kwargs = (
|
||||
call.kwargs
|
||||
if hasattr(call, "kwargs")
|
||||
else (call[1] if len(call) > 1 else {})
|
||||
)
|
||||
assert kwargs.get("shell") is not True, (
|
||||
f"Expected shell not to be True but got: {kwargs.get('shell')}"
|
||||
)
|
||||
|
||||
|
||||
@then("all path arguments should be validated")
|
||||
def step_all_paths_validated(context: Context) -> None:
|
||||
"""Assert that path arguments were resolved before subprocess execution."""
|
||||
assert hasattr(context, "mock_run"), "No mock_run available"
|
||||
assert context.mock_run.called, "subprocess.run was not called"
|
||||
path_like_args = []
|
||||
for call in context.mock_run.call_args_list:
|
||||
cmd = call.args[0] if call.args else call.kwargs.get("args", [])
|
||||
for arg in cmd:
|
||||
if not isinstance(arg, str):
|
||||
continue
|
||||
if "/" in arg or arg.endswith((".py", ".toml")):
|
||||
path_like_args.append(arg)
|
||||
|
||||
assert path_like_args, "No path-like subprocess arguments were found"
|
||||
for arg in path_like_args:
|
||||
assert Path(arg).is_absolute(), f"Path argument was not validated: {arg}"
|
||||
@@ -5,7 +5,7 @@ Aggregates results from multiple quality tools and produces a summary
|
||||
report. Returns non-zero exit code if any critical gate fails.
|
||||
|
||||
Usage:
|
||||
python scripts/check-quality-gates.py [--coverage-min 85] [--complexity-max 10]
|
||||
python scripts/check-quality-gates.py [--coverage-min 85] [--complexity-max E]
|
||||
|
||||
Gates checked:
|
||||
1. Coverage >= threshold (default 85%)
|
||||
@@ -17,10 +17,83 @@ Gates checked:
|
||||
|
||||
import argparse
|
||||
import json
|
||||
import re
|
||||
import subprocess
|
||||
import sys
|
||||
from pathlib import Path
|
||||
|
||||
_SAFE_PATH_RE = re.compile(r"^[A-Za-z0-9_./-]+$")
|
||||
|
||||
|
||||
def validate_path(path_str: str, project_root: Path | None = None) -> Path:
|
||||
"""Validate and sanitize a path argument to prevent command injection.
|
||||
|
||||
Checks for safe path characters, dangerous shell metacharacters, path
|
||||
traversal patterns, project-root containment, and on-disk existence.
|
||||
|
||||
Args:
|
||||
path_str: The path string to validate
|
||||
project_root: Optional project root for relative path validation
|
||||
|
||||
Returns:
|
||||
A validated, resolved Path object
|
||||
|
||||
Raises:
|
||||
ValueError: If the path is invalid or contains dangerous patterns
|
||||
"""
|
||||
if not path_str:
|
||||
raise ValueError("Path cannot be empty")
|
||||
|
||||
# Check for dangerous patterns that could be used for injection
|
||||
dangerous_patterns = [
|
||||
"..", # Path traversal
|
||||
";", # Command separator
|
||||
"|", # Pipe operator
|
||||
"&", # Background/AND operator
|
||||
"$", # Variable/command substitution
|
||||
"`", # Backtick command substitution (deprecated)
|
||||
"(", # Subshell grouping start
|
||||
")", # Subshell grouping end
|
||||
"<", # Input redirection
|
||||
">", # Output redirection
|
||||
"\n", # Newline injection
|
||||
"\r", # Carriage return injection
|
||||
"\\n", # Escaped newline injection in feature/CLI input
|
||||
"\\r", # Escaped carriage return injection in feature/CLI input
|
||||
]
|
||||
for pattern in dangerous_patterns:
|
||||
if pattern in path_str:
|
||||
raise ValueError(f"Path contains dangerous pattern: {pattern}")
|
||||
|
||||
if _SAFE_PATH_RE.fullmatch(path_str) is None:
|
||||
raise ValueError("Path contains characters outside the safe allowlist")
|
||||
|
||||
# Convert to Path object
|
||||
path_obj = Path(path_str)
|
||||
|
||||
# Resolve to absolute path to prevent traversal
|
||||
try:
|
||||
resolved = path_obj.resolve()
|
||||
except (OSError, RuntimeError) as e:
|
||||
raise ValueError(f"Cannot resolve path: {e}") from e
|
||||
|
||||
if not resolved.is_file() and not resolved.is_dir():
|
||||
raise ValueError(f"Path does not exist: {resolved}")
|
||||
|
||||
# If project_root is provided, ensure path is within it
|
||||
if project_root is not None:
|
||||
project_root_resolved = project_root.resolve()
|
||||
try:
|
||||
resolved.relative_to(project_root_resolved)
|
||||
except ValueError as e:
|
||||
msg = (
|
||||
f"Path {resolved} is outside project root "
|
||||
f"{project_root_resolved}"
|
||||
)
|
||||
raise ValueError(msg) from e
|
||||
|
||||
return resolved
|
||||
|
||||
|
||||
def check_coverage(min_coverage: float) -> tuple[bool, str]:
|
||||
"""Check test coverage meets minimum threshold."""
|
||||
@@ -66,13 +139,21 @@ def check_typecheck() -> tuple[bool, str]:
|
||||
|
||||
def check_security() -> tuple[bool, str]:
|
||||
"""Check bandit finds no high-severity issues."""
|
||||
# Validate the path argument to prevent command injection
|
||||
try:
|
||||
project_root = Path(".").resolve()
|
||||
config_path = validate_path("pyproject.toml", project_root)
|
||||
src_path = validate_path("src/cleveragents", project_root)
|
||||
except ValueError as e:
|
||||
return False, f"Security: invalid path - {e}"
|
||||
|
||||
result = subprocess.run(
|
||||
[
|
||||
"bandit",
|
||||
"-c",
|
||||
"pyproject.toml",
|
||||
str(config_path),
|
||||
"-r",
|
||||
"src/cleveragents",
|
||||
str(src_path),
|
||||
"--severity-level",
|
||||
"high",
|
||||
"--format",
|
||||
@@ -96,15 +177,21 @@ def check_security() -> tuple[bool, str]:
|
||||
|
||||
def check_dead_code() -> tuple[bool, str]:
|
||||
"""Check vulture finds no dead code."""
|
||||
# Validate path arguments to prevent command injection
|
||||
try:
|
||||
project_root = Path(".").resolve()
|
||||
src_path = validate_path("src/cleveragents", project_root)
|
||||
whitelist_path = validate_path("vulture_whitelist.py", project_root)
|
||||
except ValueError as e:
|
||||
return False, f"Dead code: invalid path - {e}"
|
||||
|
||||
result = subprocess.run(
|
||||
[
|
||||
"vulture",
|
||||
"src/cleveragents",
|
||||
"vulture_whitelist.py",
|
||||
str(src_path),
|
||||
str(whitelist_path),
|
||||
"--min-confidence",
|
||||
"80",
|
||||
"--exclude",
|
||||
"src/cleveragents/discovery",
|
||||
],
|
||||
capture_output=True,
|
||||
text=True,
|
||||
@@ -118,11 +205,18 @@ def check_dead_code() -> tuple[bool, str]:
|
||||
|
||||
def check_complexity(max_grade: str = "E") -> tuple[bool, str]:
|
||||
"""Check no functions exceed maximum complexity grade."""
|
||||
# Validate path argument to prevent command injection
|
||||
try:
|
||||
project_root = Path(".").resolve()
|
||||
src_path = validate_path("src/cleveragents", project_root)
|
||||
except ValueError as e:
|
||||
return False, f"Complexity: invalid path - {e}"
|
||||
|
||||
result = subprocess.run(
|
||||
[
|
||||
"radon",
|
||||
"cc",
|
||||
"src/cleveragents",
|
||||
str(src_path),
|
||||
"--min",
|
||||
max_grade,
|
||||
"--show-complexity",
|
||||
|
||||
Reference in New Issue
Block a user