Merge pull request #983 from acelaya-forks/feature/cors-allowed-methods

Feature/cors allowed methods

CHANGELOG.md

@@ -20,6 +20,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this
 ### Fixed
 * [#979](https://github.com/shlinkio/shlink/issues/979) Added missing `itemsPerPage` query param to swagger docs for short RULs list.
 * [#980](https://github.com/shlinkio/shlink/issues/980) Fixed value used for `Access-Control-Allow-Origin`, that could not work as expected when including an IP address.
+* [#947](https://github.com/shlinkio/shlink/issues/947) Fixed incorrect value returned in `Access-Control-Allow-Methods` header, which always contained all methods.
 
 
 ## [2.5.1] - 2021-01-21

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,
+        ]);
+    }
 }

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'];
     }
 }

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'];
     }
 
     /**