Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: get_cookie() may not use the cookie prefix #6082

Merged
merged 4 commits into from
Jun 16, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions system/Helpers/cookie_helper.php
Original file line number Diff line number Diff line change
Expand Up @@ -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 name 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;

Expand Down
39 changes: 38 additions & 1 deletion tests/system/Helpers/CookieHelperTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

namespace CodeIgniter\Helpers;

use CodeIgniter\Config\Factories;
use CodeIgniter\Cookie\Exceptions\CookieException;
use CodeIgniter\HTTP\IncomingRequest;
use CodeIgniter\HTTP\Response;
Expand All @@ -19,6 +20,7 @@
use CodeIgniter\Test\CIUnitTestCase;
use CodeIgniter\Test\Mock\MockResponse;
use Config\App;
use Config\Cookie as CookieConfig;
use Config\Services;

/**
Expand All @@ -33,6 +35,8 @@ final class CookieHelperTest extends CIUnitTestCase

protected function setUp(): void
{
$_COOKIE = [];

parent::setUp();

$this->name = 'greetings';
Expand Down Expand Up @@ -118,11 +122,44 @@ public function testDeleteCookie()

public function testGetCookie()
{
$_COOKIE['TEST'] = 5;
$_COOKIE['TEST'] = '5';

$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);
Expand Down
1 change: 1 addition & 0 deletions user_guide_src/source/changelogs/v4.2.1.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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 <upgrade-421-get_cookie>` for details.
7 changes: 5 additions & 2 deletions user_guide_src/source/helpers/cookie_helper.rst
Original file line number Diff line number Diff line change
Expand Up @@ -39,21 +39,24 @@ The following functions are available:
a description of its use, as this function is an alias for
:php:func:`Response::setCookie() <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 <upgrade-421-get_cookie>` for details.

This helper function gives you friendlier syntax to get browser
cookies. Refer to the :doc:`IncomingRequest Library </incoming/incomingrequest>` for
detailed description of its use, as this function acts very
similarly to ``IncomingRequest::getCookie()``, except it will also prepend
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 = '']]])

Expand Down
35 changes: 35 additions & 0 deletions user_guide_src/source/installation/upgrade_421.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
*********************
Expand Down