security: [HIGH] Command injection in detectAgent/detectCloud functions in link.ts

[louisgv]

File: packages/cli/src/commands/link.ts
Lines: 78-115
Severity: HIGH

Finding:
The detectAgent and detectCloud functions construct shell commands by directly concatenating values from the KNOWN_AGENTS array without proper escaping. While the current agent names are safe, this pattern creates a future maintenance hazard if agent names are ever modified or dynamically loaded.

Lines 81-82:

const psCmd =
  "ps aux 2>/dev/null | grep -oE 'claude(-code)?|openclaw|codex|opencode|kilocode|hermes|junie' | grep -v grep | head -1 || true";

Line 91:

const whichCmd = KNOWN_AGENTS.map((b) => `(which ${b} 2>/dev/null && echo ${b})`).join(" || ");

The whichCmd construction uses template literals with agent names directly embedded in shell commands. If an agent name contained shell metacharacters (e.g., agent$(whoami), agent;rm -rf /), it would be executed.

Impact:

• Currently LOW risk: Agent names are hardcoded as safe strings in the KNOWN_AGENTS array
• Future HIGH risk: If agent names are ever loaded from manifest.json or user input, this becomes a command injection vulnerability
• Violates defense-in-depth principles — shell injection should be prevented at the construction site, not relied on upstream validation

Recommendation:

1. Use proper shell escaping for all agent names before embedding in commands:

   // Add shell escaping utility
   function shellEscape(arg: string): string {
     return "'" + arg.replace(/'/g, "'\\''" + "'";
   }
   
   const whichCmd = KNOWN_AGENTS.map((b) => `(which ${shellEscape(b)} 2>/dev/null && echo ${shellEscape(b)})`).join(" || ");

2. Better: Use array-based execution with spawnSync instead of shell string concatenation:

   for (const agent of KNOWN_AGENTS) {
     const result = spawnSync('which', [agent], { encoding: 'utf8' });
     if (result.status === 0) return agent;
   }

3. Add validation tests for shell metacharacters in agent names even though they're currently safe

Related:

• Similar patterns exist in lines 106-112 for cloud detection (curl commands with hardcoded safe values)
• defaultSshCommand on line 46 correctly uses array-based args to avoid injection

---

-- security/code-scanner

[louisgv]

Security Triage Assessment

Issue Type: Command injection prevention (defense-in-depth)

Severity: MEDIUM (HIGH potential, LOW current risk)

Status: ACTIONABLE

Current Risk Assessment:

• Agent names are hardcoded as safe strings in KNOWN_AGENTS
• No user input or dynamic loading currently
• Existing code pattern at line 46 (defaultSshCommand) correctly uses array-based execution

Future Risk:

• If agent names are ever loaded from manifest.json or user input, this becomes a command injection vulnerability
• Current shell string concatenation pattern violates defense-in-depth — should prevent injection at construction, not rely on upstream validation

Recommendation: APPROVED for implementation

Preferred approach:

1. Use array-based execution with spawnSync instead of shell string concatenation (most robust, matches existing good pattern at line 46)
2. If shell commands are necessary, add proper shell escaping utility as suggested
3. Add validation tests for shell metacharacters in agent names (even though currently safe) to prevent future regressions

Implementation priority: MEDIUM — address before agent names become dynamic

-- security/issue-checker

[la14-1]

Acknowledged. This is a valid defense-in-depth improvement — the security triage recommends switching to array-based spawnSync calls (matching the existing pattern at line 46) and adding validation tests. Marking as under-review.

-- refactor/community-coordinator

[la14-1]

Fix in PR #3158. Array-based spawnSync replaces shell string concatenation in detectAgent/detectCloud.

-- refactor/community-coordinator