From 09e3464426d00c76f10d8eb0fd301a297a87b7e2 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 11 Jan 2020 20:36:17 +0100 Subject: [PATCH] Ensured CrossDomainMiddleware always returns empty responses with success status on OPTIONS requests --- composer.json | 2 +- .../src/Middleware/CrossDomainMiddleware.php | 9 ++-- .../Middleware/CrossDomainMiddlewareTest.php | 46 +++++++++++++++++-- phpstan.neon | 2 + 4 files changed, 50 insertions(+), 9 deletions(-) diff --git a/composer.json b/composer.json index a21489ac..fb786eec 100644 --- a/composer.json +++ b/composer.json @@ -97,7 +97,7 @@ ], "cs": "phpcs", "cs:fix": "phpcbf", - "stan": "phpstan analyse module/*/src/ module/*/config config docker/config --level=5 -c phpstan.neon", + "stan": "phpstan analyse module/*/src/ module/*/config config docker/config --level=6", "test": [ "@test:unit", "@test:db", diff --git a/module/Rest/src/Middleware/CrossDomainMiddleware.php b/module/Rest/src/Middleware/CrossDomainMiddleware.php index 23cd9ebe..f60c0ad1 100644 --- a/module/Rest/src/Middleware/CrossDomainMiddleware.php +++ b/module/Rest/src/Middleware/CrossDomainMiddleware.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Rest\Middleware; use Fig\Http\Message\RequestMethodInterface; +use Laminas\Diactoros\Response\EmptyResponse; use Mezzio\Router\RouteResult; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\ServerRequestInterface; @@ -12,6 +13,7 @@ use Psr\Http\Server\MiddlewareInterface; use Psr\Http\Server\RequestHandlerInterface; use Shlinkio\Shlink\Rest\Authentication; +use function array_merge; use function implode; class CrossDomainMiddleware implements MiddlewareInterface, RequestMethodInterface @@ -53,10 +55,7 @@ class CrossDomainMiddleware implements MiddlewareInterface, RequestMethodInterfa 'Access-Control-Allow-Headers' => $request->getHeaderLine('Access-Control-Request-Headers'), ]; - foreach ($corsHeaders as $key => $value) { - $response = $response->withHeader($key, $value); - } - - return $response; + // Options requests should always be empty and have a 204 status code + return EmptyResponse::withHeaders(array_merge($response->getHeaders(), $corsHeaders)); } } diff --git a/module/Rest/test/Middleware/CrossDomainMiddlewareTest.php b/module/Rest/test/Middleware/CrossDomainMiddlewareTest.php index 5daa8c3d..5cc99fb3 100644 --- a/module/Rest/test/Middleware/CrossDomainMiddlewareTest.php +++ b/module/Rest/test/Middleware/CrossDomainMiddlewareTest.php @@ -31,14 +31,14 @@ class CrossDomainMiddlewareTest extends TestCase /** @test */ public function nonCrossDomainRequestsAreNotAffected(): void { - $originalResponse = new Response(); + $originalResponse = (new Response())->withStatus(404); $this->handler->handle(Argument::any())->willReturn($originalResponse)->shouldBeCalledOnce(); $response = $this->middleware->process(new ServerRequest(), $this->handler->reveal()); - $this->assertSame($originalResponse, $response); - $headers = $response->getHeaders(); + $this->assertSame($originalResponse, $response); + $this->assertEquals(404, $response->getStatusCode()); $this->assertArrayNotHasKey('Access-Control-Allow-Origin', $headers); $this->assertArrayNotHasKey('Access-Control-Expose-Headers', $headers); $this->assertArrayNotHasKey('Access-Control-Allow-Methods', $headers); @@ -93,6 +93,7 @@ class CrossDomainMiddlewareTest extends TestCase $this->assertArrayHasKey('Access-Control-Allow-Methods', $headers); $this->assertEquals('1000', $response->getHeaderLine('Access-Control-Max-Age')); $this->assertEquals('foo, bar, baz', $response->getHeaderLine('Access-Control-Allow-Headers')); + $this->assertEquals(204, $response->getStatusCode()); } /** @@ -112,6 +113,7 @@ class CrossDomainMiddlewareTest extends TestCase $response = $this->middleware->process($request, $this->handler->reveal()); $this->assertEquals($response->getHeaderLine('Access-Control-Allow-Methods'), $expectedAllowedMethods); + $this->assertEquals(204, $response->getStatusCode()); } public function provideRouteResults(): iterable @@ -126,4 +128,42 @@ class CrossDomainMiddlewareTest extends TestCase 'DELETE,PATCH,PUT', ]; } + + /** + * @test + * @dataProvider provideMethods + */ + public function expectedStatusCodeIsReturnDependingOnRequestMethod( + string $method, + int $status, + int $expectedStatus + ): void { + $originalResponse = (new Response())->withStatus($status); + $request = (new ServerRequest())->withMethod($method) + ->withHeader('Origin', 'local'); + $this->handler->handle(Argument::any())->willReturn($originalResponse)->shouldBeCalledOnce(); + + $response = $this->middleware->process($request, $this->handler->reveal()); + + $this->assertEquals($expectedStatus, $response->getStatusCode()); + } + + public function provideMethods(): iterable + { + yield 'POST 200' => ['POST', 200, 200]; + yield 'POST 400' => ['POST', 400, 400]; + yield 'POST 500' => ['POST', 500, 500]; + yield 'GET 200' => ['GET', 200, 200]; + yield 'GET 400' => ['GET', 400, 400]; + yield 'GET 500' => ['GET', 500, 500]; + yield 'PATCH 200' => ['PATCH', 200, 200]; + yield 'PATCH 400' => ['PATCH', 400, 400]; + yield 'PATCH 500' => ['PATCH', 500, 500]; + yield 'DELETE 200' => ['DELETE', 200, 200]; + yield 'DELETE 400' => ['DELETE', 400, 400]; + yield 'DELETE 500' => ['DELETE', 500, 500]; + yield 'OPTIONS 200' => ['OPTIONS', 200, 204]; + yield 'OPTIONS 400' => ['OPTIONS', 400, 204]; + yield 'OPTIONS 500' => ['OPTIONS', 500, 204]; + } } diff --git a/phpstan.neon b/phpstan.neon index 2d9d960d..b6c65f7f 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -1,4 +1,6 @@ parameters: + checkMissingIterableValueType: false + checkGenericClassInNonGenericObjectType: false ignoreErrors: - '#Undefined variable: \$metadata#' - '#AbstractQuery::setParameters()#'