Skip to content

Security & Performance Refactor: Command Injection, Traversal, and Regressions#4

Open
BrandonS7 wants to merge 4 commits intoeidos-agi:mainfrom
BrandonS7:codex-security-fixes
Open

Security & Performance Refactor: Command Injection, Traversal, and Regressions#4
BrandonS7 wants to merge 4 commits intoeidos-agi:mainfrom
BrandonS7:codex-security-fixes

Conversation

@BrandonS7
Copy link
Copy Markdown

Description

This PR applies a comprehensive 4-phase remediation of critical security vulnerabilities and performance bottlenecks, guided by multiple rigorous AI audits.

Security Fixes

  1. Command Injection via shell=True: Completely removed the shell=True parameter from the primary cleanup dispatch (stats.py). All cleanup operations are now parsed safely with shlex.split and routed through a hardened safe_cleanup pipeline.
  2. AppleScript Injection: Strictly sanitized application names in memory.py using a strict regex allowlist to prevent AppleScript injection when terminating processes, while safely supporting legitimate names with apostrophes and ampersands.
  3. Arbitrary File Deletion / Symlink Traversal:
    • Replaced all instances of recursive rglob('*') with symlink-safe os.walk(followlinks=False) to prevent traversal beyond intended directories.
    • Added root-level symlink checks to prevent traversing linked roots in trash_contents.
    • Implemented strict base-path validation to ensure move_to_trash cannot delete anything outside ~, /Applications, or /Library/Caches.
  4. TOCTOU Race & State-After-Mutation: Fixed a logic bug where file metadata was read after moving it to the trash (corrupting ML telemetry). Pinned symlink evaluation closer to deletion using lstat() to tighten the TOCTOU window.
  5. Docker Label Injection: Sanitized Docker labels using a strict alphanumeric allowlist regex before displaying commands to the terminal.

Performance & Reliability Improvements

  1. Unbounded Resource Consumption (scanners): Optimised find_space_hogs() from 17 recursive rglob traversals to a single O(N) pass via os.walk.
  2. Permission Handling: Added onerror callbacks to all os.walk traversals to prevent PermissionError crashes from silently aborting the entire scan.
  3. Exception Visibility: Converted ~50 instances of silent except Exception: pass into explicit logging warnings so failures are observable.
  4. Chained Commands: Updated the new shell-less safe_cleanup to successfully process sub-commands separated by && (e.g. npm cache clean --force && rm -rf ...) so core functionality isn't degraded by the security patch.

Dependency Fixes

  • Fixed version mismatches across files to consistently reflect 0.5.0.
  • Declared missing required dependencies (psutil) in pyproject.toml.
  • Added optional [project.optional-dependencies] test = ["pytest"].

- Replaced AppleScript injection vulnerability with send2trash
- Added symlink checks to prevent path traversal
- Optimized space hogs scanner (single pass os.walk instead of multiple rglob)
- Added proper exception handling for record_removal
- Sanitized docker project names to prevent injection
- Removed shell=True from stats.run_cleanup and used shlex
- Strictly sanitized AppleScript inputs in memory.py
- Fixed TOCTOU and state-after-mutation in safe_delete.py
- Replaced recursive rglob with symlink-safe os.walk in utils.py
- Tightened Docker label sanitization to alphanumeric allowlist
- Added onerror handler and fixed pruning regression in os.walk
- Replaced silent except blocks with proper logging
- Synced package versions and declared missing dependencies
- Rewrote cleanup dispatch to use safe_cleanup instead of stats.run_cleanup
- Removed shell=True everywhere
- Improved TOCTOU checks using lstat
- Fixed os.walk traversal (added onerror and pruned correctly)
- Replaced re.sub stripping with strict regex validation in memory.py and docker.py
- Replaced silent except blocks with logging warnings
- Added psutil dependency and test extras
- Fixed functional regression in chained commands ('&&') by sequentially processing sub-commands
- Added root symlink traversal check to trash_contents()
- Enforced safe base-path validation for move_to_trash() to prevent arbitrary path deletions outside Home, Applications, or Library/Caches
- Relaxed AppleScript sanitization to safely accept legitimate application names containing apostrophes and ampersands
- Updated tests with 100% coverage for the new behaviors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant