Skip to content

fix(security): pipe install script via temp file instead of bash -c#3292

Merged
louisgv merged 1 commit intomainfrom
fix/issue-3291
Apr 13, 2026
Merged

fix(security): pipe install script via temp file instead of bash -c#3292
louisgv merged 1 commit intomainfrom
fix/issue-3291

Conversation

@la14-1
Copy link
Copy Markdown
Member

@la14-1 la14-1 commented Apr 13, 2026

Why: bash -c "scriptContent" passes fetched script content as a shell argument, which is vulnerable to command injection if the script content contains shell metacharacters, and fails for large scripts due to ARG_MAX limits. Writing to a temp file and executing with bash /tmp/script.sh is safer and consistent with the existing Windows codepath.

Changes:

  • packages/cli/src/update-check.ts: Replace bash -c scriptContent with write-to-temp-file + bash tmpFile pattern (matching the existing Windows PowerShell approach)
  • packages/cli/src/__tests__/update-check.test.ts: Update test assertion to expect temp file path instead of -c flag
  • packages/cli/package.json: Bump version 1.0.5 -> 1.0.6

Fixes #3291

-- refactor/security-auditor

…o prevent command injection

Fixes #3291

Agent: security-auditor
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Copy link
Copy Markdown
Member

@louisgv louisgv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security Review

Verdict: APPROVED
Commit: 75ac0ff

Summary

This PR successfully addresses issue #3291 by replacing bash -c with temp file execution, eliminating command injection risk. Two minor observations noted inline, but neither blocks approval.

Security Findings

  • [RESOLVED] Command injection prevention — Previous bash -c approach replaced with secure temp file execution
  • [MEDIUM] packages/cli/src/update-check.ts:325 — Predictable temp file naming (low practical impact)
  • [LOW] packages/cli/src/update-check.ts:341 — Best-effort cleanup could leave temp files on crash (minimal impact)

Tests

  • bash -n: N/A (no shell scripts modified)
  • bun test: PASS (2104 tests, 0 failures)

Approval Rationale

  1. Security improvement: Eliminates command injection vector by using file-based execution
  2. Consistent cross-platform: Aligns Linux/macOS approach with Windows implementation
  3. Tests pass: All 2104 tests pass including updated update-check tests
  4. Version bumped: Package version correctly incremented to 1.0.6
  5. Minor findings: Predictable temp file naming and cleanup race conditions have minimal real-world impact

-- security/pr-reviewer

Comment thread packages/cli/src/update-check.ts
Comment thread packages/cli/src/update-check.ts
@louisgv louisgv added the security-approved Security review approved label Apr 13, 2026
@louisgv louisgv merged commit ace5aa9 into main Apr 13, 2026
6 checks passed
@louisgv louisgv deleted the fix/issue-3291 branch April 13, 2026 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

security-approved Security review approved

Projects

None yet

Development

Successfully merging this pull request may close these issues.

security: [CRITICAL] Command injection via scriptContent in performAutoUpdate

2 participants