From f585cfe02e8a074b3f955dcb8c23b43e592fc65e Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 24 Jan 2021 13:49:57 +0100 Subject: [PATCH] Fixed CrossDomainMiddleware not inferring proper allowed methods --- .../src/Middleware/CrossDomainMiddleware.php | 33 +++++++++++-------- module/Rest/test-api/Middleware/CorsTest.php | 9 +++-- .../Middleware/CrossDomainMiddlewareTest.php | 26 +++++---------- 3 files changed, 32 insertions(+), 36 deletions(-) diff --git a/module/Rest/src/Middleware/CrossDomainMiddleware.php b/module/Rest/src/Middleware/CrossDomainMiddleware.php index 851dc955..b265fe13 100644 --- a/module/Rest/src/Middleware/CrossDomainMiddleware.php +++ b/module/Rest/src/Middleware/CrossDomainMiddleware.php @@ -6,7 +6,6 @@ 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; use Psr\Http\Server\MiddlewareInterface; @@ -42,20 +41,8 @@ class CrossDomainMiddleware implements MiddlewareInterface, RequestMethodInterfa private function addOptionsHeaders(ServerRequestInterface $request, ResponseInterface $response): ResponseInterface { - // TODO This won't work. The route has to be matched from the router as this middleware needs to be executed - // before trying to match the route - /** @var RouteResult|null $matchedRoute */ - $matchedRoute = $request->getAttribute(RouteResult::class); - $matchedMethods = $matchedRoute !== null ? $matchedRoute->getAllowedMethods() : [ - self::METHOD_GET, - self::METHOD_POST, - self::METHOD_PUT, - self::METHOD_PATCH, - self::METHOD_DELETE, - self::METHOD_OPTIONS, - ]; $corsHeaders = [ - 'Access-Control-Allow-Methods' => implode(',', $matchedMethods), + 'Access-Control-Allow-Methods' => $this->resolveCorsAllowedMethods($response), 'Access-Control-Allow-Headers' => $request->getHeaderLine('Access-Control-Request-Headers'), 'Access-Control-Max-Age' => $this->config['max_age'], ]; @@ -63,4 +50,22 @@ class CrossDomainMiddleware implements MiddlewareInterface, RequestMethodInterfa // Options requests should always be empty and have a 204 status code return EmptyResponse::withHeaders(array_merge($response->getHeaders(), $corsHeaders)); } + + private function resolveCorsAllowedMethods(ResponseInterface $response): string + { + // ImplicitOptionsMiddleware resolves allowed methods using the RouteResult request's attribute and sets them + // in the "Allow" header. + // If the header is there, we can re-use the value as it is. + if ($response->hasHeader('Allow')) { + return $response->getHeaderLine('Allow'); + } + + return implode(',', [ + self::METHOD_GET, + self::METHOD_POST, + self::METHOD_PUT, + self::METHOD_PATCH, + self::METHOD_DELETE, + ]); + } } diff --git a/module/Rest/test-api/Middleware/CorsTest.php b/module/Rest/test-api/Middleware/CorsTest.php index 1cb8676f..093a200c 100644 --- a/module/Rest/test-api/Middleware/CorsTest.php +++ b/module/Rest/test-api/Middleware/CorsTest.php @@ -71,10 +71,9 @@ class CorsTest extends ApiTestCase public function providePreflightEndpoints(): iterable { - yield 'invalid route' => ['/foo/bar', 'GET,POST,PUT,PATCH,DELETE,OPTIONS']; - yield 'short URLs routes' => ['/short-urls', 'GET,POST,PUT,PATCH,DELETE,OPTIONS']; -// yield 'short URLs routes' => ['/short-urls', 'GET,POST']; // TODO This should be the good one - yield 'tags routes' => ['/tags', 'GET,POST,PUT,PATCH,DELETE,OPTIONS']; -// yield 'tags routes' => ['/short-urls', 'GET,POST,PUT,DELETE']; // TODO This should be the good one + yield 'invalid route' => ['/foo/bar', 'GET,POST,PUT,PATCH,DELETE']; + yield 'short URLs route' => ['/short-urls', 'GET,POST']; + yield 'tags route' => ['/tags', 'GET,POST,PUT,DELETE']; + yield 'health route' => ['/health', 'GET']; } } diff --git a/module/Rest/test/Middleware/CrossDomainMiddlewareTest.php b/module/Rest/test/Middleware/CrossDomainMiddlewareTest.php index 08657992..a274e1f8 100644 --- a/module/Rest/test/Middleware/CrossDomainMiddlewareTest.php +++ b/module/Rest/test/Middleware/CrossDomainMiddlewareTest.php @@ -6,8 +6,6 @@ namespace ShlinkioTest\Shlink\Rest\Middleware; use Laminas\Diactoros\Response; use Laminas\Diactoros\ServerRequest; -use Mezzio\Router\Route; -use Mezzio\Router\RouteResult; use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\PhpUnit\ProphecyTrait; @@ -15,8 +13,6 @@ use Prophecy\Prophecy\ObjectProphecy; use Psr\Http\Server\RequestHandlerInterface; use Shlinkio\Shlink\Rest\Middleware\CrossDomainMiddleware; -use function Laminas\Stratigility\middleware; - class CrossDomainMiddlewareTest extends TestCase { use ProphecyTrait; @@ -94,13 +90,15 @@ class CrossDomainMiddlewareTest extends TestCase * @dataProvider provideRouteResults */ public function optionsRequestParsesRouteMatchToDetermineAllowedMethods( - ?RouteResult $result, + ?string $allowHeader, string $expectedAllowedMethods ): void { $originalResponse = new Response(); - $request = (new ServerRequest())->withAttribute(RouteResult::class, $result) - ->withMethod('OPTIONS') - ->withHeader('Origin', 'local'); + if ($allowHeader !== null) { + $originalResponse = $originalResponse->withHeader('Allow', $allowHeader); + } + $request = (new ServerRequest())->withHeader('Origin', 'local') + ->withMethod('OPTIONS'); $this->handler->handle(Argument::any())->willReturn($originalResponse)->shouldBeCalledOnce(); $response = $this->middleware->process($request, $this->handler->reveal()); @@ -111,15 +109,9 @@ class CrossDomainMiddlewareTest extends TestCase public function provideRouteResults(): iterable { - yield 'with no route result' => [null, 'GET,POST,PUT,PATCH,DELETE,OPTIONS']; - yield 'with failed route result' => [RouteResult::fromRouteFailure(['POST', 'GET']), 'POST,GET']; - yield 'with success route result' => [ - RouteResult::fromRoute( - new Route('/', middleware(function (): void { - }), ['DELETE', 'PATCH', 'PUT']), - ), - 'DELETE,PATCH,PUT', - ]; + yield 'no allow header in response' => [null, 'GET,POST,PUT,PATCH,DELETE']; + yield 'allow header in response' => ['POST,GET', 'POST,GET']; + yield 'also allow header in response' => ['DELETE,PATCH,PUT', 'DELETE,PATCH,PUT']; } /**