Conversation
dosubot
bot
added
the
size:XL
Summary of ChangesHello @jezekra1, 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 significant refactoring of the SDK's dependency management and agent initialization processes. The core aim is to streamline how extensions are accessed and configured, moving towards a more context-aware and less explicit dependency passing model. This change enhances the clarity and maintainability of the agent's lifecycle, making it easier to manage implicit functionalities and security configurations. The update also includes a bump in the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
jezekra1
force-pushed
the
sdk-streaming-refactor
branch
from
a7532f6 to
35660eb
Compare
![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 significantly refactors the SDK's dependency management and agent initialization, moving to contextvars for request-scoped dependency injection and centralizing configuration logic in Agent.initialize. While these changes enhance maintainability, they also introduce or expose critical security vulnerabilities, specifically Server-Side Request Forgery (SSRF) and authentication token leakage. The PlatformApiExtensionServer and MCPServiceExtensionServer allow untrusted clients to control target URLs and potentially leak platform authentication tokens. Additionally, a critical issue was identified in the logic for combining required extensions. To address the security concerns, all client-provided URLs must be strictly validated against an allow-list or rely solely on trusted server-side configuration.
| platform = PlatformApiExtensionServer.current() | ||
| if ( | ||
| platform | ||
| and platform.data | ||
| and platform.data.base_url | ||
| and transport.url.startswith(str(platform.data.base_url)) | ||
| ): | ||
| return await platform.create_httpx_auth() | ||
| oauth = self._get_oauth_server() | ||
| oauth = OAuthExtensionServer.current() | ||
| if oauth: | ||
| return await oauth.create_httpx_auth(resource_url=pydantic.AnyUrl(transport.url)) | ||
| return None |
There was a problem hiding this comment.
The _create_auth method contains a critical security flaw where it attaches the platform authentication token to requests if the target URL starts with the platform's base_url. Since both the target URL (transport.url) and the platform base_url can be controlled by a malicious client via message metadata, an attacker can trick the agent into sending its authentication token to an attacker-controlled server. This is a form of Server-Side Request Forgery (SSRF) leading to credential leakage.
| preferred_transport=preferred_transport, | ||
| additional_interfaces=additional_interfaces, | ||
| implicit_extensions=implicit_extensions, | ||
| required_extensions=(required_extensions or set()) & context_store.required_extensions, |
There was a problem hiding this comment.
The logic for combining required_extensions from the app configuration and the context store appears to be incorrect. Using a set intersection (&) means an extension will only be marked as required if it's specified in both the user-provided set and the context store's required set. This is likely not the intended behavior. For instance, if PlatformContextStore is used, it requires the PlatformApiExtension, but this extension will not be marked as required unless the user also explicitly lists it. This should probably be a set union (|) to ensure that extensions required by either the user or the context store are correctly flagged as required.
| required_extensions=(required_extensions or set()) & context_store.required_extensions, | |
| required_extensions=(required_extensions or set()) | context_store.required_extensions, |
| if self.data and self.data.auth_token and self.spec.params.auto_use: | ||
| async with self.use_client(): | ||
| yield |
There was a problem hiding this comment.
The lifespan method triggers a network request via use_client() if an authentication token is present. The target of this request is self.data.base_url, which can be specified by the client in the message metadata. Because this URL is not validated against an allow-list, an attacker can use this to perform Server-Side Request Forgery (SSRF) against internal services or exfiltrate data. The risk is elevated because this extension is now enabled by default for all agents.
| ErrorExtensionSpec.URI: ErrorExtensionServer(ErrorExtensionSpec(ErrorExtensionParams())), | ||
| PlatformApiExtensionSpec.URI: PlatformApiExtensionServer(PlatformApiExtensionSpec()), | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Adding PlatformApiExtensionServer to DEFAULT_IMPLICIT_EXTENSIONS enables this extension by default for all agents. As noted in other comments, this extension is vulnerable to SSRF because it allows clients to specify a base_url in message metadata without validation. Enabling it by default significantly increases the attack surface of the SDK.
Signed-off-by: Radek Ježek <radek.jezek@ibm.com>
jezekra1
force-pushed
the
sdk-streaming-refactor
branch
from
35660eb to
5a85c7c
Compare
1 participant
Summary
Linked Issues
Documentation
If this PR adds new feature or changes existing. Make sure documentation is adjusted accordingly. If the docs is not needed, please explain why.