Merge pull request #6544 from kenjis/fix-set-cookie

fix: set_cookie() does not use Config\Cookie values

Author: kenjis

system/HTTP/ResponseTrait.php

@@ -17,6 +17,7 @@
 use CodeIgniter\HTTP\Exceptions\HTTPException;
 use CodeIgniter\Pager\PagerInterface;
 use CodeIgniter\Security\Exceptions\SecurityException;
+use Config\Cookie as CookieConfig;
 use Config\Services;
 use DateTime;
 use DateTimeZone;
@@ -544,8 +545,8 @@ public function redirect(string $uri, string $method = 'auto', ?int $code = null
      * @param string              $domain   Cookie domain (e.g.: '.yourdomain.com')
      * @param string              $path     Cookie path (default: '/')
      * @param string              $prefix   Cookie name prefix ('': the default 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 bool|null           $secure   Whether to only transfer cookies via SSL
+     * @param bool|null           $httponly Whether only make the cookie accessible via HTTP (no javascript)
      * @param string|null         $samesite
      *
      * @return $this
@@ -557,8 +558,8 @@ public function setCookie(
         $domain = '',
         $path = '/',
         $prefix = '',
-        $secure = false,
-        $httponly = false,
+        $secure = null,
+        $httponly = null,
         $samesite = null
     ) {
         if ($name instanceof Cookie) {
@@ -567,8 +568,17 @@ public function setCookie(
             return $this;
         }
 
+        /** @var CookieConfig|null $cookieConfig */
+        $cookieConfig = config('Cookie');
+
+        if ($cookieConfig instanceof CookieConfig) {
+            $secure ??= $cookieConfig->secure;
+            $httponly ??= $cookieConfig->httponly;
+            $samesite ??= $cookieConfig->samesite;
+        }
+
         if (is_array($name)) {
-            // always leave 'name' in last place, as the loop will break otherwise, due to $$item
+            // 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) {
                 if (isset($name[$item])) {
                     ${$item} = $name[$item];

system/Helpers/cookie_helper.php

@@ -30,8 +30,8 @@
      * @param string       $domain   For site-wide cookie. Usually: .yourdomain.com
      * @param string       $path     The cookie path
      * @param string       $prefix   The cookie prefix ('': the default prefix)
-     * @param bool         $secure   True makes the cookie secure
-     * @param bool         $httpOnly True makes the cookie accessible via http(s) only (no javascript)
+     * @param bool|null    $secure   True makes the cookie secure
+     * @param bool|null    $httpOnly True makes the cookie accessible via http(s) only (no javascript)
      * @param string|null  $sameSite The cookie SameSite value
      *
      * @see \CodeIgniter\HTTP\Response::setCookie()
@@ -43,8 +43,8 @@ function set_cookie(
         string $domain = '',
         string $path = '/',
         string $prefix = '',
-        bool $secure = false,
-        bool $httpOnly = false,
+        ?bool $secure = null,
+        ?bool $httpOnly = null,
         ?string $sameSite = null
     ) {
         $response = Services::response();

tests/system/HTTP/ResponseCookieTest.php

@@ -11,6 +11,7 @@
 
 namespace CodeIgniter\HTTP;
 
+use CodeIgniter\Config\Factories;
 use CodeIgniter\Cookie\Cookie;
 use CodeIgniter\Cookie\CookieStore;
 use CodeIgniter\Cookie\Exceptions\CookieException;
@@ -135,11 +136,11 @@ public function testCookieHTTPOnly()
 
         $response->setCookie('foo', 'bar');
         $cookie = $response->getCookie('foo');
-        $this->assertFalse($cookie->isHTTPOnly());
+        $this->assertTrue($cookie->isHTTPOnly());
 
-        $response->setCookie(['name' => 'bee', 'value' => 'bop', 'httponly' => true]);
+        $response->setCookie(['name' => 'bee', 'value' => 'bop', 'httponly' => false]);
         $cookie = $response->getCookie('bee');
-        $this->assertTrue($cookie->isHTTPOnly());
+        $this->assertFalse($cookie->isHTTPOnly());
     }
 
     public function testCookieExpiry()
@@ -255,6 +256,7 @@ public function testCookieStrictSetSameSite()
 
     public function testCookieBlankSetSameSite()
     {
+        /** @var CookieConfig $config */
         $config           = config('Cookie');
         $config->samesite = '';
         $response         = new Response(new App());
@@ -314,6 +316,30 @@ public function testCookieInvalidSameSite()
         ]);
     }
 
+    public function testSetCookieConfigCookieIsUsed()
+    {
+        /** @var CookieConfig $config */
+        $config           = config('Cookie');
+        $config->secure   = true;
+        $config->httponly = true;
+        $config->samesite = 'None';
+        Factories::injectMock('config', 'Cookie', $config);
+
+        $cookieAttr = [
+            'name'   => 'bar',
+            'value'  => 'foo',
+            'expire' => 9999,
+        ];
+        $response = new Response(new App());
+        $response->setCookie($cookieAttr);
+
+        $cookie  = $response->getCookie('bar');
+        $options = $cookie->getOptions();
+        $this->assertTrue($options['secure']);
+        $this->assertTrue($options['httponly']);
+        $this->assertSame('None', $options['samesite']);
+    }
+
     public function testGetCookieStore()
     {
         $response = new Response(new App());

tests/system/Helpers/CookieHelperTest.php

@@ -20,6 +20,7 @@
 use CodeIgniter\Test\CIUnitTestCase;
 use CodeIgniter\Test\Mock\MockResponse;
 use Config\App;
+use Config\Cookie;
 use Config\Cookie as CookieConfig;
 use Config\Services;
 
@@ -89,6 +90,31 @@ public function testSetCookieByArrayParameters()
         delete_cookie($this->name);
     }
 
+    public function testSetCookieConfigCookieIsUsed()
+    {
+        /** @var Cookie $config */
+        $config           = config('Cookie');
+        $config->secure   = true;
+        $config->httponly = true;
+        $config->samesite = 'None';
+        Factories::injectMock('config', 'Cookie', $config);
+
+        $cookieAttr = [
+            'name'   => $this->name,
+            'value'  => $this->value,
+            'expire' => $this->expire,
+        ];
+        set_cookie($cookieAttr);
+
+        $cookie  = $this->response->getCookie($this->name);
+        $options = $cookie->getOptions();
+        $this->assertTrue($options['secure']);
+        $this->assertTrue($options['httponly']);
+        $this->assertSame('None', $options['samesite']);
+
+        delete_cookie($this->name);
+    }
+
     public function testSetCookieSecured()
     {
         $pre       = 'Hello, I try to';

user_guide_src/source/changelogs/v4.2.7.rst

@@ -12,6 +12,7 @@ Release Date: Unreleased
 BREAKING
 ********
 
+- The default values of the parameters in :php:func:`set_cookie()` and :php:meth:`CodeIgniter\\HTTP\\Response::setCookie()` has been fixed. Now the default values of ``$secure`` and ``$httponly`` are ``null``, and these values will be replaced with the ``Config\Cookie`` values.
 -  ``Time::__toString()`` is now locale-independent. It returns database-compatible strings like '2022-09-07 12:00:00' in any locale.
 
 Enhancements

user_guide_src/source/helpers/cookie_helper.rst

@@ -29,11 +29,14 @@ The following functions are available:
     :param    string    $domain: Cookie domain (usually: .yourdomain.com)
     :param    string    $path: Cookie path
     :param    string    $prefix: Cookie name prefix. If ``''``, the default from **app/Config/Cookie.php** is used
-    :param    bool    $secure: Whether to only send the cookie through HTTPS
-    :param    bool    $httpOnly: Whether to hide the cookie from JavaScript
+    :param    bool    $secure: Whether to only send the cookie through HTTPS. If ``null``, the default from **app/Config/Cookie.php** is used
+    :param    bool    $httpOnly: Whether to hide the cookie from JavaScript. If ``null``, the default from **app/Config/Cookie.php** is used
     :param    string    $sameSite: The value for the SameSite cookie parameter. If ``null``, the default from **app/Config/Cookie.php** is used
     :rtype:    void
 
+    .. note:: Prior to v4.2.7, the default values of ``$secure`` and ``$httpOnly`` were ``false``
+        due to a bug, and these values from **app/Config/Cookie.php** were never used.
+
     This helper function gives you friendlier syntax to set browser
     cookies. Refer to the :doc:`Response Library </outgoing/response>` for
     a description of its use, as this function is an alias for

user_guide_src/source/installation/upgrade_427.rst

@@ -15,6 +15,43 @@ Please refer to the upgrade instructions corresponding to your installation meth
 Breaking Changes
 ****************
 
+set_cookie()
+============
+
+Due to a bug, :php:func:`set_cookie()` and :php:meth:`CodeIgniter\\HTTP\\Response::setCookie()`
+in the previous versions did not use the ``$secure`` and ``$httponly`` values in ``Config\Cookie``.
+The following code did not issue a cookie with the secure flag even if you set ``$secure = true``
+in ``Config\Cookie``::
+
+    helper('cookie');
+
+    $cookie = [
+        'name'  => $name,
+        'value' => $value,
+    ];
+    set_cookie($cookie);
+    // or
+    $this->response->setCookie($cookie);
+
+But now the values in ``Config\Cookie`` are used for the options that are not specified.
+The above code issues a cookie with the secure flag if you set ``$secure = true``
+in ``Config\Cookie``.
+
+If your code depends on this bug, please change it to explicitly specify the necessary options::
+
+    $cookie = [
+        'name'     => $name,
+        'value'    => $value,
+        'secure'   => false, // Set explicitly
+        'httponly' => false, // Set explicitly
+    ];
+    set_cookie($cookie);
+    // or
+    $this->response->setCookie($cookie);
+
+Others
+======
+
 -  ``Time::__toString()`` is now locale-independent. It returns database-compatible strings like '2022-09-07 12:00:00' in any locale. Most locales are not affected by this change. But in a few locales like `ar`, `fa`, ``Time::__toString()`` (or ``(string) $time`` or implicit casting to a string) no longer returns a localized datetime string. if you want to get a localized datetime string, use :ref:`Time::toDateTimeString() <time-todatetimestring>` instead.
 
 Project Files

user_guide_src/source/outgoing/response.rst

@@ -370,11 +370,14 @@ The methods provided by the parent class that are available are:
         :param string $domain: Cookie domain
         :param string $path: Cookie path
         :param string $prefix: Cookie name prefix. If set to ``''``, the default value from **app/Config/Cookie.php** will be used
-        :param bool $secure: Whether to only transfer the cookie through HTTPS
-        :param bool $httponly: Whether to only make the cookie accessible for HTTP requests (no JavaScript)
+        :param bool $secure: Whether to only transfer the cookie through HTTPS. If set to ``null``, the default value from **app/Config/Cookie.php** will be used
+        :param bool $httponly: Whether to only make the cookie accessible for HTTP requests (no JavaScript). If set to ``null``, the default value from **app/Config/Cookie.php** will be used
         :param string $samesite: The value for the SameSite cookie parameter. If set to ``''``, no SameSite attribute will be set on the cookie. If set to ``null``, the default value from **app/Config/Cookie.php** will be used
         :rtype: void
 
+        .. note:: Prior to v4.2.7, the default values of ``$secure`` and ``$httponly`` were ``false``
+            due to a bug, and these values from **app/Config/Cookie.php** were never used.
+
         Sets a cookie containing the values you specify. There are two ways to
         pass information to this method so that a cookie can be set: Array
         Method, and Discrete Parameters: