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.

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

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

Closed
wants to merge 8 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 5 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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions app/Config/Cookie.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down
29 changes: 5 additions & 24 deletions system/HTTP/Response.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -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');
Expand Down
6 changes: 2 additions & 4 deletions system/Helpers/cookie_helper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
* the LICENSE file that was distributed with this source code.
*/

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

Expand Down Expand Up @@ -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();
Expand Down
22 changes: 8 additions & 14 deletions system/Security/Security.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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);
}

/**
Expand Down
19 changes: 5 additions & 14 deletions system/Session/Handlers/BaseHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
14 changes: 4 additions & 10 deletions system/Session/Session.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
6 changes: 0 additions & 6 deletions system/Test/Mock/MockAppConfig.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down
6 changes: 0 additions & 6 deletions system/Test/Mock/MockCLIConfig.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down
11 changes: 0 additions & 11 deletions tests/system/HTTP/ResponseTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
namespace CodeIgniter\HTTP;

use CodeIgniter\Config\Factories;
use CodeIgniter\Cookie\Exceptions\CookieException;
use CodeIgniter\HTTP\Exceptions\HTTPException;
use CodeIgniter\Test\CIUnitTestCase;
use CodeIgniter\Test\Mock\MockResponse;
Expand Down Expand Up @@ -512,14 +511,4 @@ public function testPretendOutput()

$this->assertSame('Happy days', $actual);
}

public function testInvalidSameSiteCookie()
{
$config = new App();
$config->cookieSameSite = 'Invalid';

$this->expectException(CookieException::class);
$this->expectExceptionMessage(lang('Cookie.invalidSameSite', ['Invalid']));
new Response($config);
}
}