feat(vertex): add vertexLocation config setting for Vertex AI region override#25362
feat(vertex): add vertexLocation config setting for Vertex AI region override#25362Famous077 wants to merge 26 commits intogoogle-gemini:mainfrom
Conversation
…en reader test snapshots
…on and normalizeCommand
…leRenderer and integrity
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 persistent configuration option for Vertex AI region overrides, improving usability for users accessing experimental models. Additionally, it significantly enhances the security of the shell tool by implementing robust detection for command injection vulnerabilities, alongside necessary maintenance fixes for test suites. 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. 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. |
![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 command injection detection for the shell tool, blocking command substitution syntax such as $(), backticks, and process substitution in both Bash and PowerShell. It also adds a vertexLocation configuration parameter to the core package to allow overriding the default Google Cloud location for Vertex AI. A critical security flaw was identified in the PowerShell substitution detection logic where incorrect backtick escaping inside double-quoted strings could lead to command injection bypasses.
| function detectPowerShellSubstitution(command: string): boolean { | ||
| let inSingleQuote = false; | ||
| let inDoubleQuote = false; | ||
| let i = 0; | ||
| while (i < command.length) { | ||
| const char = command[i]; | ||
| if (char === "'" && !inDoubleQuote) { | ||
| inSingleQuote = !inSingleQuote; | ||
| i++; | ||
| continue; | ||
| } | ||
| if (char === '"' && !inSingleQuote) { | ||
| inDoubleQuote = !inDoubleQuote; | ||
| i++; | ||
| continue; | ||
| } | ||
| if (inSingleQuote) { | ||
| i++; | ||
| continue; | ||
| } | ||
| if (char === '`' && !inSingleQuote && i + 1 < command.length) { | ||
| i += 2; | ||
| continue; | ||
| } | ||
| if (char === '$' && command[i + 1] === '(') { | ||
| return true; | ||
| } | ||
| if (char === '@' && command[i + 1] === '(') { | ||
| return true; | ||
| } | ||
| i++; | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The command substitution detection for PowerShell is flawed and can be bypassed, leading to a potential command injection vulnerability. The current implementation incorrectly assumes that a backtick always escapes the following character, both inside and outside of double quotes. In PowerShell, when inside a double-quoted string, a backtick is treated as a literal character unless it is followed by $, ", or another backtick (or forms a special character escape sequence like backtick-n).
This means a command like echo "x$(whoami)" would be incorrectly considered safe by the current parser, as it would treat the backtick as escaping the $. However, PowerShell would treat the backtick as a literal and execute the whoami command.
To fix this, the escape logic should be more precise, similar to the detectBashSubstitution implementation, only treating the backtick as an escape character for specific characters when inside a double-quoted string.
function detectPowerShellSubstitution(command: string): boolean {
let inSingleQuote = false;
let inDoubleQuote = false;
let i = 0;
while (i < command.length) {
const char = command[i];
if (char === "'" && !inDoubleQuote) {
inSingleQuote = !inSingleQuote;
i++;
continue;
}
if (char === "\"" && !inSingleQuote) {
inDoubleQuote = !inDoubleQuote;
i++;
continue;
}
if (inSingleQuote) {
i++;
continue;
}
if (char === "\x60" && i + 1 < command.length) {
if (inDoubleQuote) {
const next = command[i + 1];
if (["$", "\x60", "\""].includes(next)) {
i += 2;
continue;
}
} else {
i += 2;
continue;
}
}
if (char === "$" && command[i + 1] === "(") {
return true;
}
if (char === "@" && command[i + 1] === "(") {
return true;
}
i++;
}
return false;
}
gemini-cli
bot
added
area/core
|
/gemini-review |
gemini-cli
bot
added
the
area/platform
2 participants
Summary
When using Gemini CLI with Vertex AI, requests are routed to
us-central1bydefault. The problem is that preview/experimental models like
gemini-3.1-pro-previeware only released to theglobalregion first, soanyone trying to use them gets an immediate 404 error with no clear explanation
of why.
This PR adds a
vertexLocationsetting tosettings.jsonso users canoverride the region without having to set environment variables every session.
Details
The root cause was twofold:
googleCloudLocationwas being read from theGOOGLE_CLOUD_LOCATIONenvvar but never actually stored into
ContentGeneratorConfig— so even ifusers set the env var correctly, it wasn't always being honored in the Vertex
AI auth path.
There was no persistent way to set the location. Users had to remember to
export
GOOGLE_CLOUD_LOCATIONin every terminal session.The fix wires
vertexLocationfromsettings.jsonthrough the full configpipeline —
ConfigParameters→Configclass getter →createContentGeneratorConfig→GoogleGenAIclient. Settings take priorityover the env var, but the env var still works as a fallback so nothing is
broken for existing users.
Also fixed 15 pre-existing TypeScript errors in
shell.test.tswhereinvocation.execute()was being called with a rawAbortSignalinstead ofthe expected
ExecuteOptionsobject — this was breaking the build on a cleancheckout.
Related Issues
Fixes #20761
How to Validate
Option 1 — via settings.json (new behavior):
Then run with a preview model:
Expected: Request succeeds instead of returning a 404.
Option 2 — env var (existing behavior, should still work):
Option 3 — verify priority (settings wins over env var):
Set
GOOGLE_CLOUD_LOCATION=us-central1in env and"vertexLocation": "global"in settings. The request should go to
global.Run the tests:
All 27 tests should pass including the new one:
should prefer vertexLocation from config over GOOGLE_CLOUD_LOCATION env varPre-Merge Checklist