fix(mcp): add SSRF protection for HTTP/SSE connections - #317
Conversation
|
Warning Review limit reached
Next review available in: 39 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughMCP HTTP and SSE connections now validate URLs, resolve DNS safely, and use protected fetch dispatching. Tests cover public URLs, restricted addresses, DNS results, dispatcher use, and transport options. ChangesMCP URL safety
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MCPConnection as MCP connection
participant SafeFetch as createSafeMcpFetch
participant Dispatcher as safe MCP dispatcher
participant DNS as DNS resolver
MCPConnection->>SafeFetch: Send HTTP or SSE request
SafeFetch->>Dispatcher: Use protected dispatcher
Dispatcher->>DNS: Resolve and validate hostname
DNS-->>Dispatcher: Return validated addresses
Dispatcher-->>SafeFetch: Establish allowed connection
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/test/mcp/connections.test.ts (1)
6-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExecute validation by awaiting
connect()for accepted public URLs.
validateMcpUrl()runs insideconnect(), so the current public HTTPS/HTTP tests pass when only the connection object is synchronously returned. Forhttp()andsse(), mockClient.connect()/the SDK transport response and awaitconnection.connect(). This exercises the URL validator and remains network-safe.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/test/mcp/connections.test.ts` around lines 6 - 16, Update the public URL tests in packages/core/test/mcp/connections.test.ts at lines 6-16 and 104-108 to mock Client.connect() and the SDK transport response, then await connection.connect() for both http() and sse() connections. Ensure validation runs through connect() without making real network requests; both sites require this change.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/mcp/connections.ts`:
- Around line 103-128: Reorder the hostname validation in the MCP URL check so
cloud metadata endpoints are detected before generic private IPv4/IPv6 ranges,
including applying the same ordering to the normalized fd00:ec2::254 address.
Preserve the category-specific cloud-metadata error assertions in
packages/core/test/mcp/connections.test.ts at lines 66-74 and 122-127; those
test sites require no direct changes.
---
Nitpick comments:
In `@packages/core/test/mcp/connections.test.ts`:
- Around line 6-16: Update the public URL tests in
packages/core/test/mcp/connections.test.ts at lines 6-16 and 104-108 to mock
Client.connect() and the SDK transport response, then await connection.connect()
for both http() and sse() connections. Ensure validation runs through connect()
without making real network requests; both sites require this change.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f2beaf3e-8b28-4614-893e-646174744b38
📒 Files selected for processing (2)
packages/core/src/mcp/connections.tspackages/core/test/mcp/connections.test.ts
- Move cloud metadata endpoint checks before generic private IP range checks - This ensures 169.254.169.254 gets 'cloud metadata' error, not 'private IP' error - Add IPv6 bracket handling since URL.hostname keeps brackets ([fe80::1]) - Update old integration tests to use public URLs instead of localhost - All tests passing Addresses CodeRabbit feedback on PR anvia-hq#317
|
Thanks a lot for the PR! I will check back in few days since i am currently busy for other features! |
|
You are welcome! |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/safe-mcps-listen.md:
- Around line 5-6: Update the changeset entry to remove the claim that redirects
and OAuth metadata endpoints are protected, and describe only the validated
connection URL behavior currently implemented. Do not add implementation changes
unless redirect-target validation is actually being introduced elsewhere.
In `@packages/core/src/mcp/url-safety.ts`:
- Around line 142-148: Enforce protected networking for custom MCP fetch
implementations by updating createSafeMcpFetch and both Streamable HTTP and SSE
connection paths in packages/core/src/mcp/connections.ts (lines 34-41 and 53-61)
to reject custom transport.fetch values or route them through a controlled
dispatcher-aware fetch; apply the same policy at
packages/core/src/mcp/url-safety.ts (lines 142-148). Add coverage in
packages/core/test/mcp/connections.test.ts (lines 92-109) confirming custom
fetches cannot bypass private-address and redirect protections for either
connection type.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 03a3993f-f394-4ebd-a7e5-a36401307214
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
.changeset/safe-mcps-listen.mdpackages/core/package.jsonpackages/core/src/mcp/connections.tspackages/core/src/mcp/url-safety.tspackages/core/test/mcp-connections.test.tspackages/core/test/mcp/connections.test.ts
Validate and pin DNS for MCP HTTP/SSE requests, redirects, and OAuth metadata requests. Reject custom transport fetch implementations that could bypass the protected Undici dispatcher. Co-authored-by: Indra Zulfi Mushoddaq <me@indrazm.com>
8012c5a to
bd5f1e7
Compare
2 participants
Summary
Adds URL validation to MCP HTTP and SSE connections to prevent Server-Side Request Forgery (SSRF) attacks.
MCP servers extend Anvia agents with external tools via user-provided URLs. Without validation, these URLs could point to internal services: localhost endpoints, private network ranges (10.x, 192.168.x), or cloud metadata APIs (169.254.169.254 on AWS/GCP/Azure). This would let an attacker access internal infrastructure, pull IAM credentials, or reach admin panels through the agent's network context.
What Changed
Added
validateMcpUrl()topackages/core/src/mcp/connections.ts. It runs beforemcp.http()andmcp.sse()try to connect.Blocked patterns:
localhost,127.0.0.1,127.x.x.x,::1,[::1]10.0.0.0/8,172.16.0.0/12,192.168.0.0/16,169.254.0.0/16fe80::/10(link-local),fc00::/7andfd00::/8(unique local)169.254.169.254,fd00:ec2::254Not blocked:
mcp.stdio()connections (they spawn local processes, no URL involved)Error messages:
Each blocked category gets its own error:
Why These Ranges
Localhost (127.x.x.x, ::1):
Blocks services running on the same machine. Someone could reach a local Redis instance, a dev database, an admin panel bound to 127.0.0.1.
Private IPv4 (10.x, 172.16-31.x, 192.168.x):
RFC 1918 ranges for internal networks. If your agent runs inside a corporate network or VPC, these addresses reach internal infrastructure. Blocking them stops lateral movement: agent to database, agent to billing system, agent to internal tools.
Link-local (169.254.x.x):
Auto-configured local networks. More importantly: 169.254.169.254 is where AWS, GCP, and Azure serve metadata—IAM credentials, SSH keys, instance config. Hit that endpoint from a compromised agent and you leak everything.
Private IPv6 (fe80::, fc00::, fd00::):
Same idea, IPv6 version. Link-local (
fe80::) reaches local network devices. Unique local (fc00::,fd00::) works like private IPv4.Testing
Added 20+ test cases in
packages/core/test/mcp/connections.test.ts:Valid cases (should work):
https://api.example.com/mcpBlocked cases (should throw):
http://localhost:3000/mcphttp://127.0.0.1:3000/mcphttp://127.1.2.3:3000/mcp(any 127.x address)http://[::1]:3000/mcp(IPv6 localhost)http://10.0.0.1/mcp(private class A)http://172.16.0.1/mcp(private class B)http://192.168.1.1/mcp(private class C)http://169.254.1.1/mcp(link-local)http://169.254.169.254/latest/meta-data/(AWS metadata)http://[fe80::1]/mcp(IPv6 link-local)http://[fc00::1]/mcp(IPv6 unique local)http://[fd00::1]/mcp(IPv6 unique local alternate)not-a-url(invalid URL format)stdio() still works:
mcp.stdio()connections don't involve URLs, so validation doesn't applyAll tests validate both
mcp.http()andmcp.sse()since they share the same validation logic.What This Doesn't Fix
DNS rebinding:
An attacker serves a domain that resolves to a public IP, passes validation, then rebinds to 127.0.0.1 after the check. Requires controlling DNS and perfect timing. Full mitigation would mean re-validating on every redirect or using a separate resolver. Out of scope here.
IPv6 edge cases:
More exotic ranges exist (site-local, deprecated addresses). Current check covers common attack vectors. If someone reports a bypass, we'll add it.
Open redirects:
If
https://legit-mcp.comredirects tohttp://169.254.169.254, this won't catch it. You'd need to validate redirect targets in the HTTP client. Deeper change.Prompt injection:
If an attacker injects text that reconfigures MCP connections mid-run, they might reach a blocked URL. This assumes MCP URLs come from startup config or trusted sources. If your threat model includes prompt-injected URLs, validate upstream.
Breaking Changes
This blocks connections to localhost and private IP ranges. If you were using
mcp.http()ormcp.sse()with local URLs, you'll need to:mcp.stdio()(recommended—spawns the server as a child process)An opt-out flag (
allowPrivateIPs: true) can be added if needed, but I'd prefer to see real use cases first.Validation Commands Run
pnpm --filter @anvia/core typecheck pnpm --filter @anvia/core test connections.test pnpm checkAll tests pass. No other packages affected.
Summary by CodeRabbit
Security
Bug Fixes