From 6bc30542a029610785ef5717a882e592df87b82b Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandro@alejandrocelaya.com>
Date: Sun, 12 Jan 2020 10:58:00 +0100
Subject: [PATCH] Updated cross domain middleware so that it always returns
 success response on OPTIONS requests

---
 .../src/Middleware/CrossDomainMiddleware.php  |  9 ++--
 .../Middleware/CrossDomainMiddlewareTest.php  | 44 +++++++++++++++++--
 2 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/module/Rest/src/Middleware/CrossDomainMiddleware.php b/module/Rest/src/Middleware/CrossDomainMiddleware.php
index aacda9fc..09a7cc03 100644
--- a/module/Rest/src/Middleware/CrossDomainMiddleware.php
+++ b/module/Rest/src/Middleware/CrossDomainMiddleware.php
@@ -10,8 +10,10 @@ use Psr\Http\Message\ServerRequestInterface;
 use Psr\Http\Server\MiddlewareInterface;
 use Psr\Http\Server\RequestHandlerInterface;
 use Shlinkio\Shlink\Rest\Authentication;
+use Zend\Diactoros\Response\EmptyResponse;;
 use Zend\Expressive\Router\RouteResult;
 
+use function array_merge;
 use function implode;
 
 class CrossDomainMiddleware implements MiddlewareInterface, RequestMethodInterface
@@ -54,10 +56,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 1716c19e..317b8603 100644
--- a/module/Rest/test/Middleware/CrossDomainMiddlewareTest.php
+++ b/module/Rest/test/Middleware/CrossDomainMiddlewareTest.php
@@ -34,14 +34,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);
@@ -96,6 +96,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());
     }
 
     /**
@@ -115,6 +116,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
@@ -129,4 +131,40 @@ 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];
+    }
 }