fix: guard Streamable HTTP host headers#555
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds DNS rebinding protection for the Streamable HTTP ChangesMCP DNS Rebinding Protection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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: 3
🤖 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 `@docs/configuration/environment-variables.md`:
- Around line 393-396: The documentation describing the Host and Origin header
rejection behavior uses confusing multiple negative statements that require
re-reading. Rewrite the paragraph starting with "Streamable HTTP rejects `/mcp`
requests..." to use positive language stating what IS allowed (local addresses,
MCP_SERVER_URL, and listed hosts/origins) rather than negative language stating
what is NOT allowed. Apply the same positive approach to the Origin header
description that follows, making it clear and direct about what values are
acceptable rather than what is rejected.
In `@test/streamable-http-dns-rebinding.test.ts`:
- Around line 57-70: Add a new test case that calls postMcp with a malformed
JSON body (instead of valid JSON) to verify that the Host/Origin rejection
occurs before JSON parsing is attempted. Create this additional test alongside
the existing forged Host/Origin checks to demonstrate that both valid and
invalid JSON payloads are rejected at the Host/Origin validation stage,
asserting that the same 403 error is returned regardless of payload validity.
- Around line 8-9: The HOST variable can contain IPv6 addresses like ::1, which
require square brackets when combined with a port number to create valid
HOST:PORT formats. Create a helper function that checks if the HOST contains
colons (indicating IPv6) and wraps it in square brackets before concatenating
with the port, then apply this function at all locations where HOST:port string
interpolation occurs (around lines 46-47, 80, and 115) to ensure proper URL and
Host header formatting regardless of whether HOST is an IPv4 or IPv6 address.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6f76082d-aeb5-4edb-968e-d4fa407847a1
📒 Files selected for processing (5)
README.mddocs/configuration/environment-variables.mdindex.tspackage.jsontest/streamable-http-dns-rebinding.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: test
- GitHub Check: Cursor Bugbot
🧰 Additional context used
🪛 Betterleaks (1.5.0)
test/streamable-http-dns-rebinding.test.ts
[high] 10-10: Identified a GitLab Personal Access Token, risking unauthorized access to GitLab repositories and codebase exposure.
(gitlab-pat)
🔇 Additional comments (8)
README.md (1)
276-276: LGTM!Also applies to: 327-328, 481-481
test/streamable-http-dns-rebinding.test.ts (1)
14-38: LGTM!Also applies to: 40-55, 72-107, 124-146
package.json (1)
54-54: LGTM!index.ts (5)
958-1051: LGTM!
1118-1131: LGTM!
12793-12801: LGTM!
12847-12848: LGTM!
13164-13166: LGTM!
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/configuration/environment-variables.md (1)
398-407: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider documenting that invalid entries in MCP_ALLOWED_HOSTS and MCP_ALLOWED_ORIGINS cause startup failure.
The PR objectives mention "startup validation to detect invalid entries," and the code (snippet 3) shows that the server validates comma-separated entries and rejects invalid hosts/origins at startup. However, the documentation does not mention this validation or its consequences. Users might otherwise assume that invalid entries are silently ignored.
Consider adding a note such as: "Invalid entries will cause startup failure with an error message indicating the problematic value."
🤖 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 `@docs/configuration/environment-variables.md` around lines 398 - 407, The documentation sections for MCP_ALLOWED_HOSTS and MCP_ALLOWED_ORIGINS are missing information about validation behavior. Add a note to both the MCP_ALLOWED_HOSTS and MCP_ALLOWED_ORIGINS environment variable sections documenting that the server validates entries at startup and that invalid entries (such as malformed hosts or origins) will cause the server to fail startup with an error message indicating the problematic value. This clarifies that invalid entries are not silently ignored and helps users understand the validation consequences.
🤖 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.
Outside diff comments:
In `@docs/configuration/environment-variables.md`:
- Around line 398-407: The documentation sections for MCP_ALLOWED_HOSTS and
MCP_ALLOWED_ORIGINS are missing information about validation behavior. Add a
note to both the MCP_ALLOWED_HOSTS and MCP_ALLOWED_ORIGINS environment variable
sections documenting that the server validates entries at startup and that
invalid entries (such as malformed hosts or origins) will cause the server to
fail startup with an error message indicating the problematic value. This
clarifies that invalid entries are not silently ignored and helps users
understand the validation consequences.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 95c6890f-e6f5-4818-8165-4bce1bb4acfe
📒 Files selected for processing (2)
docs/configuration/environment-variables.mdtest/streamable-http-dns-rebinding.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: coverage
- GitHub Check: test
🔇 Additional comments (3)
test/streamable-http-dns-rebinding.test.ts (1)
10-18: LGTM!Also applies to: 52-52, 65-66, 75-76, 85-85, 120-131
docs/configuration/environment-variables.md (2)
393-396: ✓ Grammatical clarity improved per previous feedback.The documentation now uses positive language ("allows... when...") to describe the allowlisting behavior, directly addressing the prior review's suggestion to state what IS allowed rather than what is NOT allowed. The parallel structure for Host and Origin is clear.
393-407: The documentation accurately reflects the implementation. All three verification points are confirmed:
- Loopback addresses (
127.0.0.1,localhost,::1) are explicitly added toallowedHostsandallowedOriginsat startup (lines 1006-1009 ofgetMcpDnsRebindingProtection()).MCP_SERVER_URLis correctly parsed and added to both allowlists viaaddHost()andaddOrigin()(lines 1012-1015).- Normalization rules are properly implemented:
formatHostWithPort()handles IPv6 bracket conversion and port formatting, whiletoAllowedMcpHost()andtoAllowedMcpOrigin()normalize values through URL parsing and lowercasing before checking membership in the allowlists.
1 participant
Summary
/mcpbefore JSON parsingMCP_ALLOWED_HOSTS/MCP_ALLOWED_ORIGINSand add regression testsAddresses GHSA-vmp7-252j-cwp7.
Tests
npm run buildnode --import tsx/esm --test test/streamable-http-dns-rebinding.test.tsNote
Medium Risk
Touches the public
/mcprequest path; wrong allowlist config can block legitimate clients, but the change closes a security issue (GHSA-vmp7-252j-cwp7).Overview
Adds DNS rebinding protection for Streamable HTTP by validating
HostandOriginon/mcpbefore JSON parsing, returning 403 when headers are not on an allowlist.Allowlists are built from loopback/
HOST:PORT,MCP_SERVER_URL, and optionalMCP_ALLOWED_HOSTS/MCP_ALLOWED_ORIGINS. The same lists are passed intoStreamableHTTPServerTransportvia the MCP SDK’s rebinding options. Startup now rejects invalid entries in those env vars.README and the environment-variables reference document the new settings;
test/streamable-http-dns-rebinding.test.tscovers forged headers andMCP_SERVER_URLacceptance.Reviewed by Cursor Bugbot for commit 6adb624. Bugbot is set up for automated code reviews on this repo. Configure here.