T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/editorial.js:25
- Finding
- Shell Command Injection Through Git Commit Messages## Vulnerability Details **File Location**: `scripts/editorial.js`, lines 25-31; untrusted data reaches the vulnerable function from lines 94-116, 118-131, and 133-180 **Vulnerability Type**: OS command injection **Risk Level**: High ### Vulnerable Code ```js function gitCommit(message) { try { execSync(`git -C ${EDITORIAL_DIR} add .`, { stdio: 'ignore' }); execSync(`git -C ${EDITORIAL_DIR} commit -m "${message}"`, { stdio: 'ignore' }); } catch (e) { // Ignore commit failures (no changes, not in git repo, etc.) } } ``` Attacker-controlled command arguments and environment values are incorporated into `message`, for example: ```js const agent = process.env.OPENCLAW_AGENT || process.env.USER || 'unknown'; // ... gitCommit(`editorial: ${agent} claimed ${contentId} for ${action} on ${channel}`); ``` Equivalent untrusted interpolation also occurs when releasing and publishing content. ### Technical Analysis `execSync()` receives a single command string, so Node.js executes it through a shell. The commit message contains values derived from CLI arguments—`contentId`, `action`, and `channel`—and from `OPENCLAW_AGENT` or `USER`. None of these values are validated or safely escaped. Wrapping the message in double quotes does not neutralize shell syntax. POSIX shells still evaluate command substitutions such as `$(command)` and backticks inside double-quoted text. An attacker can therefore cause commands to run before `git commit` is invoked. The surrounding `try/catch` does not mitigate the vulnerability. Shell expansion occurs before Git processes its arguments, and any injected command may already have completed even if the subsequent Git operation fails. ### Attack Path 1. An attacker gains the ability to invoke a mutating CLI command or influence its arguments or environment. 2. The attacker supplies shell substitution syntax in `content-id`, `action`, `channel`, or `OPENCLAW_AGENT`, such as a value containing `$(attacker_command)`. 3. ...[truncated 944 chars]
- Remediation
- ## Remediation Suggestions Avoid invoking Git through a shell. Use `execFileSync()` or `spawnSync()` with an explicit argument array: ```js const { execFileSync } = require('child_process'); function gitCommit(message) { try { execFileSync('git', ['-C', EDITORIAL_DIR, 'add', '.'], { stdio: 'ignore' }); execFileSync('git', ['-C', EDITORIAL_DIR, 'commit', '-m', message], { stdio: 'ignore' }); } catch (error) { // Log or handle the failure appropriately. } } ``` Additionally: 1. Validate `contentId`, `action`, `channel`, and agent identity using strict allowlists and length limits. 2. Do not treat shell escaping as the primary fix; eliminating shell parsing is safer. 3. Distinguish expected Git outcomes, such as “nothing to commit,” from unexpected failures rather than silently suppressing every error. 4. Add regression tests containing `$()`, backticks, quotes, semicolons, newlines, and option-like values. 5. Use a controlled identity value rather than falling back to arbitrary environment data where feasible.