From afeb419d0670aab135fec15d70a892b86174cc14 Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 27 Jun 2023 14:45:44 +0900 Subject: [PATCH 01/12] refactor: remove CSRF config items in Config\App --- app/Config/App.php | 85 ------------------------------------ system/Security/Security.php | 26 ++++------- 2 files changed, 8 insertions(+), 103 deletions(-) diff --git a/app/Config/App.php b/app/Config/App.php index 69b596bcad59..186bfa86bb02 100644 --- a/app/Config/App.php +++ b/app/Config/App.php @@ -158,91 +158,6 @@ class App extends BaseConfig */ public array $proxyIPs = []; - /** - * -------------------------------------------------------------------------- - * CSRF Token Name - * -------------------------------------------------------------------------- - * - * The token name. - * - * @deprecated Use `Config\Security` $tokenName property instead of using this property. - */ - public string $CSRFTokenName = 'csrf_test_name'; - - /** - * -------------------------------------------------------------------------- - * CSRF Header Name - * -------------------------------------------------------------------------- - * - * The header name. - * - * @deprecated Use `Config\Security` $headerName property instead of using this property. - */ - public string $CSRFHeaderName = 'X-CSRF-TOKEN'; - - /** - * -------------------------------------------------------------------------- - * CSRF Cookie Name - * -------------------------------------------------------------------------- - * - * The cookie name. - * - * @deprecated Use `Config\Security` $cookieName property instead of using this property. - */ - public string $CSRFCookieName = 'csrf_cookie_name'; - - /** - * -------------------------------------------------------------------------- - * CSRF Expire - * -------------------------------------------------------------------------- - * - * The number in seconds the token should expire. - * - * @deprecated Use `Config\Security` $expire property instead of using this property. - */ - public int $CSRFExpire = 7200; - - /** - * -------------------------------------------------------------------------- - * CSRF Regenerate - * -------------------------------------------------------------------------- - * - * Regenerate token on every submission? - * - * @deprecated Use `Config\Security` $regenerate property instead of using this property. - */ - public bool $CSRFRegenerate = true; - - /** - * -------------------------------------------------------------------------- - * CSRF Redirect - * -------------------------------------------------------------------------- - * - * Redirect to previous page with error on failure? - * - * @deprecated Use `Config\Security` $redirect property instead of using this property. - */ - public bool $CSRFRedirect = false; - - /** - * -------------------------------------------------------------------------- - * CSRF SameSite - * -------------------------------------------------------------------------- - * - * Setting for CSRF SameSite cookie token. Allowed values are: - * - None - * - Lax - * - Strict - * - '' - * - * Defaults to `Lax` as recommended in this link: - * - * @see https://portswigger.net/web-security/csrf/samesite-cookies - * - * @deprecated `Config\Cookie` $samesite property is used. - */ - public string $CSRFSameSite = 'Lax'; - /** * -------------------------------------------------------------------------- * Content Security Policy diff --git a/system/Security/Security.php b/system/Security/Security.php index d7a5d49c6f09..e54e28a4d7d5 100644 --- a/system/Security/Security.php +++ b/system/Security/Security.php @@ -174,24 +174,14 @@ public function __construct(App $config) $security = config(SecurityConfig::class); // Store CSRF-related configurations - if ($security instanceof SecurityConfig) { - $this->csrfProtection = $security->csrfProtection ?? $this->csrfProtection; - $this->tokenName = $security->tokenName ?? $this->tokenName; - $this->headerName = $security->headerName ?? $this->headerName; - $this->regenerate = $security->regenerate ?? $this->regenerate; - $this->redirect = $security->redirect ?? $this->redirect; - $this->rawCookieName = $security->cookieName ?? $this->rawCookieName; - $this->expires = $security->expires ?? $this->expires; - $this->tokenRandomize = $security->tokenRandomize ?? $this->tokenRandomize; - } else { - // `Config/Security.php` is absence - $this->tokenName = $config->CSRFTokenName ?? $this->tokenName; - $this->headerName = $config->CSRFHeaderName ?? $this->headerName; - $this->regenerate = $config->CSRFRegenerate ?? $this->regenerate; - $this->rawCookieName = $config->CSRFCookieName ?? $this->rawCookieName; - $this->expires = $config->CSRFExpire ?? $this->expires; - $this->redirect = $config->CSRFRedirect ?? $this->redirect; - } + $this->csrfProtection = $security->csrfProtection ?? $this->csrfProtection; + $this->tokenName = $security->tokenName ?? $this->tokenName; + $this->headerName = $security->headerName ?? $this->headerName; + $this->regenerate = $security->regenerate ?? $this->regenerate; + $this->redirect = $security->redirect ?? $this->redirect; + $this->rawCookieName = $security->cookieName ?? $this->rawCookieName; + $this->expires = $security->expires ?? $this->expires; + $this->tokenRandomize = $security->tokenRandomize ?? $this->tokenRandomize; if ($this->isCSRFCookie()) { $cookie = config(CookieConfig::class); From a0665df04285b8492acc22df024b03faaa1f7e9a Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 27 Jun 2023 15:05:28 +0900 Subject: [PATCH 02/12] refactor: add property for SecurityConfig and use it --- system/Security/Security.php | 74 +++++++++++++++++++++--------------- 1 file changed, 43 insertions(+), 31 deletions(-) diff --git a/system/Security/Security.php b/system/Security/Security.php index e54e28a4d7d5..593dc5142b6b 100644 --- a/system/Security/Security.php +++ b/system/Security/Security.php @@ -44,6 +44,8 @@ class Security implements SecurityInterface * Protection Method for Cross Site Request Forgery protection. * * @var string 'cookie' or 'session' + * + * @deprecated 4.4.0 Use $this->config->csrfProtection. */ protected $csrfProtection = self::CSRF_PROTECTION_COOKIE; @@ -51,6 +53,8 @@ class Security implements SecurityInterface * CSRF Token Randomization * * @var bool + * + * @deprecated 4.4.0 Use $this->config->tokenRandomize. */ protected $tokenRandomize = false; @@ -69,6 +73,8 @@ class Security implements SecurityInterface * Token name for Cross Site Request Forgery protection. * * @var string + * + * @deprecated 4.4.0 Use $this->config->tokenName. */ protected $tokenName = 'csrf_token_name'; @@ -78,6 +84,8 @@ class Security implements SecurityInterface * Header name for Cross Site Request Forgery protection. * * @var string + * + * @deprecated 4.4.0 Use $this->config->headerName. */ protected $headerName = 'X-CSRF-TOKEN'; @@ -105,6 +113,8 @@ class Security implements SecurityInterface * Defaults to two hours (in seconds). * * @var int + * + * @deprecated 4.4.0 Use $this->config->expires. */ protected $expires = 7200; @@ -114,6 +124,8 @@ class Security implements SecurityInterface * Regenerate CSRF Token on every request. * * @var bool + * + * @deprecated 4.4.0 Use $this->config->regenerate. */ protected $regenerate = true; @@ -123,6 +135,8 @@ class Security implements SecurityInterface * Redirect to previous page with error on failure. * * @var bool + * + * @deprecated 4.4.0 Use $this->config->redirect. */ protected $redirect = false; @@ -163,6 +177,11 @@ class Security implements SecurityInterface */ private ?string $hashInCookie = null; + /** + * Security Config + */ + protected SecurityConfig $config; + /** * Constructor. * @@ -171,17 +190,10 @@ class Security implements SecurityInterface */ public function __construct(App $config) { - $security = config(SecurityConfig::class); - - // Store CSRF-related configurations - $this->csrfProtection = $security->csrfProtection ?? $this->csrfProtection; - $this->tokenName = $security->tokenName ?? $this->tokenName; - $this->headerName = $security->headerName ?? $this->headerName; - $this->regenerate = $security->regenerate ?? $this->regenerate; - $this->redirect = $security->redirect ?? $this->redirect; - $this->rawCookieName = $security->cookieName ?? $this->rawCookieName; - $this->expires = $security->expires ?? $this->expires; - $this->tokenRandomize = $security->tokenRandomize ?? $this->tokenRandomize; + $security = config(SecurityConfig::class); + $this->config = $security; + + $this->rawCookieName = $security->cookieName; if ($this->isCSRFCookie()) { $cookie = config(CookieConfig::class); @@ -203,7 +215,7 @@ public function __construct(App $config) private function isCSRFCookie(): bool { - return $this->csrfProtection === self::CSRF_PROTECTION_COOKIE; + return $this->config->csrfProtection === self::CSRF_PROTECTION_COOKIE; } private function configureSession(): void @@ -277,7 +289,7 @@ public function verify(RequestInterface $request) $postedToken = $this->getPostedToken($request); try { - $token = ($postedToken !== null && $this->tokenRandomize) + $token = ($postedToken !== null && $this->config->tokenRandomize) ? $this->derandomize($postedToken) : $postedToken; } catch (InvalidArgumentException $e) { $token = null; @@ -290,7 +302,7 @@ public function verify(RequestInterface $request) $this->removeTokenInRequest($request); - if ($this->regenerate) { + if ($this->config->regenerate) { $this->generateHash(); } @@ -308,13 +320,13 @@ private function removeTokenInRequest(RequestInterface $request): void $json = json_decode($request->getBody() ?? ''); - if (isset($_POST[$this->tokenName])) { + if (isset($_POST[$this->config->tokenName])) { // We kill this since we're done and we don't want to pollute the POST array. - unset($_POST[$this->tokenName]); + unset($_POST[$this->config->tokenName]); $request->setGlobal('post', $_POST); - } elseif (isset($json->{$this->tokenName})) { + } elseif (isset($json->{$this->config->tokenName})) { // We kill this since we're done and we don't want to pollute the JSON data. - unset($json->{$this->tokenName}); + unset($json->{$this->config->tokenName}); $request->setBody(json_encode($json)); } } @@ -325,19 +337,19 @@ private function getPostedToken(RequestInterface $request): ?string // Does the token exist in POST, HEADER or optionally php:://input - json data. - if ($tokenValue = $request->getPost($this->tokenName)) { + if ($tokenValue = $request->getPost($this->config->tokenName)) { return $tokenValue; } - if ($request->hasHeader($this->headerName) && ! empty($request->header($this->headerName)->getValue())) { - return $request->header($this->headerName)->getValue(); + if ($request->hasHeader($this->config->headerName) && ! empty($request->header($this->config->headerName)->getValue())) { + return $request->header($this->config->headerName)->getValue(); } $body = (string) $request->getBody(); $json = json_decode($body); if ($body !== '' && ! empty($json) && json_last_error() === JSON_ERROR_NONE) { - return $json->{$this->tokenName} ?? null; + return $json->{$this->config->tokenName} ?? null; } return null; @@ -348,7 +360,7 @@ private function getPostedToken(RequestInterface $request): ?string */ public function getHash(): ?string { - return $this->tokenRandomize ? $this->randomize($this->hash) : $this->hash; + return $this->config->tokenRandomize ? $this->randomize($this->hash) : $this->hash; } /** @@ -397,7 +409,7 @@ protected function derandomize(string $token): string */ public function getTokenName(): string { - return $this->tokenName; + return $this->config->tokenName; } /** @@ -405,7 +417,7 @@ public function getTokenName(): string */ public function getHeaderName(): string { - return $this->headerName; + return $this->config->headerName; } /** @@ -413,7 +425,7 @@ public function getHeaderName(): string */ public function getCookieName(): string { - return $this->cookieName; + return $this->config->cookieName; } /** @@ -433,7 +445,7 @@ public function isExpired(): bool */ public function shouldRedirect(): bool { - return $this->redirect; + return $this->config->redirect; } /** @@ -511,9 +523,9 @@ private function restoreHash(): void if ($this->isHashInCookie()) { $this->hash = $this->hashInCookie; } - } elseif ($this->session->has($this->tokenName)) { + } elseif ($this->session->has($this->config->tokenName)) { // Session based CSRF protection - $this->hash = $this->session->get($this->tokenName); + $this->hash = $this->session->get($this->config->tokenName); } } @@ -552,7 +564,7 @@ private function saveHashInCookie(): void $this->rawCookieName, $this->hash, [ - 'expires' => $this->expires === 0 ? 0 : Time::now()->getTimestamp() + $this->expires, + 'expires' => $this->config->expires === 0 ? 0 : Time::now()->getTimestamp() + $this->config->expires, ] ); @@ -596,6 +608,6 @@ protected function doSendCookie(): void private function saveHashInSession(): void { - $this->session->set($this->tokenName, $this->hash); + $this->session->set($this->config->tokenName, $this->hash); } } From a9554b350215042cfa856c43c6468c0ad41d01ae Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 27 Jun 2023 15:27:57 +0900 Subject: [PATCH 03/12] refactor: [BC] change Security constructor param from Config\App to Config\Security --- system/Config/Services.php | 5 ++- system/Security/Security.php | 8 ++--- tests/system/Config/ServicesTest.php | 5 +-- tests/system/Helpers/SecurityHelperTest.php | 4 +-- .../SecurityCSRFCookieRandomizeTokenTest.php | 16 +++++---- .../SecurityCSRFSessionRandomizeTokenTest.php | 36 ++++++++++--------- .../Security/SecurityCSRFSessionTest.php | 30 ++++++++-------- tests/system/Security/SecurityTest.php | 30 ++++++++-------- 8 files changed, 71 insertions(+), 63 deletions(-) diff --git a/system/Config/Services.php b/system/Config/Services.php index 89a49b4bf468..779afa3ccaf1 100644 --- a/system/Config/Services.php +++ b/system/Config/Services.php @@ -77,6 +77,7 @@ use Config\Pager as PagerConfig; use Config\Paths; use Config\Routing; +use Config\Security as SecurityConfig; use Config\Services as AppServices; use Config\Session as SessionConfig; use Config\Toolbar as ToolbarConfig; @@ -626,6 +627,8 @@ public static function router(?RouteCollectionInterface $routes = null, ?Request * secure, most notably the CSRF protection tools. * * @return Security + * + * @TODO replace the first parameter type `?App` with `?SecurityConfig` */ public static function security(?App $config = null, bool $getShared = true) { @@ -633,7 +636,7 @@ public static function security(?App $config = null, bool $getShared = true) return static::getSharedInstance('security', $config); } - $config ??= config(App::class); + $config = config(SecurityConfig::class); return new Security($config); } diff --git a/system/Security/Security.php b/system/Security/Security.php index 593dc5142b6b..c67c13ce0c1c 100644 --- a/system/Security/Security.php +++ b/system/Security/Security.php @@ -18,7 +18,6 @@ use CodeIgniter\I18n\Time; use CodeIgniter\Security\Exceptions\SecurityException; use CodeIgniter\Session\Session; -use Config\App; use Config\Cookie as CookieConfig; use Config\Security as SecurityConfig; use Config\Services; @@ -188,12 +187,11 @@ class Security implements SecurityInterface * Stores our configuration and fires off the init() method to setup * initial state. */ - public function __construct(App $config) + public function __construct(SecurityConfig $config) { - $security = config(SecurityConfig::class); - $this->config = $security; + $this->config = $config; - $this->rawCookieName = $security->cookieName; + $this->rawCookieName = $config->cookieName; if ($this->isCSRFCookie()) { $cookie = config(CookieConfig::class); diff --git a/tests/system/Config/ServicesTest.php b/tests/system/Config/ServicesTest.php index 143591aac10c..b6ea85e6971f 100644 --- a/tests/system/Config/ServicesTest.php +++ b/tests/system/Config/ServicesTest.php @@ -43,6 +43,7 @@ use CodeIgniter\View\Parser; use Config\App; use Config\Exceptions; +use Config\Security as SecurityConfig; use RuntimeException; use Tests\Support\Config\Services; @@ -329,7 +330,7 @@ public function testReset() public function testResetSingle() { Services::injectMock('response', new MockResponse(new App())); - Services::injectMock('security', new MockSecurity(new App())); + Services::injectMock('security', new MockSecurity(new SecurityConfig())); $response = service('response'); $security = service('security'); $this->assertInstanceOf(MockResponse::class, $response); @@ -411,7 +412,7 @@ public function testRouter() public function testSecurity() { - Services::injectMock('security', new MockSecurity(new App())); + Services::injectMock('security', new MockSecurity(new SecurityConfig())); $result = Services::security(); $this->assertInstanceOf(Security::class, $result); diff --git a/tests/system/Helpers/SecurityHelperTest.php b/tests/system/Helpers/SecurityHelperTest.php index c7523e3b0081..8c2c0d6efd55 100644 --- a/tests/system/Helpers/SecurityHelperTest.php +++ b/tests/system/Helpers/SecurityHelperTest.php @@ -13,7 +13,7 @@ use CodeIgniter\Test\CIUnitTestCase; use CodeIgniter\Test\Mock\MockSecurity; -use Config\App; +use Config\Security as SecurityConfig; use Tests\Support\Config\Services; /** @@ -32,7 +32,7 @@ protected function setUp(): void public function testSanitizeFilenameSimpleSuccess() { - Services::injectMock('security', new MockSecurity(new App())); + Services::injectMock('security', new MockSecurity(new SecurityConfig())); $this->assertSame('hello.doc', sanitize_filename('hello.doc')); } diff --git a/tests/system/Security/SecurityCSRFCookieRandomizeTokenTest.php b/tests/system/Security/SecurityCSRFCookieRandomizeTokenTest.php index 93f749ffc604..95ad54d58b04 100644 --- a/tests/system/Security/SecurityCSRFCookieRandomizeTokenTest.php +++ b/tests/system/Security/SecurityCSRFCookieRandomizeTokenTest.php @@ -38,19 +38,21 @@ final class SecurityCSRFCookieRandomizeTokenTest extends CIUnitTestCase */ private string $randomizedToken = '8bc70b67c91494e815c7d2219c1ae0ab005513c290126d34d41bf41c5265e0f1'; + private SecurityConfig $config; + protected function setUp(): void { parent::setUp(); $_COOKIE = []; - $config = new SecurityConfig(); - $config->csrfProtection = Security::CSRF_PROTECTION_COOKIE; - $config->tokenRandomize = true; - Factories::injectMock('config', 'Security', $config); + $this->config = new SecurityConfig(); + $this->config->csrfProtection = Security::CSRF_PROTECTION_COOKIE; + $this->config->tokenRandomize = true; + Factories::injectMock('config', 'Security', $this->config); // Set Cookie value - $security = new MockSecurity(new MockAppConfig()); + $security = new MockSecurity($this->config); $_COOKIE[$security->getCookieName()] = $this->hash; $this->resetServices(); @@ -58,7 +60,7 @@ protected function setUp(): void public function testTokenIsReadFromCookie() { - $security = new MockSecurity(new MockAppConfig()); + $security = new MockSecurity($this->config); $this->assertSame( $this->randomizedToken, @@ -74,7 +76,7 @@ public function testCSRFVerifySetNewCookie() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); - $security = new Security(new MockAppConfig()); + $security = new Security($this->config); $this->assertInstanceOf(Security::class, $security->verify($request)); $this->assertLogged('info', 'CSRF token verified.'); diff --git a/tests/system/Security/SecurityCSRFSessionRandomizeTokenTest.php b/tests/system/Security/SecurityCSRFSessionRandomizeTokenTest.php index 36bd5ea97f1c..6afc1aeefd22 100644 --- a/tests/system/Security/SecurityCSRFSessionRandomizeTokenTest.php +++ b/tests/system/Security/SecurityCSRFSessionRandomizeTokenTest.php @@ -51,6 +51,8 @@ final class SecurityCSRFSessionRandomizeTokenTest extends CIUnitTestCase */ private string $randomizedToken = '8bc70b67c91494e815c7d2219c1ae0ab005513c290126d34d41bf41c5265e0f1'; + private SecurityConfig $config; + protected function setUp(): void { parent::setUp(); @@ -58,10 +60,10 @@ protected function setUp(): void $_SESSION = []; Factories::reset(); - $config = new SecurityConfig(); - $config->csrfProtection = Security::CSRF_PROTECTION_SESSION; - $config->tokenRandomize = true; - Factories::injectMock('config', 'Security', $config); + $this->config = new SecurityConfig(); + $this->config->csrfProtection = Security::CSRF_PROTECTION_SESSION; + $this->config->tokenRandomize = true; + Factories::injectMock('config', 'Security', $this->config); $this->injectSession($this->hash); } @@ -113,7 +115,7 @@ private function injectSession(string $hash): void public function testHashIsReadFromSession() { - $security = new MockSecurity(new MockAppConfig()); + $security = new MockSecurity($this->config); $this->assertSame( $this->randomizedToken, @@ -131,7 +133,7 @@ public function testCSRFVerifyPostNoToken() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); - $security = new Security(new MockAppConfig()); + $security = new Security($this->config); $security->verify($request); } @@ -146,7 +148,7 @@ public function testCSRFVerifyPostThrowsExceptionOnNoMatch() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); - $security = new Security(new MockAppConfig()); + $security = new Security($this->config); $security->verify($request); } @@ -161,7 +163,7 @@ public function testCSRFVerifyPostInvalidToken() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); - $security = new Security(new MockAppConfig()); + $security = new Security($this->config); $security->verify($request); } @@ -174,7 +176,7 @@ public function testCSRFVerifyPostReturnsSelfOnMatch() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); - $security = new Security(new MockAppConfig()); + $security = new Security($this->config); $this->assertInstanceOf(Security::class, $security->verify($request)); $this->assertLogged('info', 'CSRF token verified.'); @@ -188,7 +190,7 @@ public function testCSRFVerifyPOSTHeaderThrowsExceptionOnNoMatch() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); $request->setHeader('X-CSRF-TOKEN', '8b9218a55906f9dcc1dc263dce7f005b'); - $security = new Security(new MockAppConfig()); + $security = new Security($this->config); $this->expectException(SecurityException::class); $this->expectExceptionMessage('The action you requested is not allowed.'); @@ -204,7 +206,7 @@ public function testCSRFVerifyPOSTHeaderReturnsSelfOnMatch() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); $request->setHeader('X-CSRF-TOKEN', $this->randomizedToken); - $security = new Security(new MockAppConfig()); + $security = new Security($this->config); $this->assertInstanceOf(Security::class, $security->verify($request)); $this->assertLogged('info', 'CSRF token verified.'); @@ -218,7 +220,7 @@ public function testCSRFVerifyPUTHeaderThrowsExceptionOnNoMatch() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); $request->setHeader('X-CSRF-TOKEN', '8b9218a55906f9dcc1dc263dce7f005b'); - $security = new Security(new MockAppConfig()); + $security = new Security($this->config); $this->expectException(SecurityException::class); $this->expectExceptionMessage('The action you requested is not allowed.'); @@ -233,7 +235,7 @@ public function testCSRFVerifyPUTHeaderReturnsSelfOnMatch() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); $request->setHeader('X-CSRF-TOKEN', $this->randomizedToken); - $security = new Security(new MockAppConfig()); + $security = new Security($this->config); $this->assertInstanceOf(Security::class, $security->verify($request)); $this->assertLogged('info', 'CSRF token verified.'); @@ -249,7 +251,7 @@ public function testCSRFVerifyJsonThrowsExceptionOnNoMatch() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); $request->setBody('{"csrf_test_name":"8b9218a55906f9dcc1dc263dce7f005b"}'); - $security = new Security(new MockAppConfig()); + $security = new Security($this->config); $security->verify($request); } @@ -261,7 +263,7 @@ public function testCSRFVerifyJsonReturnsSelfOnMatch() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); $request->setBody('{"csrf_test_name":"' . $this->randomizedToken . '","foo":"bar"}'); - $security = new Security(new MockAppConfig()); + $security = new Security($this->config); $this->assertInstanceOf(Security::class, $security->verify($request)); $this->assertLogged('info', 'CSRF token verified.'); @@ -280,7 +282,7 @@ public function testRegenerateWithFalseSecurityRegenerateProperty() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); - $security = new MockSecurity(new MockAppConfig()); + $security = new MockSecurity($this->config); $oldHash = $security->getHash(); $security->verify($request); @@ -301,7 +303,7 @@ public function testRegenerateWithTrueSecurityRegenerateProperty() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); - $security = new Security(new MockAppConfig()); + $security = new Security($this->config); $oldHash = $security->getHash(); $security->verify($request); diff --git a/tests/system/Security/SecurityCSRFSessionTest.php b/tests/system/Security/SecurityCSRFSessionTest.php index d6f2216e29f6..41ccc638331b 100644 --- a/tests/system/Security/SecurityCSRFSessionTest.php +++ b/tests/system/Security/SecurityCSRFSessionTest.php @@ -45,6 +45,8 @@ final class SecurityCSRFSessionTest extends CIUnitTestCase */ private string $hash = '8b9218a55906f9dcc1dc263dce7f005a'; + private SecurityConfig $config; + protected function setUp(): void { parent::setUp(); @@ -52,9 +54,9 @@ protected function setUp(): void $_SESSION = []; Factories::reset(); - $config = new SecurityConfig(); - $config->csrfProtection = Security::CSRF_PROTECTION_SESSION; - Factories::injectMock('config', 'Security', $config); + $this->config = new SecurityConfig(); + $this->config->csrfProtection = Security::CSRF_PROTECTION_SESSION; + Factories::injectMock('config', 'Security', $this->config); $this->injectSession($this->hash); } @@ -106,7 +108,7 @@ private function injectSession(string $hash): void public function testHashIsReadFromSession() { - $security = new Security(new MockAppConfig()); + $security = new Security($this->config); $this->assertSame($this->hash, $security->getHash()); } @@ -120,7 +122,7 @@ public function testCSRFVerifyPostThrowsExceptionOnNoMatch() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); - $security = new Security(new MockAppConfig()); + $security = new Security(new SecurityConfig()); $security->verify($request); } @@ -133,7 +135,7 @@ public function testCSRFVerifyPostReturnsSelfOnMatch() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); - $security = new Security(new MockAppConfig()); + $security = new Security($this->config); $this->assertInstanceOf(Security::class, $security->verify($request)); $this->assertLogged('info', 'CSRF token verified.'); @@ -147,7 +149,7 @@ public function testCSRFVerifyPOSTHeaderThrowsExceptionOnNoMatch() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); $request->setHeader('X-CSRF-TOKEN', '8b9218a55906f9dcc1dc263dce7f005b'); - $security = new Security(new MockAppConfig()); + $security = new Security($this->config); $this->expectException(SecurityException::class); $security->verify($request); @@ -161,7 +163,7 @@ public function testCSRFVerifyPOSTHeaderReturnsSelfOnMatch() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); $request->setHeader('X-CSRF-TOKEN', '8b9218a55906f9dcc1dc263dce7f005a'); - $security = new Security(new MockAppConfig()); + $security = new Security($this->config); $this->assertInstanceOf(Security::class, $security->verify($request)); $this->assertLogged('info', 'CSRF token verified.'); @@ -175,7 +177,7 @@ public function testCSRFVerifyPUTHeaderThrowsExceptionOnNoMatch() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); $request->setHeader('X-CSRF-TOKEN', '8b9218a55906f9dcc1dc263dce7f005b'); - $security = new Security(new MockAppConfig()); + $security = new Security(new SecurityConfig()); $this->expectException(SecurityException::class); $security->verify($request); @@ -188,7 +190,7 @@ public function testCSRFVerifyPUTHeaderReturnsSelfOnMatch() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); $request->setHeader('X-CSRF-TOKEN', '8b9218a55906f9dcc1dc263dce7f005a'); - $security = new Security(new MockAppConfig()); + $security = new Security($this->config); $this->assertInstanceOf(Security::class, $security->verify($request)); $this->assertLogged('info', 'CSRF token verified.'); @@ -203,7 +205,7 @@ public function testCSRFVerifyJsonThrowsExceptionOnNoMatch() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); $request->setBody('{"csrf_test_name":"8b9218a55906f9dcc1dc263dce7f005b"}'); - $security = new Security(new MockAppConfig()); + $security = new Security(new SecurityConfig()); $security->verify($request); } @@ -215,7 +217,7 @@ public function testCSRFVerifyJsonReturnsSelfOnMatch() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); $request->setBody('{"csrf_test_name":"8b9218a55906f9dcc1dc263dce7f005a","foo":"bar"}'); - $security = new Security(new MockAppConfig()); + $security = new Security($this->config); $this->assertInstanceOf(Security::class, $security->verify($request)); $this->assertLogged('info', 'CSRF token verified.'); @@ -233,7 +235,7 @@ public function testRegenerateWithFalseSecurityRegenerateProperty() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); - $security = new Security(new MockAppConfig()); + $security = new Security($this->config); $oldHash = $security->getHash(); $security->verify($request); @@ -253,7 +255,7 @@ public function testRegenerateWithTrueSecurityRegenerateProperty() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); - $security = new Security(new MockAppConfig()); + $security = new Security($this->config); $oldHash = $security->getHash(); $security->verify($request); diff --git a/tests/system/Security/SecurityTest.php b/tests/system/Security/SecurityTest.php index f610add149f0..82bc6b6c27db 100644 --- a/tests/system/Security/SecurityTest.php +++ b/tests/system/Security/SecurityTest.php @@ -43,7 +43,7 @@ protected function setUp(): void public function testBasicConfigIsSaved() { - $security = new MockSecurity(new MockAppConfig()); + $security = new MockSecurity(new SecurityConfig()); $hash = $security->getHash(); @@ -55,7 +55,7 @@ public function testHashIsReadFromCookie() { $_COOKIE['csrf_cookie_name'] = '8b9218a55906f9dcc1dc263dce7f005a'; - $security = new MockSecurity(new MockAppConfig()); + $security = new MockSecurity(new SecurityConfig()); $this->assertSame( '8b9218a55906f9dcc1dc263dce7f005a', @@ -65,7 +65,7 @@ public function testHashIsReadFromCookie() public function testGetHashSetsCookieWhenGETWithoutCSRFCookie() { - $security = new MockSecurity(new MockAppConfig()); + $security = new MockSecurity(new SecurityConfig()); $_SERVER['REQUEST_METHOD'] = 'GET'; @@ -80,7 +80,7 @@ public function testGetHashReturnsCSRFCookieWhenGETWithCSRFCookie() $_SERVER['REQUEST_METHOD'] = 'GET'; $_COOKIE['csrf_cookie_name'] = '8b9218a55906f9dcc1dc263dce7f005a'; - $security = new MockSecurity(new MockAppConfig()); + $security = new MockSecurity(new SecurityConfig()); $security->verify(new Request(new MockAppConfig())); @@ -93,7 +93,7 @@ public function testCSRFVerifyPostThrowsExceptionOnNoMatch() $_POST['csrf_test_name'] = '8b9218a55906f9dcc1dc263dce7f005a'; $_COOKIE['csrf_cookie_name'] = '8b9218a55906f9dcc1dc263dce7f005b'; - $security = new MockSecurity(new MockAppConfig()); + $security = new MockSecurity(new SecurityConfig()); $request = new IncomingRequest( new MockAppConfig(), new URI('http://badurl.com'), @@ -112,7 +112,7 @@ public function testCSRFVerifyPostReturnsSelfOnMatch() $_POST['csrf_test_name'] = '8b9218a55906f9dcc1dc263dce7f005a'; $_COOKIE['csrf_cookie_name'] = '8b9218a55906f9dcc1dc263dce7f005a'; - $security = new MockSecurity(new MockAppConfig()); + $security = new MockSecurity(new SecurityConfig()); $request = new IncomingRequest( new MockAppConfig(), new URI('http://badurl.com'), @@ -131,7 +131,7 @@ public function testCSRFVerifyHeaderThrowsExceptionOnNoMatch() $_SERVER['REQUEST_METHOD'] = 'POST'; $_COOKIE['csrf_cookie_name'] = '8b9218a55906f9dcc1dc263dce7f005b'; - $security = new MockSecurity(new MockAppConfig()); + $security = new MockSecurity(new SecurityConfig()); $request = new IncomingRequest( new MockAppConfig(), new URI('http://badurl.com'), @@ -151,7 +151,7 @@ public function testCSRFVerifyHeaderReturnsSelfOnMatch() $_POST['foo'] = 'bar'; $_COOKIE['csrf_cookie_name'] = '8b9218a55906f9dcc1dc263dce7f005a'; - $security = new MockSecurity(new MockAppConfig()); + $security = new MockSecurity(new SecurityConfig()); $request = new IncomingRequest( new MockAppConfig(), new URI('http://badurl.com'), @@ -172,7 +172,7 @@ public function testCSRFVerifyJsonThrowsExceptionOnNoMatch() $_SERVER['REQUEST_METHOD'] = 'POST'; $_COOKIE['csrf_cookie_name'] = '8b9218a55906f9dcc1dc263dce7f005b'; - $security = new MockSecurity(new MockAppConfig()); + $security = new MockSecurity(new SecurityConfig()); $request = new IncomingRequest( new MockAppConfig(), new URI('http://badurl.com'), @@ -193,7 +193,7 @@ public function testCSRFVerifyJsonReturnsSelfOnMatch() $_SERVER['REQUEST_METHOD'] = 'POST'; $_COOKIE['csrf_cookie_name'] = '8b9218a55906f9dcc1dc263dce7f005a'; - $security = new MockSecurity(new MockAppConfig()); + $security = new MockSecurity(new SecurityConfig()); $request = new IncomingRequest( new MockAppConfig(), new URI('http://badurl.com'), @@ -213,7 +213,7 @@ public function testCSRFVerifyJsonReturnsSelfOnMatch() public function testSanitizeFilename() { - $security = new MockSecurity(new MockAppConfig()); + $security = new MockSecurity(new SecurityConfig()); $filename = './'; @@ -230,7 +230,7 @@ public function testRegenerateWithFalseSecurityRegenerateProperty() $config->regenerate = false; Factories::injectMock('config', 'Security', $config); - $security = new MockSecurity(new MockAppConfig()); + $security = new MockSecurity($config); $request = new IncomingRequest( new MockAppConfig(), new URI('http://badurl.com'), @@ -255,7 +255,7 @@ public function testRegenerateWithFalseSecurityRegeneratePropertyManually() $config->regenerate = false; Factories::injectMock('config', 'Security', $config); - $security = new MockSecurity(new MockAppConfig()); + $security = new MockSecurity(new SecurityConfig()); $request = new IncomingRequest( new MockAppConfig(), new URI('http://badurl.com'), @@ -281,7 +281,7 @@ public function testRegenerateWithTrueSecurityRegenerateProperty() $config->regenerate = true; Factories::injectMock('config', 'Security', $config); - $security = new MockSecurity(new MockAppConfig()); + $security = new MockSecurity(new SecurityConfig()); $request = new IncomingRequest( new MockAppConfig(), new URI('http://badurl.com'), @@ -298,7 +298,7 @@ public function testRegenerateWithTrueSecurityRegenerateProperty() public function testGetters(): void { - $security = new MockSecurity(new MockAppConfig()); + $security = new MockSecurity(new SecurityConfig()); $this->assertIsString($security->getHash()); $this->assertIsString($security->getTokenName()); From 5e7a1e6e5b54b7964d5d846f18eccb240c05a112 Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 27 Jun 2023 15:29:46 +0900 Subject: [PATCH 04/12] docs: add changelog and upgrade note --- user_guide_src/source/changelogs/v4.4.0.rst | 1 + user_guide_src/source/installation/upgrade_440.rst | 10 ++++++++++ 2 files changed, 11 insertions(+) diff --git a/user_guide_src/source/changelogs/v4.4.0.rst b/user_guide_src/source/changelogs/v4.4.0.rst index b4e705557979..d8efc4622037 100644 --- a/user_guide_src/source/changelogs/v4.4.0.rst +++ b/user_guide_src/source/changelogs/v4.4.0.rst @@ -135,6 +135,7 @@ Changes - **Images:** The default quality for WebP in ``GDHandler`` has been changed from 80 to 90. - **Config:** The deprecated Cookie items in **app/Config/App.php** has been removed. - **Config:** The deprecated Session items in **app/Config/App.php** has been removed. +- **Config:** The deprecated CSRF items in **app/Config/App.php** has been removed. - **Config:** Routing settings have been moved to **app/Config/Routing.php** config file. See :ref:`Upgrading Guide `. - **DownloadResponse:** When generating response headers, does not replace the ``Content-Disposition`` header if it was previously specified. diff --git a/user_guide_src/source/installation/upgrade_440.rst b/user_guide_src/source/installation/upgrade_440.rst index c82723aa129d..3eb8a9728555 100644 --- a/user_guide_src/source/installation/upgrade_440.rst +++ b/user_guide_src/source/installation/upgrade_440.rst @@ -115,6 +115,16 @@ The Cookie config items in **app/Config/App.php** are no longer used. 2. Remove the properties (from ``$cookiePrefix`` to ``$cookieSameSite``) in **app/Config/App.php**. +app/Config/Security.php +----------------------- + +The CSRF config items in **app/Config/App.php** are no longer used. + +1. Copy **app/Config/Security.php** from the new framework to your **app/Config** + directory, and configure it. +2. Remove the properties (from ``$CSRFTokenName`` to ``$CSRFSameSite``) in + **app/Config/App.php**. + app/Config/Session.php ---------------------- From 7a7e98089a63f42dfb227d3b17e66cecc2077763 Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 27 Jun 2023 15:38:16 +0900 Subject: [PATCH 05/12] refactor: remove CSRF properties in MockAppConfig --- system/Test/Mock/MockAppConfig.php | 8 -------- 1 file changed, 8 deletions(-) diff --git a/system/Test/Mock/MockAppConfig.php b/system/Test/Mock/MockAppConfig.php index 0d3a22cc3ff6..0f1d5bf59975 100644 --- a/system/Test/Mock/MockAppConfig.php +++ b/system/Test/Mock/MockAppConfig.php @@ -18,14 +18,6 @@ class MockAppConfig extends App public string $baseURL = 'http://example.com/'; public string $uriProtocol = 'REQUEST_URI'; public array $proxyIPs = []; - public string $CSRFTokenName = 'csrf_test_name'; - public string $CSRFHeaderName = 'X-CSRF-TOKEN'; - public string $CSRFCookieName = 'csrf_cookie_name'; - public int $CSRFExpire = 7200; - public bool $CSRFRegenerate = true; - public array $CSRFExcludeURIs = ['http://example.com']; - public bool $CSRFRedirect = false; - public string $CSRFSameSite = 'Lax'; public bool $CSPEnabled = false; public string $defaultLocale = 'en'; public bool $negotiateLocale = false; From 848436f861ae472d717b4b4a14109801d94087b5 Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 27 Jun 2023 15:49:56 +0900 Subject: [PATCH 06/12] test: extract method to create instance --- .../SecurityCSRFSessionRandomizeTokenTest.php | 27 ++++++++------ .../Security/SecurityCSRFSessionTest.php | 27 ++++++++------ tests/system/Security/SecurityTest.php | 35 +++++++++++-------- 3 files changed, 53 insertions(+), 36 deletions(-) diff --git a/tests/system/Security/SecurityCSRFSessionRandomizeTokenTest.php b/tests/system/Security/SecurityCSRFSessionRandomizeTokenTest.php index 6afc1aeefd22..087f3a7a74df 100644 --- a/tests/system/Security/SecurityCSRFSessionRandomizeTokenTest.php +++ b/tests/system/Security/SecurityCSRFSessionRandomizeTokenTest.php @@ -113,6 +113,11 @@ private function injectSession(string $hash): void Services::injectMock('session', $session); } + private function createSecurity(): Security + { + return new Security($this->config); + } + public function testHashIsReadFromSession() { $security = new MockSecurity($this->config); @@ -133,7 +138,7 @@ public function testCSRFVerifyPostNoToken() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); - $security = new Security($this->config); + $security = $this->createSecurity(); $security->verify($request); } @@ -148,7 +153,7 @@ public function testCSRFVerifyPostThrowsExceptionOnNoMatch() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); - $security = new Security($this->config); + $security = $this->createSecurity(); $security->verify($request); } @@ -163,7 +168,7 @@ public function testCSRFVerifyPostInvalidToken() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); - $security = new Security($this->config); + $security = $this->createSecurity(); $security->verify($request); } @@ -176,7 +181,7 @@ public function testCSRFVerifyPostReturnsSelfOnMatch() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); - $security = new Security($this->config); + $security = $this->createSecurity(); $this->assertInstanceOf(Security::class, $security->verify($request)); $this->assertLogged('info', 'CSRF token verified.'); @@ -190,7 +195,7 @@ public function testCSRFVerifyPOSTHeaderThrowsExceptionOnNoMatch() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); $request->setHeader('X-CSRF-TOKEN', '8b9218a55906f9dcc1dc263dce7f005b'); - $security = new Security($this->config); + $security = $this->createSecurity(); $this->expectException(SecurityException::class); $this->expectExceptionMessage('The action you requested is not allowed.'); @@ -206,7 +211,7 @@ public function testCSRFVerifyPOSTHeaderReturnsSelfOnMatch() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); $request->setHeader('X-CSRF-TOKEN', $this->randomizedToken); - $security = new Security($this->config); + $security = $this->createSecurity(); $this->assertInstanceOf(Security::class, $security->verify($request)); $this->assertLogged('info', 'CSRF token verified.'); @@ -220,7 +225,7 @@ public function testCSRFVerifyPUTHeaderThrowsExceptionOnNoMatch() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); $request->setHeader('X-CSRF-TOKEN', '8b9218a55906f9dcc1dc263dce7f005b'); - $security = new Security($this->config); + $security = $this->createSecurity(); $this->expectException(SecurityException::class); $this->expectExceptionMessage('The action you requested is not allowed.'); @@ -235,7 +240,7 @@ public function testCSRFVerifyPUTHeaderReturnsSelfOnMatch() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); $request->setHeader('X-CSRF-TOKEN', $this->randomizedToken); - $security = new Security($this->config); + $security = $this->createSecurity(); $this->assertInstanceOf(Security::class, $security->verify($request)); $this->assertLogged('info', 'CSRF token verified.'); @@ -251,7 +256,7 @@ public function testCSRFVerifyJsonThrowsExceptionOnNoMatch() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); $request->setBody('{"csrf_test_name":"8b9218a55906f9dcc1dc263dce7f005b"}'); - $security = new Security($this->config); + $security = $this->createSecurity(); $security->verify($request); } @@ -263,7 +268,7 @@ public function testCSRFVerifyJsonReturnsSelfOnMatch() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); $request->setBody('{"csrf_test_name":"' . $this->randomizedToken . '","foo":"bar"}'); - $security = new Security($this->config); + $security = $this->createSecurity(); $this->assertInstanceOf(Security::class, $security->verify($request)); $this->assertLogged('info', 'CSRF token verified.'); @@ -303,7 +308,7 @@ public function testRegenerateWithTrueSecurityRegenerateProperty() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); - $security = new Security($this->config); + $security = $this->createSecurity(); $oldHash = $security->getHash(); $security->verify($request); diff --git a/tests/system/Security/SecurityCSRFSessionTest.php b/tests/system/Security/SecurityCSRFSessionTest.php index 41ccc638331b..e683e6327485 100644 --- a/tests/system/Security/SecurityCSRFSessionTest.php +++ b/tests/system/Security/SecurityCSRFSessionTest.php @@ -106,9 +106,14 @@ private function injectSession(string $hash): void Services::injectMock('session', $session); } + private function createSecurity(): Security + { + return new Security($this->config); + } + public function testHashIsReadFromSession() { - $security = new Security($this->config); + $security = $this->createSecurity(); $this->assertSame($this->hash, $security->getHash()); } @@ -122,7 +127,7 @@ public function testCSRFVerifyPostThrowsExceptionOnNoMatch() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); - $security = new Security(new SecurityConfig()); + $security = $this->createSecurity(); $security->verify($request); } @@ -135,7 +140,7 @@ public function testCSRFVerifyPostReturnsSelfOnMatch() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); - $security = new Security($this->config); + $security = $this->createSecurity(); $this->assertInstanceOf(Security::class, $security->verify($request)); $this->assertLogged('info', 'CSRF token verified.'); @@ -149,7 +154,7 @@ public function testCSRFVerifyPOSTHeaderThrowsExceptionOnNoMatch() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); $request->setHeader('X-CSRF-TOKEN', '8b9218a55906f9dcc1dc263dce7f005b'); - $security = new Security($this->config); + $security = $this->createSecurity(); $this->expectException(SecurityException::class); $security->verify($request); @@ -163,7 +168,7 @@ public function testCSRFVerifyPOSTHeaderReturnsSelfOnMatch() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); $request->setHeader('X-CSRF-TOKEN', '8b9218a55906f9dcc1dc263dce7f005a'); - $security = new Security($this->config); + $security = $this->createSecurity(); $this->assertInstanceOf(Security::class, $security->verify($request)); $this->assertLogged('info', 'CSRF token verified.'); @@ -177,7 +182,7 @@ public function testCSRFVerifyPUTHeaderThrowsExceptionOnNoMatch() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); $request->setHeader('X-CSRF-TOKEN', '8b9218a55906f9dcc1dc263dce7f005b'); - $security = new Security(new SecurityConfig()); + $security = $this->createSecurity(); $this->expectException(SecurityException::class); $security->verify($request); @@ -190,7 +195,7 @@ public function testCSRFVerifyPUTHeaderReturnsSelfOnMatch() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); $request->setHeader('X-CSRF-TOKEN', '8b9218a55906f9dcc1dc263dce7f005a'); - $security = new Security($this->config); + $security = $this->createSecurity(); $this->assertInstanceOf(Security::class, $security->verify($request)); $this->assertLogged('info', 'CSRF token verified.'); @@ -205,7 +210,7 @@ public function testCSRFVerifyJsonThrowsExceptionOnNoMatch() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); $request->setBody('{"csrf_test_name":"8b9218a55906f9dcc1dc263dce7f005b"}'); - $security = new Security(new SecurityConfig()); + $security = $this->createSecurity(); $security->verify($request); } @@ -217,7 +222,7 @@ public function testCSRFVerifyJsonReturnsSelfOnMatch() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); $request->setBody('{"csrf_test_name":"8b9218a55906f9dcc1dc263dce7f005a","foo":"bar"}'); - $security = new Security($this->config); + $security = $this->createSecurity(); $this->assertInstanceOf(Security::class, $security->verify($request)); $this->assertLogged('info', 'CSRF token verified.'); @@ -235,7 +240,7 @@ public function testRegenerateWithFalseSecurityRegenerateProperty() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); - $security = new Security($this->config); + $security = $this->createSecurity(); $oldHash = $security->getHash(); $security->verify($request); @@ -255,7 +260,7 @@ public function testRegenerateWithTrueSecurityRegenerateProperty() $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); - $security = new Security($this->config); + $security = $this->createSecurity(); $oldHash = $security->getHash(); $security->verify($request); diff --git a/tests/system/Security/SecurityTest.php b/tests/system/Security/SecurityTest.php index 82bc6b6c27db..e47e112b72bc 100644 --- a/tests/system/Security/SecurityTest.php +++ b/tests/system/Security/SecurityTest.php @@ -41,9 +41,16 @@ protected function setUp(): void $this->resetServices(); } + private function createMockSecurity(?SecurityConfig $config = null): MockSecurity + { + $config ??= new SecurityConfig(); + + return new MockSecurity($config); + } + public function testBasicConfigIsSaved() { - $security = new MockSecurity(new SecurityConfig()); + $security = $this->createMockSecurity(); $hash = $security->getHash(); @@ -55,7 +62,7 @@ public function testHashIsReadFromCookie() { $_COOKIE['csrf_cookie_name'] = '8b9218a55906f9dcc1dc263dce7f005a'; - $security = new MockSecurity(new SecurityConfig()); + $security = $this->createMockSecurity(); $this->assertSame( '8b9218a55906f9dcc1dc263dce7f005a', @@ -65,7 +72,7 @@ public function testHashIsReadFromCookie() public function testGetHashSetsCookieWhenGETWithoutCSRFCookie() { - $security = new MockSecurity(new SecurityConfig()); + $security = $this->createMockSecurity(); $_SERVER['REQUEST_METHOD'] = 'GET'; @@ -80,7 +87,7 @@ public function testGetHashReturnsCSRFCookieWhenGETWithCSRFCookie() $_SERVER['REQUEST_METHOD'] = 'GET'; $_COOKIE['csrf_cookie_name'] = '8b9218a55906f9dcc1dc263dce7f005a'; - $security = new MockSecurity(new SecurityConfig()); + $security = $this->createMockSecurity(); $security->verify(new Request(new MockAppConfig())); @@ -93,7 +100,7 @@ public function testCSRFVerifyPostThrowsExceptionOnNoMatch() $_POST['csrf_test_name'] = '8b9218a55906f9dcc1dc263dce7f005a'; $_COOKIE['csrf_cookie_name'] = '8b9218a55906f9dcc1dc263dce7f005b'; - $security = new MockSecurity(new SecurityConfig()); + $security = $this->createMockSecurity(); $request = new IncomingRequest( new MockAppConfig(), new URI('http://badurl.com'), @@ -112,7 +119,7 @@ public function testCSRFVerifyPostReturnsSelfOnMatch() $_POST['csrf_test_name'] = '8b9218a55906f9dcc1dc263dce7f005a'; $_COOKIE['csrf_cookie_name'] = '8b9218a55906f9dcc1dc263dce7f005a'; - $security = new MockSecurity(new SecurityConfig()); + $security = $this->createMockSecurity(); $request = new IncomingRequest( new MockAppConfig(), new URI('http://badurl.com'), @@ -131,7 +138,7 @@ public function testCSRFVerifyHeaderThrowsExceptionOnNoMatch() $_SERVER['REQUEST_METHOD'] = 'POST'; $_COOKIE['csrf_cookie_name'] = '8b9218a55906f9dcc1dc263dce7f005b'; - $security = new MockSecurity(new SecurityConfig()); + $security = $this->createMockSecurity(); $request = new IncomingRequest( new MockAppConfig(), new URI('http://badurl.com'), @@ -151,7 +158,7 @@ public function testCSRFVerifyHeaderReturnsSelfOnMatch() $_POST['foo'] = 'bar'; $_COOKIE['csrf_cookie_name'] = '8b9218a55906f9dcc1dc263dce7f005a'; - $security = new MockSecurity(new SecurityConfig()); + $security = $this->createMockSecurity(); $request = new IncomingRequest( new MockAppConfig(), new URI('http://badurl.com'), @@ -172,7 +179,7 @@ public function testCSRFVerifyJsonThrowsExceptionOnNoMatch() $_SERVER['REQUEST_METHOD'] = 'POST'; $_COOKIE['csrf_cookie_name'] = '8b9218a55906f9dcc1dc263dce7f005b'; - $security = new MockSecurity(new SecurityConfig()); + $security = $this->createMockSecurity(); $request = new IncomingRequest( new MockAppConfig(), new URI('http://badurl.com'), @@ -193,7 +200,7 @@ public function testCSRFVerifyJsonReturnsSelfOnMatch() $_SERVER['REQUEST_METHOD'] = 'POST'; $_COOKIE['csrf_cookie_name'] = '8b9218a55906f9dcc1dc263dce7f005a'; - $security = new MockSecurity(new SecurityConfig()); + $security = $this->createMockSecurity(); $request = new IncomingRequest( new MockAppConfig(), new URI('http://badurl.com'), @@ -213,7 +220,7 @@ public function testCSRFVerifyJsonReturnsSelfOnMatch() public function testSanitizeFilename() { - $security = new MockSecurity(new SecurityConfig()); + $security = $this->createMockSecurity(); $filename = './'; @@ -255,7 +262,7 @@ public function testRegenerateWithFalseSecurityRegeneratePropertyManually() $config->regenerate = false; Factories::injectMock('config', 'Security', $config); - $security = new MockSecurity(new SecurityConfig()); + $security = $this->createMockSecurity($config); $request = new IncomingRequest( new MockAppConfig(), new URI('http://badurl.com'), @@ -281,7 +288,7 @@ public function testRegenerateWithTrueSecurityRegenerateProperty() $config->regenerate = true; Factories::injectMock('config', 'Security', $config); - $security = new MockSecurity(new SecurityConfig()); + $security = $this->createMockSecurity($config); $request = new IncomingRequest( new MockAppConfig(), new URI('http://badurl.com'), @@ -298,7 +305,7 @@ public function testRegenerateWithTrueSecurityRegenerateProperty() public function testGetters(): void { - $security = new MockSecurity(new SecurityConfig()); + $security = $this->createMockSecurity(); $this->assertIsString($security->getHash()); $this->assertIsString($security->getTokenName()); From 241ac4c6ff6251a94d9c91ec538402a88218d6fd Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 27 Jun 2023 15:53:17 +0900 Subject: [PATCH 07/12] test: update MockSecurity constructor param --- tests/system/CommonFunctionsTest.php | 3 ++- tests/system/CommonSingleServiceTest.php | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/system/CommonFunctionsTest.php b/tests/system/CommonFunctionsTest.php index 884b2819f912..6246286abc23 100644 --- a/tests/system/CommonFunctionsTest.php +++ b/tests/system/CommonFunctionsTest.php @@ -33,6 +33,7 @@ use Config\Logger; use Config\Modules; use Config\Routing; +use Config\Security as SecurityConfig; use Config\Services; use Config\Session as SessionConfig; use Kint; @@ -336,7 +337,7 @@ public function testAppTimezone() public function testCSRFToken() { - Services::injectMock('security', new MockSecurity(new App())); + Services::injectMock('security', new MockSecurity(new SecurityConfig())); $this->assertSame('csrf_test_name', csrf_token()); } diff --git a/tests/system/CommonSingleServiceTest.php b/tests/system/CommonSingleServiceTest.php index 58854f27df68..676aa529bb48 100644 --- a/tests/system/CommonSingleServiceTest.php +++ b/tests/system/CommonSingleServiceTest.php @@ -15,7 +15,7 @@ use CodeIgniter\Config\Services; use CodeIgniter\Test\CIUnitTestCase; use CodeIgniter\Test\Mock\MockSecurity; -use Config\App; +use Config\Security as SecurityConfig; use ReflectionClass; use ReflectionMethod; @@ -31,7 +31,7 @@ final class CommonSingleServiceTest extends CIUnitTestCase */ public function testSingleServiceWithNoParamsSupplied(string $service): void { - Services::injectMock('security', new MockSecurity(new App())); + Services::injectMock('security', new MockSecurity(new SecurityConfig())); $service1 = single_service($service); $service2 = single_service($service); From 812b172e371fa5f71df27fda89e765d86ff8dc11 Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 27 Jun 2023 16:27:40 +0900 Subject: [PATCH 08/12] test: remove out-of-dated test case $CSRFExpire was removed. --- tests/system/CommonFunctionsTest.php | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/system/CommonFunctionsTest.php b/tests/system/CommonFunctionsTest.php index 6246286abc23..5792cdc59efa 100644 --- a/tests/system/CommonFunctionsTest.php +++ b/tests/system/CommonFunctionsTest.php @@ -497,7 +497,6 @@ public function testReallyWritable() public function testSlashItem() { $this->assertSame('en/', slash_item('defaultLocale')); // en - $this->assertSame('7200/', slash_item('CSRFExpire')); // int 7200 $this->assertSame('', slash_item('negotiateLocale')); // false } From eccbae455c5c9bef00bf91adddeb93075926f118 Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 27 Jun 2023 16:06:46 +0900 Subject: [PATCH 09/12] docs: add Deprecations in changelog --- user_guide_src/source/changelogs/v4.4.0.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/user_guide_src/source/changelogs/v4.4.0.rst b/user_guide_src/source/changelogs/v4.4.0.rst index d8efc4622037..fe4d428998ff 100644 --- a/user_guide_src/source/changelogs/v4.4.0.rst +++ b/user_guide_src/source/changelogs/v4.4.0.rst @@ -162,6 +162,10 @@ Deprecations ``$sessionExpiration``, ``$sessionSavePath``, ``$sessionMatchIP``, ``$sessionTimeToUpdate``, and ``$sessionRegenerateDestroy`` in ``Session`` are deprecated, and no longer used. Use ``$config`` instead. +- **Security:** The property ``$csrfProtection``, ``$tokenRandomize``, + ``$tokenName``, ``$headerName``, ``$expires``, ``$regenerate``, and + ``$redirect`` in ``Security`` are deprecated, and no longer used. Use + ``$config`` instead. Bugs Fixed ********** From 2868f5cb1dabc00976abb552ca656c52927b9996 Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 27 Jun 2023 17:29:52 +0900 Subject: [PATCH 10/12] docs: add method signature change --- user_guide_src/source/changelogs/v4.4.0.rst | 4 ++++ user_guide_src/source/installation/upgrade_440.rst | 7 +++++++ 2 files changed, 11 insertions(+) diff --git a/user_guide_src/source/changelogs/v4.4.0.rst b/user_guide_src/source/changelogs/v4.4.0.rst index fe4d428998ff..638cf2efc266 100644 --- a/user_guide_src/source/changelogs/v4.4.0.rst +++ b/user_guide_src/source/changelogs/v4.4.0.rst @@ -40,6 +40,8 @@ Interface Changes - **Validation:** Added the ``getValidated()`` method in ``ValidationInterface``. +.. _v440-method-signature-changes: + Method Signature Changes ======================== @@ -52,6 +54,8 @@ Method Signature Changes - **Session:** The first parameter of ``__construct()`` in ``BaseHandler``, ``DatabaseHandler``, ``FileHandler``, ``MemcachedHandler``, and ``RedisHandler`` has been changed from ``Config\App`` to ``Config\Session``. +- **Security:** The first parameter of ``Security::__construct()`` has been + changed from ``Config\App`` to ``Config\Security``. Enhancements ************ diff --git a/user_guide_src/source/installation/upgrade_440.rst b/user_guide_src/source/installation/upgrade_440.rst index 3eb8a9728555..0080a767bdc0 100644 --- a/user_guide_src/source/installation/upgrade_440.rst +++ b/user_guide_src/source/installation/upgrade_440.rst @@ -75,6 +75,13 @@ performance. If you extend ``RouteCollection`` and use the ``$routes``, update your code to match the new array structure. +Method Signature Changes +======================== + +Some method signature changes have been made. Classes that extend them should +update their APIs to reflect the changes. See :ref:`v440-method-signature-changes` +for details. + Mandatory File Changes ********************** From 713db9e2c98e4ec87fb54f093d287bc894722805 Mon Sep 17 00:00:00 2001 From: kenjis Date: Thu, 29 Jun 2023 18:30:44 +0900 Subject: [PATCH 11/12] fix: change Services::security() param type --- system/Config/Services.php | 6 ++---- user_guide_src/source/changelogs/v4.4.0.rst | 2 ++ 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/system/Config/Services.php b/system/Config/Services.php index 779afa3ccaf1..5e0c4e70b96e 100644 --- a/system/Config/Services.php +++ b/system/Config/Services.php @@ -627,16 +627,14 @@ public static function router(?RouteCollectionInterface $routes = null, ?Request * secure, most notably the CSRF protection tools. * * @return Security - * - * @TODO replace the first parameter type `?App` with `?SecurityConfig` */ - public static function security(?App $config = null, bool $getShared = true) + public static function security(?SecurityConfig $config = null, bool $getShared = true) { if ($getShared) { return static::getSharedInstance('security', $config); } - $config = config(SecurityConfig::class); + $config ??= config(SecurityConfig::class); return new Security($config); } diff --git a/user_guide_src/source/changelogs/v4.4.0.rst b/user_guide_src/source/changelogs/v4.4.0.rst index 638cf2efc266..403e2ecb2829 100644 --- a/user_guide_src/source/changelogs/v4.4.0.rst +++ b/user_guide_src/source/changelogs/v4.4.0.rst @@ -45,6 +45,8 @@ Interface Changes Method Signature Changes ======================== +- **Services:** The first parameter of ``Services::security()`` has been + changed from ``Config\App`` to ``Config\Security``. - **Routing:** The third parameter ``Routing $routing`` has been added to ``RouteCollection::__construct()``. - **Validation:** The method signature of ``Validation::check()`` has been changed. From 24e9cad7b7956dd7e2a3e5570eb6c65131d8bfa7 Mon Sep 17 00:00:00 2001 From: kenjis Date: Thu, 29 Jun 2023 19:37:15 +0900 Subject: [PATCH 12/12] docs: add instruction in upgrade note --- user_guide_src/source/changelogs/v4.4.0.rst | 17 +++++++++++++---- .../source/installation/upgrade_440.rst | 18 +++++++++++------- 2 files changed, 24 insertions(+), 11 deletions(-) diff --git a/user_guide_src/source/changelogs/v4.4.0.rst b/user_guide_src/source/changelogs/v4.4.0.rst index 403e2ecb2829..fd9fd2740a1c 100644 --- a/user_guide_src/source/changelogs/v4.4.0.rst +++ b/user_guide_src/source/changelogs/v4.4.0.rst @@ -45,12 +45,13 @@ Interface Changes Method Signature Changes ======================== +.. _v440-parameter-type-changes: + +Parameter Type Changes +---------------------- + - **Services:** The first parameter of ``Services::security()`` has been changed from ``Config\App`` to ``Config\Security``. -- **Routing:** The third parameter ``Routing $routing`` has been added to - ``RouteCollection::__construct()``. -- **Validation:** The method signature of ``Validation::check()`` has been changed. - The ``string`` typehint on the ``$rule`` parameter was removed. - **Session:** The second parameter of ``Session::__construct()`` has been changed from ``Config\App`` to ``Config\Session``. - **Session:** The first parameter of ``__construct()`` in ``BaseHandler``, @@ -58,6 +59,14 @@ Method Signature Changes has been changed from ``Config\App`` to ``Config\Session``. - **Security:** The first parameter of ``Security::__construct()`` has been changed from ``Config\App`` to ``Config\Security``. +- **Validation:** The method signature of ``Validation::check()`` has been changed. + The ``string`` typehint on the ``$rule`` parameter was removed. + +Added Parameters +---------------- + +- **Routing:** The third parameter ``Routing $routing`` has been added to + ``RouteCollection::__construct()``. Enhancements ************ diff --git a/user_guide_src/source/installation/upgrade_440.rst b/user_guide_src/source/installation/upgrade_440.rst index 0080a767bdc0..f2018c9684dd 100644 --- a/user_guide_src/source/installation/upgrade_440.rst +++ b/user_guide_src/source/installation/upgrade_440.rst @@ -66,6 +66,17 @@ Interface Changes Some interface changes have been made. Classes that implement them should update their APIs to reflect the changes. See :ref:`v440-interface-changes` for details. +Method Signature Changes +======================== + +Some method signature changes have been made. Classes that extend them should +update their APIs to reflect the changes. See :ref:`v440-method-signature-changes` +for details. + +Also, the parameter types of some constructors and ``Services::security()`` have changed. +If you call them with the parameters, change the parameter values. +See :ref:`v440-parameter-type-changes` for details. + RouteCollection::$routes ======================== @@ -75,13 +86,6 @@ performance. If you extend ``RouteCollection`` and use the ``$routes``, update your code to match the new array structure. -Method Signature Changes -======================== - -Some method signature changes have been made. Classes that extend them should -update their APIs to reflect the changes. See :ref:`v440-method-signature-changes` -for details. - Mandatory File Changes **********************