Repository navigation
fix(cli): avoid shell interpolation in sandbox build and network setup - #29492
princeraj2572 wants to merge 3 commits into
Conversation
Under BUILD_SANDBOX=1 the CLI built the sandbox image with execSync and a shell string that interpolated the checkout path and the project sandbox.Dockerfile path, so shell metacharacters in either path were executed on the host. The container network inspect/create calls used the same pattern. Use execFileSync with argv arrays (and cwd instead of `cd`) so no shell parses these values, and replace the `inspect || create` shell chaining with a try/catch. Fixes google-gemini#29070
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the security of the sandbox environment setup by eliminating shell interpolation in command execution. By transitioning from shell-string commands to direct process execution with argument arrays, the system is now protected against potential command injection via path names containing shell metacharacters. The changes cover sandbox image building, network inspection and creation, and proxy network connection, while maintaining existing functionality. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
📊 PR Size: size/M
|
![gemini-code-assist[bot]](/web-api/iframe-proxy?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors sandbox-related executions to use execFileSync and execFileAsync instead of shell-based execSync and execAsync, preventing potential shell injection and metacharacter interpretation issues. Feedback on these changes points out a cross-platform compatibility bug on Windows where splitting gcPath with a forward slash (/packages/) will fail and cause an ENOTDIR crash. It is recommended to normalize path separators before splitting and to use process.execPath instead of the global 'node' command.
| // Pass paths as argv (no shell) so metacharacters in the checkout or | ||
| // project path cannot be interpreted as shell syntax. | ||
| execFileSync('node', ['scripts/build_sandbox.js', '-s', ...buildArgs], { | ||
| cwd: gcRoot, | ||
| stdio: 'inherit', | ||
| env: { | ||
| ...process.env, | ||
| GEMINI_SANDBOX: command, // in case sandbox is enabled via flags (see config.ts under cli package) | ||
| }, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Issues Identified
-
Windows Path Splitting Bug (
ENOTDIRcrash):
ThegcRootvariable is computed on line 435 usinggcPath.split('/packages/')[0]. On Windows, paths use backslashes (\\), so this split fails and leavesgcRootas the full file path of the executable (e.g.,C:\\Users\\...\\packages\\cli\\bin\\gemini.js). Passing a file path tocwdinexecFileSyncwill throw anENOTDIRerror and crash the process. -
Node Executable Robustness:
Using'node'directly relies on the globalPATH, which might not point to the correct Node.js version or might not be available in some environments. Usingprocess.execPathensures the exact same Node.js binary running the CLI is used to execute the script.
Recommendation
Normalize the path separators to forward slashes before splitting to ensure cross-platform compatibility, and use process.execPath instead of 'node'.
// Pass paths as argv (no shell) so metacharacters in the checkout or
// project path cannot be interpreted as shell syntax.
const crossPlatformGcRoot = gcRoot.replace(/\\/g, '/').split('/packages/')[0];
execFileSync(process.execPath, ['scripts/build_sandbox.js', '-s', ...buildArgs], {
cwd: crossPlatformGcRoot,
stdio: 'inherit',
env: {
...process.env,
GEMINI_SANDBOX: command, // in case sandbox is enabled via flags (see config.ts under cli package)
},
});There was a problem hiding this comment.
Re: the Windows gcRoot split. This isn't reachable on Windows: the guard just above, gcPath.includes('gemini-cli/packages/'), uses forward slashes, so a backslash path throws "Cannot build sandbox using installed gemini binary" before gcRoot is computed. The gcPath.split('/packages/')[0] logic is also unchanged from the previous cd ${gcRoot} version, so this PR doesn't introduce or worsen it. Making the build path Windows-aware would mean reworking that guard as well, which is outside the scope of #29070 (shell injection).
|
Hi there! Thank you for your interest in contributing to Gemini CLI. To ensure we maintain high code quality and focus on our prioritized roadmap, we only guarantee review and consideration of pull requests for issues that are explicitly labeled as 'help wanted'. This PR will be closed in 7 days if it remains without that designation. We encourage you to find and contribute to existing 'help wanted' issues in our backlog! Thank you for your understanding. |
This branch has not been deployed
1 participant
Summary
Under
BUILD_SANDBOX=1,start_sandboxbuilt the sandbox image withexecSyncand a shell string that interpolated the checkout path (gcRoot) and the projectsandbox.Dockerfilepath. Shell metacharacters in either path (for example a checkout in/tmp/evil; touch /tmp/pwned; echo) were executed on the host, outside the sandbox. This PR removes shell parsing from those calls.Details
execFileSync('node', ['scripts/build_sandbox.js', '-s', ...buildArgs], { cwd: gcRoot, ... })replacescd ${gcRoot} && node .... Paths are argv entries.network inspect || network createshell chaining becomes anensureNetwork()helper usingexecFileSyncwith try/catch. Behavior is unchanged: any failedinspectfalls through tocreate.network connectusesexecFileAsyncinstead of a shell string. Its inputs were already whitelisted or constant, so this is for consistency.GEMINI_SANDBOX_PROXY_COMMAND(a user-supplied command, run withshell: trueby design) and the fixeduntil timeout 0.25 curl ...wait.Related Issues
Fixes #29070
How to Validate
npx vitest run packages/cli/src/utils/sandbox.test.ts(44 tests pass). ThenetworkAccess: falsetest now expectsexecFileSync('docker', ['network', 'create', '--internal', 'gemini-cli-sandbox']).evil; touch PWNED; echo: the oldexecSyncpattern createsPWNED; theexecFileSyncpattern does not.Pre-Merge Checklist