fix(core): prevent indirect prompt injection via build file modifications and untrusted flags - #29250
Conversation
…ions and untrusted flags
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
📊 PR Size: size/XL
|
🛑 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. |
GitHub Actions
Error
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 introduces a robust security layer to mitigate indirect prompt injection attacks by tracking untrusted context and enforcing strict confirmation requirements for build-related operations. It adds mechanisms to detect untrusted shell flags and mandates user approval when modifying or executing build configuration files, significantly hardening the workspace against unauthorized modifications and command execution. 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 build file protection and untrusted context tracking to prevent unauthorized build modifications and command execution with untrusted flags. The reviewer provided critical feedback highlighting several security and architectural vulnerabilities: naive whitespace splitting and command parsing in findUntrustedFlags and isBuildOrTestCommand can be bypassed with quotes or prefixed environment variables; the policy engine's build file protection can be bypassed via MCP filesystem tools; and tracking modified build files using a module-level global Set violates repository rules against global state, risking race conditions in concurrent environments. The reviewer recommended using shell-quote and getCommandRoots for robust command parsing, expanding tool name matching to cover MCP tools, adopting a session-scoped WeakMap for tracking modified files, and suppressing persistent approval options in the UI when modified build files are detected.
Note: Security Review did not run due to the size of the PR.
| const detected = new Set<string>(); | ||
|
|
||
| // Tokenize the command arguments | ||
| const rawTokens = command.trim().split(/\s+/); |
There was a problem hiding this comment.
The current implementation of findUntrustedFlags tokenizes the command using a naive whitespace split (command.trim().split(/\s+/)). This fails to correctly parse arguments containing spaces (e.g., inside quotes) and leaves quotes attached to the tokens (e.g., '"//foo/bar:target"' or '--flag="value"'), which prevents exact matching against untrustedTokens (where quotes are stripped). This allows attackers to easily bypass the prompt injection protection by simply quoting their arguments or flags. Use the existing shell-quote library to parse the command safely and accurately.
// Parse the command safely using shell-quote to handle quotes and escapes correctly
let parsed: ReturnType<typeof shellParse>;
try {
parsed = shellParse(command);
} catch {
// Fallback to whitespace split if parsing fails
parsed = command.trim().split(/\s+/);
}
const rawTokens = parsed
.map((x) => {
if (typeof x === 'string') return x;
if (x && typeof x === 'object' && 'pattern' in x) return x.pattern;
return '';
})
.filter(Boolean);There was a problem hiding this comment.
Done
@gemini-code-assist check my changes and let me know if you have additional comments in this thread?
There was a problem hiding this comment.
The implementation of findUntrustedFlags now correctly uses shell-quote to parse the command, which properly handles quoted arguments and escapes. This effectively mitigates the identified prompt injection bypass risk. The logic for extracting tokens from the parsed output is also correct. The changes look good.
| export function isBuildOrTestCommand(command: string): boolean { | ||
| if (!command) { | ||
| return false; | ||
| } | ||
|
|
||
| const trimmed = command.trim(); | ||
| const root = trimmed.split(/\s+/)[0]; | ||
| if (!root) { | ||
| return false; | ||
| } | ||
|
|
||
| const normalized = root.toLowerCase(); | ||
| return BUILD_TEST_COMMAND_ROOTS.has(normalized); | ||
| } |
There was a problem hiding this comment.
The current implementation of isBuildOrTestCommand only checks the very first token of the command. This can be easily bypassed by prefixing the command with environment variables (e.g., VAR=1 npm test) or common shell builtins/chains (e.g., cd . && npm test). Use the existing getCommandRoots utility from shell-utils.js to extract all command roots and check if any of them match the build/test command roots.
| export function isBuildOrTestCommand(command: string): boolean { | |
| if (!command) { | |
| return false; | |
| } | |
| const trimmed = command.trim(); | |
| const root = trimmed.split(/\s+/)[0]; | |
| if (!root) { | |
| return false; | |
| } | |
| const normalized = root.toLowerCase(); | |
| return BUILD_TEST_COMMAND_ROOTS.has(normalized); | |
| } | |
| import { getCommandRoots } from './shell-utils.js'; | |
| export function isBuildOrTestCommand(command: string): boolean { | |
| if (!command) { | |
| return false; | |
| } | |
| try { | |
| const roots = getCommandRoots(command); | |
| return roots.some((root) => BUILD_TEST_COMMAND_ROOTS.has(root.toLowerCase())); | |
| } catch { | |
| // Fallback if parsing fails | |
| const trimmed = command.trim(); | |
| const root = trimmed.split(/\s+/)[0]; | |
| return root ? BUILD_TEST_COMMAND_ROOTS.has(root.toLowerCase()) : false; | |
| } | |
| } |
| const isFileEditTool = toolNamesToTry.some( | ||
| (name) => | ||
| EDIT_TOOL_NAMES.has(name) || | ||
| name === 'replace' || | ||
| name === 'write_file', | ||
| ); |
There was a problem hiding this comment.
The "Build File Protection" check only triggers if the tool name is in a hardcoded list of core edit tools (EDIT_TOOL_NAMES, replace, write_file). This allows an agent to completely bypass the protection by using MCP filesystem tools (such as the official @modelcontextprotocol/server-filesystem tool write_file or edit_file) to modify build files. Update the check to also match any tool names containing write/edit keywords when they target a build file.
const isFileEditTool = toolNamesToTry.some(
(name) =>
EDIT_TOOL_NAMES.has(name) ||
name === 'replace' ||
name === 'write_file' ||
/write|edit|replace|patch|update|create|append|save/i.test(name),
);| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| import type { Content } from '@google/genai'; |
There was a problem hiding this comment.
Import parse from shell-quote to enable safe and robust shell command parsing in findUntrustedFlags.
| import type { Content } from '@google/genai'; | |
| import type { Content } from '@google/genai'; | |
| import { parse as shellParse } from 'shell-quote'; |
| const sessionModifiedBuildFiles = new Set<string>(); | ||
|
|
||
| /** | ||
| * Records that a build configuration file was modified in this session. | ||
| */ | ||
| export function recordModifiedBuildFile(filePath: string): void { | ||
| sessionModifiedBuildFiles.add(filePath); | ||
| } | ||
|
|
||
| /** | ||
| * Returns all build configuration files that were modified in this session. | ||
| */ | ||
| export function getModifiedBuildFiles(): string[] { | ||
| return Array.from(sessionModifiedBuildFiles); | ||
| } | ||
|
|
||
| /** | ||
| * Resets the tracked modified build files (primarily for testing or session reset). | ||
| */ | ||
| export function resetModifiedBuildFiles(): void { | ||
| sessionModifiedBuildFiles.clear(); | ||
| } |
There was a problem hiding this comment.
Using a module-level global Set (sessionModifiedBuildFiles) to track modified build files violates the repository's general rules against global state. In concurrent environments (such as multiple agent sessions running in the same Node.js process), this state will be shared across all sessions, leading to race conditions and cross-session information leaks. Use a WeakMap keyed by the session's Config or context instance to ensure session-scoped isolation.
const globalSessionKey = {};
const sessionModifiedBuildFiles = new WeakMap<object, Set<string>>();
/**
* Records that a build configuration file was modified in this session.
*/
export function recordModifiedBuildFile(filePath: string, sessionKey: object = globalSessionKey): void {
let files = sessionModifiedBuildFiles.get(sessionKey);
if (!files) {
files = new Set<string>();
sessionModifiedBuildFiles.set(sessionKey, files);
}
files.add(filePath);
}
/**
* Returns all build configuration files that were modified in this session.
*/
export function getModifiedBuildFiles(sessionKey: object = globalSessionKey): string[] {
const files = sessionModifiedBuildFiles.get(sessionKey);
return files ? Array.from(files) : [];
}
/**
* Resets the tracked modified build files (primarily for testing or session reset).
*/
export function resetModifiedBuildFiles(sessionKey: object = globalSessionKey): void {
sessionModifiedBuildFiles.delete(sessionKey);
}References
- Avoid module-level global variables for state to prevent race conditions in concurrent environments. Instead, use instance-scoped properties, such as within a session class, for state that is primarily used within that scope.
| const hasUntrustedFlags = | ||
| confirmationDetails.untrustedFlags && | ||
| confirmationDetails.untrustedFlags.length > 0; | ||
| if (isTrustedFolder && !hasUntrustedFlags) { |
There was a problem hiding this comment.
When modifiedBuildFiles is present, the shell tool's shouldConfirmExecute will always force confirmation on subsequent build commands, regardless of any session-scoped or permanent allow rules. Offering the user "Allow for this session" or "Allow this command for all future sessions" in this state is confusing because those options will be silently ignored on the next execution. Suppress these persistent approval options when modifiedBuildFiles is detected, just like we do for untrustedFlags.
const hasUntrustedFlags =
confirmationDetails.untrustedFlags &&
confirmationDetails.untrustedFlags.length > 0;
const hasModifiedBuildFiles =
confirmationDetails.modifiedBuildFiles &&
confirmationDetails.modifiedBuildFiles.length > 0;
if (isTrustedFolder && !hasUntrustedFlags && !hasModifiedBuildFiles) {
| if (isBuildFile(this.resolvedPath)) { | ||
| recordModifiedBuildFile(this.resolvedPath); | ||
| } |
There was a problem hiding this comment.
Pass this.config as the session key to recordModifiedBuildFile to ensure session-scoped tracking of modified build files.
| if (isBuildFile(this.resolvedPath)) { | |
| recordModifiedBuildFile(this.resolvedPath); | |
| } | |
| if (isBuildFile(this.resolvedPath)) { | |
| recordModifiedBuildFile(this.resolvedPath, this.config); | |
| } |
| if (isBuildFile(this.resolvedPath)) { | ||
| recordModifiedBuildFile(this.resolvedPath); | ||
| } |
There was a problem hiding this comment.
Pass this.config as the session key to recordModifiedBuildFile to ensure session-scoped tracking of modified build files.
| if (isBuildFile(this.resolvedPath)) { | |
| recordModifiedBuildFile(this.resolvedPath); | |
| } | |
| if (isBuildFile(this.resolvedPath)) { | |
| recordModifiedBuildFile(this.resolvedPath, this.config); | |
| } |
| const command = stripShellWrapper(this.params.command); | ||
| const history = this.getHistory(); | ||
| const untrustedContext = extractUntrustedContext(history); | ||
| const untrustedFlags = findUntrustedFlags(command, untrustedContext); | ||
| const modifiedBuildFiles = getModifiedBuildFiles(); | ||
| const isBuildCmd = isBuildOrTestCommand(command); |
There was a problem hiding this comment.
Pass this.context.config to getModifiedBuildFiles to retrieve only the build files modified within the current session.
| const command = stripShellWrapper(this.params.command); | |
| const history = this.getHistory(); | |
| const untrustedContext = extractUntrustedContext(history); | |
| const untrustedFlags = findUntrustedFlags(command, untrustedContext); | |
| const modifiedBuildFiles = getModifiedBuildFiles(); | |
| const isBuildCmd = isBuildOrTestCommand(command); | |
| const command = stripShellWrapper(this.params.command); | |
| const history = this.getHistory(); | |
| const untrustedContext = extractUntrustedContext(history); | |
| const untrustedFlags = findUntrustedFlags(command, untrustedContext); | |
| const modifiedBuildFiles = getModifiedBuildFiles(this.context.config); | |
| const isBuildCmd = isBuildOrTestCommand(command); |
| const history = this.getHistory(); | ||
| const untrustedContext = extractUntrustedContext(history); | ||
| const untrustedFlags = findUntrustedFlags(command, untrustedContext); | ||
| const modifiedBuildFiles = getModifiedBuildFiles(); | ||
| const isBuildCmd = isBuildOrTestCommand(command); |
There was a problem hiding this comment.
Pass this.context.config to getModifiedBuildFiles to retrieve only the build files modified within the current session.
| const history = this.getHistory(); | |
| const untrustedContext = extractUntrustedContext(history); | |
| const untrustedFlags = findUntrustedFlags(command, untrustedContext); | |
| const modifiedBuildFiles = getModifiedBuildFiles(); | |
| const isBuildCmd = isBuildOrTestCommand(command); | |
| const history = this.getHistory(); | |
| const untrustedContext = extractUntrustedContext(history); | |
| const untrustedFlags = findUntrustedFlags(command, untrustedContext); | |
| const modifiedBuildFiles = getModifiedBuildFiles(this.context.config); | |
| const isBuildCmd = isBuildOrTestCommand(command); |
gemini-cli
Bot
added
the
status/need-issue
GitHub Actions
Error
…d configuration security
GitHub Actions
Waiting
1 participant
Summary
This PR implements robust mechanisms to improve workspace boundary validation (specifically focusing on build configuration files and external command parameters) under restricted workspace mode. It refactors built-in execution paths (including
shell,edit, andwrite_file) to check for command flags or arguments guided by external context tags (such as Google Docs, Buganizer, web fetch, or MCP server responses) and require explicit user confirmation.Details
<untrusted_context>blocks, and verify whether a command contains parameters guided by external input.package.json,Makefile,pyproject.toml,BUILD.bazel) when edited or created. If build configuration changes are detected within the current session, any subsequent build or test command execution (such asnpm run,make,cargo,blaze) is surfaced for explicit user confirmation.ToolConfirmationMessageto display detailed context to the user, highlighting specific parameters or recent build modifications, and explaining the execution options.Related Issues
Related to FixBug-cla-445881265
How to Validate
npm test -w @google/gemini-cli
Pre-Merge Checklist