feat(tarball): Add tarball/source build helper (Issue #452)

AAR-2026-01-12.md

@@ -0,0 +1,191 @@
+# After Action Report: Cortex PR #557 & #558 Security Review
+
+**Date:** 2026-01-12
+**Reviewer:** Claude Opus 4.5
+**Target PRs:** #557 (systemd_helper.py), #558 (tarball_helper.py)
+**Status:** COMPLETE - All issues fixed, all tests passing
+
+---
+
+## Executive Summary
+
+Performed comprehensive security and code quality review of two draft PRs for Cortex. Identified **43 issues** across both files (5 CRITICAL, 11 HIGH, 16 MEDIUM, 11 LOW). All issues have been fixed and validated through automated testing.
+
+| Metric | Before | After |
+|--------|--------|-------|
+| CRITICAL Issues | 5 | 0 |
+| HIGH Issues | 11 | 0 |
+| MEDIUM Issues | 16 | 0 |
+| LOW Issues | 11 | 0 |
+| Tests Passing | 1061 | 1081 |
+| Test Coverage | 59.51% | 59.57% |
+
+---
+
+## Issues Found & Fixed
+
+### CRITICAL Security Issues (5)
+
+#### 1. Shell Injection in systemd Environment Variables (systemd_helper.py:464)
+**Risk:** Remote code execution via malicious environment variable values
+**Root Cause:** Missing `$`, backtick, and newline escaping in Environment= values
+**Fix Applied:**
+```python
+escaped_value = value.replace("\n", " ")
+escaped_value = escaped_value.replace("\\", "\\\\")
+escaped_value = escaped_value.replace("$", "\\$")
+escaped_value = escaped_value.replace("`", "\\`")
+escaped_value = escaped_value.replace('"', '\\"')
+```
+
+#### 2. Command Injection via apt-cache search (tarball_helper.py:475)
+**Risk:** ReDoS, information disclosure via malicious directory names
+**Root Cause:** Directory name used as regex without escaping
+**Fix Applied:**
+```python
+escaped_name = re.escape(name)
+result = subprocess.run(["apt-cache", "search", f"^{escaped_name}$"], ...)
+```
+
+#### 3. No Timeout on apt-cache Calls (tarball_helper.py:475, 485)
+**Risk:** Indefinite hang, resource exhaustion
+**Root Cause:** subprocess.run() calls without timeout parameter
+**Fix Applied:** Added `timeout=APT_CACHE_TIMEOUT` (10s) to all apt-cache calls
+
+#### 4. Silent Data Corruption with errors='ignore' (tarball_helper.py:249, 294, 339)
+**Risk:** Parsing failures, incorrect dependency detection
+**Root Cause:** Non-UTF8 bytes silently dropped instead of replaced
+**Fix Applied:** Changed `errors="ignore"` to `errors="replace"` throughout
+
+#### 5. Race Condition in cleanup_installation (tarball_helper.py:564)
+**Risk:** Data loss, concurrent modification issues
+**Root Cause:** History modified between load and save
+**Fix Applied:** Reload history immediately before saving
+
+---
+
+### HIGH Priority Issues (11)
+
+| # | File | Issue | Fix |
+|---|------|-------|-----|
+| 1 | systemd_helper | Race condition in journalctl timeout (15s) | Increased to 30s + added `--since "1 hour ago"` |
+| 2 | systemd_helper | No subprocess error handling in diagnose_failure | Added returncode check and stderr reporting |
+| 3 | systemd_helper | Missing FileNotFoundError handling | Added try/except around subprocess calls |
+| 4 | systemd_helper | Service name validation missing | Added regex validation `^[a-zA-Z0-9_.-]+$` |
+| 5 | systemd_helper | Dependency tree parsing fragile | Added robust parsing with error recovery |
+| 6 | systemd_helper | Unused import (shlex) | Removed |
+| 7 | tarball_helper | No validation of prefix path | Added absolute path and location validation |
+| 8 | tarball_helper | File size not limited | Added MAX_FILE_SIZE (5MB) check |
+| 9 | tarball_helper | Regex catastrophic backtracking | Limited repetitions with `{0,1000}` |
+| 10 | tarball_helper | No PermissionError handling | Added try/except with helpful message |
+| 11 | tarball_helper | No check if apt-get exists | Added `shutil.which("apt-get")` check |
+
+---
+
+### MEDIUM Priority Issues (16)
+
+| # | File | Issue | Fix |
+|---|------|-------|-----|
+| 1 | systemd_helper | Exit code explanations incomplete | Added formula for signal codes + common codes |
+| 2 | systemd_helper | interactive_unit_generator() return unused | CLI now uses return value |
+| 3 | systemd_helper | No sudo check in instructions | Added "sudo" note to step 1 |
+| 4 | systemd_helper | Rich markup in exceptions | Plain text in exceptions only |
+| 5 | systemd_helper | Memory/CPU fields raw values | Converted to human-readable format |
+| 6 | systemd_helper | Magic numbers | Defined constants at module level |
+| 7 | systemd_helper | Inconsistent error messages | Standardized tone and format |
+| 8 | tarball_helper | Duplicate dependencies not deduplicated | Used dict to dedupe |
+| 9 | tarball_helper | Build commands hardcoded/unsafe | Added safer defaults (--prefix, --user, venv) |
+| 10 | tarball_helper | Tarball detection missing | Added file detection with extraction suggestion |
+| 11 | tarball_helper | No progress indicator | Added Rich progress spinners |
+| 12 | tarball_helper | JSON history no version field | Added `_version: "1.0"` schema field |
+| 13 | tarball_helper | Inconsistent naming | Standardized enum and method names |
+| 14 | tarball_helper | Magic strings | Defined as constants |
+| 15 | tarball_helper | Dead code (files_installed) | Documented as future feature |
+| 16 | tarball_helper | Docstring errors | Fixed missing Raises sections |
+
+---
+
+### LOW Priority Issues (11)
+
+| # | File | Issue | Fix |
+|---|------|-------|-----|
+| 1-5 | systemd_helper | Docstring inconsistency, no timeout tests, no env var edge case tests, no malicious service name tests, CLI --lines not validated | All fixed |
+| 6-11 | tarball_helper | No timeout tests, no concurrent access tests, no regex injection tests, CLI source_dir not validated, --packages parsing fragile | All fixed |
+
+---
+
+## Test Results
+
+### Final Test Run (VM: bounty-dev)
+```
+======================== 1081 passed, 8 skipped in 173.29s ========================
+Coverage: 59.57% (Required: 55%)
+```
+
+### Test Fixes Required
+Updated test assertions in `tests/test_tarball_helper.py` to match new safer build commands:
+- CMake: Now outputs `-DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=/usr/local`
+- Autotools: Now outputs `--prefix=/usr/local`
+- Meson: Now outputs `--prefix=/usr/local`
+- Make: Now outputs `PREFIX=/usr/local`
+- Python/pip: Now recommends venv with `--user` fallback
+
+---
+
+## Files Modified
+
+| File | Lines Changed | Type |
+|------|---------------|------|
+| `cortex/systemd_helper.py` | ~200 | NEW (complete rewrite with fixes) |
+| `cortex/tarball_helper.py` | ~150 | MODIFIED (security + quality fixes) |
+| `cortex/cli.py` | ~5 | MODIFIED (packages parsing fix) |
+| `tests/test_tarball_helper.py` | ~40 | MODIFIED (updated assertions) |
+
+---
+
+## Security Hardening Summary
+
+### Before
+- Shell injection possible via env vars
+- ReDoS via directory names
+- Silent data corruption
+- No timeouts on external commands
+- No input validation
+
+### After
+- All user input escaped/validated
+- Timeouts on all external commands (10-30s)
+- Proper error handling with helpful messages
+- Schema versioning for data migration
+- Safer default build commands
+
+---
+
+## Recommendations
+
+1. **Add integration tests** for timeout behavior
+2. **Add fuzz testing** for regex patterns
+3. **Consider** adding `checkinstall` as default instead of `sudo make install`
+4. **Monitor** for edge cases in environment variable escaping
+
+---
+
+## Appendix: Constants Added
+
+```python
+# systemd_helper.py
+SUBPROCESS_TIMEOUT = 30  # seconds
+JOURNALCTL_TIMEOUT = 30
+SERVICE_NAME_PATTERN = re.compile(r"^[a-zA-Z0-9_.-]+$")
+
+# tarball_helper.py
+APT_CACHE_TIMEOUT = 10  # seconds
+DPKG_QUERY_TIMEOUT = 5
+MAX_FILE_SIZE = 5 * 1024 * 1024  # 5 MB
+MAX_REGEX_REPETITIONS = 1000
+```
+
+---
+
+**Report Generated:** 2026-01-12T$(date +%H:%M:%S)
+**Validated On:** bounty-dev (e2-standard-4, us-central1-a)