fix(core): detect zsh from $SHELL to prevent shopt errors#26912
fix(core): detect zsh from $SHELL to prevent shopt errors#26912kimjune01 wants to merge 2 commits into
Conversation
On macOS (where zsh is the default shell), run_shell_command fails
because getShellConfiguration() hardcodes 'bash' for all Unix systems.
This causes ensurePromptvarsDisabled() to inject 'shopt -u ...' which
is bash-only and fails in zsh with "command not found: shopt".
Changes:
- Add 'zsh' to ShellType union
- Update getShellConfiguration() to detect shell from process.env.SHELL
- Parse shell basename to set correct ShellType ('bash' or 'zsh')
- Update parseCommandDetails() and hasRedirection() to handle zsh
using the bash parser (syntax is compatible)
- ensurePromptvarsDisabled() already guards on shell === 'bash',
so no zsh commands will receive the shopt guard
Fixes google-gemini#26754
The implementation now reads $SHELL to detect zsh vs bash, but the tests were not mocking it. On zsh hosts (macOS default), the Linux and macOS tests fail because they expect hardcoded 'bash' but get '/bin/zsh'. Mock process.env.SHELL explicitly and add dedicated zsh and unset-SHELL test cases.
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 improves shell environment detection and compatibility by allowing the system to identify and utilize the user's configured shell (specifically zsh) rather than assuming bash. By enabling zsh support and reusing existing bash parsing logic, the changes resolve issues where shell-specific commands like 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
|
![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 shell detection logic to support zsh alongside bash on Unix-like systems, updating command parsing and redirection checks accordingly. However, using the bash parser for zsh poses a security risk as it may fail to identify nested commands in zsh-specific syntax, potentially bypassing access controls. There is also a compatibility concern where unknown shells are incorrectly identified as bash, which could lead to execution failures; a fallback to a known shell is recommended. Finally, the new unit tests violate the repository style guide by directly modifying process.env instead of using vi.stubEnv.
| if (configuration.shell === 'bash' || configuration.shell === 'zsh') { | ||
| // Use bash parser for both bash and zsh (syntax is similar enough) | ||
| return parseBashCommandDetails(command); | ||
| } |
There was a problem hiding this comment.
The parseCommandDetails function uses the bash parser (parseBashCommandDetails) for both bash and zsh. While the syntax is similar, zsh has unique features like =(command) process substitution that the bash parser does not recognize as a command execution. This leads to a bypass of the command restriction policy (IDOR/Broken Access Control) because getCommandRoots will fail to identify and validate nested commands executed via zsh-specific syntax.
| const shellEnv = process.env['SHELL']; | ||
| const executable = shellEnv || 'bash'; | ||
|
|
||
| // Determine shell type from the executable path | ||
| let shellType: ShellType = 'bash'; | ||
| if (shellEnv) { | ||
| const basename = path.basename(shellEnv); | ||
| if (basename.includes('zsh')) { | ||
| shellType = 'zsh'; | ||
| } else if (basename.includes('bash')) { | ||
| shellType = 'bash'; | ||
| } | ||
| // Default to bash for unknown shells to maintain compatibility | ||
| } | ||
|
|
||
| return { | ||
| executable, | ||
| argsPrefix: ['-c'], | ||
| shell: shellType, | ||
| }; |
There was a problem hiding this comment.
The current implementation introduces a regression for users with non-bash/non-zsh shells (e.g., sh, dash, or fish). If $SHELL is set to an unknown shell, the code uses it as the executable but identifies the shellType as bash. This causes the execution service to inject bash-specific shopt commands, which will result in errors in those shells. To maintain compatibility and prevent execution failures, the tool should only use the environment's shell if it is explicitly recognized as bash or zsh, otherwise it should fall back to the standard bash executable. Additionally, ensure that the optional SHELL environment variable is handled correctly by coding against the interface contract.
const shellEnv = process.env['SHELL'];
if (shellEnv) {
const basename = path.basename(shellEnv);
if (basename.includes('zsh')) {
return { executable: shellEnv, argsPrefix: ['-c'], shell: 'zsh' };
}
if (basename.includes('bash')) {
return { executable: shellEnv, argsPrefix: ['-c'], shell: 'bash' };
}
}
// Fallback to bash for unknown shells or if SHELL is not set to maintain compatibility
return {
executable: 'bash',
argsPrefix: ['-c'],
shell: 'bash',
};References
- When consuming an object, if a property is optional in its type definition (interface), callers must handle the undefined case (e.g., by providing a default with ??). Code against the interface contract.
gemini-cli
Bot
added
priority/p2
|
Investigated the security concern. The bash parser reuse for zsh does not create a net-new bypass: zsh-specific syntax (=(), ${(flags)}, anonymous functions): The tree-sitter bash parser sets Precommand modifiers (noglob, nocorrect): These parse cleanly but are misidentified as the root command, hiding the real executable. However:
The actual risk is a policy downgrade: a DENY rule for Will fix the two non-security items: unknown shell fallback (fish/dash to bash executable) and test style (vi.stubEnv). |
1 participant
Summary
$SHELLenv var ingetShellConfiguration()to detect the user's actual shell instead of hardcodingbashzshto theShellTypeunion so zsh users get their shell invoked directly (preventsshopterrors when bash is not the default shell)hasRedirectionto handle zsh alongside bashFixes #26754
Test plan
process.env.SHELLfor deterministic behavior