refactor(a2a-server): enforce path trust check prior to environment loading and isolate task environment - #28319
Conversation
|
📊 PR Size: size/XL
|
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 addresses a critical security vulnerability that allowed for Remote Code Execution (RCE) by exploiting the environment loading mechanism in untrusted workspaces. By refactoring the startup sequence to ensure workspace trust is established before loading environment files, and by enforcing strict variable whitelisting and sanitization, the server now prevents malicious environment configurations from compromising the system. 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 review |
There was a problem hiding this comment.
Code Review
This pull request implements workspace trust checks and environment variable sanitization/whitelisting when loading .env files in packages/a2a-server to mitigate RCE vulnerabilities. The review feedback identifies critical security issues: the dynamic use of process.cwd() allows loading malicious .env files from arbitrary paths, and the server blindly trusts the client-supplied isTrusted flag. To resolve these, the reviewer recommends performing server-side trust verification, passing explicit workspace paths instead of changing the global directory, and completely bypassing workspace-level .env loading for untrusted workspaces, which simplifies the loading logic and requires updating the associated tests.
There was a problem hiding this comment.
Code Review
This pull request implements security mitigations to prevent Remote Code Execution (RCE) in untrusted workspaces by restricting environment variable loading to a whitelist and sanitizing values. However, a critical security vulnerability remains: the server relies on the client-supplied agentSettings.isTrusted parameter to determine workspace trust, which can be easily bypassed by an attacker. The trust check should be performed entirely on the server side.
565449a to
749b6f1
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces workspace trust verification for loading environment variables and settings, ensuring that untrusted workspaces do not load sensitive environment variables. While these changes improve security, critical vulnerabilities remain: workspace-level extensions are still loaded unconditionally, which could allow Remote Code Execution (RCE) in untrusted workspaces. Additionally, the environment loading logic is susceptible to path traversal above the home directory, and the new tests lack hermeticity because os.homedir is not mocked.
b41911e to
af80408
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces security mitigations to prevent loading environment variables and extensions from untrusted workspaces. The review feedback highlights a critical security vulnerability where folder trust settings are read from the untrusted workspace itself, potentially allowing a bypass. Additionally, the feedback points out a functional regression in environment variable overriding, an inconsistent function call in app.ts, and style guide violations in the new test file regarding direct process.env mutations instead of using Vitest's stubbing utilities.
a1fbbf3 to
1af8646
Compare
|
/gemini review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements security mitigations against Remote Code Execution (RCE) vulnerabilities (b-519269096) by isolating environment variable and extension loading for untrusted workspaces. It prevents untrusted workspaces from loading workspace-level .env files or extensions, restricts workspace paths to allowed root directories, and passes isolated environment variables to child processes (such as checkers and shell tools) instead of polluting the global process.env. Additionally, external editor spawning is disabled in headless/server mode, and tests have been added to verify these security boundaries. I have no feedback to provide as there are no review comments.
Note: Security Review did not run due to the size of the PR.
|
Hi there! Thank you for your interest in contributing to Gemini CLI. To ensure we maintain high code quality and focus on our prioritized roadmap, we only guarantee review and consideration of pull requests for issues that are explicitly labeled as 'help wanted'. This PR will be closed in 7 days if it remains without that designation. We encourage you to find and contribute to existing 'help wanted' issues in our backlog! Thank you for your understanding. |
Critical Finding: Discarded Task Isolation and Proxy LogicA comparison between the pull request description and the actual codebase However, during inspection of the branch history, it was noted that these Code Discrepancies and ImpactThe removal of the task isolation logic would have
Recommended ActionHere are a couple paths forward (I recommend option 1):
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements workspace isolation and environment variable sandboxing for the agent-to-agent server (a2a-server) to mitigate remote code execution vulnerabilities. It introduces task-specific environment isolation using AsyncLocalStorage and a Proxy on process.env, and updates configuration loading to bypass untrusted workspace .env files. However, the envProxy implementation intercepts non-string properties (such as Symbols) and returns undefined or false, which can break standard Node.js utilities like util.inspect or third-party libraries. It is recommended to delegate non-string properties directly to the original process.env target.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements task-level environment and workspace isolation in the a2a-server package using AsyncLocalStorage and a Proxy on process.env, alongside monkey-patching process.cwd and process.chdir. It also disables external editor spawning in headless/server mode and ensures isolated environment variables are propagated to shell execution and content generation. Feedback was provided regarding a security vulnerability where the envProxy lacks a defineProperty trap, which could allow dependencies to bypass the proxy's set trap and pollute the global process.env.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces task-level environment and working directory isolation in the a2a-server package using AsyncLocalStorage and a Proxy on process.env to mitigate potential remote code execution (RCE) vulnerabilities from untrusted workspaces. It also updates core services to propagate these isolated environments and disables external editor spawning in headless mode. However, a critical security issue was identified where the main task execution loop in execute is not wrapped in the isolated environment context, causing it to fall back to the global process environment and working directory.
Note: Security Review did not run due to the size of the PR.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces workspace isolation and environment variable sandboxing to mitigate remote code execution (RCE) vulnerabilities. It leverages AsyncLocalStorage and a Proxy on process.env to isolate environment variables per task, restricts .env and extension loading in untrusted workspaces, and disables external editor spawning in headless mode. Feedback on the changes highlights a potential unhandled exception in execute where validateWorkspacePath is called synchronously before entering the isolated environment, which could lead to server crashes; wrapping this setup in a try-catch block and notifying the event bus is recommended.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements workspace and environment isolation for tasks in the a2a-server package to mitigate remote code execution (RCE) vulnerabilities. It uses AsyncLocalStorage and a Proxy on process.env to isolate environment variables and the working directory per task, restricts .env loading in untrusted workspaces, and updates core packages to pass isolated environment variables. The review feedback identifies two key issues: the monkey-patched process.chdir should throw ENOENT errors matching Node's native format, and a .catch block should be added to runInIsolatedEnv in execute to prevent unhandled promise rejections and client hangs during setup failures.
Note: Security Review did not run due to the size of the PR.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces robust workspace and environment isolation to mitigate Remote Code Execution (RCE) vulnerabilities in the Agent-to-Agent (A2A) server. It implements task-level environment isolation using AsyncLocalStorage and a Proxy on process.env, and monkey-patches process.cwd and process.chdir to prevent cross-task interference. Untrusted workspaces are restricted from loading workspace-level .env files or extensions, and workspace paths are validated against allowed root directories. Additionally, core services like ShellExecutionService and CheckerRunner have been updated to respect these isolated configurations, and external editor spawning is disabled in headless mode. There are no review comments provided, so I have no feedback to offer.
Note: Security Review did not run due to the size of the PR.
Hi @DavidAPierce , I have updated the code, it's ready to next round of code review. |
Summary
This PR refactors the initialization lifecycle and environment loading order inside
CoderAgentExecutor(a2a-server) to ensure that workspace path trust checks occur prior to loading workspace-level environment variables. Furthermore, it introducesAsyncLocalStorage(envStorage) to isolateprocess.envmodifications on a per-task basis.By refactoring the startup sequence, environment loading mechanism, and introducing robust task-level environment and process isolation in
a2a-server, we ensure that workspace-level environment files (.envand.gemini/.env) are completely ignored unless the workspace is explicitly trusted by the user. Thisaligns the
a2a-serverbackend's security model with the existing secure implementation in the CLI frontend.Details
loadEnvironment()inpackages/a2a-server/src/http/app.ts(createApp) andpackages/a2a-server/src/agent/executor.ts(getConfig) until afterworkspace trust is evaluated (
checkPathTrust/setIsTrusted).GEMINI_CLI_TRUST_WORKSPACE=trueinside a malicious
.gemini/.envfile to self-validate their ownuntrusted workspace before trust is checked.
loadEnvironment(isTrusted)inpackages/a2a-server/src/config/config.tsto accept the trust state.isTrustedisfalse, workspace-level environment files (both.envand
.gemini/.env) are completely ignored. Instead, the loader only loadsenvironment variables from the user's trusted home directory (e.g.,
~/.gemini/.envor~/.env). This is a safer and more secure approachthat completely isolates untrusted workspaces from environment loading.
envStorage(anAsyncLocalStoragecontext) and a robustProxyonprocess.envto completely isolate environment variables pertask, preventing cross-task credential leakage and race conditions in
concurrent multi-request server environments.
getConfigandexecuteinpackages/a2a-server/src/agent/executor.tsto executeloadEnvironmentinside the
envStorage.runblock, ensuring that environment variables arenever written to the global
process.env.envProxyto correctly handle non-string properties(Symbols like
Symbol.toStringTagorutil.inspect.custom) by delegatingthem to the original
process.envobject, preventing standard Node.jsutilities (like
util.inspect) from breaking and avoiding strict-modeTypeErrors.Reflectnamespace to complywith the project's linter rules (
no-restricted-syntax).deletedKeysSymbolto track explicitly deleted keyswithin the task context, ensuring that subsequent reads and checks
correctly treat deleted keys as non-existent instead of falling back to
the global
process.env.process.chdirRace Condition:process.chdirfromsetTargetDirinpackages/a2a-server/src/config/config.ts. Instead of globally mutatingthe process's working directory, we now pass
workspaceDirexplicitly asan argument to
loadConfigand other services, completely eliminating therace condition where concurrent requests could hijack each other's working
directory.
vi.stubEnvcalls in tests to use an empty string''instead of
undefinedto unset environment variables, and updated theassertions to use
toBeFalsy()to match.Related Issues
Fixes #519269096 (Buganizer: b-519269096) Related to #496967516 (Buganizer:
b-496967516)
How to Validate
Important
Because the previous test failures affected all environments (Linux, macOS,
and Windows) due to platform-independent headless and mock context issues,
validation must be performed across all three platforms to guarantee
complete compatibility and prevent any future pipeline regressions.
successfully on your platform:
Pre-Merge Checklist