Skip to content

refactor: remove Cookie config items in Config\App #7011

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.

Sign up for GitHub

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

Closed
wants to merge 8 commits into from
+103 −202
Closed

refactor: remove Cookie config items in Config\App #7011

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ac27184
refactor: remove Cookie config items in Config\App
kenjis Dec 23, 2022
ece1c57
refactor: add @var to specify type
kenjis Dec 23, 2022
ab2a0e0
refactor: private method
kenjis Dec 23, 2022
fc5dc3b
test: remove test that are no longer needed
kenjis Dec 23, 2022
a27dbb1
docs: add @phpstan-var
kenjis Dec 23, 2022
a870044
test: update failed tests
kenjis Dec 23, 2022
26b554a
test: remove test that are no longer needed
kenjis Dec 23, 2022
68683ba
test: update tests
kenjis Dec 23, 2022
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
79 changes: 0 additions & 79 deletions app/Config/App.php
View file
Original file line number Diff line number Diff line change
@@ -243,85 +243,6 @@ class App extends BaseConfig
*/
public ?string $sessionDBGroup = null;

/**
* --------------------------------------------------------------------------
* Cookie Prefix
* --------------------------------------------------------------------------
*
* Set a cookie name prefix if you need to avoid collisions.
*
* @deprecated use Config\Cookie::$prefix property instead.
*/
public string $cookiePrefix = '';

/**
* --------------------------------------------------------------------------
* Cookie Domain
* --------------------------------------------------------------------------
*
* Set to `.your-domain.com` for site-wide cookies.
*
* @deprecated use Config\Cookie::$domain property instead.
*/
public string $cookieDomain = '';

/**
* --------------------------------------------------------------------------
* Cookie Path
* --------------------------------------------------------------------------
*
* Typically will be a forward slash.
*
* @deprecated use Config\Cookie::$path property instead.
*/
public string $cookiePath = '/';

/**
* --------------------------------------------------------------------------
* Cookie Secure
* --------------------------------------------------------------------------
*
* Cookie will only be set if a secure HTTPS connection exists.
*
* @deprecated use Config\Cookie::$secure property instead.
*/
public bool $cookieSecure = false;

/**
* --------------------------------------------------------------------------
* Cookie HttpOnly
* --------------------------------------------------------------------------
*
* Cookie will only be accessible via HTTP(S) (no JavaScript).
*
* @deprecated use Config\Cookie::$httponly property instead.
*/
public bool $cookieHTTPOnly = true;

/**
* --------------------------------------------------------------------------
* Cookie SameSite
* --------------------------------------------------------------------------
*
* Configure cookie SameSite setting. Allowed values are:
* - None
* - Lax
* - Strict
* - ''
*
* Alternatively, you can use the constant names:
* - `Cookie::SAMESITE_NONE`
* - `Cookie::SAMESITE_LAX`
* - `Cookie::SAMESITE_STRICT`
*
* Defaults to `Lax` for compatibility with modern browsers. Setting `''`
* (empty string) means default SameSite attribute set by browsers (`Lax`)
* will be set on cookies. If set to `None`, `$cookieSecure` must also be set.
*
* @deprecated use Config\Cookie::$samesite property instead.
*/
public ?string $cookieSameSite = 'Lax';

/**
* --------------------------------------------------------------------------
* Reverse Proxy IPs
2 changes: 2 additions & 0 deletions app/Config/Cookie.php
View file
Original file line number Diff line number Diff line change
@@ -84,6 +84,8 @@ class Cookie extends BaseConfig
* Defaults to `Lax` for compatibility with modern browsers. Setting `''`
* (empty string) means default SameSite attribute set by browsers (`Lax`)
* will be set on cookies. If set to `None`, `$secure` must also be set.
*
* @phpstan-var 'None'|'Lax'|'Strict'|''
*/
public string $samesite = 'Lax';

29 changes: 5 additions & 24 deletions system/HTTP/Response.php
View file
Original file line number Diff line number Diff line change
@@ -13,9 +13,9 @@

use CodeIgniter\Cookie\Cookie;
use CodeIgniter\Cookie\CookieStore;
use CodeIgniter\Cookie\Exceptions\CookieException;
use CodeIgniter\HTTP\Exceptions\HTTPException;
use Config\App;
use Config\Cookie as CookieConfig;
use Config\Services;

/**
@@ -156,31 +156,12 @@ public function __construct($config)

$this->CSPEnabled = $config->CSPEnabled;

// DEPRECATED COOKIE MANAGEMENT

$this->cookiePrefix = $config->cookiePrefix;
$this->cookieDomain = $config->cookieDomain;
$this->cookiePath = $config->cookiePath;
$this->cookieSecure = $config->cookieSecure;
$this->cookieHTTPOnly = $config->cookieHTTPOnly;
$this->cookieSameSite = $config->cookieSameSite ?? Cookie::SAMESITE_LAX;
$this->cookieStore = new CookieStore([]);

$config->cookieSameSite ??= Cookie::SAMESITE_LAX;
/** @var CookieConfig $cookie */
$cookie = config('Cookie');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kenjis, is there a special reason why you didn't write as below?

        $cookie = config(Cookie::class);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems it is convention.
But there are a few lines using ::class.

Targets
Occurrences of ' config(' in Directory /Users/kenji/work/codeigniter/official/CodeIgniter4/system
Found Occurrences in Directory /Users/kenji/work/codeigniter/official/CodeIgniter4/system
BaseCommand.php
        $config    = config('Exceptions');
BaseConfig.php
        static::$moduleConfig = config('Modules');
BaseHandler.php
        $reserved = config('Cache')->reservedCharacters ?? self::RESERVED_CHARACTERS;
BaseHandler.php
        $config ??= config('Encryption');
BaseHandler.php
        $session = config('Session');
        $cookie = config('Cookie');
BaseService.php
            $config = config('Modules');
            $config = config('Modules');
CIUnitTestCase.php
        $config  = config('App');
ClearCache.php
        $config  = config('Cache');
CodeIgniter.php
        $config = config(KintConfig::class);
                $multipleFiltersEnabled = config('Feature')->multipleFilters ?? false;
        $multipleFiltersEnabled = config('Feature')->multipleFilters ?? false;
Common.php
        $config = config('App');
    function config(string $name, bool $getShared = true)
        $baseURL = config('App')->baseURL;
        $config = config('App');
        $config   = config(View::class);
Config.php
            $dbConfig = config('Database');
        $config = config('Database');
Config.php
        $config = config('App');
ContentSecurityPolicy.php
        $appConfig        = config('App');
Controller.php
            $validation = config('Validation');
ControllerTester.php
            $this->appConfig = config('App');
ControllerTestTrait.php
            $this->appConfig = config('App');
cookie_helper.php
            $cookie = config('Cookie');
CreateDatabase.php
            $config = config('Database');
CURLRequest.php
        $configCURLRequest  = config('CURLRequest');
Database.php
        $config = config('Toolbar');
DatabaseHandler.php
        $session = config('Session');
            $this->DBGroup = $session->DBGroup ?? config(Database::class)->defaultGroup;
            $this->DBGroup = $config->sessionDBGroup ?? config(Database::class)->defaultGroup;
Events.php
        $config = config('Modules');
Fabricator.php
            $locale = config('App')->defaultLocale;
Factories.php
 * @method static BaseConfig|null config(...$arguments)
            : config('Factory')->{$component} ?? [];
FeatureTestCase.php
        $config = config('App');
FeatureTestTrait.php
        $config  = config(App::class);
FilterCollector.php
        $config = config('Filters');
FilterFinder.php
        $multipleFiltersEnabled = config('Feature')->multipleFilters ?? false;
Filters.php
        $this->modules = $modules ?? config('Modules');
FilterTestTrait.php
        $this->filtersConfig ??= config('Filters');
form_helper.php
            $config = config('App');
        $config = config('Validation');
        $config = config('Validation');
InfoCache.php
        $config = config('Cache');
MemcachedHandler.php
        $session = config('Session');
Migration.php
        $this->forge = $forge ?? Database::forge($this->DBGroup ?? config('Database')->defaultGroup);
MigrationGenerator.php
            $data['DBDriver'] = config('Database')->{$data['DBGroup']}['DBDriver'];
            $config = config('App');
            $session = config('Session');
MigrationRunner.php
        $config      = config('Database');
        $group    = $instance->getDBGroup() ?? config('Database')->defaultGroup;
Publisher.php
        $this->restrictions = config('Publisher')->restrictions;
RedisHandler.php
        $session = config('Session');
RequestTrait.php
        $proxyIPs = $this->proxyIPs ?? config('App')->proxyIPs;
Response.php
        $cookie = config('Cookie');
ResponseTrait.php
        $cookieConfig = config('Cookie');
RouteCollection.php
            $config = config('App');
Router.php
            $autoRoutesImproved = config('Feature')->autoRoutesImproved ?? false;
                $multipleFiltersEnabled = config('Feature')->multipleFilters ?? false;
                        && ! in_array($matched['locale'], config('App')->supportedLocales, true)) {
Routes.php
            $autoRoutesImproved = config('Feature')->autoRoutesImproved ?? false;
Security.php
        $security = config('Security');
            $cookie = config('Cookie');
Services.php
        $config ??= config('App');
        $config ??= config('App');
        $config ??= config('ContentSecurityPolicy');
        $config ??= config('App');
            $config = config('Email');
        $config ??= config('Encryption');
        $config ??= config('Exceptions');
        $config ??= config('Filters');
        $config ??= config('Format');
        $config ??= config('Honeypot');
        $config ??= config('Images');
        $config ??= config('Migrations');
        $config ??= config('Pager');
        $viewPath = $viewPath ?: config('Paths')->viewDirectory;
        $config ??= config('View');
        $viewPath = $viewPath ?: config('Paths')->viewDirectory;
        $config ??= config('View');
        $config ??= config('App');
        $config ??= config('App');
        $config ??= config('App');
        return new RouteCollection(AppServices::locator(), config('Modules'));
        $config ??= config('App');
        $config ??= config('App');
        $sessionConfig = config('Session');
            $DBGroup = $sessionConfig->DBGroup ?? $config->sessionDBGroup ?? config(Database::class)->defaultGroup;
        $config ??= config('Toolbar');
        $config ??= config('Validation');
Session.php
        $session = config('Session');
        $cookie = config('Cookie');
SessionMigrationGenerator.php
        $data['matchIP'] = config('App')->sessionMatchIP ?? false;
URI.php
        $config  = config('App');
url_helper.php
        $config ??= config('App');
        $config            = clone config('App');
        $config = $altConfig ?? config('App');
        $config = $altConfig ?? config('App');
        $config = $altConfig ?? config('App');
View.php
            $toolbarCollectors = config(Toolbar::class)->collectors;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with the change to use ::class.


if (! in_array(strtolower($config->cookieSameSite ?: Cookie::SAMESITE_LAX), Cookie::ALLOWED_SAMESITE_VALUES, true)) {
throw CookieException::forInvalidSameSite($config->cookieSameSite);
}

$this->cookieStore = new CookieStore([]);
Cookie::setDefaults(config('Cookie') ?? [
// @todo Remove this fallback when deprecated `App` members are removed
'prefix' => $config->cookiePrefix,
'path' => $config->cookiePath,
'domain' => $config->cookieDomain,
'secure' => $config->cookieSecure,
'httponly' => $config->cookieHTTPOnly,
'samesite' => $config->cookieSameSite ?? Cookie::SAMESITE_LAX,
]);
Cookie::setDefaults($cookie);

// Default to an HTML Content-Type. Devs can override if needed.
$this->setContentType('text/html');
6 changes: 2 additions & 4 deletions system/Helpers/cookie_helper.php
View file
Original file line number Diff line number Diff line change
@@ -9,7 +9,6 @@
* the LICENSE file that was distributed with this source code.
*/

use Config\App;
use Config\Cookie;
use Config\Services;

@@ -68,11 +67,10 @@ function set_cookie(
function get_cookie($index, bool $xssClean = false, ?string $prefix = '')
{
if ($prefix === '') {
/** @var Cookie|null $cookie */
/** @var Cookie $cookie */
$cookie = config('Cookie');

// @TODO Remove Config\App fallback when deprecated `App` members are removed.
$prefix = $cookie instanceof Cookie ? $cookie->prefix : config('App')->cookiePrefix;
$prefix = $cookie->prefix;
}

$request = Services::request();
22 changes: 8 additions & 14 deletions system/Security/Security.php
View file
Original file line number Diff line number Diff line change
@@ -195,7 +195,10 @@ public function __construct(App $config)
}

if ($this->isCSRFCookie()) {
$this->configureCookie($config);
/** @var CookieConfig $cookie */
$cookie = config('Cookie');

$this->configureCookie($cookie);
} else {
// Session based CSRF protection
$this->configureSession();
@@ -220,20 +223,11 @@ private function configureSession(): void
$this->session = Services::session();
}

private function configureCookie(App $config): void
private function configureCookie(CookieConfig $cookie): void
{
/** @var CookieConfig|null $cookie */
$cookie = config('Cookie');

if ($cookie instanceof CookieConfig) {
$cookiePrefix = $cookie->prefix;
$this->cookieName = $cookiePrefix . $this->rawCookieName;
Cookie::setDefaults($cookie);
} else {
// `Config/Cookie.php` is absence
$cookiePrefix = $config->cookiePrefix;
$this->cookieName = $cookiePrefix . $this->rawCookieName;
}
$cookiePrefix = $cookie->prefix;
$this->cookieName = $cookiePrefix . $this->rawCookieName;
Cookie::setDefaults($cookie);
}

/**
19 changes: 5 additions & 14 deletions system/Session/Handlers/BaseHandler.php
View file
Original file line number Diff line number Diff line change
@@ -122,22 +122,13 @@ public function __construct(AppConfig $config, string $ipAddress)
$this->savePath = $config->sessionSavePath;
}

/** @var CookieConfig|null $cookie */
/** @var CookieConfig $cookie */
$cookie = config('Cookie');

if ($cookie instanceof CookieConfig) {
// Session cookies have no prefix.
$this->cookieDomain = $cookie->domain;
$this->cookiePath = $cookie->path;
$this->cookieSecure = $cookie->secure;
} else {
// @TODO Remove this fallback when deprecated `App` members are removed.
// `Config/Cookie.php` is absence
// Session cookies have no prefix.
$this->cookieDomain = $config->cookieDomain;
$this->cookiePath = $config->cookiePath;
$this->cookieSecure = $config->cookieSecure;
}
// Session cookies have no prefix.
$this->cookieDomain = $cookie->domain;
$this->cookiePath = $cookie->path;
$this->cookieSecure = $cookie->secure;

$this->ipAddress = $ipAddress;
}
14 changes: 4 additions & 10 deletions system/Session/Session.php
View file
Original file line number Diff line number Diff line change
@@ -188,22 +188,16 @@ public function __construct(SessionHandlerInterface $driver, App $config)
$this->sessionRegenerateDestroy = $config->sessionRegenerateDestroy ?? $this->sessionRegenerateDestroy;
}

// DEPRECATED COOKIE MANAGEMENT
$this->cookiePath = $config->cookiePath ?? $this->cookiePath;
$this->cookieDomain = $config->cookieDomain ?? $this->cookieDomain;
$this->cookieSecure = $config->cookieSecure ?? $this->cookieSecure;
$this->cookieSameSite = $config->cookieSameSite ?? $this->cookieSameSite;

/** @var CookieConfig $cookie */
$cookie = config('Cookie');

$this->cookie = (new Cookie($this->sessionCookieName, '', [
'expires' => $this->sessionExpiration === 0 ? 0 : Time::now()->getTimestamp() + $this->sessionExpiration,
'path' => $cookie->path ?? $config->cookiePath,
'domain' => $cookie->domain ?? $config->cookieDomain,
'secure' => $cookie->secure ?? $config->cookieSecure,
'path' => $cookie->path,
'domain' => $cookie->domain,
'secure' => $cookie->secure,
'httponly' => true, // for security
'samesite' => $cookie->samesite ?? $config->cookieSameSite ?? Cookie::SAMESITE_LAX,
'samesite' => $cookie->samesite ?? Cookie::SAMESITE_LAX,
'raw' => $cookie->raw ?? false,
]))->withPrefix(''); // Cookie prefix should be ignored.

6 changes: 0 additions & 6 deletions system/Test/Mock/MockAppConfig.php
View file
Original file line number Diff line number Diff line change
@@ -17,12 +17,6 @@ class MockAppConfig extends App
{
public string $baseURL = 'http://example.com/';
public string $uriProtocol = 'REQUEST_URI';
public string $cookiePrefix = '';
public string $cookieDomain = '';
public string $cookiePath = '/';
public bool $cookieSecure = false;
public bool $cookieHTTPOnly = false;
public ?string $cookieSameSite = 'Lax';
public array $proxyIPs = [];
public string $CSRFTokenName = 'csrf_test_name';
public string $CSRFHeaderName = 'X-CSRF-TOKEN';
6 changes: 0 additions & 6 deletions system/Test/Mock/MockCLIConfig.php
View file
Original file line number Diff line number Diff line change
@@ -17,12 +17,6 @@ class MockCLIConfig extends App
{
public string $baseURL = 'http://example.com/';
public string $uriProtocol = 'REQUEST_URI';
public string $cookiePrefix = '';
public string $cookieDomain = '';
public string $cookiePath = '/';
public bool $cookieSecure = false;
public bool $cookieHTTPOnly = false;
public ?string $cookieSameSite = 'Lax';
public array $proxyIPs = [];
public string $CSRFTokenName = 'csrf_test_name';
public string $CSRFCookieName = 'csrf_cookie_name';
42 changes: 30 additions & 12 deletions tests/system/API/ResponseTraitTest.php
View file
Original file line number Diff line number Diff line change
@@ -11,6 +11,7 @@

namespace CodeIgniter\API;

use CodeIgniter\Config\Factories;
use CodeIgniter\Format\FormatterInterface;
use CodeIgniter\Format\JSONFormatter;
use CodeIgniter\Format\XMLFormatter;
@@ -20,6 +21,7 @@
use CodeIgniter\Test\Mock\MockIncomingRequest;
use CodeIgniter\Test\Mock\MockResponse;
use Config\App;
use Config\Cookie;
use stdClass;

/**
@@ -59,17 +61,25 @@ protected function makeController(array $userConfig = [], string $uri = 'http://
'negotiateLocale' => false,
'supportedLocales' => ['en'],
'CSPEnabled' => false,
'cookiePrefix' => '',
'cookieDomain' => '',
'cookiePath' => '/',
'cookieSecure' => false,
'cookieHTTPOnly' => false,
'proxyIPs' => [],
'cookieSameSite' => 'Lax',
] as $key => $value) {
$config->{$key} = $value;
}

$cookie = new Cookie();

foreach ([
'prefix' => '',
'domain' => '',
'path' => '/',
'secure' => false,
'httponly' => false,
'samesite' => 'Lax',
] as $key => $value) {
$cookie->{$key} = $value;
}
Factories::injectMock('config', 'Cookie', $cookie);

if ($this->request === null) {
$this->request = new MockIncomingRequest($config, new URI($uri), null, new UserAgent());
$this->response = new MockResponse($config);
@@ -532,17 +542,25 @@ public function testFormatByRequestNegotiateIfFormatIsNotJsonOrXML()
'negotiateLocale' => false,
'supportedLocales' => ['en'],
'CSPEnabled' => false,
'cookiePrefix' => '',
'cookieDomain' => '',
'cookiePath' => '/',
'cookieSecure' => false,
'cookieHTTPOnly' => false,
'proxyIPs' => [],
'cookieSameSite' => 'Lax',
] as $key => $value) {
$config->{$key} = $value;
}

$cookie = new Cookie();

foreach ([
'prefix' => '',
'domain' => '',
'path' => '/',
'secure' => false,
'httponly' => false,
'samesite' => 'Lax',
] as $key => $value) {
$cookie->{$key} = $value;
}
Factories::injectMock('config', 'Cookie', $cookie);

$request = new MockIncomingRequest($config, new URI($config->baseURL), null, new UserAgent());
$response = new MockResponse($config);

Loading