Skip to content

Commit 3a40738

Browse files
sveneldsveneld
committed
[Server] Address review feedback on middleware defaults
- 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.
1 parent 908285a commit 3a40738

8 files changed

Lines changed: 226 additions & 70 deletions

File tree

‎src/Server/Transport/Http/JsonRpcErrorResponse.php‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,9 @@
1717
use Psr\Http\Message\StreamFactoryInterface;
1818

1919
/**
20-
* Builds the canonical JSON-RPC error response used by the HTTP transport
21-
* and its middleware: a PSR-7 response with the given HTTP status, a
22-
* `Content-Type: application/json` header, and a body containing a single
23-
* `Error::forInvalidRequest($message)` payload.
20+
* Builds a PSR-7 response with the given HTTP status and a JSON-RPC
21+
* `Error` payload as body. Caller decides which `Error::for*` factory
22+
* to use so the JSON-RPC error code matches the failure semantics.
2423
*
2524
* @internal
2625
*/
@@ -30,9 +29,9 @@ public static function create(
3029
ResponseFactoryInterface $responseFactory,
3130
StreamFactoryInterface $streamFactory,
3231
int $statusCode,
33-
string $message,
32+
Error $error,
3433
): ResponseInterface {
35-
$body = json_encode(Error::forInvalidRequest($message), \JSON_THROW_ON_ERROR);
34+
$body = json_encode($error, \JSON_THROW_ON_ERROR);
3635

3736
return $responseFactory
3837
->createResponse($statusCode)

‎src/Server/Transport/Http/Middleware/CorsMiddleware.php‎

Lines changed: 40 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -11,21 +11,24 @@
1111

1212
namespace Mcp\Server\Transport\Http\Middleware;
1313

14+
use Mcp\Exception\InvalidArgumentException;
1415
use Mcp\Server\Transport\StreamableHttpTransport;
1516
use Psr\Http\Message\ResponseInterface;
1617
use Psr\Http\Message\ServerRequestInterface;
1718
use Psr\Http\Server\MiddlewareInterface;
1819
use Psr\Http\Server\RequestHandlerInterface;
1920

2021
/**
21-
* Applies CORS headers to all responses produced by the inner pipeline.
22+
* Applies CORS headers to responses produced by the inner pipeline.
2223
*
2324
* By default no `Access-Control-Allow-Origin` header is set, which effectively
2425
* blocks cross-origin browser requests (secure-by-default). Configure
2526
* `$allowedOrigins` with a concrete list, or `['*']` to allow any origin.
2627
*
27-
* Headers already set by inner middleware are preserved — this middleware only
28-
* adds defaults when they are absent.
28+
* `Access-Control-Allow-Methods` and `Access-Control-Allow-Headers` are emitted
29+
* only on preflight responses (`OPTIONS` with an `Access-Control-Request-Method`
30+
* header), per the CORS specification. Headers already set by inner middleware
31+
* are preserved — this middleware only adds defaults when they are absent.
2932
*
3033
* @author Volodymyr Panivko <sveneld300@gmail.com>
3134
*/
@@ -38,10 +41,11 @@ final class CorsMiddleware implements MiddlewareInterface
3841
private readonly ?string $exposedHeadersHeader;
3942

4043
/**
41-
* @param list<string> $allowedOrigins Origins permitted for cross-origin requests. Empty disables `Access-Control-Allow-Origin`. Use `['*']` to allow any origin.
42-
* @param list<string> $allowedMethods Methods advertised via `Access-Control-Allow-Methods`
43-
* @param list<string> $allowedHeaders Headers advertised via `Access-Control-Allow-Headers`
44-
* @param list<string> $exposedHeaders Headers advertised via `Access-Control-Expose-Headers`
44+
* @param list<string> $allowedOrigins Origins permitted for cross-origin requests. Empty disables `Access-Control-Allow-Origin`. Use `['*']` to allow any origin.
45+
* @param list<string> $allowedMethods Methods advertised via `Access-Control-Allow-Methods` (preflight only)
46+
* @param list<string> $allowedHeaders Headers advertised via `Access-Control-Allow-Headers` (preflight only)
47+
* @param list<string> $exposedHeaders Headers advertised via `Access-Control-Expose-Headers`
48+
* @param bool $allowCredentials Whether to emit `Access-Control-Allow-Credentials: true`. Incompatible with `allowedOrigins: ['*']` — combining them throws.
4549
*/
4650
public function __construct(
4751
private readonly array $allowedOrigins = [],
@@ -55,8 +59,14 @@ public function __construct(
5559
StreamableHttpTransport::SESSION_HEADER,
5660
],
5761
array $exposedHeaders = [StreamableHttpTransport::SESSION_HEADER],
62+
private readonly bool $allowCredentials = false,
5863
) {
5964
$this->isWildcard = \in_array('*', $allowedOrigins, true);
65+
66+
if ($this->isWildcard && $allowCredentials) {
67+
throw new InvalidArgumentException('Access-Control-Allow-Origin: * is incompatible with Access-Control-Allow-Credentials: true. Configure an explicit allowedOrigins list when credentialed requests are required.');
68+
}
69+
6070
$this->varyOnOrigin = [] !== $allowedOrigins && !$this->isWildcard;
6171
$this->allowedMethodsHeader = implode(', ', $allowedMethods);
6272
$this->allowedHeadersHeader = implode(', ', $allowedHeaders);
@@ -72,16 +82,22 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface
7282
$response = $response->withHeader('Access-Control-Allow-Origin', $allowedOrigin);
7383
}
7484

85+
if ($this->allowCredentials && null !== $allowedOrigin && !$response->hasHeader('Access-Control-Allow-Credentials')) {
86+
$response = $response->withHeader('Access-Control-Allow-Credentials', 'true');
87+
}
88+
7589
if ($this->varyOnOrigin) {
7690
$response = $this->ensureVaryOrigin($response);
7791
}
7892

79-
if (!$response->hasHeader('Access-Control-Allow-Methods')) {
80-
$response = $response->withHeader('Access-Control-Allow-Methods', $this->allowedMethodsHeader);
81-
}
93+
if ($this->isPreflight($request)) {
94+
if (!$response->hasHeader('Access-Control-Allow-Methods')) {
95+
$response = $response->withHeader('Access-Control-Allow-Methods', $this->allowedMethodsHeader);
96+
}
8297

83-
if (!$response->hasHeader('Access-Control-Allow-Headers')) {
84-
$response = $response->withHeader('Access-Control-Allow-Headers', $this->allowedHeadersHeader);
98+
if (!$response->hasHeader('Access-Control-Allow-Headers')) {
99+
$response = $response->withHeader('Access-Control-Allow-Headers', $this->allowedHeadersHeader);
100+
}
85101
}
86102

87103
if (null !== $this->exposedHeadersHeader && !$response->hasHeader('Access-Control-Expose-Headers')) {
@@ -91,6 +107,12 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface
91107
return $response;
92108
}
93109

110+
private function isPreflight(ServerRequestInterface $request): bool
111+
{
112+
return 'OPTIONS' === $request->getMethod()
113+
&& $request->hasHeader('Access-Control-Request-Method');
114+
}
115+
94116
private function resolveAllowedOrigin(string $origin): ?string
95117
{
96118
if ([] === $this->allowedOrigins) {
@@ -116,7 +138,12 @@ private function ensureVaryOrigin(ResponseInterface $response): ResponseInterfac
116138
return $response->withHeader('Vary', 'Origin');
117139
}
118140

119-
if ('*' === trim($current) || false !== stripos($current, 'origin')) {
141+
if ('*' === trim($current)) {
142+
return $response;
143+
}
144+
145+
$tokens = array_map('strtolower', array_map('trim', explode(',', $current)));
146+
if (\in_array('origin', $tokens, true)) {
120147
return $response;
121148
}
122149

‎src/Server/Transport/Http/Middleware/DnsRebindingProtectionMiddleware.php‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@
1212
namespace Mcp\Server\Transport\Http\Middleware;
1313

1414
use Http\Discovery\Psr17FactoryDiscovery;
15-
use Mcp\Server\Transport\Http\JsonRpcErrorResponse;
1615
use Psr\Http\Message\ResponseFactoryInterface;
1716
use Psr\Http\Message\ResponseInterface;
1817
use Psr\Http\Message\ServerRequestInterface;
@@ -44,11 +43,12 @@ final class DnsRebindingProtectionMiddleware implements MiddlewareInterface
4443

4544
/**
4645
* @param list<string> $allowedHosts Hostnames (without port) that are permitted. Defaults to localhost variants.
46+
* IPv6 addresses must be bracketed (e.g. `[::1]`) — that is the canonical form returned by `parse_url`.
4747
* @param ResponseFactoryInterface|null $responseFactory PSR-17 response factory (auto-discovered if null)
4848
* @param StreamFactoryInterface|null $streamFactory PSR-17 stream factory (auto-discovered if null)
4949
*/
5050
public function __construct(
51-
array $allowedHosts = ['localhost', '127.0.0.1', '[::1]', '::1'],
51+
array $allowedHosts = ['localhost', '127.0.0.1', '[::1]'],
5252
?ResponseFactoryInterface $responseFactory = null,
5353
?StreamFactoryInterface $streamFactory = null,
5454
) {
@@ -78,12 +78,12 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface
7878

7979
private function isAllowedOrigin(string $origin): bool
8080
{
81-
$parsed = parse_url($origin);
82-
if (false === $parsed || !isset($parsed['host'])) {
81+
$host = parse_url($origin, \PHP_URL_HOST);
82+
if (!\is_string($host) || '' === $host) {
8383
return false;
8484
}
8585

86-
return \in_array(strtolower($parsed['host']), $this->allowedHosts, true);
86+
return \in_array(strtolower($host), $this->allowedHosts, true);
8787
}
8888

8989
private function isAllowedHost(string $host): bool
@@ -103,6 +103,9 @@ private function isAllowedHost(string $host): bool
103103

104104
private function createForbiddenResponse(string $message): ResponseInterface
105105
{
106-
return JsonRpcErrorResponse::create($this->responseFactory, $this->streamFactory, 403, $message);
106+
return $this->responseFactory
107+
->createResponse(403)
108+
->withHeader('Content-Type', 'text/plain')
109+
->withBody($this->streamFactory->createStream($message));
107110
}
108111
}

‎src/Server/Transport/Http/Middleware/ProtocolVersionMiddleware.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313

1414
use Http\Discovery\Psr17FactoryDiscovery;
1515
use Mcp\Schema\Enum\ProtocolVersion;
16+
use Mcp\Schema\JsonRpc\Error;
1617
use Mcp\Server\Transport\Http\JsonRpcErrorResponse;
1718
use Mcp\Server\Transport\StreamableHttpTransport;
1819
use Psr\Http\Message\ResponseFactoryInterface;
@@ -92,6 +93,6 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface
9293
implode(', ', $this->supportedVersions),
9394
);
9495

95-
return JsonRpcErrorResponse::create($this->responseFactory, $this->streamFactory, 400, $message);
96+
return JsonRpcErrorResponse::create($this->responseFactory, $this->streamFactory, 400, Error::forInvalidParams($message));
9697
}
9798
}

‎src/Server/Transport/StreamableHttpTransport.php‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,14 @@ public function __construct(
6060
$this->responseFactory = $responseFactory ?? Psr17FactoryDiscovery::findResponseFactory();
6161
$this->streamFactory = $streamFactory ?? Psr17FactoryDiscovery::findStreamFactory();
6262

63-
$this->middleware = self::normalizeMiddleware($middleware ?? self::defaultMiddleware());
63+
if (null === $middleware) {
64+
$this->middleware = self::defaultMiddleware();
65+
} else {
66+
$this->middleware = self::normalizeMiddleware($middleware);
67+
if ([] === $this->middleware) {
68+
$this->logger->warning('Streamable HTTP transport started with an empty middleware list. Default security protections (CORS, DNS rebinding, protocol version validation) are disabled. Pass null (or omit the argument) to use the secure defaults, or include them via [...StreamableHttpTransport::defaultMiddleware(), $yourMiddleware].');
69+
}
70+
}
6471
}
6572

6673
/**

‎tests/Unit/Server/Transport/Http/Middleware/CorsMiddlewareTest.php‎

Lines changed: 96 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,10 @@
1111

1212
namespace Mcp\Tests\Unit\Server\Transport\Http\Middleware;
1313

14+
use Mcp\Exception\InvalidArgumentException;
1415
use Mcp\Server\Transport\Http\Middleware\CorsMiddleware;
1516
use PHPUnit\Framework\Attributes\TestDox;
17+
use Psr\Http\Message\ServerRequestInterface;
1618

1719
final class CorsMiddlewareTest extends MiddlewareTestCase
1820
{
@@ -26,9 +28,35 @@ public function testDefaultDoesNotSetAllowOrigin(): void
2628
$response = $middleware->process($request, $this->passthroughHandler);
2729

2830
$this->assertFalse($response->hasHeader('Access-Control-Allow-Origin'));
29-
$this->assertTrue($response->hasHeader('Access-Control-Allow-Methods'));
30-
$this->assertTrue($response->hasHeader('Access-Control-Allow-Headers'));
3131
$this->assertTrue($response->hasHeader('Access-Control-Expose-Headers'));
32+
// Non-preflight: Methods/Headers must NOT be emitted per CORS spec.
33+
$this->assertFalse($response->hasHeader('Access-Control-Allow-Methods'));
34+
$this->assertFalse($response->hasHeader('Access-Control-Allow-Headers'));
35+
}
36+
37+
#[TestDox('preflight request receives Access-Control-Allow-Methods and Access-Control-Allow-Headers')]
38+
public function testPreflightReceivesMethodAndHeaderAdvertisements(): void
39+
{
40+
$middleware = new CorsMiddleware(allowedOrigins: ['https://app.example.com']);
41+
$request = $this->preflightRequest('https://app.example.com');
42+
43+
$response = $middleware->process($request, $this->passthroughHandler);
44+
45+
$this->assertSame('GET, POST, DELETE, OPTIONS', $response->getHeaderLine('Access-Control-Allow-Methods'));
46+
$this->assertNotSame('', $response->getHeaderLine('Access-Control-Allow-Headers'));
47+
}
48+
49+
#[TestDox('non-preflight OPTIONS request does not receive Methods/Headers advertisements')]
50+
public function testPlainOptionsIsNotTreatedAsPreflight(): void
51+
{
52+
$middleware = new CorsMiddleware(allowedOrigins: ['*']);
53+
// OPTIONS without `Access-Control-Request-Method` is not a CORS preflight.
54+
$request = $this->factory->createServerRequest('OPTIONS', 'https://example.com');
55+
56+
$response = $middleware->process($request, $this->passthroughHandler);
57+
58+
$this->assertFalse($response->hasHeader('Access-Control-Allow-Methods'));
59+
$this->assertFalse($response->hasHeader('Access-Control-Allow-Headers'));
3260
}
3361

3462
#[TestDox('wildcard allowedOrigins sets Access-Control-Allow-Origin to *')]
@@ -77,7 +105,7 @@ public function testPreExistingHeadersAreNotOverwritten(): void
77105
]);
78106

79107
$middleware = new CorsMiddleware(allowedOrigins: ['*']);
80-
$request = $this->factory->createServerRequest('POST', 'https://example.com');
108+
$request = $this->preflightRequest();
81109

82110
$response = $middleware->process($request, $inner);
83111

@@ -173,4 +201,69 @@ public function testVaryOriginIsNotDuplicated(): void
173201

174202
$this->assertSame('Accept-Encoding, Origin', $response->getHeaderLine('Vary'));
175203
}
204+
205+
#[TestDox('does not treat a substring match like Origin-Other as the Origin token')]
206+
public function testVarySubstringDoesNotPreventAppending(): void
207+
{
208+
// `Origin-Resource-Policy` contains the substring "origin" but is a different token —
209+
// tokenized comparison must still treat the response as missing the `Origin` value.
210+
$inner = $this->handlerReturning(200, ['Vary' => 'Origin-Resource-Policy']);
211+
212+
$middleware = new CorsMiddleware(allowedOrigins: ['https://app.example.com']);
213+
$request = $this->factory->createServerRequest('POST', 'https://example.com')
214+
->withHeader('Origin', 'https://app.example.com');
215+
216+
$response = $middleware->process($request, $inner);
217+
218+
$this->assertSame('Origin-Resource-Policy, Origin', $response->getHeaderLine('Vary'));
219+
}
220+
221+
#[TestDox('allowCredentials emits Access-Control-Allow-Credentials when an origin matches')]
222+
public function testAllowCredentialsHeaderEmitted(): void
223+
{
224+
$middleware = new CorsMiddleware(
225+
allowedOrigins: ['https://app.example.com'],
226+
allowCredentials: true,
227+
);
228+
$request = $this->factory->createServerRequest('POST', 'https://example.com')
229+
->withHeader('Origin', 'https://app.example.com');
230+
231+
$response = $middleware->process($request, $this->passthroughHandler);
232+
233+
$this->assertSame('https://app.example.com', $response->getHeaderLine('Access-Control-Allow-Origin'));
234+
$this->assertSame('true', $response->getHeaderLine('Access-Control-Allow-Credentials'));
235+
}
236+
237+
#[TestDox('allowCredentials does not emit credentials header when no origin matches')]
238+
public function testAllowCredentialsSkippedWhenOriginUnmatched(): void
239+
{
240+
$middleware = new CorsMiddleware(
241+
allowedOrigins: ['https://app.example.com'],
242+
allowCredentials: true,
243+
);
244+
$request = $this->factory->createServerRequest('POST', 'https://example.com')
245+
->withHeader('Origin', 'https://evil.example.com');
246+
247+
$response = $middleware->process($request, $this->passthroughHandler);
248+
249+
$this->assertFalse($response->hasHeader('Access-Control-Allow-Origin'));
250+
$this->assertFalse($response->hasHeader('Access-Control-Allow-Credentials'));
251+
}
252+
253+
#[TestDox('combining wildcard origin with allowCredentials throws')]
254+
public function testWildcardWithCredentialsRejected(): void
255+
{
256+
$this->expectException(InvalidArgumentException::class);
257+
258+
new CorsMiddleware(allowedOrigins: ['*'], allowCredentials: true);
259+
}
260+
261+
private function preflightRequest(string $origin = 'https://app.example.com'): ServerRequestInterface
262+
{
263+
return $this->factory
264+
->createServerRequest('OPTIONS', 'https://example.com')
265+
->withHeader('Origin', $origin)
266+
->withHeader('Access-Control-Request-Method', 'POST')
267+
->withHeader('Access-Control-Request-Headers', 'Content-Type');
268+
}
176269
}

‎tests/Unit/Server/Transport/Http/Middleware/DnsRebindingProtectionMiddlewareTest.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,8 @@ public function testRejectsForeignOrigin(): void
4747
$response = $middleware->process($request, $this->passthroughHandler);
4848

4949
$this->assertSame(403, $response->getStatusCode());
50-
$this->assertSame('application/json', $response->getHeaderLine('Content-Type'));
50+
$this->assertSame('text/plain', $response->getHeaderLine('Content-Type'));
51+
$this->assertStringContainsString('Origin', (string) $response->getBody());
5152
}
5253

5354
#[TestDox('Origin header takes precedence over Host')]

0 commit comments

Comments
 (0)