[Server] Add DNS rebinding protection feature with middleware#260
[Server] Add DNS rebinding protection feature with middleware#260chr-hertel wants to merge 2 commits intomainfrom
Conversation
Use immutable withBody()/withHeader() pattern instead of mutable stream write, and add missing Content-Type: text/plain header on 403 responses. Aligns error response style with other middleware. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds a server-side HTTP middleware to mitigate DNS rebinding attacks by validating Host and Origin headers against an allowlist, and wires it into the conformance test server to satisfy the conformance suite requirement.
Changes:
- Introduces
DnsRebindingProtectionMiddlewareforHost/Originallowlist validation (defaulting to localhost variants). - Adds unit tests covering allow/deny behavior, case-insensitivity, and custom allowlists.
- Enables the middleware in the conformance server and updates docs/changelog accordingly.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/Server/Transport/Http/Middleware/DnsRebindingProtectionMiddleware.php |
Implements DNS rebinding protection by rejecting non-allowed Host/Origin. |
tests/Unit/Server/Transport/Http/Middleware/DnsRebindingProtectionMiddlewareTest.php |
Unit tests for middleware allow/deny and ordering behavior. |
tests/Conformance/server.php |
Adds the new middleware to the conformance server transport pipeline. |
tests/Conformance/conformance-baseline.yml |
Removes the DNS rebinding protection item from the baseline list. |
docs/transports.md |
Documents how to enable/configure the DNS rebinding middleware. |
CHANGELOG.md |
Notes the addition of DnsRebindingProtectionMiddleware. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Strip port from Host header (e.g., "localhost:8000" -> "localhost") | ||
| $host = strtolower(preg_replace('/:\d+$/', '', $hostHeader) ?? $hostHeader); | ||
|
|
There was a problem hiding this comment.
isAllowedHost() strips a trailing :<digits> to remove ports, but this also matches IPv6 literals without brackets (e.g. Host: ::1 becomes :) and will be rejected even though ::1 is in the default allowlist. Consider parsing Host more robustly (handle bracketed IPv6 + optional port separately, and only strip :<port> for hostnames/IPv4), and normalize comparisons to a consistent representation.
| // Strip port from Host header (e.g., "localhost:8000" -> "localhost") | |
| $host = strtolower(preg_replace('/:\d+$/', '', $hostHeader) ?? $hostHeader); | |
| $hostHeader = trim($hostHeader); | |
| if ('' === $hostHeader) { | |
| return false; | |
| } | |
| // Handle bracketed IPv6 literals: "[::1]" or "[::1]:8000" | |
| if ($hostHeader[0] === '[') { | |
| $endBracketPos = strpos($hostHeader, ']'); | |
| if (false === $endBracketPos) { | |
| // Malformed bracketed host | |
| return false; | |
| } | |
| $ipv6 = strtolower(substr($hostHeader, 1, $endBracketPos - 1)); | |
| // Optional port after closing bracket, e.g. "[::1]:8000" | |
| if (strlen($hostHeader) > $endBracketPos + 1) { | |
| if (':' !== $hostHeader[$endBracketPos + 1]) { | |
| return false; | |
| } | |
| $port = substr($hostHeader, $endBracketPos + 2); | |
| if ('' === $port || !ctype_digit($port)) { | |
| return false; | |
| } | |
| } | |
| // Compare both bracketed and unbracketed forms against the allowlist | |
| $candidates = [ | |
| $ipv6, | |
| '[' . $ipv6 . ']', | |
| ]; | |
| foreach ($candidates as $candidate) { | |
| if (\in_array($candidate, $this->allowedHosts, true)) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| } | |
| $normalized = strtolower($hostHeader); | |
| // For non-bracketed hosts, only strip ":port" when it is clearly a host:port pair. | |
| $colonCount = substr_count($normalized, ':'); | |
| if (0 === $colonCount) { | |
| $host = $normalized; | |
| } elseif (1 === $colonCount) { | |
| // Potential "host:port" | |
| [$hostPart, $maybePort] = explode(':', $normalized, 2); | |
| if ('' !== $hostPart && '' !== $maybePort && ctype_digit($maybePort)) { | |
| $host = $hostPart; | |
| } else { | |
| // Not a numeric port; treat entire value as host (e.g., unusual but valid hostname) | |
| $host = $normalized; | |
| } | |
| } else { | |
| // Multiple colons: likely an unbracketed IPv6 literal such as "::1" | |
| // Do not attempt to strip a port; use the full literal. | |
| $host = $normalized; | |
| } |
| public function __construct( | ||
| private readonly array $allowedHosts = ['localhost', '127.0.0.1', '[::1]', '::1'], | ||
| ?ResponseFactoryInterface $responseFactory = null, | ||
| ?StreamFactoryInterface $streamFactory = null, | ||
| ) { | ||
| $this->responseFactory = $responseFactory ?? Psr17FactoryDiscovery::findResponseFactory(); | ||
| $this->streamFactory = $streamFactory ?? Psr17FactoryDiscovery::findStreamFactory(); | ||
| } |
There was a problem hiding this comment.
Host/Origin values are lowercased before comparison, but $allowedHosts is not normalized. Custom allowlists containing uppercase or bracketed IPv6 only (e.g. [::1]) may fail to match Origin parsing (which yields ::1) due to strict in_array. Normalizing $allowedHosts in the constructor (lowercase + strip surrounding brackets for IPv6) would make behavior consistent and case-insensitive as advertised.
|
|
||
| The SDK ships with `DnsRebindingProtectionMiddleware`, which validates `Host` and `Origin` headers to prevent | ||
| [DNS rebinding attacks](https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#security-warning). | ||
| By default it only allows localhost variants (`localhost`, `127.0.0.1`, `[::1]`): |
There was a problem hiding this comment.
The text says the default allowlist includes only [::1], but the middleware also allows ::1 (and Origin parsing will typically produce ::1 without brackets). Update this sentence to include ::1 (or clarify bracket/normalization behavior) to avoid confusing users configuring allowedHosts.
| By default it only allows localhost variants (`localhost`, `127.0.0.1`, `[::1]`): | |
| By default it only allows localhost variants (`localhost`, `127.0.0.1`, `[::1]` and `::1`): |
| $middleware = new DnsRebindingProtectionMiddleware(responseFactory: $this->factory); | ||
| $request = $this->factory->createServerRequest('POST', 'http://localhost:8000/') | ||
| ->withHeader('Host', 'localhost:8000'); | ||
|
|
||
| $response = $middleware->process($request, $this->handler); |
There was a problem hiding this comment.
These tests pass only responseFactory into the middleware, so StreamFactoryInterface is still auto-discovered. Passing streamFactory: $this->factory as well would make the unit tests independent of PSR-17 discovery configuration and ensure the response body creation uses the same factory implementation.
Adding HTTP middleware for DNS middleware protection - following conformance requirements.