Merge pull request #5429 from kenjis/fix-security-sends-cookies

fix: Security class sends cookies immediately
codeigniter4 · Dec 8, 2021 · 76ad2ad · 76ad2ad
2 parents 23d9047 + 49cd879
commit 76ad2ad
Show file tree

Hide file tree

Showing 11 changed files with 222 additions and 89 deletions.
diff --git a/depfile.yaml b/depfile.yaml
@@ -172,6 +172,7 @@ ruleset:
   HTTP:
     - Cookie
     - Files
+    - Security
     - URI
   Images:
     - Files

diff --git a/system/Cookie/CookieStore.php b/system/Cookie/CookieStore.php
@@ -158,6 +158,8 @@ public function remove(string $name, string $prefix = '')
 
     /**
      * Dispatches all cookies in store.
+     *
+     * @deprecated Response should dispatch cookies.
      */
     public function dispatch(): void
     {
@@ -232,6 +234,8 @@ protected function validateCookies(array $cookies): void
      * Extracted call to `setrawcookie()` in order to run unit tests on it.
      *
      * @codeCoverageIgnore
+     *
+     * @deprecated
      */
     protected function setRawCookie(string $name, string $value, array $options): void
     {
@@ -242,6 +246,8 @@ protected function setRawCookie(string $name, string $value, array $options): vo
      * Extracted call to `setcookie()` in order to run unit tests on it.
      *
      * @codeCoverageIgnore
+     *
+     * @deprecated
      */
     protected function setCookie(string $name, string $value, array $options): void
     {

diff --git a/system/HTTP/ResponseTrait.php b/system/HTTP/ResponseTrait.php
@@ -16,13 +16,14 @@
 use CodeIgniter\Cookie\Exceptions\CookieException;
 use CodeIgniter\HTTP\Exceptions\HTTPException;
 use CodeIgniter\Pager\PagerInterface;
+use CodeIgniter\Security\Exceptions\SecurityException;
 use Config\Services;
 use DateTime;
 use DateTimeZone;
 use InvalidArgumentException;
 
 /**
- * Request Trait
+ * Response Trait
  *
  * Additional methods to make a PSR-7 Response class
  * compliant with the framework's own ResponseInterface.
@@ -446,7 +447,7 @@ public function send()
     }
 
     /**
-     * Sends the headers of this HTTP request to the browser.
+     * Sends the headers of this HTTP response to the browser.
      *
      * @return Response
      */
@@ -535,15 +536,15 @@ public function redirect(string $uri, string $method = 'auto', ?int $code = null
      * Accepts an arbitrary number of binds (up to 7) or an associative
      * array in the first parameter containing all the values.
      *
-     * @param array|string $name     Cookie name or array containing binds
-     * @param string       $value    Cookie value
-     * @param string       $expire   Cookie expiration time in seconds
-     * @param string       $domain   Cookie domain (e.g.: '.yourdomain.com')
-     * @param string       $path     Cookie path (default: '/')
-     * @param string       $prefix   Cookie name prefix
-     * @param bool         $secure   Whether to only transfer cookies via SSL
-     * @param bool         $httponly Whether only make the cookie accessible via HTTP (no javascript)
-     * @param string|null  $samesite
+     * @param array|Cookie|string $name     Cookie name / array containing binds / Cookie object
+     * @param string              $value    Cookie value
+     * @param string              $expire   Cookie expiration time in seconds
+     * @param string              $domain   Cookie domain (e.g.: '.yourdomain.com')
+     * @param string              $path     Cookie path (default: '/')
+     * @param string              $prefix   Cookie name prefix
+     * @param bool                $secure   Whether to only transfer cookies via SSL
+     * @param bool                $httponly Whether only make the cookie accessible via HTTP (no javascript)
+     * @param string|null         $samesite
      *
      * @return $this
      */
@@ -558,6 +559,12 @@ public function setCookie(
         $httponly = false,
         $samesite = null
     ) {
+        if ($name instanceof Cookie) {
+            $this->cookieStore = $this->cookieStore->put($name);
+
+            return $this;
+        }
+
         if (is_array($name)) {
             // always leave 'name' in last place, as the loop will break otherwise, due to $$item
             foreach (['samesite', 'value', 'expire', 'domain', 'path', 'prefix', 'secure', 'httponly', 'name'] as $item) {
@@ -689,7 +696,51 @@ protected function sendCookies()
             return;
         }
 
-        $this->cookieStore->dispatch();
+        $this->dispatchCookies();
+    }
+
+    private function dispatchCookies(): void
+    {
+        /** @var IncomingRequest $request */
+        $request = Services::request();
+
+        foreach ($this->cookieStore->display() as $cookie) {
+            if ($cookie->isSecure() && ! $request->isSecure()) {
+                throw SecurityException::forDisallowedAction();
+            }
+
+            $name    = $cookie->getPrefixedName();
+            $value   = $cookie->getValue();
+            $options = $cookie->getOptions();
+
+            if ($cookie->isRaw()) {
+                $this->doSetRawCookie($name, $value, $options);
+            } else {
+                $this->doSetCookie($name, $value, $options);
+            }
+        }
+
+        $this->cookieStore->clear();
+    }
+
+    /**
+     * Extracted call to `setrawcookie()` in order to run unit tests on it.
+     *
+     * @codeCoverageIgnore
+     */
+    private function doSetRawCookie(string $name, string $value, array $options): void
+    {
+        setrawcookie($name, $value, $options);
+    }
+
+    /**
+     * Extracted call to `setcookie()` in order to run unit tests on it.
+     *
+     * @codeCoverageIgnore
+     */
+    private function doSetCookie(string $name, string $value, array $options): void
+    {
+        setcookie($name, $value, $options);
     }
 
     /**

diff --git a/system/Security/Security.php b/system/Security/Security.php
@@ -13,6 +13,7 @@
 
 use CodeIgniter\Cookie\Cookie;
 use CodeIgniter\HTTP\RequestInterface;
+use CodeIgniter\HTTP\Response;
 use CodeIgniter\Security\Exceptions\SecurityException;
 use CodeIgniter\Session\Session;
 use Config\App;
@@ -528,13 +529,18 @@ private function saveHashInCookie(): void
                 'expires' => $this->expires === 0 ? 0 : time() + $this->expires,
             ]
         );
-        $this->sendCookie($this->request);
+
+        /** @var Response $response */
+        $response = Services::response();
+        $response->setCookie($this->cookie);
     }
 
     /**
      * CSRF Send Cookie
      *
      * @return false|Security
+     *
+     * @deprecated Set cookies to Response object instead.
      */
     protected function sendCookie(RequestInterface $request)
     {
@@ -553,6 +559,8 @@ protected function sendCookie(RequestInterface $request)
      * Extracted for this to be unit tested.
      *
      * @codeCoverageIgnore
+     *
+     * @deprecated Set cookies to Response object instead.
      */
     protected function doSendCookie(): void
     {

diff --git a/tests/system/CommonFunctionsTest.php b/tests/system/CommonFunctionsTest.php
@@ -41,10 +41,10 @@ final class CommonFunctionsTest extends CIUnitTestCase
 {
     protected function setUp(): void
     {
-        parent::setUp();
-        $renderer = Services::renderer();
-        $renderer->resetData();
         unset($_ENV['foo'], $_SERVER['foo']);
+        Services::reset();
+
+        parent::setUp();
     }
 
     public function testStringifyAttributes()

diff --git a/tests/system/HTTP/ResponseCookieTest.php b/tests/system/HTTP/ResponseCookieTest.php
@@ -65,6 +65,17 @@ public function testCookiesAll()
         $this->assertTrue($response->hasCookie('bee'));
     }
 
+    public function testSetCookieObject()
+    {
+        $cookie   = new Cookie('foo', 'bar');
+        $response = new Response(new App());
+
+        $response->setCookie($cookie);
+
+        $this->assertCount(1, $response->getCookies());
+        $this->assertTrue($response->hasCookie('foo'));
+    }
+
     public function testCookieGet()
     {
         $response = new Response(new App());

diff --git a/tests/system/HTTP/ResponseSendTest.php b/tests/system/HTTP/ResponseSendTest.php
@@ -11,8 +11,10 @@
 
 namespace CodeIgniter\HTTP;
 
+use CodeIgniter\Security\Exceptions\SecurityException;
 use CodeIgniter\Test\CIUnitTestCase;
 use Config\App;
+use Config\Services;
 
 /**
  * This test suite has been created separately from
@@ -135,4 +137,38 @@ public function testRedirectResponseCookies()
         $this->assertHeaderEmitted('Set-Cookie: foo=bar;');
         $this->assertHeaderEmitted('Set-Cookie: login_time');
     }
+
+    /**
+     * Make sure secure cookies are not sent with HTTP request
+     *
+     * @ runInSeparateProcess
+     * @ preserveGlobalState  disabled
+     */
+    public function testDoNotSendUnSecureCookie(): void
+    {
+        $this->expectException(SecurityException::class);
+        $this->expectExceptionMessage('The action you requested is not allowed');
+
+        $request = $this->createMock(IncomingRequest::class);
+        $request->method('isSecure')->willReturn(false);
+        Services::injectMock('request', $request);
+
+        $response = new Response(new App());
+        $response->pretend(false);
+        $body = 'Hello';
+        $response->setBody($body);
+
+        $response->setCookie(
+            'foo',
+            'bar',
+            '',
+            '',
+            '/',
+            '',
+            true
+        );
+
+        // send it
+        $response->send();
+    }
 }
diff --git a/tests/system/HTTP/ResponseTest.php b/tests/system/HTTP/ResponseTest.php
@@ -30,14 +30,18 @@ final class ResponseTest extends CIUnitTestCase
 
     protected function setUp(): void
     {
-        parent::setUp();
         $this->server = $_SERVER;
+
+        Services::reset();
+
+        parent::setUp();
     }
 
     protected function tearDown(): void
     {
-        $_SERVER = $this->server;
         Factories::reset('config');
+
+        $_SERVER = $this->server;
     }
 
     public function testCanSetStatusCode()