From 27d9984476d17a1e997ac98961f88e555b4768af Mon Sep 17 00:00:00 2001 From: kenjis Date: Thu, 2 Dec 2021 16:03:58 +0900 Subject: [PATCH 01/15] fix: move secure cookie check from Security to CookieStore --- system/Cookie/CookieStore.php | 8 ++++++ system/Security/Security.php | 3 --- tests/system/HTTP/ResponseSendTest.php | 36 ++++++++++++++++++++++++++ 3 files changed, 44 insertions(+), 3 deletions(-) diff --git a/system/Cookie/CookieStore.php b/system/Cookie/CookieStore.php index 87dc472d1fe5..3edcd506d644 100644 --- a/system/Cookie/CookieStore.php +++ b/system/Cookie/CookieStore.php @@ -13,6 +13,8 @@ use ArrayIterator; use CodeIgniter\Cookie\Exceptions\CookieException; +use CodeIgniter\Security\Exceptions\SecurityException; +use Config\Services; use Countable; use IteratorAggregate; use Traversable; @@ -161,7 +163,13 @@ public function remove(string $name, string $prefix = '') */ public function dispatch(): void { + $request = Services::request(); + foreach ($this->cookies as $cookie) { + if ($cookie->isSecure() && ! $request->isSecure()) { + throw SecurityException::forDisallowedAction(); + } + $name = $cookie->getPrefixedName(); $value = $cookie->getValue(); $options = $cookie->getOptions(); diff --git a/system/Security/Security.php b/system/Security/Security.php index 35e889a85d69..dd03d4ac68d9 100644 --- a/system/Security/Security.php +++ b/system/Security/Security.php @@ -538,9 +538,6 @@ private function saveHashInCookie(): void */ protected function sendCookie(RequestInterface $request) { - if ($this->cookie->isSecure() && ! $request->isSecure()) { - return false; - } $this->doSendCookie(); log_message('info', 'CSRF cookie sent.'); diff --git a/tests/system/HTTP/ResponseSendTest.php b/tests/system/HTTP/ResponseSendTest.php index 5fedc69da7b1..6610321040a2 100644 --- a/tests/system/HTTP/ResponseSendTest.php +++ b/tests/system/HTTP/ResponseSendTest.php @@ -11,8 +11,10 @@ namespace CodeIgniter\HTTP; +use CodeIgniter\Security\Exceptions\SecurityException; use CodeIgniter\Test\CIUnitTestCase; use Config\App; +use Config\Services; /** * This test suite has been created separately from @@ -135,4 +137,38 @@ public function testRedirectResponseCookies() $this->assertHeaderEmitted('Set-Cookie: foo=bar;'); $this->assertHeaderEmitted('Set-Cookie: login_time'); } + + /** + * Make sure secure cookies are not sent with HTTP request + * + * @ runInSeparateProcess + * @ preserveGlobalState disabled + */ + public function testDoNotSendUnSecureCookie(): void + { + $this->expectException(SecurityException::class); + $this->expectExceptionMessage('The action you requested is not allowed'); + + $request = $this->createMock(IncomingRequest::class); + $request->method('isSecure')->willReturn(false); + Services::injectMock('request', $request); + + $response = new Response(new App()); + $response->pretend(false); + $body = 'Hello'; + $response->setBody($body); + + $response->setCookie( + 'foo', + 'bar', + '', + '', + '/', + '', + true + ); + + // send it + $response->send(); + } } From a3c65d75fef7c95fb322214544aaa89383bb80c6 Mon Sep 17 00:00:00 2001 From: kenjis Date: Thu, 2 Dec 2021 16:06:36 +0900 Subject: [PATCH 02/15] docs: fix doc comments --- system/HTTP/ResponseTrait.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/system/HTTP/ResponseTrait.php b/system/HTTP/ResponseTrait.php index 7ec5f1e69e6b..39657af8464f 100644 --- a/system/HTTP/ResponseTrait.php +++ b/system/HTTP/ResponseTrait.php @@ -22,7 +22,7 @@ use InvalidArgumentException; /** - * Request Trait + * Response Trait * * Additional methods to make a PSR-7 Response class * compliant with the framework's own ResponseInterface. @@ -446,7 +446,7 @@ public function send() } /** - * Sends the headers of this HTTP request to the browser. + * Sends the headers of this HTTP response to the browser. * * @return Response */ From fd72e192cb9050abf6233bdd4f2daa10a73d4739 Mon Sep 17 00:00:00 2001 From: kenjis Date: Thu, 2 Dec 2021 16:06:58 +0900 Subject: [PATCH 03/15] test: fix setUp() --- tests/system/CommonFunctionsTest.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/system/CommonFunctionsTest.php b/tests/system/CommonFunctionsTest.php index ba4c1e7a4e07..c6f1d6b9b2e1 100644 --- a/tests/system/CommonFunctionsTest.php +++ b/tests/system/CommonFunctionsTest.php @@ -41,10 +41,10 @@ final class CommonFunctionsTest extends CIUnitTestCase { protected function setUp(): void { - parent::setUp(); - $renderer = Services::renderer(); - $renderer->resetData(); unset($_ENV['foo'], $_SERVER['foo']); + Services::reset(); + + parent::setUp(); } public function testStringifyAttributes() From 3aadd002fa4753cfe5ae97aed93fb9a278ff423a Mon Sep 17 00:00:00 2001 From: kenjis Date: Thu, 2 Dec 2021 16:09:28 +0900 Subject: [PATCH 04/15] fix: Security class does not send cookies Add Response::setCookieStore() --- system/HTTP/ResponseTrait.php | 8 ++++ system/Security/Security.php | 28 ++----------- tests/system/Security/SecurityTest.php | 58 +++----------------------- 3 files changed, 17 insertions(+), 77 deletions(-) diff --git a/system/HTTP/ResponseTrait.php b/system/HTTP/ResponseTrait.php index 39657af8464f..46835cc515c6 100644 --- a/system/HTTP/ResponseTrait.php +++ b/system/HTTP/ResponseTrait.php @@ -596,6 +596,14 @@ public function getCookieStore() return $this->cookieStore; } + /** + * Sets the CookieStore. + */ + public function setCookieStore(CookieStore $cookieStore) + { + $this->cookieStore = $cookieStore; + } + /** * Checks to see if the Response has a specified cookie or not. */ diff --git a/system/Security/Security.php b/system/Security/Security.php index dd03d4ac68d9..a484310d40c5 100644 --- a/system/Security/Security.php +++ b/system/Security/Security.php @@ -528,32 +528,12 @@ private function saveHashInCookie(): void 'expires' => $this->expires === 0 ? 0 : time() + $this->expires, ] ); - $this->sendCookie($this->request); - } - - /** - * CSRF Send Cookie - * - * @return false|Security - */ - protected function sendCookie(RequestInterface $request) - { - - $this->doSendCookie(); - log_message('info', 'CSRF cookie sent.'); - return $this; - } + $response = Services::response(); + $cookieStore = $response->getCookieStore(); + $cookieStore = $cookieStore->put($this->cookie); - /** - * Actual dispatching of cookies. - * Extracted for this to be unit tested. - * - * @codeCoverageIgnore - */ - protected function doSendCookie(): void - { - cookies([$this->cookie], false)->dispatch(); + $response->setCookieStore($cookieStore); } private function saveHashInSession(): void diff --git a/tests/system/Security/SecurityTest.php b/tests/system/Security/SecurityTest.php index cc9fcf753853..6bb8d3a809b5 100644 --- a/tests/system/Security/SecurityTest.php +++ b/tests/system/Security/SecurityTest.php @@ -20,8 +20,8 @@ use CodeIgniter\Test\CIUnitTestCase; use CodeIgniter\Test\Mock\MockAppConfig; use CodeIgniter\Test\Mock\MockSecurity; -use Config\Cookie as CookieConfig; use Config\Security as SecurityConfig; +use Config\Services; /** * @backupGlobals enabled @@ -41,11 +41,7 @@ protected function setUp(): void public function testBasicConfigIsSaved() { - $config = new MockAppConfig(); - $security = $this->getMockBuilder(Security::class) - ->setConstructorArgs([$config]) - ->onlyMethods(['doSendCookie']) - ->getMock(); + $security = new MockSecurity(new MockAppConfig()); $hash = $security->getHash(); @@ -57,11 +53,7 @@ public function testHashIsReadFromCookie() { $_COOKIE['csrf_cookie_name'] = '8b9218a55906f9dcc1dc263dce7f005a'; - $config = new MockAppConfig(); - $security = $this->getMockBuilder(Security::class) - ->setConstructorArgs([$config]) - ->onlyMethods(['doSendCookie']) - ->getMock(); + $security = new MockSecurity(new MockAppConfig()); $this->assertSame('8b9218a55906f9dcc1dc263dce7f005a', $security->getHash()); } @@ -74,7 +66,8 @@ public function testGetHashSetsCookieWhenGETWithoutCSRFCookie() $security->verify(new Request(new MockAppConfig())); - $this->assertSame($_COOKIE['csrf_cookie_name'], $security->getHash()); + $cookie = Services::response()->getCookie('csrf_cookie_name'); + $this->assertSame($security->getHash(), $cookie->getValue()); } public function testGetHashReturnsCSRFCookieWhenGETWithCSRFCookie() @@ -238,45 +231,4 @@ public function testGetters(): void $this->assertIsString($security->getCookieName()); $this->assertIsBool($security->shouldRedirect()); } - - public function testSendingCookiesFalse(): void - { - $request = $this->createMock(IncomingRequest::class); - $request->method('isSecure')->willReturn(false); - - $config = new CookieConfig(); - - $config->secure = true; - Factories::injectMock('config', CookieConfig::class, $config); - - $security = $this->getMockBuilder(Security::class) - ->setConstructorArgs([new MockAppConfig()]) - ->onlyMethods(['doSendCookie']) - ->getMock(); - - $sendCookie = $this->getPrivateMethodInvoker($security, 'sendCookie'); - - $security->expects($this->never())->method('doSendCookie'); - $this->assertFalse($sendCookie($request)); - } - - public function testSendingGoodCookies(): void - { - $request = $this->createMock(IncomingRequest::class); - $request->method('isSecure')->willReturn(true); - - $config = new MockAppConfig(); - - $config->cookieSecure = true; - - $security = $this->getMockBuilder(Security::class) - ->setConstructorArgs([$config]) - ->onlyMethods(['doSendCookie']) - ->getMock(); - - $sendCookie = $this->getPrivateMethodInvoker($security, 'sendCookie'); - - $security->expects($this->once())->method('doSendCookie'); - $this->assertInstanceOf(Security::class, $sendCookie($request)); - } } From 7ea464cd4d44b713d4d86a558bcf23865cc69b53 Mon Sep 17 00:00:00 2001 From: kenjis Date: Thu, 2 Dec 2021 16:10:36 +0900 Subject: [PATCH 05/15] test: break long lines --- tests/system/Security/SecurityTest.php | 69 ++++++++++++++++++++++---- 1 file changed, 58 insertions(+), 11 deletions(-) diff --git a/tests/system/Security/SecurityTest.php b/tests/system/Security/SecurityTest.php index 6bb8d3a809b5..160e35b17cb6 100644 --- a/tests/system/Security/SecurityTest.php +++ b/tests/system/Security/SecurityTest.php @@ -55,7 +55,10 @@ public function testHashIsReadFromCookie() $security = new MockSecurity(new MockAppConfig()); - $this->assertSame('8b9218a55906f9dcc1dc263dce7f005a', $security->getHash()); + $this->assertSame( + '8b9218a55906f9dcc1dc263dce7f005a', + $security->getHash() + ); } public function testGetHashSetsCookieWhenGETWithoutCSRFCookie() @@ -89,7 +92,12 @@ public function testCSRFVerifyPostThrowsExceptionOnNoMatch() $_COOKIE['csrf_cookie_name'] = '8b9218a55906f9dcc1dc263dce7f005b'; $security = new MockSecurity(new MockAppConfig()); - $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); + $request = new IncomingRequest( + new MockAppConfig(), + new URI('http://badurl.com'), + null, + new UserAgent() + ); $this->expectException(SecurityException::class); $security->verify($request); @@ -103,7 +111,12 @@ public function testCSRFVerifyPostReturnsSelfOnMatch() $_COOKIE['csrf_cookie_name'] = '8b9218a55906f9dcc1dc263dce7f005a'; $security = new MockSecurity(new MockAppConfig()); - $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); + $request = new IncomingRequest( + new MockAppConfig(), + new URI('http://badurl.com'), + null, + new UserAgent() + ); $this->assertInstanceOf(Security::class, $security->verify($request)); $this->assertLogged('info', 'CSRF token verified.'); @@ -117,7 +130,12 @@ public function testCSRFVerifyHeaderThrowsExceptionOnNoMatch() $_COOKIE['csrf_cookie_name'] = '8b9218a55906f9dcc1dc263dce7f005b'; $security = new MockSecurity(new MockAppConfig()); - $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); + $request = new IncomingRequest( + new MockAppConfig(), + new URI('http://badurl.com'), + null, + new UserAgent() + ); $request->setHeader('X-CSRF-TOKEN', '8b9218a55906f9dcc1dc263dce7f005a'); @@ -132,7 +150,12 @@ public function testCSRFVerifyHeaderReturnsSelfOnMatch() $_COOKIE['csrf_cookie_name'] = '8b9218a55906f9dcc1dc263dce7f005a'; $security = new MockSecurity(new MockAppConfig()); - $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); + $request = new IncomingRequest( + new MockAppConfig(), + new URI('http://badurl.com'), + null, + new UserAgent() + ); $request->setHeader('X-CSRF-TOKEN', '8b9218a55906f9dcc1dc263dce7f005a'); @@ -148,9 +171,16 @@ public function testCSRFVerifyJsonThrowsExceptionOnNoMatch() $_COOKIE['csrf_cookie_name'] = '8b9218a55906f9dcc1dc263dce7f005b'; $security = new MockSecurity(new MockAppConfig()); - $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); + $request = new IncomingRequest( + new MockAppConfig(), + new URI('http://badurl.com'), + null, + new UserAgent() + ); - $request->setBody('{"csrf_test_name":"8b9218a55906f9dcc1dc263dce7f005a"}'); + $request->setBody( + '{"csrf_test_name":"8b9218a55906f9dcc1dc263dce7f005a"}' + ); $this->expectException(SecurityException::class); $security->verify($request); @@ -162,9 +192,16 @@ public function testCSRFVerifyJsonReturnsSelfOnMatch() $_COOKIE['csrf_cookie_name'] = '8b9218a55906f9dcc1dc263dce7f005a'; $security = new MockSecurity(new MockAppConfig()); - $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); + $request = new IncomingRequest( + new MockAppConfig(), + new URI('http://badurl.com'), + null, + new UserAgent() + ); - $request->setBody('{"csrf_test_name":"8b9218a55906f9dcc1dc263dce7f005a","foo":"bar"}'); + $request->setBody( + '{"csrf_test_name":"8b9218a55906f9dcc1dc263dce7f005a","foo":"bar"}' + ); $this->assertInstanceOf(Security::class, $security->verify($request)); $this->assertLogged('info', 'CSRF token verified.'); @@ -192,7 +229,12 @@ public function testRegenerateWithFalseSecurityRegenerateProperty() Factories::injectMock('config', 'Security', $config); $security = new MockSecurity(new MockAppConfig()); - $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); + $request = new IncomingRequest( + new MockAppConfig(), + new URI('http://badurl.com'), + null, + new UserAgent() + ); $oldHash = $security->getHash(); $security->verify($request); @@ -212,7 +254,12 @@ public function testRegenerateWithTrueSecurityRegenerateProperty() Factories::injectMock('config', 'Security', $config); $security = new MockSecurity(new MockAppConfig()); - $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); + $request = new IncomingRequest( + new MockAppConfig(), + new URI('http://badurl.com'), + null, + new UserAgent() + ); $oldHash = $security->getHash(); $security->verify($request); From 628db685b1fa1667535f08aa50828739cebcd707 Mon Sep 17 00:00:00 2001 From: kenjis Date: Thu, 2 Dec 2021 17:29:18 +0900 Subject: [PATCH 06/15] test: fix setUp() --- tests/system/HTTP/ResponseTest.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/system/HTTP/ResponseTest.php b/tests/system/HTTP/ResponseTest.php index b4eba4c547c9..a94cd8e8b2b5 100644 --- a/tests/system/HTTP/ResponseTest.php +++ b/tests/system/HTTP/ResponseTest.php @@ -30,14 +30,18 @@ final class ResponseTest extends CIUnitTestCase protected function setUp(): void { - parent::setUp(); $this->server = $_SERVER; + + Services::reset(); + + parent::setUp(); } protected function tearDown(): void { - $_SERVER = $this->server; Factories::reset('config'); + + $_SERVER = $this->server; } public function testCanSetStatusCode() From d94cca96462ff14108160ed447b4814d964f4a4f Mon Sep 17 00:00:00 2001 From: kenjis Date: Fri, 3 Dec 2021 09:24:55 +0900 Subject: [PATCH 07/15] chore: add rule Cookie depends on Security --- depfile.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/depfile.yaml b/depfile.yaml index 2c87969f4fc4..61b85383b329 100644 --- a/depfile.yaml +++ b/depfile.yaml @@ -157,6 +157,8 @@ ruleset: Controller: - HTTP - Validation + Cookie: + - Security Database: - Entity - Events From d472ec1faafaee79e386aa68176b88f98c3eae7b Mon Sep 17 00:00:00 2001 From: kenjis Date: Fri, 3 Dec 2021 19:52:07 +0900 Subject: [PATCH 08/15] refactor: move cookie dispatching to Response class --- system/Cookie/CookieStore.php | 14 +++++------ system/HTTP/ResponseTrait.php | 47 ++++++++++++++++++++++++++++++++++- 2 files changed, 52 insertions(+), 9 deletions(-) diff --git a/system/Cookie/CookieStore.php b/system/Cookie/CookieStore.php index 3edcd506d644..af7dd2f0e437 100644 --- a/system/Cookie/CookieStore.php +++ b/system/Cookie/CookieStore.php @@ -13,8 +13,6 @@ use ArrayIterator; use CodeIgniter\Cookie\Exceptions\CookieException; -use CodeIgniter\Security\Exceptions\SecurityException; -use Config\Services; use Countable; use IteratorAggregate; use Traversable; @@ -160,16 +158,12 @@ public function remove(string $name, string $prefix = '') /** * Dispatches all cookies in store. + * + * @deprecated Response should dispatch cookies. */ public function dispatch(): void { - $request = Services::request(); - foreach ($this->cookies as $cookie) { - if ($cookie->isSecure() && ! $request->isSecure()) { - throw SecurityException::forDisallowedAction(); - } - $name = $cookie->getPrefixedName(); $value = $cookie->getValue(); $options = $cookie->getOptions(); @@ -240,6 +234,8 @@ protected function validateCookies(array $cookies): void * Extracted call to `setrawcookie()` in order to run unit tests on it. * * @codeCoverageIgnore + * + * @deprecated */ protected function setRawCookie(string $name, string $value, array $options): void { @@ -250,6 +246,8 @@ protected function setRawCookie(string $name, string $value, array $options): vo * Extracted call to `setcookie()` in order to run unit tests on it. * * @codeCoverageIgnore + * + * @deprecated */ protected function setCookie(string $name, string $value, array $options): void { diff --git a/system/HTTP/ResponseTrait.php b/system/HTTP/ResponseTrait.php index 46835cc515c6..3b2a42c9b82d 100644 --- a/system/HTTP/ResponseTrait.php +++ b/system/HTTP/ResponseTrait.php @@ -16,6 +16,7 @@ use CodeIgniter\Cookie\Exceptions\CookieException; use CodeIgniter\HTTP\Exceptions\HTTPException; use CodeIgniter\Pager\PagerInterface; +use CodeIgniter\Security\Exceptions\SecurityException; use Config\Services; use DateTime; use DateTimeZone; @@ -697,7 +698,51 @@ protected function sendCookies() return; } - $this->cookieStore->dispatch(); + $this->dispatchCookies(); + } + + private function dispatchCookies(): void + { + /** @var IncomingRequest $request */ + $request = Services::request(); + + foreach ($this->cookieStore->display() as $cookie) { + if ($cookie->isSecure() && ! $request->isSecure()) { + throw SecurityException::forDisallowedAction(); + } + + $name = $cookie->getPrefixedName(); + $value = $cookie->getValue(); + $options = $cookie->getOptions(); + + if ($cookie->isRaw()) { + $this->doSetRawCookie($name, $value, $options); + } else { + $this->doSetCookie($name, $value, $options); + } + } + + $this->cookieStore->clear(); + } + + /** + * Extracted call to `setrawcookie()` in order to run unit tests on it. + * + * @codeCoverageIgnore + */ + private function doSetRawCookie(string $name, string $value, array $options): void + { + setrawcookie($name, $value, $options); + } + + /** + * Extracted call to `setcookie()` in order to run unit tests on it. + * + * @codeCoverageIgnore + */ + private function doSetCookie(string $name, string $value, array $options): void + { + setcookie($name, $value, $options); } /** From 246f3358ba33118f842cfc32a76f500feb506f5e Mon Sep 17 00:00:00 2001 From: kenjis Date: Fri, 3 Dec 2021 19:53:56 +0900 Subject: [PATCH 09/15] refactor: add @var To explicitly indicate dependency --- system/Security/Security.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/system/Security/Security.php b/system/Security/Security.php index a484310d40c5..bacbc9ee74d9 100644 --- a/system/Security/Security.php +++ b/system/Security/Security.php @@ -13,6 +13,7 @@ use CodeIgniter\Cookie\Cookie; use CodeIgniter\HTTP\RequestInterface; +use CodeIgniter\HTTP\Response; use CodeIgniter\Security\Exceptions\SecurityException; use CodeIgniter\Session\Session; use Config\App; @@ -529,6 +530,7 @@ private function saveHashInCookie(): void ] ); + /** @var Response $response */ $response = Services::response(); $cookieStore = $response->getCookieStore(); $cookieStore = $cookieStore->put($this->cookie); From 11e22972ca7190026b42bad84195ed11a9d934da Mon Sep 17 00:00:00 2001 From: kenjis Date: Fri, 3 Dec 2021 19:55:38 +0900 Subject: [PATCH 10/15] chore: update deptrac rule --- depfile.yaml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/depfile.yaml b/depfile.yaml index 61b85383b329..301f17076f82 100644 --- a/depfile.yaml +++ b/depfile.yaml @@ -157,8 +157,6 @@ ruleset: Controller: - HTTP - Validation - Cookie: - - Security Database: - Entity - Events @@ -174,6 +172,7 @@ ruleset: HTTP: - Cookie - Files + - Security - URI Images: - Files From 3376a7cbe4e188bb32906a8040b9cc26791e7d0e Mon Sep 17 00:00:00 2001 From: kenjis Date: Fri, 3 Dec 2021 20:10:10 +0900 Subject: [PATCH 11/15] fix: restore removed methods as deprecated --- system/Security/Security.php | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/system/Security/Security.php b/system/Security/Security.php index bacbc9ee74d9..344ef5cfe00c 100644 --- a/system/Security/Security.php +++ b/system/Security/Security.php @@ -538,6 +538,38 @@ private function saveHashInCookie(): void $response->setCookieStore($cookieStore); } + /** + * CSRF Send Cookie + * + * @return false|Security + * + * @deprecated Set cookies to Response object instead. + */ + protected function sendCookie(RequestInterface $request) + { + if ($this->cookie->isSecure() && ! $request->isSecure()) { + return false; + } + + $this->doSendCookie(); + log_message('info', 'CSRF cookie sent.'); + + return $this; + } + + /** + * Actual dispatching of cookies. + * Extracted for this to be unit tested. + * + * @codeCoverageIgnore + * + * @deprecated Set cookies to Response object instead. + */ + protected function doSendCookie(): void + { + cookies([$this->cookie], false)->dispatch(); + } + private function saveHashInSession(): void { $this->session->set($this->tokenName, $this->hash); From 23ba12cfe1b3ee5354559258cde5947120a16c54 Mon Sep 17 00:00:00 2001 From: kenjis Date: Sat, 4 Dec 2021 10:35:43 +0900 Subject: [PATCH 12/15] feat: Response::setCookie() can get Cookie object as the first parameter --- system/HTTP/ResponseTrait.php | 24 +++++++++++++++--------- tests/system/HTTP/ResponseCookieTest.php | 11 +++++++++++ 2 files changed, 26 insertions(+), 9 deletions(-) diff --git a/system/HTTP/ResponseTrait.php b/system/HTTP/ResponseTrait.php index 3b2a42c9b82d..7caa5ebbbcca 100644 --- a/system/HTTP/ResponseTrait.php +++ b/system/HTTP/ResponseTrait.php @@ -536,15 +536,15 @@ public function redirect(string $uri, string $method = 'auto', ?int $code = null * Accepts an arbitrary number of binds (up to 7) or an associative * array in the first parameter containing all the values. * - * @param array|string $name Cookie name or array containing binds - * @param string $value Cookie value - * @param string $expire Cookie expiration time in seconds - * @param string $domain Cookie domain (e.g.: '.yourdomain.com') - * @param string $path Cookie path (default: '/') - * @param string $prefix Cookie name prefix - * @param bool $secure Whether to only transfer cookies via SSL - * @param bool $httponly Whether only make the cookie accessible via HTTP (no javascript) - * @param string|null $samesite + * @param array|Cookie|string $name Cookie name / array containing binds / Cookie object + * @param string $value Cookie value + * @param string $expire Cookie expiration time in seconds + * @param string $domain Cookie domain (e.g.: '.yourdomain.com') + * @param string $path Cookie path (default: '/') + * @param string $prefix Cookie name prefix + * @param bool $secure Whether to only transfer cookies via SSL + * @param bool $httponly Whether only make the cookie accessible via HTTP (no javascript) + * @param string|null $samesite * * @return $this */ @@ -559,6 +559,12 @@ public function setCookie( $httponly = false, $samesite = null ) { + if ($name instanceof Cookie) { + $this->cookieStore = $this->cookieStore->put($name); + + return $this; + } + if (is_array($name)) { // always leave 'name' in last place, as the loop will break otherwise, due to $$item foreach (['samesite', 'value', 'expire', 'domain', 'path', 'prefix', 'secure', 'httponly', 'name'] as $item) { diff --git a/tests/system/HTTP/ResponseCookieTest.php b/tests/system/HTTP/ResponseCookieTest.php index 721a2fbd47b7..678928463cc4 100644 --- a/tests/system/HTTP/ResponseCookieTest.php +++ b/tests/system/HTTP/ResponseCookieTest.php @@ -65,6 +65,17 @@ public function testCookiesAll() $this->assertTrue($response->hasCookie('bee')); } + public function testSetCookieObject() + { + $cookie = new Cookie('foo', 'bar'); + $response = new Response(new App()); + + $response->setCookie($cookie); + + $this->assertCount(1, $response->getCookies()); + $this->assertTrue($response->hasCookie('foo')); + } + public function testCookieGet() { $response = new Response(new App()); From 943b4e04051774fa5be8ca89ea8667dc7dca832b Mon Sep 17 00:00:00 2001 From: kenjis Date: Sat, 4 Dec 2021 10:37:53 +0900 Subject: [PATCH 13/15] refactor: use Response::setCookie() --- system/HTTP/ResponseTrait.php | 8 -------- system/Security/Security.php | 7 ++----- 2 files changed, 2 insertions(+), 13 deletions(-) diff --git a/system/HTTP/ResponseTrait.php b/system/HTTP/ResponseTrait.php index 7caa5ebbbcca..278143fd3c3a 100644 --- a/system/HTTP/ResponseTrait.php +++ b/system/HTTP/ResponseTrait.php @@ -603,14 +603,6 @@ public function getCookieStore() return $this->cookieStore; } - /** - * Sets the CookieStore. - */ - public function setCookieStore(CookieStore $cookieStore) - { - $this->cookieStore = $cookieStore; - } - /** * Checks to see if the Response has a specified cookie or not. */ diff --git a/system/Security/Security.php b/system/Security/Security.php index 344ef5cfe00c..008e94aeb612 100644 --- a/system/Security/Security.php +++ b/system/Security/Security.php @@ -531,11 +531,8 @@ private function saveHashInCookie(): void ); /** @var Response $response */ - $response = Services::response(); - $cookieStore = $response->getCookieStore(); - $cookieStore = $cookieStore->put($this->cookie); - - $response->setCookieStore($cookieStore); + $response = Services::response(); + $response->setCookie($this->cookie); } /** From 6817dc6f5f99cf9636e36479daf0259c512f136d Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 8 Dec 2021 10:21:19 +0900 Subject: [PATCH 14/15] docs: fix header level --- user_guide_src/source/changelogs/v4.1.6.rst | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/user_guide_src/source/changelogs/v4.1.6.rst b/user_guide_src/source/changelogs/v4.1.6.rst index 4cddcc21f953..e747e9181fa8 100644 --- a/user_guide_src/source/changelogs/v4.1.6.rst +++ b/user_guide_src/source/changelogs/v4.1.6.rst @@ -10,13 +10,15 @@ Release Date: Not released :depth: 2 BREAKING -======== +******** + - Multiple table names will no longer be stored in ``BaseBuilder::$tableName`` - an empty string will be used instead. .. _changelog-v416-validation-changes: Validation changes ------------------- +================== + - The previous version of the Validation can't handle an array item. Because of the bug fix, the validation results may be different, or raise a ``TypeError``. @@ -28,14 +30,15 @@ Validation changes On the other hand, the current version passes the array to the validation rule as a whole. Enhancements -============ +************ + - Database pane on debug toolbar now displays location where Query was called from. Also displays full backtrace. Changes -======= +******* Deprecations -============ +************ Bugs Fixed -========== +********** From 49cd87991b218e67641557285ef5f3b4ba3c51fb Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 8 Dec 2021 10:46:06 +0900 Subject: [PATCH 15/15] docs: add changelog and update Response::setCookie() --- user_guide_src/source/changelogs/v4.1.6.rst | 14 ++++++++++++++ user_guide_src/source/outgoing/response.rst | 2 +- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/user_guide_src/source/changelogs/v4.1.6.rst b/user_guide_src/source/changelogs/v4.1.6.rst index e747e9181fa8..82b03127cda2 100644 --- a/user_guide_src/source/changelogs/v4.1.6.rst +++ b/user_guide_src/source/changelogs/v4.1.6.rst @@ -37,8 +37,22 @@ Enhancements Changes ******* +- The process of sending cookies has been moved to the ``Response`` class. Now the ``Security`` and ``CookieStore`` class don't send cookies, set them to the Response. + Deprecations ************ +Sending Cookies +=============== + +The process of sending cookies has been moved to the ``Response`` class. +And the following methods are deprecated: + +- ``CookieStore::dispatch()`` +- ``CookieStore::setRawCookie()`` +- ``CookieStore::setCookie()`` +- ``Security::sendCookie()`` +- ``Security::doSendCookie()`` + Bugs Fixed ********** diff --git a/user_guide_src/source/outgoing/response.rst b/user_guide_src/source/outgoing/response.rst index 060e2cd28291..2cd619dbd390 100644 --- a/user_guide_src/source/outgoing/response.rst +++ b/user_guide_src/source/outgoing/response.rst @@ -396,7 +396,7 @@ The methods provided by the parent class that are available are: .. php:method:: setCookie($name = ''[, $value = ''[, $expire = ''[, $domain = ''[, $path = '/'[, $prefix = ''[, $secure = false[, $httponly = false[, $samesite = null]]]]]]]]) - :param mixed $name: Cookie name or an array of parameters + :param array|Cookie|string $name: Cookie name or an array of parameters or an instance of ``CodeIgniter\Cookie\Cookie`` :param string $value: Cookie value :param int $expire: Cookie expiration time in seconds :param string $domain: Cookie domain