T09 · Insecure Skill Coding Practices
Warning
- Location
- scripts/test.js:24
- Finding
- Shell Command Injection Through an Unquoted Project Path in the Test Runner<![CDATA[ ## Vulnerability Details **File Location**: `scripts/test.js`, lines 24-26 **Vulnerability Type**: OS command injection **Risk Level**: Medium ### Vulnerable Code ```javascript const cmd = `node ../index.js --file ${testFile} --patch-file ${patchFile}`; console.log(`Running: ${cmd}`); execSync(cmd, { cwd: __dirname }); ``` ### Technical Analysis `execSync()` receives a command string and executes it through a system shell. The `testFile` and `patchFile` values are derived from `__dirname` and interpolated into the command without quoting or shell escaping. Although these values are not supplied through normal CLI arguments, they include the directory in which the project was checked out or extracted. If an attacker can influence that directory name, shell metacharacters such as semicolons, command substitutions, or variable expansions can become part of the generated command and be interpreted by the shell. This issue is limited to execution of `scripts/test.js`, such as through `npm test`. The production JSON modifier in `index.js` does not construct or execute shell commands. ### Attack Path 1. An attacker causes the project to be cloned, extracted, or moved into a directory whose name contains shell syntax. 2. The victim runs `npm test`. 3. `scripts/test.js` constructs `testFile` and `patchFile` using the attacker-influenced project directory. 4. These paths are inserted directly into the `cmd` string. 5. `execSync()` passes the string to a shell. 6. The shell interprets the injected metacharacters and executes the attacker's command. For example, a directory component containing syntax conceptually equivalent to: ```text package;touch${IFS}proof;# ``` can cause the shell to treat `touch proof` as a separate command when the generated test command is executed. ### Impact Assessment Successful exploitation permits arbitrary command execution with the privileges of the user or automation account running the tests. An attacker could con ...[truncated 566 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions Avoid constructing shell command strings. Invoke the Node.js executable directly with a discrete argument array so that path values are never parsed as shell syntax: ```javascript const { execFileSync } = require('child_process'); execFileSync( process.execPath, [ path.join(__dirname, '..', 'index.js'), '--file', testFile, '--patch-file', patchFile ], { cwd: __dirname, stdio: 'inherit' } ); ``` Alternatively, use `spawnSync()` with an argument array and explicitly retain `shell: false`. Additional hardening measures include: 1. Do not attempt to fix the issue solely by adding quotation marks, because shell quoting is platform-dependent and can remain vulnerable to edge cases. 2. Use absolute paths for the script and all file arguments. 3. Add a regression test that runs the project from a temporary directory containing spaces and shell metacharacters. 4. Run tests and CI jobs with least-privileged accounts and avoid exposing unnecessary credentials to test processes. ]]>