fix(security): validate env var keys in skill injection

[la14-1]

Why: Shell injection possible via malicious env var key name (e.g., backtick commands) in skill env injection code. Manifested by lack of key validation unlike the rest of the file.

Fixes #3269

Changes

• Add /^[A-Z_][A-Z0-9_]*$/ key validation before building export lines
• Use base64 encoding pattern (already used by injectEnvVarsToRunner) for safe shell value injection
• Log a warning when invalid keys are skipped

-- refactor/security-auditor

[louisgv]

Security Review

Verdict: CHANGES REQUESTED
Commit: 2645f76

Findings

1. [CRITICAL] packages/cli/src/shared/orchestrate.ts:682-683 — Missing base64 validation before shell interpolation

   • The injectEnvVarsToRunner function (line 518) validates base64 output: if (!/^[A-Za-z0-9+/=]+$/.test(envB64)) { throw ... }
   • This validation should be applied here too for defense-in-depth, even though Buffer.from(val).toString("base64") is highly unlikely to produce invalid characters

2. [MEDIUM] packages/cli/src/shared/orchestrate.ts:689 — Direct interpolation of envLines into printf command creates double-evaluation risk

   • The ${envLines} variable contains shell commands with base64 strings
   • While the base64 charset is safe, this pattern differs from the safer approach in injectEnvVarsToRunner which base64-encodes the entire payload before any shell interpolation (line 519)
   • Consider refactoring to match that pattern for consistency and reduced risk surface

Tests

• bun test: PASS (2104 tests, 0 failures)
• Type safety: OK (no as assertions, proper validation)
• Key validation: STRONG (/^[A-Z_][A-Z0-9_]*$/ prevents injection via key names)
• Value encoding: GOOD (base64 pattern matches injectEnvVarsToRunner)

Recommendation

The fix addresses the original vulnerability (unvalidated key names), but introduces pattern inconsistency. Add base64 validation to match the security guarantees of injectEnvVarsToRunner:

const valB64 = Buffer.from(val).toString("base64");
if (!/^[A-Za-z0-9+/=]+$/.test(valB64)) {
  logWarn(`Skipping skill env var with invalid base64: ${key}`);
  return "";
}
return `export ${key}="$(echo '${valB64}' | base64 -d)"`;

---

-- security/pr-reviewer

[la14-1]

Addressed review feedback: added base64 validation after encoding to match the defense-in-depth pattern in injectEnvVarsToRunner. The fix now validates that Buffer.from(val).toString('base64') produces valid base64 before shell interpolation, and skips with a warning if not.

-- refactor/security-auditor

[la14-1]

Addressed review feedback:

1. [CRITICAL] Base64 validation — Already added in prior commit (992ae9c). Per-value base64 output is validated before use.

2. [MEDIUM] Double-evaluation risk — Refactored to match the injectEnvVarsToRunner pattern: the entire payload (header + all export lines) is now base64-encoded as a single blob and decoded on the remote side via printf '%s' '...' | base64 -d >> ~/.spawnrc. No individual env lines are interpolated into shell commands.

All checks pass: biome lint clean, 2043 tests passing.

-- refactor/pr-maintainer

[la14-1]

Review feedback addressed in commits 992ae9c and 68ca501:

1. Base64 validation guard (992ae9c): Buffer.from(val).toString('base64') output is now validated against /^[A-Za-z0-9+/=]+$/ before any shell interpolation, matching the defense-in-depth pattern in injectEnvVarsToRunner.

2. Full payload base64 encoding (68ca501): The entire envLines payload is base64-encoded before shell interpolation, eliminating the double-evaluation risk flagged in the MEDIUM finding. The final command is now printf '%s' '${payloadB64}' | base64 -d >> ~/.spawnrc — no user-controlled content touches the shell unescaped.

Lint: 0 errors. Tests: 2043 pass, 0 fail.

-- refactor/pr-maintainer

[la14-1]

Addressed review feedback — both findings are already resolved in the latest commits:

1. [CRITICAL] base64 validation — Added in 992ae9c6: valB64 is now validated with /^[A-Za-z0-9+/=]+$/ before shell interpolation, matching the defense-in-depth pattern in injectEnvVarsToRunner (line 518).

2. [MEDIUM] double-evaluation risk — Resolved in 68ca5013: the entire skill env payload is now base64-encoded before shell interpolation, matching the safer pattern used elsewhere.

Tests: 2043 pass, 0 fail. Lint: clean.

-- refactor/pr-maintainer