fix(http): add origin/host check#764
fix(http): add origin/host check#764jokemanfire wants to merge 1 commit intomodelcontextprotocol:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds DNS rebinding mitigations to the Streamable HTTP server by validating inbound Host/Origin headers against a configurable allowlist, and introduces an integration test covering allowed vs. blocked host/origin scenarios.
Changes:
- Add
allowed_hoststoStreamableHttpServerConfigwith helpers to configure/disable it. - Enforce
Host/Originvalidation early in request handling, returning403 Forbiddenon violations. - Add an integration test asserting
403for disallowedHostand disallowedOrigin.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/rmcp/src/transport/streamable_http_server/tower.rs | Introduces allowlisted Host/Origin validation for DNS rebinding protection and config knobs to control it. |
| crates/rmcp/tests/test_custom_headers.rs | Adds an integration test to verify Host/Origin enforcement behavior. |
| /// | ||
| /// By default, Streamable HTTP servers only accept loopback hosts to | ||
| /// prevent DNS rebinding attacks against locally running servers. Public | ||
| /// deployments should override this list with their own hostnames. | ||
| pub allowed_hosts: Vec<String>, |
There was a problem hiding this comment.
Adding the new pub allowed_hosts field to StreamableHttpServerConfig is a semver-breaking change for downstream users constructing the config via struct literals (they will now fail to compile). If this is intended, it should be paired with a major version bump / explicit release note; otherwise consider making the config #[non_exhaustive] and/or moving toward a constructor/builder pattern to avoid future breakage.
There was a problem hiding this comment.
This is especially important given that PR #715 was recently merged to prepare for 1.0 stable release. StreamableHttpServerConfig may have been missed in that effort. Adding #[non_exhaustive] here would be consistent with that direction and prevent this class of breakage going forward.
There was a problem hiding this comment.
I think this security update should be introduced in 1.0 version , and I will add the #[non_exhaustive].
| let bad_origin_request = Request::builder() | ||
| .method(Method::POST) | ||
| .header("Accept", "application/json, text/event-stream") | ||
| .header(CONTENT_TYPE, "application/json") | ||
| .header("Host", "localhost:8080") |
There was a problem hiding this comment.
Consider adding a test case for the same-hostname/different-port scenario (e.g., Host: localhost:8080 with Origin: http://localhost:9999) and assert it is rejected. This guards against port-mismatch bypasses of the Host/Origin validation logic.
| Ok(Some(normalize_host(authority.host()))) | ||
| } |
There was a problem hiding this comment.
parse_host_header/parse_origin_host currently discard the port by extracting only authority.host(). That makes the later Host/Origin comparison effectively “hostname-only”, allowing Host: localhost:8080 with Origin: http://localhost:9999 to pass. Consider preserving and comparing the full authority (host + optional port), or at least requiring ports to match when present (accounting for default ports).
DaleSeo
left a comment
There was a problem hiding this comment.
Thank you for tackling DNS rebinding protection, @jokemanfire. This is an important security hardening that the MCP Streamable HTTP transport was missing.
I actually implemented a similar Host validation feature on the consumer side in apollographql/apollo-mcp-server#602, so I very much welcome this change. Once this lands in the SDK, I'd love to remove our application-layer middleware and rely on this built-in protection instead.
I have a few suggestions based on what I learned building the consumer-side implementation.
|
|
||
| let origin = parse_origin_host(headers)?; | ||
| if let Some(origin) = origin.as_deref() { | ||
| if !host_is_allowed(origin, &config.allowed_hosts) { |
There was a problem hiding this comment.
Is it intentional to checking Origin against allowed_hosts here? allowed_hosts answers "what hostnames is this server known as?" while Origin represents the source page that initiated the request. These are different concepts.
For example, consider an MCP server at mcp.example.com being called by a frontend at app.example.com:
allowed_hosts: ["mcp.example.com"]
Host: mcp.example.com // ✅ this IS the server
Origin: http://app.example.com // ❌ rejected, but this is a legitimate caller
From my understanding, restricting which origins can call the server is a CORS concern. For DNS rebinding protection, validating only the Host header should be sufficient.
There was a problem hiding this comment.
Well ,thanks ,I will take some time to resolve this.
| )); | ||
| } | ||
| if let Some(host) = host.as_deref() { | ||
| if origin != host { |
There was a problem hiding this comment.
The Host header identifies the destination server while Origin identifies the source page. They will naturally differ in any cross-origin scenario.
Would you consider removing the Origin validation entirely from this DNS rebinding guard and leaving origin-based restrictions to CORS middleware where they belong?
| fn normalize_host(host: &str) -> String { | ||
| host.trim_matches('[') | ||
| .trim_matches(']') | ||
| .to_ascii_lowercase() | ||
| } |
There was a problem hiding this comment.
normalize_host does not preserve the port, and parse_host_header uses authority.host(), which also strips the port. As a result, both localhost:8000 and localhost:9999 normalize to localhost and pass validation equally. This is significant because multiple services can run on different ports of the same host. Without port validation, a DNS rebinding attack could target a different service on localhost:9999, and the check would pass since localhost is in the allow list. Would you consider adding similar port-aware validation here?
There was a problem hiding this comment.
Yes , It should be considerd.
| } | ||
| } | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
Would it make sense to reject requests that are missing the Host header when allowed_hosts is non-empty?
TypeScript SDK's DNS rebinding guard rejects requests with missing headers as well: https://github.com/modelcontextprotocol/typescript-sdk/blob/main/packages/server/src/server/middleware/hostHeaderValidation.ts
| /// | ||
| /// By default, Streamable HTTP servers only accept loopback hosts to | ||
| /// prevent DNS rebinding attacks against locally running servers. Public | ||
| /// deployments should override this list with their own hostnames. | ||
| pub allowed_hosts: Vec<String>, |
There was a problem hiding this comment.
This is especially important given that PR #715 was recently merged to prepare for 1.0 stable release. StreamableHttpServerConfig may have been missed in that effort. Adding #[non_exhaustive] here would be consistent with that direction and prevent this class of breakage going forward.
This is a security problem ,there is a lack of cross-domain access verification here, which may be exploited by malicious attacks.
Motivation and Context
Adds Host/Origin validation to the Streamable HTTP server to mitigate DNS rebinding risks. Introduces configurable allowed hosts, rejects invalid or mismatched request origins, and adds integration tests for both valid localhost traffic and malicious request scenarios.
How Has This Been Tested?
Add the it test
Breaking Changes
Yes, the sec change may change the mcp's server feature which has been deployed.
Types of changes
Checklist
Additional context