Support per-request API keys and update staging/autopush config#140
Support per-request API keys and update staging/autopush config#140keyurva merged 13 commits intodatacommonsorg:mainfrom
Conversation
Summary of ChangesHello @keyurva, 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 configuration issue where hosted MCP servers in non-production environments were incorrectly communicating with production APIs. It introduces a robust solution by implementing per-request API key support via a new middleware, ensuring that 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. 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces support for per-request API keys via a Starlette middleware and updates deployment configurations for staging and autopush environments. However, a High-severity Server-Side Request Forgery (SSRF) vulnerability has been identified in the API key validation logic, potentially allowing arbitrary URL requests. There's also a Medium-severity issue in the new API key middleware due to improper exception handling, which could leak sensitive API keys into logs. Additionally, a suggestion has been made to improve the readability of the middleware code.
clincoln8
left a comment
There was a problem hiding this comment.
Thanks Keyur!
re:testing on autopush -- I think we can just submit the PR and monitor/test directly on autopush. We can rollback or forward if modifications are required.
| # When using a local instance, you may also need to use the | ||
| # --skip-api-key-validation command-line flag if running without a DC_API_KEY. |
There was a problem hiding this comment.
Should these two comments be moved to be closer to DC_API_KEY_VALIDATION_ROOT?
packages/datacommons-mcp/.env.sample
Outdated
There was a problem hiding this comment.
Should we move away from using the word "local" in these comments?
packages/datacommons-mcp/.env.sample
Outdated
| # ============================================================================= | ||
| # HOSTED MCP SERVER OPTIONAL CONFIGURATION | ||
| # ============================================================================= |
There was a problem hiding this comment.
Should we remove this section break and keep DC_API_KEY_VALIDATION_ROOT in the same section as DC_API_ROOT and DC_SEARCH_ROOT? It seems like that might be the same logical grouping.
packages/datacommons-mcp/.env.sample
Outdated
|
|
||
| # Root URL for Data Commons API key validation | ||
| # Configure for non-prod environments | ||
| # DC_API_KEY_VALIDATION_ROOT=https://api.datacommons.org |
There was a problem hiding this comment.
How are we thinking about api key validation for CustomDCs? Since this setting does affect custom instances (since api key is enabled by default and we fallback to validating against api.datacommons.org), we should make sure this is communicated effectively -- ie even for custom instances, the api key is required and validated against base DC?
(there's not really an action item here, just something to think about!)
There was a problem hiding this comment.
We mention these as base DC only configs so Custom DC users should not be using them. We could be more explicit about it but leaving it as-is for now.
| from collections.abc import Awaitable, Callable | ||
|
|
||
| from datacommons_client import use_api_key | ||
| from starlette.middleware.base import BaseHTTPMiddleware |
There was a problem hiding this comment.
ooc, why use this class instead of fastmcp's https://gofastmcp.com/servers/middleware#modifying-requests?
(no need to change it since it's working, just curious!)
There was a problem hiding this comment.
This is what gemini recommended :)
I also liked that it was explicitly registered for http mode only.
keyurva
left a comment
There was a problem hiding this comment.
Thanks for the review!
re:testing on autopush -- I think we can just submit the PR and monitor/test directly on autopush. We can rollback or forward if modifications are required.
SG.
packages/datacommons-mcp/.env.sample
Outdated
| # ============================================================================= | ||
| # HOSTED MCP SERVER OPTIONAL CONFIGURATION | ||
| # ============================================================================= |
packages/datacommons-mcp/.env.sample
Outdated
|
|
||
| # Root URL for Data Commons API key validation | ||
| # Configure for non-prod environments | ||
| # DC_API_KEY_VALIDATION_ROOT=https://api.datacommons.org No newline at end of file |
There was a problem hiding this comment.
We mention these as base DC only configs so Custom DC users should not be using them. We could be more explicit about it but leaving it as-is for now.
| from collections.abc import Awaitable, Callable | ||
|
|
||
| from datacommons_client import use_api_key | ||
| from starlette.middleware.base import BaseHTTPMiddleware |
There was a problem hiding this comment.
This is what gemini recommended :)
I also liked that it was explicitly registered for http mode only.
This became a much deeper PR than what I'd initially thought. This is because the hosted MCP servers in all environments are connected to the prod APIs instead of to the ones in the respective environments. This PR changes that.
Updating those environments then needed updating secrets, other env vars and also the MCP startup validation routines.
As far as the code changes go, they are as follows:
X-API-Keyheaders todatacommons_clientDC_API_KEY_VALIDATION_ROOTstaging.yamlandautopush.yamlwith necessary environment variables and secretsscriptsfolder was just a result of running ruff format. It has no real code changes,Testing
In addition to unit tests, did the following manual tests:
autopush.yamlfor details.