fix(patch): cherry-pick 171103a to release/v0.20.0-preview.2-pr-14742 to patch version v0.20.0-preview.2 and create version 0.20.0-preview.5 #14752
Conversation
…ion (#14742) Co-authored-by: Jack Wotherspoon <[email protected]> Co-authored-by: christine betts <[email protected]>
Summary of ChangesHello @gemini-cli-robot, 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 implements a critical security enhancement for the 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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces environment variable sanitization for shell commands executed within a CI environment, which is a good security practice. However, I've identified a critical issue with the implementation. The whitelist of allowed environment variables is too restrictive and will likely break many common CI workflows that depend on tools for cloud providers, proxies, or containers. My review includes a suggestion to expand the allowlist to support these common use cases while maintaining a strong security posture.
| // For CI runs (GitHub), we sanitize the environment for security. | ||
| const env: NodeJS.ProcessEnv = {}; | ||
| const essentialVars = [ | ||
| // Cross-platform | ||
| 'PATH', | ||
| // Windows specific | ||
| 'Path', | ||
| 'SYSTEMROOT', | ||
| 'SystemRoot', | ||
| 'COMSPEC', | ||
| 'ComSpec', | ||
| 'PATHEXT', | ||
| 'WINDIR', | ||
| 'TEMP', | ||
| 'TMP', | ||
| 'USERPROFILE', | ||
| 'SYSTEMDRIVE', | ||
| 'SystemDrive', | ||
| // Unix/Linux/macOS specific | ||
| 'HOME', | ||
| 'LANG', | ||
| 'SHELL', | ||
| 'TMPDIR', | ||
| 'USER', | ||
| 'LOGNAME', | ||
| ]; | ||
|
|
||
| for (const key of essentialVars) { | ||
| if (process.env[key] !== undefined) { | ||
| env[key] = process.env[key]; | ||
| } | ||
| } | ||
|
|
||
| // Always carry over test-specific variables for our own integration tests. | ||
| for (const key in process.env) { | ||
| if (key.startsWith('GEMINI_CLI_TEST')) { | ||
| env[key] = process.env[key]; | ||
| } | ||
| } |
There was a problem hiding this comment.
The current whitelisting approach for environment variables is too restrictive and will likely break many common CI/CD use cases. Commands for interacting with cloud providers (like aws, gcloud, az), container tools (docker, kubectl), or tools behind corporate proxies rely on specific environment variables (e.g., AWS_*, GOOGLE_APPLICATION_CREDENTIALS, HTTP_PROXY) for authentication and configuration. By stripping these, the ShellExecutionService will cause such commands to fail when run in a GitHub Actions environment.
This is a critical correctness issue that severely limits the utility of executing shell commands in a CI context.
A more robust solution would be to allow certain prefixes for well-known tools and specific variables for common configurations, in addition to the essential variables. This would maintain a good security posture while enabling common workflows.
// For CI runs (GitHub), we sanitize the environment for security.
const env: NodeJS.ProcessEnv = {};
const essentialVars = [
// Cross-platform
'PATH',
// Windows specific
'Path',
'SYSTEMROOT',
'SystemRoot',
'COMSPEC',
'ComSpec',
'PATHEXT',
'WINDIR',
'TEMP',
'TMP',
'USERPROFILE',
'SYSTEMDRIVE',
'SystemDrive',
// Unix/Linux/macOS specific
'HOME',
'LANG',
'SHELL',
'TMPDIR',
'USER',
'LOGNAME',
];
// Start with essential variables.
for (const key of essentialVars) {
if (process.env[key] !== undefined) {
env[key] = process.env[key];
}
}
// Allow-list for common tools and configurations.
const allowedPrefixes = [
'AWS_',
'GCP_',
'GOOGLE_',
'AZURE_',
'DOCKER_',
'CI_',
'GEMINI_CLI_TEST', // For integration tests
];
const allowedExact = [
'HTTP_PROXY',
'HTTPS_PROXY',
'NO_PROXY',
'http_proxy',
'https_proxy',
'no_proxy',
'KUBECONFIG',
'SSL_CERT_FILE',
'SSL_CERT_DIR',
];
// Add other allowed variables.
for (const key in process.env) {
if (allowedExact.includes(key) || allowedPrefixes.some(prefix => key.startsWith(prefix))) {
env[key] = process.env[key];
}
}
|
Size Change: +866 B (0%) Total Size: 21.5 MB ℹ️ View Unchanged
|
scidomino
merged commit 8872ee0
into
release/v0.20.0-preview.2-pr-14742
scidomino
deleted the
hotfix/v0.20.0-preview.2/0.20.0-preview.5/preview/cherry-pick-171103a/pr-14742
branch
3 participants
This PR automatically cherry-picks commit 171103a to patch version v0.20.0-preview.2 in the preview release to create version 0.20.0-preview.5.