Skip to content

Add support for DNS rebinding protections #284

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add support for DNS rebinding protections #284

wants to merge 3 commits into from
+923 −9

Conversation

ddworken
Copy link

Motivation and Context

This implements the mitigations described here. To avoid breaking existing applications this doesn't enable any changes by-default, but enabling this feature is heavily encouraged for any local MCP servers using the SSE transport.

How Has This Been Tested?

Tested via unit tests.

Breaking Changes

To avoid introducing any breaking changes, the DNS rebinding protections are disabled by default. Ideally we should find a way to enable them by default. But for now, adding them as disabled is a good first step.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

@ddworken ddworken marked this pull request as ready for review May 30, 2025 17:01
@chemicL chemicL self-assigned this Jun 11, 2025
chemicL
chemicL requested changes Jun 17, 2025
View reviewed changes
Copy link
Member

@chemicL chemicL left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR. I did the first round by going through the code in the mcp module. I believe the comments would apply also to the mcp-spring modules. Some questions, some suggestions here and there, but the most important themes are:

  1. HTTP(2) protocol compliance - will how to make it work when there is :authority pseudo-header instead of Host?
  2. MCP Spec compliance - does Host require checking? The spec only mentions Origin.
  3. Since validation is enforced by the spec as a MUST i do feel we should enable validation with a breaking change. @tzolov looking forward to your input here as well.
  4. Builder pattern consistency.
  5. Javadocs improvements and filling the gaps.
  6. Removing code duplication.

Thanks in advance for a follow-up and the work so far!

.../test/java/io/modelcontextprotocol/server/transport/HttpServletSseHeaderValidationTests.java Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check AbstractMcpAsyncServerTests and the implementations. Do you think we could be able to have a generic test suite with just the transport-specific settings in the inheriting classes in the same fashion?

@chemicL
Copy link
Member

chemicL commented Jun 17, 2025

Ah, please also remember to run spring-javaformat:apply before commiting to ensure the formatting checks pass.

jgrandja
jgrandja reviewed Jun 23, 2025
View reviewed changes
mcp/src/main/java/io/modelcontextprotocol/server/transport/DnsRebindingProtectionConfig.java Outdated

private final Set<String> allowedHosts = new HashSet<>();

private final Set<String> allowedOrigins = new HashSet<>();
Copy link

@jgrandja jgrandja Jun 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though the spec states:

Servers MUST validate the Origin header on all incoming connections to prevent DNS rebinding attacks

It doesn't make sense to me because Origin is NOT required and therefore can be empty depending on the type of request and browser behaviour.

I don't think validating the Origin header is valid. We should only have to validate the Host header.

DNS rebinding is all about changing the underlying IP address associated to a domain name (Host). For example, http://example.com initially points to 34.192.228.43 but can be dynamically changed to a private internal address of 192.168.1.77 enabled the attack on the internal network. For a more detailed description, check out this article.

In summary, the MCP server that receives the request should validate that it’s own host matches the host being requested.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the MCP server that receives the request should validate that it’s own host matches the host being requested.

I agree that this is the most critical part of DNS rebinding protection, though it is pretty common to also check the Origin header since it provides another layer of defense and also mitigates a variety of other web-based attacks.

It doesn't make sense to me because Origin is NOT required and therefore can be empty depending on the type of request and browser behaviour.

I don't think validating the Origin header is valid. We should only have to validate the Host header.

Modern browsers send the Origin header for ~all cross-origin requests with the main exceptions being simple GET requests and navigational requests. So for the purposes of MCP, I believe it is valid and should always be set.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ddworken

though it is pretty common to also check the Origin header

Can you please provide references to other MCP implementations that validate the Origin header so I can further investigate.

and also mitigates a variety of other web-based attacks

What other web-based attacks are you referring to?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please provide references to other MCP implementations that validate the Origin header so I can further investigate.

We also are hoping to enable this for other MCP implementations too.

and also mitigates a variety of other web-based attacks

What other web-based attacks are you referring to?

E.g. CSRF attacks. If an MCP server accepts simple requests then it can be CSRF attacked, and validating the Origin header blocks this.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 2x references you provided are PR's submitted by you and only merged last month. Do you have other references where the Origin header is validated to mitigate DNS Rebinding attack?

E.g. CSRF attacks. If an MCP server accepts simple requests then it can be CSRF attacked, and validating the Origin header blocks this.

This is not correct. The recommended mitigation for CSRF attack are the use of tokens. See token-based mitigation.

@tzolov tzolov mentioned this pull request Jun 26, 2025
Open
9 tasks
ddworken and others added 2 commits June 26, 2025 17:03
@ddworken @claude
Extract DNS rebinding validation logic to separate methods in Spring …
7d44740 
…transport providers

- Add validateDnsRebindingProtection() method to WebFluxSseServerTransportProvider
- Add validateDnsRebindingProtection() method to WebMvcSseServerTransportProvider
- Remove duplicated validation logic from GET and POST endpoints
- Maintain framework-specific return types (Mono<ServerResponse> vs ServerResponse)
- Apply Spring Java formatting to all modules

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@ddworken
Add javadocs
145b592
@ddworken
Copy link
Author

Since validation is enforced by the spec as a MUST i do feel we should enable validation with a breaking change. @tzolov looking forward to your input here as well.

I'll say I think there is a pretty good argument for enabling validation by default, but since that is a breaking change I think it ought to:

  1. Be deferred to a separate PR
  2. Be done in a unified way across the MCP ecosystem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jgrandja jgrandja jgrandja left review comments

@chemicL chemicL chemicL requested changes

Assignees

@chemicL chemicL

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ddworken @chemicL @jgrandja