Repository navigation
fix(mcp): reject config-supplied stdio MCP servers by default - #1528
yusmer96-maker wants to merge 1 commit into
Conversation
|
Hi @yusmer96-maker, thank you for your contribution! We appreciate you taking the time to submit this pull request. I noticed that the test cases are currently failing. Could you please take a look and address those issue? |
damianmomotgoogle
left a comment
There was a problem hiding this comment.
We need to keep it backward compatible - i.e. if default behavior was allowing to read those settings, default must stay the same for now - otherwise version bump may break existing user.
Other change I'd like to propose here is to remove env variable - i.e. base behavior only on a static flag switch. Is that something which would address your scenario?
|
@damianmomotgoogle @hemasekhar-p Thanks — both points addressed in
One question: with only the setter, someone running agents through
Note: the failing checks are in |
McpToolset.fromConfig() accepts stdioServerParams / stdioConnectionParams straight from an agent config. Those carry a command and args that the MCP stdio transport launches as a local process, so loading an agent config starts the configured process while tools are resolved on the first turn, before the model is contacted. Agent configs are not necessarily written by the person running the agent: they are shared as bundles, templates, samples and registry entries. Add a switch that makes fromConfig() reject both stdio parameter shapes. The default keeps the current behaviour (the one restored in google#1360), so existing configs work unchanged. The rejection is turned on either from the embedding application with McpToolset.setAllowConfigStdioServers(false) or, without changing code, at launch with the system property -Dadk.mcp.allowConfigStdioServers=false. Remote transports (sseServerParams) and McpToolset instances built directly in Java are unaffected.
74b851e to
28810cd
Compare
Link to Issue or Description of Change
2. Or, if no issue exists, describe the change:
Problem:
McpToolset.fromConfig()acceptsstdioServerParams/stdioConnectionParamsstraight from an agent config. Those carry acommandandargsthat the MCP stdio transport launches as a local process, so loading an agent config starts the configured process while tools are resolved on the first turn — before the model is contacted.Agent configs are not necessarily written by the person running the agent: they are shared as bundles, templates, samples and registry entries. That makes loading a config equivalent to running code for anyone who runs an agent they did not author.
No guard applies on this path: there is no module denylist, class allowlist or blocked-key check reaching it in any mode.
McpToolsetresolves from the pre-registeredComponentRegistryentry (ComponentRegistry.java:132-142), so the class allowlist added in #1237 does not cover it — that allowlist permitscom.google.adk.*, andMcpToolsetiscom.google.adk.tools.mcp.McpToolset.Solution:
fromConfig()now rejectsstdioServerParamsandstdioConnectionParamsunless the operator opts in, either by settingADK_ALLOW_CONFIG_STDIO_MCP_SERVERS=1or by callingMcpToolset.setAllowConfigStdioServers(true)from the embedding application. The programmatic override is tri-state:nulldefers to the environment variable, a non-null value wins over it.This keeps the behaviour #1360 restored working for anyone who wants it — the path is opt-in rather than removed — while the default stops an untrusted config from starting a process.
Unaffected: remote transports (
sseServerParams), constructingMcpToolsetdirectly in Java code, and applications that opt in.Testing Plan
Unit Tests:
mvn -pl core -am -Dtest=McpToolsetTest test→ Tests run: 26, Failures: 0, Errors: 0, Skipped: 0.New:
testFromConfig_stdioServerParams_rejectedByDefaulttestFromConfig_stdioConnectionParams_rejectedByDefaulttestFromConfig_stdioServerParams_allowedWhenOptedIntestFromConfig_sseParams_unaffectedByStdioRejectionUpdated to opt in, since they exercise the stdio branch:
testFromConfig_validStdioParams_createsToolsettestFromConfig_validStdioConnectionParams_createsToolsettestFromConfig_onlyStdioParams_doesNotUseSseBranchtestFromConfig_stdioParamsNoToolFilter_createsToolsettestFromConfig_emptyToolFilter_createsToolsetThe opt-in state is reset by an
@Afterfixture (restoreConfigStdioDefault) rather than per-testtry/finally.Manual End-to-End (E2E) Tests:
Two single-file agent bundles (
root_agent.yaml) served by thewebgoal ofgoogle-adk-maven-plugin, built from source at33e28e3, run twice: once at that commit unmodified, once with this change applied. The payload is/bin/sh -c "id > /tmp/<canary>", so an executed process leaves a file behind.1 — stdio MCP server declared in an agent config,
stdioConnectionParamsshape:2 — same,
stdioServerParamsshape:Unmodified
33e28e3,POST /runon each app:With this change, same two configs, same requests:
and tool resolution stops at config load:
3 — remote MCP server in an agent config (unchanged):
The toolset builds normally and the run proceeds to the model step (
testFromConfig_sseParams_unaffectedByStdioRejectioncovers the same path).Checklist