Bug: get_cookie() cookie prefix behavior

[colethorsen]

PHP Version

7.3

CodeIgniter4 Version

4.1.9

CodeIgniter4 Installation Method

Composer (as dependency to an existing project)

Which operating systems have you tested for this bug?

macOS, Linux

Which server did you use?

apache

Database

MySQL 8

What happened?

The cookie prefixing to prevent collisions doesn't exactly work as expected. There is an issue with at least the get_cookie() helper function. The cookie prefix is specifically toted as a way to prevent collisions, however this function doesn't solve for collisions.

    function get_cookie($index, bool $xssClean = false)
    {
        $prefix  = isset($_COOKIE[$index]) ? '' : config(App::class)->cookiePrefix;
        $request = Services::request();
        $filter  = $xssClean ? FILTER_SANITIZE_FULL_SPECIAL_CHARS : FILTER_DEFAULT;

        return $request->getCookie($prefix . $index, $filter);
    }

This function looks to see if the base cookie without the prefix exists, and if it does not exist then it looks for the prefixed cookie. It should look for the prefixed cookie and reject the un-prefixed cookie entirely, otherwise it creates collisions.

Steps to Reproduce

If you take the following example:

Config/App.php
public $cookiePrefix = 'prefix_';

Theoretical array of cookies:

$_COOKIES = [
    'prefix_test' => 'Right Value',
    'text'        => 'Wrong Value',
];

get_cookie('test');

returns 'Wrong Value' by default, ideally it should return 'Right Value' and even if 'prefix_text' is not set, it should return null and never 'Wrong Value'

Expected Output

returns 'Wrong Value' by default, ideally it should return 'Right Value' and even if 'prefix_text' is not set, it should return null and never 'Wrong Value'

Anything else?

This could be fairly easily fixed by just enforcing the prefix:
$prefix = config(App::class)->cookiePrefix ?? '';

However, I'm not sure if this should be enforced at a deeper stage of getting the cookie, and/or the function should be extended to allow for a prefix to be set independently if desired like:

    function get_cookie($index, bool $xssClean = false, string $prefix = null)
    {
        $prefix  = $prefix !== null ? $prefix : config(App::class)->cookiePrefix;
        $request = Services::request();
        $filter  = $xssClean ? FILTER_SANITIZE_FULL_SPECIAL_CHARS : FILTER_DEFAULT;

        return $request->getCookie($prefix . $index, $filter);
    }

[kenjis]

I don't know this is a bug or not.
It came from CI3:
https://github.com/bcit-ci/CodeIgniter/blob/87f9ac6ca2eb125f5991243865e7e5e96fd3987b/system/helpers/cookie_helper.php#L89-L93

[paulbalandan]

In my understanding of the function, $index is assumed to be the prefixed cookie name because of this check:
$prefix = isset($_COOKIE[$index]) ? '' : config(App::class)->cookiePrefix;

If a cookie with the name $index (assumed prefixed) already exists in the $_COOKIE array, then there's no need to get the prefix and prepend that to the name.

[kenjis]

If we set prefix in the config, all issued cookies by CI have the prefixed names.

This helper function gives you friendlier syntax to get browser cookies. Refer to the IncomingRequest Library 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.
https://codeigniter4.github.io/CodeIgniter4/helpers/cookie_helper.html#get_cookie

Unlike the Cookie Helper function get_cookie(), this method does NOT prepend your configured Config\Cookie::$prefix value.
https://codeigniter4.github.io/CodeIgniter4/incoming/incomingrequest.html#getGetPost

It seems the user guide says get_cookie() always prepend the prefix.

When the prefix is prefix_ and

$_COOKIES = [
    'prefix_test' => 'CI cookie',
    'test'        => 'Non CI cookie',
];

get_cookie('test'); // Non CI cookie
get_cookie('prefix_test'); // CI cookie

Why do we need to get cookie test? It is not issued by CI.
Why do we need to get prefix_testto get the CI cookie.

[colethorsen]

If there is no way to reliably only get the prefixed cookie and reject all others, then there isn't really any point to the entire cookie prefixing setup as it would not save collisions between 2 otherwise similar apps installed on the same domain, and if its not solving this problem, then what problem is it solving exactly?

[kenjis]

@colethorsen Good question!
The current my answer is "I don't know".

[kenjis]

I thought about this.

It seems the current get_cookie() wants to get all cookies including out of CI.
If we change the behavior, it can't get test cookie:

$_COOKIES = [
    'prefix_test' => 'CI cookie',
    'test'        => 'Non CI cookie',
];

If you want to get CI cookie surely, you must call with prefixed name like get_cookie('prefix_test').

Even if the current behavior is correct, the user guide explanation is not correct.

Probably we have to go one way or the other.

1. Fix the user guide
2. Change this behavior

[colethorsen]

If you were to “fix the guide” then there is basically no useful way to globally prefix all the cookies and that entire functionality wouldn’t really be relevant. There is already the current cookie functionality incomingrequest::getCookie() which will allow users to get any cookie prefixed or not that they may need to. get_cookie() should work in tandem with the set_cookie() which prefixes or they should both be adjusted to just be direct shortcuts to the incomingrequest::getCookie() and additional functions added that work with the prefix setup. Given that the incoming request cookie functionality exists I don’t really see a reason why the get_cookie doesn’t function with the prefix as that’s how it’s always been documented and without it the entire prefixing setup as it’s designed doesn’t really work.

[kenjis]

set_cookie($name[, $value = ''[, $expire = ''[, $domain = ''[, $path = '/'[, $prefix = ''[, $secure = false[, $httpOnly = false[, $sameSite = '']]]]]]]])
https://codeigniter4.github.io/userguide/helpers/cookie_helper.html#set_cookie

This function has the parameter $prefix.
This is not consistent either.

I certainly agree that the current get_cookie() behavior is strange.

[colethorsen]

The set_cookie works a bit differently if you start digging into it. There's basically an option within the Cookie object that will either use the prefix that is manually sent initializing the cookie (set_cookie is basically a shortcut to doing this), or it will fall back to the default global prefix (or that which was defined using the static function setDefaults

I'm not overly sure of what real world benefits being able to set other prefixes has at the get_cookie and set_cookie level given their use cases, but at the Cookie object level, it allows for multiple different prefixes to be used in the same app, which there could probably be a use case for at some point.

[kenjis]

@colethorsen I sent another PR: #6082
How about this?

[colethorsen]

Sure this appears to fix it it more or less the same way, except adding in support for a prefix from another spot where the prefix could be set. While not addressing the second part of the cookie prefix problem.

I'm using #6024 in production and will continue to do so as this only addresses part of the issue with cookie prefix inconsistencies.

[colethorsen]

After testing it out more, your PR has a major implementation drawback, that it can't actually check for an unprefixed cookie if you wanted to, where as the #6024 can because of the way its configured:

using:

function get_cookie($index, bool $xssClean = false, ?string $prefix = '')

and then checking to see if its empty would always lead to different prefix, whereas using:

function get_cookie($index, bool $xssClean = false, ?string $prefix = null)

and then checking for null would allow you explicitly check for an unprefixed cookie by using get_cookie('whole_cookie_name', true, '')

[kenjis]

Thank you for looking into #6082.

But sorry, I don't get what you say.
I think it can check for all cookies, and it solves all the cookie prefix problems.

What's the second part of the cookie prefix problem?

it can't actually check for an unprefixed cookie if you wanted to

get_cookie($index, $xssClean, null) returns unprefixed cookie.

[colethorsen]

Sorry you're right it would solve it just in the opposite manner by passing null instead of an empty string.

The second part of the problem is that the session cookie sometimes adds the prefix and sometimes doesn't and ends up having multiple cookies and/or multiple sessions. This is solved in the #6024

[kenjis]

Thank you for making it clear.

Session cookie should not have the prefix, so there is a bug in Session.

[kenjis]

I've found the Session cookie name bug. #6091