[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
…esign) User MCP servers were gated off by default via AGENTA_AGENT_MCP_SERVERS_ENABLED. Flip the default to on in the service resolver and across every deployment default (docker-compose, env examples, Helm chart + examples, Railway). http user MCP runs behind the runner's https + SSRF guards; stdio user MCP still fails loud (disabled by design), so flipping the default does not expose stdio. The runner SSRF guard is scheme+host only; the residual hardening (DNS-rebinding pinning + redirect re-validation) is tracked separately. Refs #4911 Claude-Session: https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc
dosubot
Bot
added
the
size:M
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
Walkthrough
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.
1 participant
Context
Mahmoud's direction: "MCP should not be off by default." User-declared MCP servers were gated behind
AGENTA_AGENT_MCP_SERVERS_ENABLED, which defaulted off. This PR flips the default to on.The flag gates only user-declared
mcpServers. The internal agenta-tools channel was never gated, so it is unaffected. http user MCP is verified working end-to-end behind the runner's https + SSRF guards. stdio user MCP still fails loud for both harnesses, so flipping the default does not expose stdio.What changed
services/oss/src/agent/tools/resolver.py—_mcp_enabled()now reads the flag with a"true"default, so an unset flag enables user MCP.${AGENTA_AGENT_MCP_SERVERS_ENABLED:-false}->:-trueenv.*.examplefiles:# AGENTA_AGENT_MCP_SERVERS_ENABLED=false->=truevalues.yamlenableMcp: false->true,_helpers.tpldefault false->default true, plus the oss/eevalues.*.example.yamlcommented examplesoss/scripts/configure.sh:-false->:-trueservices/agent/src/engines/sandbox_agent/mcp.ts— docstring updated to "on by default" and now spells out the SSRF residual.services/oss/tests/pytest/unit/agent/tools/test_resolution.py— a new test asserts_mcp_enabled()isTruewhen the flag is unset; the two disabled-path tests now set the flagfalseexplicitly.A self-hoster disables user MCP by setting
AGENTA_AGENT_MCP_SERVERS_ENABLEDto a falsy value (oragentRunner.enableMcp: falseon Helm).Scope / risk
validateUserMcpUrlinservices/agent/src/engines/sandbox_agent/mcp.ts) is scheme + host only — no DNS-rebinding pinning, no redirect re-validation. With user MCP now on by default, an author-controlled config could reach an internal host via DNS rebinding or an https->internal redirect. Secrets ride headers only to https hosts, so the blast radius is bounded, but it is a real residual for an OSS default-on. Tracked in (feat) Harden user-MCP http SSRF guard (DNS rebinding + redirects) for OSS default-on #4911 (AGE-3843). Decision needed: harden the guard first, or ship default-on with the residual tracked.How to QA
Prereqs: a deployment with the flag UNSET (so the new default applies), a Claude harness, and a project that holds the MCP server's named secret in the vault.
AGENTA_AGENT_MCP_SERVERS_ENABLEDunset, run a Claude agent whose config declares an https user MCP server (transport: "http", aurl, and a named secret).MCPDisabledError.command).AGENTA_AGENT_MCP_SERVERS_ENABLED=falseand repeat step 1.MCPDisabledErrornaming the server. The disable path still works.Unit test:
cd services && uv run --no-sync python -m pytest oss/tests/pytest/unit/agent/tools/test_resolution.py -q-> 6 passed.Edge cases: empty
mcpServerswith the flag on -> clean empty list; an http url that fails the SSRF guard (non-https or internal host) -> throws before any credential is attached.Config reference
resolver._mcp_enabled()defaultos.getenv(..., "")(off)os.getenv(..., "true")(on)${AGENTA_AGENT_MCP_SERVERS_ENABLED:-false}:-trueenv.*.example(x4)# AGENTA_AGENT_MCP_SERVERS_ENABLED=false# ...=trueagentRunner.enableMcpfalsetrueconfigure.sh:-false:-trueRefs #4911
https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc