[feat] Enable user MCP servers by default (http; stdio stays off-by-design)#4912
[feat] Enable user MCP servers by default (http; stdio stays off-by-design)#4912mmabrouk wants to merge 1 commit into
Conversation
dosubot
Bot
added
the
size:M
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthrough
MCP Default-On Enablement
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🤖 The AI agent says: Changes made This flips user MCP servers to on by default, per your direction that MCP should not be off by default.
Decision needed (this is a design call, not a code review) The runner's http SSRF guard is scheme + host only. It does not pin DNS resolution to the validated host, and it does not re-validate redirects. With user MCP now on by default, an author-controlled config could reach an internal host through DNS rebinding or an https->internal redirect. Secrets ride headers only to https hosts, so the blast radius is bounded, but this is a real residual for an OSS default-on. Options:
My recommendation: option 2 if this rides only on What I need from you: the decision above, harden-first vs ship-with-residual. The code change is a one-line default flip plus the matching deployment defaults; a quick correctness skim is welcome, but the real ask is the security call. Do not merge. I am leaving this for you / the orchestrator to fold back. |
| # runner's https + SSRF guards; stdio servers still fail loud (disabled by design). Set | ||
| # the flag to a falsy value to turn user MCP servers off entirely. | ||
| return ( | ||
| os.getenv("AGENTA_AGENT_MCP_SERVERS_ENABLED", "true").strip().lower() in TRUTHY |
There was a problem hiding this comment.
🤖 The AI agent says: This is the whole behavior change: the default moved from "" (off) to "true" (on). An unset AGENTA_AGENT_MCP_SERVERS_ENABLED now enables user MCP. To turn it back off, set the flag to any falsy value. Everything else in this PR is the matching deployment defaults.
| * - reject loopback, link-local (incl. the `169.254.169.254` cloud metadata host), and private | ||
| * address literals unless the host is in `AGENTA_AGENT_MCP_HOST_ALLOWLIST` (comma-separated). | ||
| * | ||
| * RESIDUAL (tracked in #4911): the guard does NOT pin DNS resolution to the validated host and |
There was a problem hiding this comment.
🤖 The AI agent says: This is the residual behind the decision. The guard validates scheme + host but does not pin DNS resolution or re-validate redirects, so DNS rebinding or an https->internal redirect can still reach an internal sink. Bounded (secrets ride headers only to https hosts) but real now that the flag defaults on. Hardening is tracked in #4911.
|
🤖 The AI agent says: triggering an automated review. @coderabbitai review |
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d9caf658-3bfc-47eb-886c-2b00d46ecbd0
📒 Files selected for processing (19)
hosting/docker-compose/ee/docker-compose.dev.ymlhosting/docker-compose/ee/docker-compose.gh.local.ymlhosting/docker-compose/ee/docker-compose.gh.ymlhosting/docker-compose/ee/env.ee.dev.examplehosting/docker-compose/ee/env.ee.gh.examplehosting/docker-compose/oss/docker-compose.dev.ymlhosting/docker-compose/oss/docker-compose.gh.local.ymlhosting/docker-compose/oss/docker-compose.gh.ssl.ymlhosting/docker-compose/oss/docker-compose.gh.ymlhosting/docker-compose/oss/env.oss.dev.examplehosting/docker-compose/oss/env.oss.gh.examplehosting/kubernetes/ee/values.ee.example.yamlhosting/kubernetes/helm/templates/_helpers.tplhosting/kubernetes/helm/values.yamlhosting/kubernetes/oss/values.oss.example.yamlhosting/railway/oss/scripts/configure.shservices/agent/src/engines/sandbox_agent/mcp.tsservices/oss/src/agent/tools/resolver.pyservices/oss/tests/pytest/unit/agent/tools/test_resolution.py
| DOCKER_NETWORK_MODE: ${DOCKER_NETWORK_MODE:-bridge} | ||
| AGENTA_AGENT_RUNNER_URL: ${AGENTA_AGENT_RUNNER_URL:-http://sandbox-agent:8765} | ||
| AGENTA_AGENT_MCP_SERVERS_ENABLED: ${AGENTA_AGENT_MCP_SERVERS_ENABLED:-false} | ||
| AGENTA_AGENT_MCP_SERVERS_ENABLED: ${AGENTA_AGENT_MCP_SERVERS_ENABLED:-true} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Don't enable user MCP by default in self-hosted compose until the runner hardening lands.
Line 456 turns this OSS compose stack into opt-out for author-controlled HTTP MCP even though the PR notes the runner guard still lacks DNS pinning and redirect re-validation. In this same topology, services already gets host.docker.internal:host-gateway, so the residual SSRF path becomes reachable on default installs instead of only on explicit opt-in. Keep the self-hosted compose/env default false until the runner fix ships.
Suggested minimal mitigation
- AGENTA_AGENT_MCP_SERVERS_ENABLED: ${AGENTA_AGENT_MCP_SERVERS_ENABLED:-true}
+ AGENTA_AGENT_MCP_SERVERS_ENABLED: ${AGENTA_AGENT_MCP_SERVERS_ENABLED:-false}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| AGENTA_AGENT_MCP_SERVERS_ENABLED: ${AGENTA_AGENT_MCP_SERVERS_ENABLED:-true} | |
| AGENTA_AGENT_MCP_SERVERS_ENABLED: ${AGENTA_AGENT_MCP_SERVERS_ENABLED:-false} |
| - SCRIPT_NAME=/services | ||
| - AGENTA_AGENT_RUNNER_URL=${AGENTA_AGENT_RUNNER_URL:-http://sandbox-agent:8765} | ||
| - AGENTA_AGENT_MCP_SERVERS_ENABLED=${AGENTA_AGENT_MCP_SERVERS_ENABLED:-false} | ||
| - AGENTA_AGENT_MCP_SERVERS_ENABLED=${AGENTA_AGENT_MCP_SERVERS_ENABLED:-true} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Keep MCP disabled by default in the OSS compose stack. Defaulting AGENTA_AGENT_MCP_SERVERS_ENABLED to true exposes self-hosted installs to the MCP SSRF surface before the HTTP guard hardening lands.
| {{- end }} | ||
| - name: AGENTA_AGENT_MCP_SERVERS_ENABLED | ||
| value: {{ default false $runner.enableMcp | quote }} | ||
| value: {{ default true $runner.enableMcp | quote }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve explicit false for enableMcp
default treats false as empty here, so agentRunner.enableMcp: false still renders "true". Use a nil/hasKey check instead if the chart needs to support disabling MCP.
| # On by default: an unset flag enables user MCP servers. http servers run behind the | ||
| # runner's https + SSRF guards; stdio servers still fail loud (disabled by design). Set | ||
| # the flag to a falsy value to turn user MCP servers off entirely. | ||
| return ( | ||
| os.getenv("AGENTA_AGENT_MCP_SERVERS_ENABLED", "true").strip().lower() in TRUTHY | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Keep the unset default opt-in until the runner hardening lands.
services/agent/src/engines/sandbox_agent/mcp.ts:44-48 now explicitly documents that DNS rebinding and HTTPS→internal redirects can still bypass the current guard. Making _mcp_enabled() return True when the flag is unset exposes that residual in fresh OSS/self-hosted installs that never opted into user MCP. Please keep the unset default falsy here (or scope default-on to controlled environments only) until that hardening ships.
…f-by-design) Recut of the stale #4912 against the current tree (new flag name AGENTA_AGENT_MCPS_ENABLED, post runner-rename). Unset now means enabled; set the flag to false to disable. What default-on activates: http-only user MCP, Claude-harness-only, behind the scheme+host-literal SSRF guard. Owner ruling: the DNS-rebinding/redirect hardening stays tracked in #4911, out of MVP scope. - resolver.py: _mcp_enabled() defaults to true; docstrings state the new default and the #4911 residual. - 7 compose files + railway + helm _helpers.tpl: :-false -> :-true. - 4 env examples: documented default flipped (kept commented). - Helm example values: re-add the minimal commented agentRunner block that a prior cleanup deleted (values.schema.json still documents the shape, but self-hosters had no example line to uncomment). - tests: new test_mcp_enabled_by_default_when_unset; the disabled-path tests now set the flag to false explicitly. - running-the-agent.md: stop claiming default false. Claude-Session: https://claude.ai/code/session_01HhBEUFbXETNjYdcz71AGrT
mmabrouk
force-pushed
the
feat/mcp-user-servers-default-on
branch
from
f209643 to
7003fe5
Compare
|
Ready for review. What I need from you: (1) OK the default-on-with-#4911-open framing as written in the resolver docstring, and (2) OK the re-added commented agentRunner example block in the three Helm values files (a cleanup had deleted it; without it self-hosters have no line to uncomment). Everything else is one-line default flips. Tests: 6/6 on the touched file. |
1 participant
What this changes
User-declared MCP servers now resolve by default. Before, every deployment shipped with
AGENTA_AGENT_MCPS_ENABLED=false, so an agent config that declaredmcpsfailed withMCPDisabledErroruntil the operator found and flipped the flag. After, the same config works out of the box; operators set the flag tofalseto opt out.What default-on actually enables (scope check)
validateUserMcpUrlrequires https and rejects loopback, link-local, metadata, and private hosts, withAGENTA_AGENT_MCPS_HOST_ALLOWLISTas the escape hatch.Owner ruling baked in: the guard's residual (DNS rebinding, redirect re-validation) stays tracked in #4911 and is out of MVP scope. The resolver docstrings and docs now say so explicitly.
This is a recut, not a rebase
The original branch was 516 commits behind and predated two renames (
AGENTA_AGENT_MCP_SERVERS_ENABLED->AGENTA_AGENT_MCPS_ENABLED,services/agent/->services/runner/). The branch was recut as one commit against currentbig-agents; the old history is gone by design.Review guide (one commit, 19 files, ~5 minutes)
services/oss/src/agent/tools/resolver.pyis the only behavior change:_mcp_enabled()defaults to"true". Everything else is deployment defaults and docs following it._helpers.tpl: one-linefalse->trueflips.true.agentRunner:block. A prior cleanup deleted the whole block, so self-hosters had no example to uncomment even thoughvalues.schema.jsonstill documents the shape.test_mcp_enabled_by_default_when_unset; the two disabled-path tests now setfalseexplicitly.6 passed.running-the-agent.mdstops claiming defaultfalse. The other design docs that mention the default are being edited on the feat(agent): deliver client tools to Claude over the internal MCP channel #4985 lane, so they get one docs pass after both merge (avoids a cross-lane conflict).How to QA
Unset the flag, declare an https MCP server with a vault secret, run on Claude: the server resolves and its tools are callable. Set the flag to
false: the same config fails loud withMCPDisabledError.https://claude.ai/code/session_01HhBEUFbXETNjYdcz71AGrT