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: set_cookie() does not use Config\Cookie values #6544

Merged
merged 6 commits into from
Sep 20, 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: 15 additions & 5 deletions system/HTTP/ResponseTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -557,8 +558,8 @@ public function setCookie(
$domain = '',
$path = '/',
$prefix = '',
$secure = false,
$httponly = false,
$secure = null,
$httponly = null,
$samesite = null
) {
if ($name instanceof Cookie) {
Expand All @@ -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];
Expand Down
8 changes: 4 additions & 4 deletions system/Helpers/cookie_helper.php
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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();
Expand Down
32 changes: 29 additions & 3 deletions tests/system/HTTP/ResponseCookieTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

namespace CodeIgniter\HTTP;

use CodeIgniter\Config\Factories;
use CodeIgniter\Cookie\Cookie;
use CodeIgniter\Cookie\CookieStore;
use CodeIgniter\Cookie\Exceptions\CookieException;
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -255,6 +256,7 @@ public function testCookieStrictSetSameSite()

public function testCookieBlankSetSameSite()
{
/** @var CookieConfig $config */
$config = config('Cookie');
$config->samesite = '';
$response = new Response(new App());
Expand Down Expand Up @@ -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());
Expand Down
26 changes: 26 additions & 0 deletions tests/system/Helpers/CookieHelperTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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';
Expand Down
1 change: 1 addition & 0 deletions user_guide_src/source/changelogs/v4.2.7.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
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 @@ -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
Expand Down
37 changes: 37 additions & 0 deletions user_guide_src/source/installation/upgrade_427.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 5 additions & 2 deletions user_guide_src/source/outgoing/response.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down