Merge pull request #16 from weierophinney/hotfix/cors-preflight-normal-segregation

Better response handling

Author: bakura10

CHANGELOG.md

@@ -1,3 +1,11 @@
+# 1.1.0
+
+- Segregate preflight vs. inflight CORS requests. Preflight detection continues
+  to happen during the "route" event. However, inflight requests are detected
+  now during the "finish" event in order to ensure they operate on the same
+  response object as will be sent back to the client.
+  ([#16](https://github.com/zf-fr/zfr-cors/pull/16))
+
 # 1.0.2
 
 - Properly set "Access-Control-Allow-Credentials" for normal requests if credentials are allowed ([#13](https://github.com/zf-fr/zfr-cors/pull/13)).

src/ZfrCors/Mvc/CorsRequestListener.php

@@ -39,6 +39,13 @@ class CorsRequestListener extends AbstractListenerAggregate
      */
     protected $corsService;
 
+    /**
+     * Whether or not a preflight request was detected
+     *
+     * @var bool
+     */
+    protected $isPreflight = false;
+
     /**
      * @param CorsService $corsService
      */
@@ -52,17 +59,24 @@ public function __construct(CorsService $corsService)
      */
     public function attach(EventManagerInterface $events)
     {
-        $this->listeners[] = $events->attach(MvcEvent::EVENT_ROUTE, array($this, 'onCorsRequest'), -1);
+        // Preflight can be handled during the route event, and should return early
+        $this->listeners[] = $events->attach(MvcEvent::EVENT_ROUTE,  array($this, 'onCorsPreflight'), -1);
+
+        // "in"flight should be handled during "FINISH" to ensure we operate on the actual route being returned
+        $this->listeners[] = $events->attach(MvcEvent::EVENT_FINISH, array($this, 'onCorsRequest'),     100);
     }
 
     /**
-     * Handle a CORS request (either preflight or normal CORS request)
+     * Handle a CORS preflight request
      *
-     * @param  MvcEvent $event
-     * @return mixed
+     * @param  MvcEvent          $event
+     * @return null|HttpResponse
      */
-    public function onCorsRequest(MvcEvent $event)
+    public function onCorsPreflight(MvcEvent $event)
     {
+        // Reset state flag
+        $this->isPreflight = false;
+
         /** @var $request HttpRequest */
         $request  = $event->getRequest();
         /** @var $response HttpResponse */
@@ -72,23 +86,49 @@ public function onCorsRequest(MvcEvent $event)
             return;
         }
 
-        // First, the preflight request
-        if ($this->corsService->isPreflightRequest($request)) {
-            return $this->corsService->createPreflightCorsResponse($request);
+        // If this isn't a preflight, done
+        if (!$this->corsService->isPreflightRequest($request)) {
+            return;
+        }
+
+        // Preflight -- return a response now!
+        $this->isPreflight = true;
+
+        return $this->corsService->createPreflightCorsResponse($request);
+    }
+
+    /**
+     * Handle a CORS request (non-preflight, normal CORS request)
+     *
+     * @param MvcEvent $event
+     */
+    public function onCorsRequest(MvcEvent $event)
+    {
+        // Do nothing if we previously created a preflight response
+        if ($this->isPreflight) {
+            return;
+        }
+
+        /** @var $request HttpRequest */
+        $request  = $event->getRequest();
+        /** @var $response HttpResponse */
+        $response = $event->getResponse();
+
+        if (!$request instanceof HttpRequest || !$this->corsService->isCorsRequest($request)) {
+            return;
         }
 
-        // Otherwise, it is the second step of the CORS request, and we let ZF continue
+        // This is the second step of the CORS request, and we let ZF continue
         // processing the response
         try {
             $response = $this->corsService->populateCorsResponse($request, $response);
-            $event->setResponse($response);
         } catch (DisallowedOriginException $exception) {
             $response = new HttpResponse(); // Clear response for security
-
             $response->setStatusCode(403)
                      ->setReasonPhrase($exception->getMessage());
 
-            return $response;
         }
+
+        $event->setResponse($response);
     }
 }

src/ZfrCors/Options/CorsOptions.php

@@ -88,7 +88,7 @@ public function getAllowedOrigins()
     }
 
     /**
-     * @param array $allowedMethods
+     * @param  array $allowedMethods
      * @return void
      */
     public function setAllowedMethods(array $allowedMethods)
@@ -109,7 +109,7 @@ public function getAllowedMethods()
     }
 
     /**
-     * @param array $allowedHeaders
+     * @param  array $allowedHeaders
      * @return void
      */
     public function setAllowedHeaders(array $allowedHeaders)
@@ -126,7 +126,7 @@ public function getAllowedHeaders()
     }
 
     /**
-     * @param int $maxAge
+     * @param  int  $maxAge
      * @return void
      */
     public function setMaxAge($maxAge)
@@ -143,7 +143,7 @@ public function getMaxAge()
     }
 
     /**
-     * @param array $exposedHeaders
+     * @param  array $exposedHeaders
      * @return void
      */
     public function setExposedHeaders(array $exposedHeaders)
@@ -160,7 +160,7 @@ public function getExposedHeaders()
     }
 
     /**
-     * @param bool $allowedCredentials
+     * @param  bool $allowedCredentials
      * @return void
      */
     public function setAllowedCredentials($allowedCredentials)

src/ZfrCors/Service/CorsService.php

@@ -51,7 +51,7 @@ public function __construct(CorsOptions $options)
      * Check if the HTTP request is a CORS request by checking if the Origin header is present and that the
      * request URI is not the same as the one in the Origin
      *
-     * @param HttpRequest $request
+     * @param  HttpRequest $request
      * @return bool
      */
     public function isCorsRequest(HttpRequest $request)
@@ -70,7 +70,7 @@ public function isCorsRequest(HttpRequest $request)
     /**
      * Check if the CORS request is a preflight request
      *
-     * @param HttpRequest $request
+     * @param  HttpRequest $request
      * @return bool
      */
     public function isPreflightRequest(HttpRequest $request)
@@ -109,8 +109,8 @@ public function createPreflightCorsResponse(HttpRequest $request)
     /**
      * Populate a simple CORS response
      *
-     * @param  HttpRequest $request
-     * @param  HttpResponse $response
+     * @param  HttpRequest               $request
+     * @param  HttpResponse              $response
      * @return HttpResponse
      * @throws DisallowedOriginException If the origin is not allowed
      */
@@ -147,7 +147,7 @@ public function populateCorsResponse(HttpRequest $request, HttpResponse $respons
                 $headers->addHeaderLine('Vary', 'Origin');
             }
         }
-        
+
         if ($this->options->getAllowedCredentials()) {
             $headers->addHeaderLine('Access-Control-Allow-Credentials', 'true');
         }

tests/ZfrCorsTest/Mvc/CorsRequestListenerTest.php

@@ -63,9 +63,13 @@ public function testAttach()
         $eventManager = $this->getMock('Zend\EventManager\EventManagerInterface');
 
         $eventManager
-            ->expects($this->once())
+            ->expects($this->at(0))
             ->method('attach')
-            ->with(MvcEvent::EVENT_ROUTE, $this->isType('callable'), $this->lessThan(0));
+            ->with(MvcEvent::EVENT_ROUTE, $this->isType('callable'), $this->LessThan(1));
+        $eventManager
+            ->expects($this->at(1))
+            ->method('attach')
+            ->with(MvcEvent::EVENT_FINISH, $this->isType('callable'), $this->greaterThan(1));
 
         $this->corsListener->attach($eventManager);
     }
@@ -79,6 +83,7 @@ public function testReturnNothingForNonCorsRequest()
         $mvcEvent->setRequest($request)
                  ->setResponse($response);
 
+        $this->assertNull($this->corsListener->onCorsPreflight($mvcEvent));
         $this->assertNull($this->corsListener->onCorsRequest($mvcEvent));
     }
 
@@ -95,7 +100,7 @@ public function testImmediatelyReturnResponseForPreflightCorsRequest()
         $mvcEvent->setRequest($request)
                  ->setResponse($response);
 
-        $this->assertInstanceOf('Zend\Http\Response', $this->corsListener->onCorsRequest($mvcEvent));
+        $this->assertInstanceOf('Zend\Http\Response', $this->corsListener->onCorsPreflight($mvcEvent));
     }
 
     public function testReturnNothingForNormalAuthorizedCorsRequest()
@@ -125,9 +130,10 @@ public function testReturnUnauthorizedResponseForNormalUnauthorizedCorsRequest()
         $mvcEvent->setRequest($request)
                  ->setResponse($response);
 
-        $newResponse = $this->corsListener->onCorsRequest($mvcEvent);
+        $this->corsListener->onCorsRequest($mvcEvent);
 
         // NOTE: a new response is created for security purpose
+        $newResponse = $mvcEvent->getResponse();
         $this->assertNotEquals($response, $newResponse);
         $this->assertEquals(403, $newResponse->getStatusCode());
         $this->assertEquals('', $newResponse->getContent());

tests/ZfrCorsTest/Util/ServiceManagerFactory.php

@@ -56,7 +56,7 @@ public static function getApplicationConfig()
     }
 
     /**
-     * @param array|null $config
+     * @param  array|null     $config
      * @return ServiceManager
      */
     public static function getServiceManager(array $config = null)