From 0720d83b07013de8e1ae84462fd8fcf99fe3afac Mon Sep 17 00:00:00 2001 From: kenjis Date: Mon, 6 Jun 2022 17:08:11 +0900 Subject: [PATCH 1/4] fix: get_cookie() may not use cookie prefix --- system/Helpers/cookie_helper.php | 20 +++++++----- tests/system/Helpers/CookieHelperTest.php | 37 +++++++++++++++++++++++ 2 files changed, 49 insertions(+), 8 deletions(-) diff --git a/system/Helpers/cookie_helper.php b/system/Helpers/cookie_helper.php index 4f3eb9a9140f..96a8bb3c998a 100755 --- a/system/Helpers/cookie_helper.php +++ b/system/Helpers/cookie_helper.php @@ -56,21 +56,25 @@ function set_cookie( /** * Fetch an item from the $_COOKIE array * - * @param string $index + * @param string $index + * @param string|null $prefix Cookie prefix. + * '': the prefix in Config\Cookie + * null: no prefix * - * @return mixed + * @return array|string|null * * @see \CodeIgniter\HTTP\IncomingRequest::getCookie() */ - function get_cookie($index, bool $xssClean = false) + function get_cookie($index, bool $xssClean = false, ?string $prefix = '') { - /** @var Cookie|null $cookie */ - $cookie = config('Cookie'); + if ($prefix === '') { + /** @var Cookie|null $cookie */ + $cookie = config('Cookie'); - // @TODO Remove Config\App fallback when deprecated `App` members are removed. - $cookiePrefix = $cookie instanceof Cookie ? $cookie->prefix : config(App::class)->cookiePrefix; + // @TODO Remove Config\App fallback when deprecated `App` members are removed. + $prefix = $cookie instanceof Cookie ? $cookie->prefix : config(App::class)->cookiePrefix; + } - $prefix = isset($_COOKIE[$index]) ? '' : $cookiePrefix; $request = Services::request(); $filter = $xssClean ? FILTER_SANITIZE_FULL_SPECIAL_CHARS : FILTER_DEFAULT; diff --git a/tests/system/Helpers/CookieHelperTest.php b/tests/system/Helpers/CookieHelperTest.php index 2b09aa2afb3a..e9a208bd03a1 100755 --- a/tests/system/Helpers/CookieHelperTest.php +++ b/tests/system/Helpers/CookieHelperTest.php @@ -11,6 +11,7 @@ namespace CodeIgniter\Helpers; +use CodeIgniter\Config\Factories; use CodeIgniter\Cookie\Exceptions\CookieException; use CodeIgniter\HTTP\IncomingRequest; use CodeIgniter\HTTP\Response; @@ -19,6 +20,7 @@ use CodeIgniter\Test\CIUnitTestCase; use CodeIgniter\Test\Mock\MockResponse; use Config\App; +use Config\Cookie as CookieConfig; use Config\Services; /** @@ -33,6 +35,8 @@ final class CookieHelperTest extends CIUnitTestCase protected function setUp(): void { + $_COOKIE = []; + parent::setUp(); $this->name = 'greetings'; @@ -123,6 +127,39 @@ public function testGetCookie() $this->assertSame('5', get_cookie('TEST')); } + public function testGetCookieDefaultPrefix() + { + $_COOKIE['prefix_TEST'] = '5'; + + $config = new CookieConfig(); + $config->prefix = 'prefix_'; + Factories::injectMock('config', CookieConfig::class, $config); + + $this->assertSame('5', get_cookie('TEST', false, '')); + } + + public function testGetCookiePrefix() + { + $_COOKIE['abc_TEST'] = '5'; + + $config = new CookieConfig(); + $config->prefix = 'prefix_'; + Factories::injectMock('config', CookieConfig::class, $config); + + $this->assertSame('5', get_cookie('TEST', false, 'abc_')); + } + + public function testGetCookieNoPrefix() + { + $_COOKIE['abc_TEST'] = '5'; + + $config = new CookieConfig(); + $config->prefix = 'prefix_'; + Factories::injectMock('config', CookieConfig::class, $config); + + $this->assertSame('5', get_cookie('abc_TEST', false, null)); + } + public function testDeleteCookieAfterLastSet() { delete_cookie($this->name); From e23f9402674324e637620dc33f695c727e44030a Mon Sep 17 00:00:00 2001 From: kenjis Date: Mon, 6 Jun 2022 17:19:02 +0900 Subject: [PATCH 2/4] test: fix $_COOKIE value It should be a string. --- tests/system/Helpers/CookieHelperTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/system/Helpers/CookieHelperTest.php b/tests/system/Helpers/CookieHelperTest.php index e9a208bd03a1..640a43cb3551 100755 --- a/tests/system/Helpers/CookieHelperTest.php +++ b/tests/system/Helpers/CookieHelperTest.php @@ -122,7 +122,7 @@ public function testDeleteCookie() public function testGetCookie() { - $_COOKIE['TEST'] = 5; + $_COOKIE['TEST'] = '5'; $this->assertSame('5', get_cookie('TEST')); } From a09683de94b8913b93ca8808a1aee91f2efdf608 Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 8 Jun 2022 15:20:36 +0900 Subject: [PATCH 3/4] docs: fix doc comment --- system/Helpers/cookie_helper.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/system/Helpers/cookie_helper.php b/system/Helpers/cookie_helper.php index 96a8bb3c998a..5537de9c3899 100755 --- a/system/Helpers/cookie_helper.php +++ b/system/Helpers/cookie_helper.php @@ -57,7 +57,7 @@ function set_cookie( * Fetch an item from the $_COOKIE array * * @param string $index - * @param string|null $prefix Cookie prefix. + * @param string|null $prefix Cookie name prefix. * '': the prefix in Config\Cookie * null: no prefix * From 4d3e4c2c22dd9ce56edfedd629039c797a059ce6 Mon Sep 17 00:00:00 2001 From: kenjis Date: Fri, 10 Jun 2022 11:28:11 +0900 Subject: [PATCH 4/4] docs: add documentation --- user_guide_src/source/changelogs/v4.2.1.rst | 1 + .../source/helpers/cookie_helper.rst | 7 ++-- .../source/installation/upgrade_421.rst | 35 +++++++++++++++++++ 3 files changed, 41 insertions(+), 2 deletions(-) diff --git a/user_guide_src/source/changelogs/v4.2.1.rst b/user_guide_src/source/changelogs/v4.2.1.rst index d591d08cf008..fddd5ca08f7d 100644 --- a/user_guide_src/source/changelogs/v4.2.1.rst +++ b/user_guide_src/source/changelogs/v4.2.1.rst @@ -16,3 +16,4 @@ Behavior Changes ================ - Guessing the file extension from the MIME type has been changed if the proposed extension is not valid. Previously, the guessing will early terminate and return ``null``. Now, if a proposed extension is given and is invalid, the MIME guessing will continue checking using the mapping of extension to MIME types. +- If there is a cookie with a prefixed name and a cookie with the same name without a prefix, the previous ``get_cookie()`` had the tricky behavior of returning the cookie without the prefix. Now the behavior has been fixed as a bug, and has been changed. See :ref:`Upgrading ` for details. diff --git a/user_guide_src/source/helpers/cookie_helper.rst b/user_guide_src/source/helpers/cookie_helper.rst index 5976c48ea99a..b3328c3dc466 100755 --- a/user_guide_src/source/helpers/cookie_helper.rst +++ b/user_guide_src/source/helpers/cookie_helper.rst @@ -39,13 +39,16 @@ The following functions are available: a description of its use, as this function is an alias for :php:func:`Response::setCookie() `. -.. php:function:: get_cookie($index[, $xssClean = false]) +.. php:function:: get_cookie($index[, $xssClean = false[, $prefix = '']]) :param string $index: Cookie name :param bool $xssClean: Whether to apply XSS filtering to the returned value + :param string|null $prefix: Cookie name prefix. If set to ``''``, the default value from **app/Config/Cookie.php** will be used. If set to ``null``, no prefix :returns: The cookie value or null if not found :rtype: mixed + .. note:: Since v4.2.1, the third parameter ``$prefix`` has been introduced and the behavior has been changed a bit due to a bug fix. See :ref:`Upgrading ` for details. + This helper function gives you friendlier syntax to get browser cookies. Refer to the :doc:`IncomingRequest Library ` for detailed description of its use, as this function acts very @@ -53,7 +56,7 @@ The following functions are available: the ``Config\Cookie::$prefix`` that you might've set in your **app/Config/Cookie.php** file. -.. warning:: Using XSS filtering is a bad practice. It does not prevent XSS attacks perfectly. Using ``esc()`` with the correct ``$context`` in the views is recommended. + .. warning:: Using XSS filtering is a bad practice. It does not prevent XSS attacks perfectly. Using ``esc()`` with the correct ``$context`` in the views is recommended. .. php:function:: delete_cookie($name[, $domain = ''[, $path = '/'[, $prefix = '']]]) diff --git a/user_guide_src/source/installation/upgrade_421.rst b/user_guide_src/source/installation/upgrade_421.rst index 9359d8ecc796..6b3b0f7cb995 100644 --- a/user_guide_src/source/installation/upgrade_421.rst +++ b/user_guide_src/source/installation/upgrade_421.rst @@ -23,6 +23,41 @@ app/Config/Mimes.php Breaking Changes **************** +.. _upgrade-421-get_cookie: + +get_cookie() +============ + +If there is a cookie with a prefixed name and a cookie with the same name without a prefix, the previous ``get_cookie()`` had the tricky behavior of returning the cookie without the prefix. + +For example, when ``Config\Cookie::$prefix`` is ``prefix_``, there are two cookies, ``test`` and ``prefix_test``: + +.. code-block:: php + + $_COOKIES = [ + 'test' => 'Non CI Cookie', + 'prefix_test' => 'CI Cookie', + ]; + +Previously, ``get_cookie()`` returns the following: + +.. code-block:: php + + get_cookie('test'); // returns "Non CI Cookie" + get_cookie('prefix_test'); // returns "CI Cookie" + +Now the behavior has been fixed as a bug, and has been changed like the following. + +.. code-block:: php + + get_cookie('test'); // returns "CI Cookie" + get_cookie('prefix_test'); // returns null + get_cookie('test', false, null); // returns "Non CI Cookie" + +If you depend on the previous behavior, you need to change your code. + +.. note:: In the example above, if there is only one cookie ``prefix_test``, + the previous ``get_cookie('test')`` also returns ``"CI Cookie"``. Breaking Enhancements *********************