diff --git a/features/quality_gates_command_injection.feature b/features/quality_gates_command_injection.feature new file mode 100644 index 000000000..6b18268b5 --- /dev/null +++ b/features/quality_gates_command_injection.feature @@ -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 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}" diff --git a/scripts/check-quality-gates.py b/scripts/check-quality-gates.py index 91d4ee883..9a64defea 100644 --- a/scripts/check-quality-gates.py +++ b/scripts/check-quality-gates.py @@ -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,80 @@ 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 {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 +136,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 +174,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 +202,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",