[P0] Command Execution Timeout Missing - DoS Vulnerability

[jeremyeder]

Summary

The CommandFix.apply() method calls subprocess.run() without a timeout, creating a DoS vulnerability where malicious or buggy commands can hang indefinitely. This bypasses the project's security guardrails (all other subprocess calls use safe_subprocess_run() with 120s timeout).

Impact

• Malicious commands can hang indefinitely (e.g., sleep 999999)
• Blocks entire assessment process
• Resource exhaustion on CI/CD systems
• Inconsistent with project's established security patterns

Location

• File: src/agentready/models/fix.py
• Lines: 165-172
• Function: CommandFix.apply()

Current Code

subprocess.run(
    cmd_list,
    cwd=cwd,
    check=True,
    capture_output=True,
    text=True,
    # Security: Never use shell=True - explicitly removed
)

Solution

Replace direct subprocess.run() call with project's safe_subprocess_run() wrapper:

from ..utils.subprocess_utils import safe_subprocess_run, SUBPROCESS_TIMEOUT

try:
    result = safe_subprocess_run(
        cmd_list,
        cwd=cwd,
        check=True,
        capture_output=True,
        text=True,
        timeout=SUBPROCESS_TIMEOUT,  # 120 seconds
    )

    return FixResult(
        success=True,
        message=f"Command executed successfully: {' '.join(cmd_list)}",
        details=result.stdout if result.stdout else None,
    )
except subprocess.TimeoutExpired as e:
    return FixResult(
        success=False,
        message=f"Command timed out after {SUBPROCESS_TIMEOUT}s: {' '.join(cmd_list)}",
        details=f"Timeout limit: {SUBPROCESS_TIMEOUT}s. Command may be hanging or taking too long.",
    )
except subprocess.CalledProcessError as e:
    return FixResult(
        success=False,
        message=f"Command failed with exit code {e.returncode}: {' '.join(cmd_list)}",
        details=e.stderr if e.stderr else str(e),
    )

Testing

# 1. Run unit tests
pytest tests/unit/test_fix.py -v

# 2. Manual timeout test (should complete in ~120s, not hang forever)
python -c "
from agentready.models import CommandFix, Repository
from pathlib import Path
import time

fix = CommandFix.from_dict({
    'attribute_id': 'test',
    'priority': 1,
    'description': 'Timeout test',
    'command': 'sleep 300',
    'auto_apply': False
})

repo = Repository(path=Path.cwd())
start = time.time()
result = fix.apply(repo)
duration = time.time() - start

print(f'Duration: {duration:.1f}s')
assert duration < 130, 'Should timeout around 120s'
assert not result.success
"

Acceptance Criteria

• Import safe_subprocess_run added to fix.py
• Direct subprocess.run() call removed
• Timeout exception handling added with user-friendly messages
• Unit test for timeout behavior added
• Manual timeout test passes (completes in ~120s)
• Regular commands still work (e.g., echo "test")
• All existing tests pass

References

• Project pattern: src/agentready/utils/subprocess_utils.py (SUBPROCESS_TIMEOUT = 120)
• All other subprocess calls use safe_subprocess_run()
• Full remediation plan: .plans/code-review-remediation-plan.md

Labels: security, bug, P0, good-first-issue
Milestone: v1.24.0

[jeremyeder]

@claude implement this

[github-actions]

Claude finished @jeremyeder's task —— View job • claude/issue-102-20251123-0615 • Create PR ➔

---

Implementation Complete ✅

I've successfully fixed the DoS vulnerability in CommandFix.apply() by adding timeout protection.

Changes Made

1. Fixed src/agentready/models/fix.py:142-184

• Imported safe_subprocess_run and SUBPROCESS_TIMEOUT from utils.subprocess_utils
• Replaced direct subprocess.run() call with safe_subprocess_run() wrapper
• Added explicit 120-second timeout parameter
• Added TimeoutExpired exception handling that returns False on timeout
• Updated docstring to document timeout protection against DoS attacks

2. Added unit tests in tests/unit/test_models.py:789-818

• test_command_fix_timeout() - Verifies that commands exceeding 120s timeout return False
• test_command_fix_apply_success() - Verifies that normal commands execute successfully

Security Improvement

Before: Commands could hang indefinitely (e.g., sleep 999999), blocking assessments and causing resource exhaustion.

After: All commands are protected by a 120-second timeout, consistent with the project's security patterns used in all other subprocess calls.

Testing Notes

The timeout test (test_command_fix_timeout) will take approximately 120 seconds to complete when run, as it needs to verify the timeout actually triggers. The CI pipeline will validate both tests pass.

---

|

[jeremyeder]

fixed by e785fab