T09 · Insecure Skill Coding Practices
Error
- Location
- src/sync.ts:39
- Finding
- Shell Command Injection in the Exported Calendar Synchronization API## Vulnerability Details **File Location**: `src/sync.ts`, lines 39-42 **Vulnerability Type**: OS command injection through unsafe shell interpolation **Risk Level**: High ### Vulnerable Code ```typescript const { stdout, stderr } = await execAsync( `python3 "${scriptPath}" --market ${market}`, { timeout: 60000 } ); ``` ### Technical Analysis `CalendarSyncer.syncMarket()` constructs a shell command by directly interpolating `market` into a string passed to `child_process.exec()`. The `exec()` API invokes a shell, so shell metacharacters contained in the value are interpreted as command syntax rather than as part of a literal argument. Although `market` has the compile-time TypeScript type `Market`, TypeScript types are erased at runtime. The `CalendarSyncer` class and singleton are publicly exported, and `syncMarket()` does not independently validate its argument. The command-level handler validates market values before calling this method, but direct API consumers can bypass that handler. The constructor-controlled `scriptsPath` is also interpolated into the shell command. Quoting the path is insufficient protection if an untrusted caller can instantiate `CalendarSyncer` with a path containing quote characters or shell syntax. ### Attack Path 1. An application exposes or otherwise invokes the exported `calendarSyncer.syncMarket()` method using an attacker-controlled value. 2. The attacker supplies a runtime string containing shell metacharacters instead of one of the expected market identifiers. 3. `syncMarket()` inserts that value into the command string without runtime validation or shell escaping. 4. `execAsync()` passes the assembled string to the operating-system shell. 5. The shell interprets the injected syntax and executes an additional attacker-selected command. The normal `/finance-cron sync` handler validates values and is not, by itself, a demonstrated injection entry point. Exploitation applies ...[truncated 704 chars]
- Remediation
- ## Remediation Suggestions 1. Replace `exec()` with `execFile()` or `spawn()` and pass arguments as a separate array so no shell parses the values: ```typescript import { execFile } from 'child_process'; import { promisify } from 'util'; const execFileAsync = promisify(execFile); const allowedMarkets: readonly Market[] = ['US', 'CN', 'HK']; async syncMarket(market: Market): Promise<SyncResult> { if (!allowedMarkets.includes(market)) { throw new Error('Invalid market'); } const scriptPath = path.resolve(this.scriptsPath, 'sync_calendars.py'); const { stdout, stderr } = await execFileAsync( 'python3', [scriptPath, '--market', market], { timeout: 60000 } ); // Continue processing the result. } ``` 2. Perform runtime allowlist validation inside `syncMarket()` itself, even if callers are also expected to validate input. 3. Resolve the script path to a canonical absolute path and verify that it remains within the expected bundled scripts directory. 4. Avoid accepting a caller-controlled scripts directory unless this configurability is required. If required, validate it before use. 5. Add negative tests using shell metacharacters, quotes, command substitutions, and invalid market names to verify that they cannot alter process execution.