Add support for DNS rebinding protections #284
Conversation
There was a problem hiding this comment.
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:
- HTTP(2) protocol compliance - will how to make it work when there is
:authoritypseudo-header instead ofHost? - MCP Spec compliance - does
Hostrequire checking? The spec only mentionsOrigin. - 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.
- Builder pattern consistency.
- Javadocs improvements and filling the gaps.
- Removing code duplication.
Thanks in advance for a follow-up and the work so far!
|
|
||
| private final Set<String> allowedOrigins; | ||
|
|
||
| private final boolean enableDnsRebindingProtection; |
There was a problem hiding this comment.
| private final boolean enableDnsRebindingProtection; | |
| private final boolean enabled; |
|
|
||
| private final boolean enableDnsRebindingProtection; | ||
|
|
||
| private DnsRebindingProtectionConfig(Builder builder) { |
There was a problem hiding this comment.
Consider passing explicit arguments instead of the builder. Should the builder change, and it is mutable, this constructor could create instances that are in inconsistent state. The pattern present throughout the codebase is rather to pass explicit parameters instead of this cyclic dependency.
| return new Builder(); | ||
| } | ||
|
|
||
| public static class Builder { |
There was a problem hiding this comment.
In order to avoid having two means to instantiate the Builder, the constructor should be private. A static factory exists and it is the pattern that's used by this project.
| return true; | ||
| } | ||
|
|
||
| public static Builder builder() { |
| return new Builder(); | ||
| } | ||
|
|
||
| public static class Builder { |
There was a problem hiding this comment.
Please add javadocs to the public class and its methods.
| @@ -475,6 +527,8 @@ public static class Builder { | |||
|
|
|||
| private String sseEndpoint = DEFAULT_SSE_ENDPOINT; | |||
|
|
|||
| private DnsRebindingProtectionConfig dnsRebindingProtectionConfig; | |||
There was a problem hiding this comment.
Shouldn't the default behaviour be injected always unless the user specifically overrides it?
| @@ -202,6 +222,18 @@ protected void doGet(HttpServletRequest request, HttpServletResponse response) | |||
| return; | |||
| } | |||
|
|
|||
| // Validate headers if DNS rebinding protection is configured | |||
| if (dnsRebindingProtectionConfig != null) { | |||
| String hostHeader = request.getHeader("Host"); | |||
There was a problem hiding this comment.
To make it work with HTTP2, shouldn't https://jakarta.ee/specifications/servlet/6.0/apidocs/jakarta.servlet/jakarta/servlet/servletrequest#getServerName() be used instead?
There was a problem hiding this comment.
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?
| * Configuration for DNS rebinding protection in SSE server transports. Provides | ||
| * validation for Host and Origin headers to prevent DNS rebinding attacks. | ||
| */ | ||
| public class DnsRebindingProtectionConfig { |
There was a problem hiding this comment.
DnsRebindingProtectionConfig -> DnsRebindingProtection.
I view this more as an entity that has behaviour via the validate() method, not just a set of configurations, so I'd skip the "Config" part.
| * @param originHeader The value of the Origin header (may be null) | ||
| * @return true if the headers are valid, false otherwise | ||
| */ | ||
| public boolean validate(String hostHeader, String originHeader) { |
There was a problem hiding this comment.
| public boolean validate(String hostHeader, String originHeader) { | |
| public boolean isValid(String hostHeader, String originHeader) { |
There was a problem hiding this comment.
I would expect a validate method to throw an exception or have some effect instead. Since the return is a boolean, I think it's more reasonable to call the method isValid.
|
Ah, please also remember to run |
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
Checklist
Additional context