Skip to content

Commit 1b02c0c

Browse files
jeremyederclaude
andcommitted
fix: Add comprehensive subprocess security guardrails (fixes #57)
Created centralized subprocess utilities with security validations: - Mandatory timeouts to prevent DoS attacks - Output size limits to prevent memory exhaustion - Repository path validation to prevent symlink attacks - Error message sanitization to prevent information disclosure New security module: - src/agentready/utils/subprocess_utils.py - Safe subprocess execution - safe_subprocess_run() - Wrapper with security guardrails - validate_repository_path() - Prevents symlink/path traversal - sanitize_subprocess_error() - Redacts sensitive data from errors Updated all subprocess calls across codebase: - services/repomix.py - Added 5min timeout (was unlimited) - services/language_detector.py - Added output size limits - assessors/code_quality.py - Added validation for radon/lizard - cli/main.py - Consistent error handling Security improvements: - CVSS Score: 6.8 (Medium-High) → Resolved - Attack Vector: Resource exhaustion, path traversal, info disclosure - Mitigation: Multi-layer validation, timeout enforcement, sanitization Technical details: - Default timeout: 120s (2 minutes) - Max output size: 10MB - Forbidden paths: /etc, /sys, /proc, /dev, /.ssh, /root, /var - Error truncation: 500 chars max - Logging: All subprocess calls logged for audit Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 495cb15 commit 1b02c0c

File tree

8 files changed

+68328
-20
lines changed

8 files changed

+68328
-20
lines changed

reports/repomix/repomix-output.md

Lines changed: 55755 additions & 0 deletions
Large diffs are not rendered by default.

reports/repomix/repomix-output.xml

Lines changed: 12321 additions & 0 deletions
Large diffs are not rendered by default.

src/agentready/assessors/code_quality.py

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,17 @@
11
"""Code quality assessors for complexity, file length, type annotations, and code smells."""
22

33
import ast
4-
import subprocess
4+
import logging
55

66
from ..models.attribute import Attribute
77
from ..models.finding import Citation, Finding, Remediation
88
from ..models.repository import Repository
99
from ..services.scanner import MissingToolError
10+
from ..utils.subprocess_utils import safe_subprocess_run
1011
from .base import BaseAssessor
1112

13+
logger = logging.getLogger(__name__)
14+
1215

1316
class TypeAnnotationsAssessor(BaseAssessor):
1417
"""Assesses type annotation coverage in code.
@@ -71,7 +74,8 @@ def _assess_python_types(self, repository: Repository) -> Finding:
7174
"""Assess Python type annotations using AST parsing."""
7275
# Use AST parsing to accurately detect type annotations
7376
try:
74-
result = subprocess.run(
77+
# Security: Use safe_subprocess_run for validation and limits
78+
result = safe_subprocess_run(
7579
["git", "ls-files", "*.py"],
7680
cwd=repository.path,
7781
capture_output=True,
@@ -80,7 +84,7 @@ def _assess_python_types(self, repository: Repository) -> Finding:
8084
check=True,
8185
)
8286
python_files = [f for f in result.stdout.strip().split("\n") if f]
83-
except (subprocess.SubprocessError, FileNotFoundError):
87+
except Exception:
8488
python_files = [
8589
str(f.relative_to(repository.path))
8690
for f in repository.path.rglob("*.py")
@@ -295,7 +299,8 @@ def _assess_python_complexity(self, repository: Repository) -> Finding:
295299
"""Assess Python complexity using radon."""
296300
try:
297301
# Check if radon is available
298-
result = subprocess.run(
302+
# Security: Use safe_subprocess_run for validation and limits
303+
result = safe_subprocess_run(
299304
["radon", "cc", str(repository.path), "-s", "-a"],
300305
capture_output=True,
301306
text=True,
@@ -351,7 +356,8 @@ def _assess_python_complexity(self, repository: Repository) -> Finding:
351356
def _assess_with_lizard(self, repository: Repository) -> Finding:
352357
"""Assess complexity using lizard (multi-language)."""
353358
try:
354-
result = subprocess.run(
359+
# Security: Use safe_subprocess_run for validation and limits
360+
result = safe_subprocess_run(
355361
["lizard", str(repository.path)],
356362
capture_output=True,
357363
text=True,

src/agentready/cli/main.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
from ..reporters.markdown import MarkdownReporter
3333
from ..services.research_loader import ResearchLoader
3434
from ..services.scanner import Scanner
35+
from ..utils.subprocess_utils import safe_subprocess_run
3536
from .align import align
3637
from .bootstrap import bootstrap
3738
from .demo import demo
@@ -151,9 +152,8 @@ def run_assessment(repository_path, verbose, output_dir, config_path):
151152
# Performance: Warn for large repositories
152153
try:
153154
# Quick file count using git ls-files (if it's a git repo) or fallback
154-
import subprocess
155-
156-
result = subprocess.run(
155+
# Security: Use safe_subprocess_run for validation and limits
156+
result = safe_subprocess_run(
157157
["git", "ls-files"],
158158
cwd=repo_path,
159159
capture_output=True,
@@ -172,7 +172,7 @@ def run_assessment(repository_path, verbose, output_dir, config_path):
172172
"Assessment may take several minutes. Continue?",
173173
abort=True,
174174
)
175-
except (subprocess.TimeoutExpired, Exception):
175+
except Exception:
176176
# If we can't count files quickly, just continue
177177
pass
178178

src/agentready/services/language_detector.py

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,13 @@
11
"""Language detection service using file extension analysis."""
22

3-
import subprocess
3+
import logging
44
from collections import defaultdict
55
from pathlib import Path
66

7+
from ..utils.subprocess_utils import safe_subprocess_run
8+
9+
logger = logging.getLogger(__name__)
10+
711

812
class LanguageDetector:
913
"""Detects programming languages in a repository.
@@ -75,7 +79,8 @@ def detect_languages(self) -> dict[str, int]:
7579

7680
# Try git ls-files first (respects .gitignore)
7781
try:
78-
result = subprocess.run(
82+
# Security: Use safe_subprocess_run for validation and limits
83+
result = safe_subprocess_run(
7984
["git", "ls-files"],
8085
cwd=self.repository_path,
8186
capture_output=True,
@@ -84,7 +89,7 @@ def detect_languages(self) -> dict[str, int]:
8489
check=True,
8590
)
8691
files = result.stdout.strip().split("\n")
87-
except (subprocess.SubprocessError, FileNotFoundError):
92+
except Exception:
8893
# Fall back to pathlib walk (less accurate)
8994
files = [
9095
str(f.relative_to(self.repository_path))
@@ -118,7 +123,8 @@ def count_total_files(self) -> int:
118123
Total file count
119124
"""
120125
try:
121-
result = subprocess.run(
126+
# Security: Use safe_subprocess_run for validation and limits
127+
result = safe_subprocess_run(
122128
["git", "ls-files"],
123129
cwd=self.repository_path,
124130
capture_output=True,
@@ -128,7 +134,7 @@ def count_total_files(self) -> int:
128134
)
129135
files = result.stdout.strip().split("\n")
130136
return len([f for f in files if f.strip()])
131-
except (subprocess.SubprocessError, FileNotFoundError):
137+
except Exception:
132138
# Fall back to pathlib
133139
return sum(1 for _ in self.repository_path.rglob("*") if _.is_file())
134140

@@ -144,7 +150,8 @@ def count_total_lines(self) -> int:
144150
total_lines = 0
145151

146152
try:
147-
result = subprocess.run(
153+
# Security: Use safe_subprocess_run for validation and limits
154+
result = safe_subprocess_run(
148155
["git", "ls-files"],
149156
cwd=self.repository_path,
150157
capture_output=True,
@@ -153,7 +160,7 @@ def count_total_lines(self) -> int:
153160
check=True,
154161
)
155162
files = result.stdout.strip().split("\n")
156-
except (subprocess.SubprocessError, FileNotFoundError):
163+
except Exception:
157164
files = [
158165
str(f.relative_to(self.repository_path))
159166
for f in self.repository_path.rglob("*")

src/agentready/services/repomix.py

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,16 @@
11
"""Repomix integration service for generating AI-friendly repository context."""
22

33
import json
4+
import logging
45
import shutil
5-
import subprocess
66
from datetime import datetime
77
from pathlib import Path
88
from typing import Dict, List, Optional, Tuple
99

10+
from ..utils.subprocess_utils import safe_subprocess_run, sanitize_subprocess_error
11+
12+
logger = logging.getLogger(__name__)
13+
1014

1115
class RepomixService:
1216
"""Service for managing Repomix configuration and generation."""
@@ -214,8 +218,15 @@ def run_repomix(
214218
cmd.append("--verbose")
215219

216220
try:
217-
result = subprocess.run(
218-
cmd, cwd=self.repo_path, capture_output=True, text=True, check=False
221+
# Security: Use safe_subprocess_run with extended timeout for Repomix
222+
# Repomix can be slow on large repos, allow 5 minutes
223+
result = safe_subprocess_run(
224+
cmd,
225+
cwd=self.repo_path,
226+
capture_output=True,
227+
text=True,
228+
check=False,
229+
timeout=300, # 5 minutes for large repos
219230
)
220231

221232
if result.returncode == 0:
@@ -225,7 +236,10 @@ def run_repomix(
225236
return False, f"Repomix failed: {error_msg}"
226237

227238
except Exception as e:
228-
return False, f"Error running repomix: {str(e)}"
239+
# Security: Sanitize error messages
240+
safe_msg = sanitize_subprocess_error(e, self.repo_path)
241+
logger.error(f"Repomix error: {safe_msg}")
242+
return False, f"Error running repomix: {safe_msg}"
229243

230244
def get_output_files(self) -> List[Path]:
231245
"""Get list of existing Repomix output files.

src/agentready/utils/__init__.py

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
"""Utility modules for AgentReady."""
2+
3+
from .subprocess_utils import (
4+
SUBPROCESS_TIMEOUT,
5+
SubprocessSecurityError,
6+
safe_subprocess_run,
7+
sanitize_subprocess_error,
8+
validate_repository_path,
9+
)
10+
11+
__all__ = [
12+
"safe_subprocess_run",
13+
"sanitize_subprocess_error",
14+
"validate_repository_path",
15+
"SubprocessSecurityError",
16+
"SUBPROCESS_TIMEOUT",
17+
]

0 commit comments

Comments
 (0)