fix: prevent shell injection in deployment file operations#278
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces shell-based commands with native Node.js fs methods and execFile to improve security and portability. In source-processor.js, cp -R is replaced with fsp.cp, and in universal-maker.js, exec is replaced with execFile for running the universal maker binary. Feedback suggests using fsp.realpath when copying directories to ensure symbolic links are handled correctly and to match the previous behavior of copying contents into the destination directory.
| if (stats.isDirectory()) { | ||
| // Copy directory contents | ||
| await execAsync(`cp -R "${file}/." "${tempDir}/"`); | ||
| await fsp.cp(file, tempDir, { recursive: true }); |
There was a problem hiding this comment.
The use of fsp.cp with dereference: false (the default) will cause a failure if file is a symbolic link to a directory. In this case, fsp.cp attempts to copy the symbolic link itself to the destination path tempDir. Since tempDir is an existing directory, the operation will fail with an error (e.g., EEXIST or EISDIR) because Node.js does not automatically append the source's basename to the destination directory like the shell cp command does.
To maintain the behavior of the original cp -R "${file}/." command (which followed the source symlink and copied its contents), you should resolve the source path using fsp.realpath() before copying.
| await fsp.cp(file, tempDir, { recursive: true }); | |
| await fsp.cp(await fsp.realpath(file), tempDir, { recursive: true }); |
|
I have already signed the Google CLA. Please recheck. |
source-processor.js used exec() with string interpolation for file copying, allowing shell metacharacters in paths to execute arbitrary commands. Replace with execFile() which does not spawn a shell. universal-maker.js had the same issue. Replace with execFile() with argument arrays.
q1uf3ng
force-pushed
the
fix/shell-injection-exec
branch
from
e8c75d6 to
240d7de
Compare
1 participant
Summary
source-processor.jsanduniversal-maker.jsuseexec()with string interpolation to run shell commands. User-controlled file paths are interpolated directly into the command string, allowing shell metacharacters to execute arbitrary commands.Fix
source-processor.js: Replaceexec('cp -R ...')withexecFile('cp', [args])— no shell spawneduniversal-maker.js: Replaceexec(command_string)withexecFile(binPath, [args])— arguments passed as arrayThis is consistent with the safe pattern already used in
compose.js(line 147).