fix(core): resolve MCP process leaks, deadlocks, and config regressions Fix #23344#23363
fix(core): resolve MCP process leaks, deadlocks, and config regressions Fix #23344#23363PrajwaL-N-TECHIE wants to merge 3 commits intogoogle-gemini:mainfrom
Conversation
This PR addresses multiple high-impact bugs: 1. MCP processes hanging during shutdown (fixed with timeouts and hard-kills). 2. Configuration regression for custom excludes (implemented getCustomExcludes). 3. Redundant telemetry signal handlers (removed). Fixes google-gemini#23344
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 resolves critical bugs related to MCP process management, configuration handling, and telemetry within the Gemini CLI. It introduces process timeouts and hard-kills to prevent MCP processes from hanging during shutdown, corrects a regression in custom file exclusion configurations, and optimizes telemetry signal handling. 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
|
gemini-cli
bot
added
the
area/core
![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 focuses on fixes for MCP process management, including addressing process leaks and deadlocks during shutdown. A critical security vulnerability has been identified: the MCP client's SSE fallback logic is susceptible to OAuth token leakage due to incorrect server name usage, potentially allowing malicious endpoints to steal credentials. Additionally, the pull request includes fixes for custom excludes configuration and redundant telemetry handlers. Consider improving type safety when accessing internal SDK properties to align with repository best practices.
| const result = await connectWithSSETransport( | ||
| mcpClient, | ||
| mcpServerConfig, | ||
| await getStoredOAuthToken(mcpServerName), | ||
| ); |
There was a problem hiding this comment.
The tool's SSE fallback logic retrieves a stored OAuth token using only the MCP server name as a key and transmits it to the URL specified in the server's configuration. This mechanism is vulnerable to token leakage if an attacker can provide a malicious MCP server configuration that uses the same name as a trusted server but a different URL. By causing the initial connection to fail, the attacker can force the tool to fall back to SSE and send the trusted server's sensitive OAuth token to an attacker-controlled endpoint.
Remediation:
Modify the token retrieval and transmission logic to ensure that OAuth tokens are only sent to the specific URL(s) for which they were issued. Key the token storage by both the server name and its base URL, and validate the destination URL before including the Authorization header in the request.
| getPid(): number | undefined { | ||
| let t = this.transport; | ||
| if (t instanceof XcodeMcpBridgeFixTransport) { | ||
| // Access the internal transport which is not exposed in the FixTransport type. | ||
| /* eslint-disable @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-type-assertion */ | ||
| t = (t as any).transport; | ||
| /* eslint-enable @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-type-assertion */ | ||
| } | ||
| if (t instanceof StdioClientTransport) { | ||
| // StdioClientTransport has a process property at runtime but it's not in the public types. | ||
| /* eslint-disable @typescript-eslint/no-unsafe-type-assertion, @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-return */ | ||
| return (t as any).process?.pid; | ||
| /* eslint-enable @typescript-eslint/no-unsafe-type-assertion, @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-return */ | ||
| } | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
To improve type safety and maintainability when accessing internal properties of SDK objects, it's better to avoid casting to any and disabling ESLint rules. As per repository rules for handling undocumented properties, if such usage is unavoidable, a local interface should be defined for these properties, and a detailed comment explaining the rationale and why a public API could not be used must be added. The provided suggestion uses a more constrained type assertion (as unknown as { property: Type }), which is an improvement, but ensure the rationale for accessing internal properties is clearly documented.
getPid(): number | undefined {
let t = this.transport;
if (t instanceof XcodeMcpBridgeFixTransport) {
// Access the internal transport which is not exposed in the FixTransport type.
t = (t as unknown as { transport: Transport }).transport;
}
if (t instanceof StdioClientTransport) {
// StdioClientTransport has a process property at runtime but it's not in the public types.
return (t as unknown as { process?: { pid?: number } }).process?.pid;
}
return undefined;
}References
- The current implementation uses
as anyto access internal SDK properties. The repository rule states that if using internal or undocumented SDK properties is unavoidable, a local interface should be defined for them, and a detailed comment explaining the rationale should be added.
This commit addresses PR feedback: 1. Security: OAuth tokens are now keyed and validated by both server name and URL to prevent leakage during SSE fallback. 2. Type Safety: Refactored McpClient.getPid() to use recommended 'as unknown as' type assertions instead of disabling ESLint rules. 3. Quality: Added File global polyfill to test-setup.ts to fix undici environment issues in Vitest. 4. Testing: Updated hybrid-token-storage tests to match new 2-argument signatures.
|
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 have updated our contribution policy (see Discussion #17383). We only guarantee review and consideration of pull requests for issues that are explicitly labeled as 'help wanted'. All other community pull requests are subject to closure after 14 days if they do not align with our current focus areas. For this reason, we strongly recommend that contributors only submit pull requests against issues explicitly labeled as 'help-wanted'. This pull request is being closed as it has been open for 14 days without a 'help wanted' designation. We encourage you to find and contribute to existing 'help wanted' issues in our backlog! Thank you for your understanding and for being part of our community! |
1 participant
This PR addresses multiple high-impact bugs:
Fixes #23344
Summary
Details
Related Issues
How to Validate
Pre-Merge Checklist