[Server] Compose StreamableHttpTransport security middleware via defaultMiddleware() factory#307
Conversation
b087c0f to
32ed494
Compare
32ed494 to
87484a5
Compare
|
Thanks for that proposal @sveneld - my solution was not on point, true. |
|
Yes, if you pass your own middleware list, the default protection middleware will not be applied automatically. That is why in the examples I explicitly show that custom middleware should be merged with the default middleware, rather than replacing it entirely: This keeps the default security protections enabled while still allowing custom middleware to be added. |
| return $response->withHeader('Vary', 'Origin'); | ||
| } | ||
|
|
||
| if ('*' === trim($current) || false !== stripos($current, 'origin')) { |
There was a problem hiding this comment.
this could have false positives, suggestion:
$tokens = array_map('strtolower', array_map('trim', explode(',', $current)));
if ('*' === trim($current) || in_array('origin', $tokens, true)) {
return $response;
}| if (!$response->hasHeader('Access-Control-Allow-Methods')) { | ||
| $response = $response->withHeader('Access-Control-Allow-Methods', $this->allowedMethodsHeader); | ||
| } | ||
|
|
||
| if (!$response->hasHeader('Access-Control-Allow-Headers')) { | ||
| $response = $response->withHeader('Access-Control-Allow-Headers', $this->allowedHeadersHeader); | ||
| } |
There was a problem hiding this comment.
These make sense only if in a preflight request, suggestion:
$isPreflight = 'OPTIONS' === $request->getMethod()
&& $request->hasHeader('Access-Control-Request-Method');
if ($isPreflight && !$response->hasHeader('Access-Control-Allow-Methods')) {
$response = $response->withHeader('Access-Control-Allow-Methods', $this->allowedMethodsHeader);
}
if ($isPreflight && !$response->hasHeader('Access-Control-Allow-Headers')) {
$response = $response->withHeader('Access-Control-Allow-Headers', $this->allowedHeadersHeader);
}| StreamableHttpTransport::PROTOCOL_VERSION_HEADER, | ||
| StreamableHttpTransport::SESSION_HEADER, | ||
| ], | ||
| array $exposedHeaders = [StreamableHttpTransport::SESSION_HEADER], |
There was a problem hiding this comment.
| array $exposedHeaders = [StreamableHttpTransport::SESSION_HEADER], | |
| array $exposedHeaders = [StreamableHttpTransport::SESSION_HEADER], | |
| bool $allowCredentials = false, |
| $this->varyOnOrigin = [] !== $allowedOrigins && !$this->isWildcard; | ||
| $this->allowedMethodsHeader = implode(', ', $allowedMethods); | ||
| $this->allowedHeadersHeader = implode(', ', $allowedHeaders); | ||
| $this->exposedHeadersHeader = [] === $exposedHeaders ? null : implode(', ', $exposedHeaders); |
There was a problem hiding this comment.
if ($this->isWildcard && $allowCredentials) {
throw new InvalidArgumentException(
'Access-Control-Allow-Origin: * is incompatible with Access-Control-Allow-Credentials: true.',
);
}| if (null !== $allowedOrigin && !$response->hasHeader('Access-Control-Allow-Origin')) { | ||
| $response = $response->withHeader('Access-Control-Allow-Origin', $allowedOrigin); | ||
| } | ||
|
|
There was a problem hiding this comment.
add Access-Control-Allow-Credentials: true
Something like:
if ($this->allowCredentials && null !== $allowedOrigin
&& !$response->hasHeader('Access-Control-Allow-Credentials')) {
$response = $response->withHeader('Access-Control-Allow-Credentials', 'true');
}- tests
| $parsed = parse_url($origin); | ||
| if (false === $parsed || !isset($parsed['host'])) { | ||
| return false; | ||
| } | ||
|
|
||
| return \in_array(strtolower($parsed['host']), $this->allowedHosts, true); |
There was a problem hiding this comment.
| $parsed = parse_url($origin); | |
| if (false === $parsed || !isset($parsed['host'])) { | |
| return false; | |
| } | |
| return \in_array(strtolower($parsed['host']), $this->allowedHosts, true); | |
| $host = parse_url($origin, \PHP_URL_HOST); | |
| if (!is_string($host) || '' === $host) { | |
| return false; | |
| } | |
| return \in_array(strtolower($host), $this->allowedHosts, true); |
| * @param StreamFactoryInterface|null $streamFactory PSR-17 stream factory (auto-discovered if null) | ||
| */ | ||
| public function __construct( | ||
| array $allowedHosts = ['localhost', '127.0.0.1', '[::1]', '::1'], |
There was a problem hiding this comment.
| array $allowedHosts = ['localhost', '127.0.0.1', '[::1]', '::1'], | |
| array $allowedHosts = ['localhost', '127.0.0.1', '[::1]'], |
Useless value will never get matched below
| private function createForbiddenResponse(string $message): ResponseInterface | ||
| { | ||
| return JsonRpcErrorResponse::create($this->responseFactory, $this->streamFactory, 403, $message); | ||
| } |
There was a problem hiding this comment.
| private function createForbiddenResponse(string $message): ResponseInterface | |
| { | |
| return JsonRpcErrorResponse::create($this->responseFactory, $this->streamFactory, 403, $message); | |
| } | |
| private function createForbiddenResponse(string $message): ResponseInterface | |
| { | |
| return $this->responseFactory->createResponse(403) | |
| ->withHeader('Content-Type', 'text/plain') | |
| ->withBody($this->streamFactory->createStream($message)); | |
| } |
I feel Error::forInvalidRequest($message) is wrong here.
| public static function create( | ||
| ResponseFactoryInterface $responseFactory, | ||
| StreamFactoryInterface $streamFactory, | ||
| int $statusCode, | ||
| string $message, | ||
| ): ResponseInterface { | ||
| $body = json_encode(Error::forInvalidRequest($message), \JSON_THROW_ON_ERROR); | ||
|
|
||
| return $responseFactory | ||
| ->createResponse($statusCode) | ||
| ->withHeader('Content-Type', 'application/json') | ||
| ->withBody($streamFactory->createStream($body)); | ||
| } |
There was a problem hiding this comment.
| public static function create( | |
| ResponseFactoryInterface $responseFactory, | |
| StreamFactoryInterface $streamFactory, | |
| int $statusCode, | |
| string $message, | |
| ): ResponseInterface { | |
| $body = json_encode(Error::forInvalidRequest($message), \JSON_THROW_ON_ERROR); | |
| return $responseFactory | |
| ->createResponse($statusCode) | |
| ->withHeader('Content-Type', 'application/json') | |
| ->withBody($streamFactory->createStream($body)); | |
| } | |
| public static function create( | |
| ResponseFactoryInterface $responseFactory, | |
| StreamFactoryInterface $streamFactory, | |
| int $statusCode, | |
| Error $error, | |
| ): ResponseInterface { | |
| $body = json_encode($error, \JSON_THROW_ON_ERROR); | |
| return $responseFactory->createResponse($statusCode) | |
| ->withHeader('Content-Type', 'application/json') | |
| ->withBody($streamFactory->createStream($body)); | |
| } |
| implode(', ', $this->supportedVersions), | ||
| ); | ||
|
|
||
| return JsonRpcErrorResponse::create($this->responseFactory, $this->streamFactory, 400, $message); |
There was a problem hiding this comment.
| return JsonRpcErrorResponse::create($this->responseFactory, $this->streamFactory, 400, $message); | |
| return JsonRpcErrorResponse::create($this->responseFactory, $this->streamFactory, 400, Error::forInvalidParams($message)); |
|
Thanks for the review, @soyuka — all inline comments addressed in the latest commit:
@chr-hertel — on the "easy to opt out" concern: passing |
Introduce three PSR-15 middleware for `StreamableHttpTransport` exposed through a public `StreamableHttpTransport::defaultMiddleware()` factory composed automatically when no middleware is passed. - `CorsMiddleware`: secure-by-default (no `Access-Control-Allow-Origin`), configurable allowlist, reflects matching origin with `Vary: Origin` to protect shared caches. - `DnsRebindingProtectionMiddleware`: validates `Origin`/`Host` against a hostname allowlist (localhost-only by default). - `ProtocolVersionMiddleware`: rejects requests carrying an unsupported `Mcp-Protocol-Version` header with `400 Bad Request`. The transport no longer applies CORS via an `instanceof + array_unshift` post-hook; the middleware parameter is nullable — `null` installs the secure defaults, `[]` disables them, and users compose by spreading `StreamableHttpTransport::defaultMiddleware()`. `SESSION_HEADER` and `PROTOCOL_VERSION_HEADER` are promoted to public constants so middleware can reuse them. BC breaks: - The `corsHeaders` constructor parameter is removed; the `middleware` parameter shifts one position. Positional callers passing the old `corsHeaders` argument must switch to named arguments or drop it. - Default `Access-Control-Allow-Origin` is no longer `*`. Addresses modelcontextprotocol#260 (DNS rebinding), modelcontextprotocol#277 (CORS extraction) and modelcontextprotocol#306 (protocol version validation).
- CorsMiddleware: emit Access-Control-Allow-Methods/Headers only on preflight per CORS spec; tokenize Vary header to avoid substring false-positives; add allowCredentials flag that emits Access-Control-Allow-Credentials and throws when combined with wildcard origin. - DnsRebindingProtectionMiddleware: return plain-text 403 body (HTTP-level rejection, not JSON-RPC); use parse_url(..., PHP_URL_HOST) for origin parsing; drop redundant unbracketed '::1' from default allowlist. - ProtocolVersionMiddleware: use Error::forInvalidParams instead of forInvalidRequest -- JSON-RPC -32602 fits header-value rejection better. - JsonRpcErrorResponse::create now accepts a JsonRpc\Error so callers choose the error code. - StreamableHttpTransport: emit a warning log when an explicit empty middleware list is passed, so operators can spot accidental bypass of the default security stack.
c09d2c0 to
0b825ef
Compare
Replace the anonymous AbstractLogger fixture in StreamableHttpTransportTest with `$this->createMock(LoggerInterface::class)` so the test does not declare a `log()` signature at all. That sidesteps both the psr/log v1 LSP incompatibility (untyped parameter) and the phpstan `missingType.parameter` complaint in one move, and matches how the rest of the suite (CallToolHandlerTest etc.) fakes loggers.
0b825ef to
ddf10bb
Compare
Summary
Replaces the implicit
instanceof + array_unshiftinjection of CORS handling inStreamableHttpTransportwith an explicit, composable PSR-15 stack exposed through a publicStreamableHttpTransport::defaultMiddleware()factory. Adds three middleware that together cover the recommended HTTP-level hardening for MCP Streamable HTTP:CorsMiddleware— secure-by-default (noAccess-Control-Allow-Originheader is set; cross-origin browser requests are blocked). ConfigurableallowedOriginsreflect a matching origin and automatically emitVary: Originso shared caches don't poison.DnsRebindingProtectionMiddleware— validatesOrigin/Hostagainst an allowlist of hostnames (localhost variants by default).ProtocolVersionMiddleware— rejects requests carrying an unsupportedMcp-Protocol-Versionheader with400 Bad Request(spec compliance).The transport itself becomes oblivious to the middleware list. The
$middlewareconstructor argument is nullable:nullStreamableHttpTransport::defaultMiddleware()[]...StreamableHttpTransport::defaultMiddleware()SESSION_HEADERand a newPROTOCOL_VERSION_HEADERare promoted to public constants on the transport so middleware can reuse them without duplicating string literals.Why this design
The three open items below all reach for the same shape — a piece of mandatory security logic that the transport should apply unless the user opts out. PRs #260 and #277 each implement this with their own
instanceof MyMiddlewareClasscheck +array_unshiftinside the transport constructor. That pattern scales linearly with the number of "mandatory" middleware and breaks once a user supplies a custom implementation of the same concern under a different class name. AddingProtocolVersionMiddlewarefor #306 would mean a third copy of the same pattern.This PR collapses all three into a single composition model: the transport is a thin PSR-15 host, and the recommended stack lives in a factory the user can spread, filter, or replace. No
instanceofmagic, no surprise prepending, no position arguments to bypass.Relation to other work
CorsMiddleware) — same goal of replacing the inlinewithCorsHeaders()post-hook with a PSR-15 middleware. This PR additionally drops theinstanceof CorsMiddlewareauto-prepend in favour ofdefaultMiddleware()composition (which is what @CodeWithKyrian suggested in #277 review and @chr-hertel agreed with). Also addresses Hardcoded Wildcard CORS (Access-Control-Allow-Origin: *) #280 (wildcard CORS default) the same way [Server] Extract CORS handling into CorsMiddleware #277 does —Access-Control-Allow-Originis no longer set by default. AddsVary: Originwhen reflecting a specific origin, which [Server] Extract CORS handling into CorsMiddleware #277 doesn't.localhost,127.0.0.1,[::1],::1); avoids theinstanceof DnsRebindingProtectionMiddlewareauto-prepend pattern for the same scalability reason.MCP-Protocol-Versionaccepted) — newProtocolVersionMiddlewarerejects with400 Bad Requestper spec. Tolerates a missing header (initialize round-trip / legacy clients don't send it); strict per-session version-match validation is out of scope for an HTTP-layer middleware and can be layered on if maintainers want it.If maintainers prefer to land #260 / #277 separately first, this PR can be rebased onto them and reduced to the composition refactor +
ProtocolVersionMiddleware. Happy to do that.BC breaks
StreamableHttpTransport::__construct— the$corsHeadersparameter is removed. The$middlewareparameter consequently shifts one position. Positional callers passing the old$corsHeadersargument will get a TypeError at construction; switch to named arguments or drop the argument.logger/middlewareare now positions 4/5 instead of 5/6.Access-Control-Allow-Originis no longer*. Browser clients on a different origin will be blocked unlessCorsMiddlewareis configured with explicitallowedOrigins.CHANGELOG.mdentry added under 0.6.0.Test plan
vendor/bin/phpunit --testsuite=unit— 38 new/updated tests pass (3 middleware × ~10 tests each + transport integration). Full suite: 1 error + 2 failures are pre-existing onupstream/main(DocBlockParserTestmissingComposer\Semver\VersionParserdev-dep, twoDiscoveryTestfixtures) — verified by running the same tests against vanillamainwith the branch stashed.vendor/bin/php-cs-fixer fix --dry-run— cleanvendor/bin/phpstan analyse— clean for changed code; the one pre-existing error inDocBlockParserTest(missing dev-depComposer\Semver\VersionParser) is unchangedoauth-keycloak,oauth-microsoft) updated to spread the default stack so their behaviour is preserved