diff --git a/system/Security/Security.php b/system/Security/Security.php index 5e7d0bfeb679..873fee7469a8 100644 --- a/system/Security/Security.php +++ b/system/Security/Security.php @@ -18,7 +18,6 @@ use CodeIgniter\Exceptions\LogicException; use CodeIgniter\HTTP\IncomingRequest; use CodeIgniter\HTTP\Method; -use CodeIgniter\HTTP\Request; use CodeIgniter\HTTP\RequestInterface; use CodeIgniter\I18n\Time; use CodeIgniter\Security\Exceptions\SecurityException; @@ -26,6 +25,7 @@ use Config\Cookie as CookieConfig; use Config\Security as SecurityConfig; use ErrorException; +use JsonException; use SensitiveParameter; /** @@ -233,32 +233,27 @@ private function configureCookie(CookieConfig $cookie): void Cookie::setDefaults($cookie); } - /** - * CSRF verification. - * - * @return $this - * - * @throws SecurityException - */ public function verify(RequestInterface $request) { - // Protects POST, PUT, DELETE, PATCH - $method = $request->getMethod(); - $methodsToProtect = [Method::POST, Method::PUT, Method::DELETE, Method::PATCH]; - if (! in_array($method, $methodsToProtect, true)) { + $method = $request->getMethod(); + + // Protect POST, PUT, DELETE, PATCH requests only + if (! in_array($method, [Method::POST, Method::PUT, Method::DELETE, Method::PATCH], true)) { return $this; } + assert($request instanceof IncomingRequest); + $postedToken = $this->getPostedToken($request); try { - $token = ($postedToken !== null && $this->config->tokenRandomize) - ? $this->derandomize($postedToken) : $postedToken; + $token = $postedToken !== null && $this->config->tokenRandomize + ? $this->derandomize($postedToken) + : $postedToken; } catch (InvalidArgumentException) { $token = null; } - // Do the tokens match? if (! isset($token, $this->hash) || ! hash_equals($this->hash, $token)) { throw SecurityException::forDisallowedAction(); } @@ -277,66 +272,107 @@ public function verify(RequestInterface $request) /** * Remove token in POST or JSON request data */ - private function removeTokenInRequest(RequestInterface $request): void + private function removeTokenInRequest(IncomingRequest $request): void { - assert($request instanceof Request); - $superglobals = service('superglobals'); - if ($superglobals->post($this->config->tokenName) !== null) { - // We kill this since we're done and we don't want to pollute the POST array. - $superglobals->unsetPost($this->config->tokenName); + $tokenName = $this->config->tokenName; + + // If the token is found in POST data, we can safely remove it. + if (is_string($superglobals->post($tokenName))) { + $superglobals->unsetPost($tokenName); $request->setGlobal('post', $superglobals->getPostArray()); - } else { - $body = $request->getBody() ?? ''; - $json = json_decode($body); - if ($json !== null && json_last_error() === JSON_ERROR_NONE) { - // We kill this since we're done and we don't want to pollute the JSON data. - unset($json->{$this->config->tokenName}); - $request->setBody(json_encode($json)); - } else { - parse_str($body, $parsed); - // We kill this since we're done and we don't want to pollute the BODY data. - unset($parsed[$this->config->tokenName]); - $request->setBody(http_build_query($parsed)); - } + + return; + } + + $body = $request->getBody() ?? ''; + + if ($body === '') { + return; + } + + // If the token is found in JSON data, we can safely remove it. + try { + $json = json_decode($body, flags: JSON_THROW_ON_ERROR); + } catch (JsonException) { + $json = null; } + + if (is_object($json) && property_exists($json, $tokenName)) { + unset($json->{$tokenName}); + $request->setBody(json_encode($json)); + + return; + } + + // If the token is found in form-encoded data, we can safely remove it. + parse_str($body, $result); + unset($result[$tokenName]); + $request->setBody(http_build_query($result)); } - private function getPostedToken(RequestInterface $request): ?string + private function getPostedToken(IncomingRequest $request): ?string { - assert($request instanceof IncomingRequest); + $tokenName = $this->config->tokenName; + $headerName = $this->config->headerName; - // Does the token exist in POST, HEADER or optionally php:://input - json data or PUT, DELETE, PATCH - raw data. + // 1. Check POST data first. + $token = $request->getPost($tokenName); - if ($tokenValue = $request->getPost($this->config->tokenName)) { - return is_string($tokenValue) ? $tokenValue : null; + if ($this->isNonEmptyTokenString($token)) { + return $token; } - if ($request->hasHeader($this->config->headerName)) { - $tokenValue = $request->header($this->config->headerName)->getValue(); + // 2. Check header data next. + if ($request->hasHeader($headerName)) { + $token = $request->header($headerName)->getValue(); - return (is_string($tokenValue) && $tokenValue !== '') ? $tokenValue : null; + if ($this->isNonEmptyTokenString($token)) { + return $token; + } } - $body = (string) $request->getBody(); + // 3. Finally, check the raw input data for JSON or form-encoded data. + $body = $request->getBody() ?? ''; - if ($body !== '') { - $json = json_decode($body); - if ($json !== null && json_last_error() === JSON_ERROR_NONE) { - $tokenValue = $json->{$this->config->tokenName} ?? null; + if ($body === '') { + return null; + } - return is_string($tokenValue) ? $tokenValue : null; + // 3a. Check if a JSON payload exists and contains the token. + try { + $json = json_decode($body, flags: JSON_THROW_ON_ERROR); + } catch (JsonException) { + $json = null; + } + + if (is_object($json) && property_exists($json, $tokenName)) { + $token = $json->{$tokenName}; + + if ($this->isNonEmptyTokenString($token)) { + return $token; } + } - parse_str($body, $parsed); - $tokenValue = $parsed[$this->config->tokenName] ?? null; + // 3b. Check if form-encoded data exists and contains the token. + parse_str($body, $result); + $token = $result[$tokenName] ?? null; - return is_string($tokenValue) ? $tokenValue : null; + if ($this->isNonEmptyTokenString($token)) { + return $token; } return null; } + /** + * @phpstan-assert-if-true non-empty-string $token + */ + private function isNonEmptyTokenString(mixed $token): bool + { + return is_string($token) && $token !== ''; + } + /** * Returns the CSRF Token. */ diff --git a/tests/system/Security/SecurityTest.php b/tests/system/Security/SecurityTest.php index 8c2a46760810..90f2139b2ccd 100644 --- a/tests/system/Security/SecurityTest.php +++ b/tests/system/Security/SecurityTest.php @@ -16,7 +16,6 @@ use CodeIgniter\Config\Factories; use CodeIgniter\Config\Services; use CodeIgniter\HTTP\IncomingRequest; -use CodeIgniter\HTTP\Request; use CodeIgniter\HTTP\SiteURI; use CodeIgniter\HTTP\UserAgent; use CodeIgniter\Security\Exceptions\SecurityException; @@ -36,13 +35,17 @@ #[Group('Others')] final class SecurityTest extends CIUnitTestCase { + private const CORRECT_CSRF_HASH = '8b9218a55906f9dcc1dc263dce7f005a'; + private const INVALID_CSRF_HASH = '8b9218a55906f9dcc1dc263dce7f005b'; + protected function setUp(): void { parent::setUp(); $this->resetServices(); - Services::injectMock('superglobals', new Superglobals(null, null, null, [])); + // Ensure that POST and COOKIE superglobals are reset for each test to prevent cross-test pollution. + Services::injectMock('superglobals', new Superglobals(post: [], cookie: [])); } private static function createMockSecurity(SecurityConfig $config = new SecurityConfig()): MockSecurity @@ -54,12 +57,7 @@ private static function createIncomingRequest(): IncomingRequest { $config = new MockAppConfig(); - return new IncomingRequest( - $config, - new SiteURI($config), - null, - new UserAgent(), - ); + return new IncomingRequest($config, new SiteURI($config), null, new UserAgent()); } public function testBasicConfigIsSaved(): void @@ -74,47 +72,44 @@ public function testBasicConfigIsSaved(): void public function testHashIsReadFromCookie(): void { - service('superglobals')->setCookie('csrf_cookie_name', '8b9218a55906f9dcc1dc263dce7f005a'); + service('superglobals')->setCookie('csrf_cookie_name', self::CORRECT_CSRF_HASH); $security = $this->createMockSecurity(); - $this->assertSame( - '8b9218a55906f9dcc1dc263dce7f005a', - $security->getHash(), - ); + $this->assertSame(self::CORRECT_CSRF_HASH, $security->getHash()); } - public function testGetHashSetsCookieWhenGETWithoutCSRFCookie(): void + public function testGetHashSetsCookieWhenGetWithoutCsrfCookie(): void { $security = $this->createMockSecurity(); service('superglobals')->setServer('REQUEST_METHOD', 'GET'); - $security->verify(new Request(new MockAppConfig())); + $security->verify($this->createIncomingRequest()); $cookie = service('response')->getCookie('csrf_cookie_name'); $this->assertSame($security->getHash(), $cookie->getValue()); } - public function testGetHashReturnsCSRFCookieWhenGETWithCSRFCookie(): void + public function testGetHashReturnsCsrfCookieWhenGetWithCsrfCookie(): void { service('superglobals') ->setServer('REQUEST_METHOD', 'GET') - ->setCookie('csrf_cookie_name', '8b9218a55906f9dcc1dc263dce7f005a'); + ->setCookie('csrf_cookie_name', self::CORRECT_CSRF_HASH); $security = $this->createMockSecurity(); - $security->verify(new Request(new MockAppConfig())); + $security->verify($this->createIncomingRequest()); - $this->assertSame('8b9218a55906f9dcc1dc263dce7f005a', $security->getHash()); + $this->assertSame(self::CORRECT_CSRF_HASH, $security->getHash()); } - public function testCSRFVerifyPostThrowsExceptionOnNoMatch(): void + public function testCsrfVerifyPostThrowsExceptionOnNoMatch(): void { service('superglobals') ->setServer('REQUEST_METHOD', 'POST') - ->setPost('csrf_test_name', '8b9218a55906f9dcc1dc263dce7f005a') - ->setCookie('csrf_cookie_name', '8b9218a55906f9dcc1dc263dce7f005b'); + ->setPost('csrf_test_name', self::CORRECT_CSRF_HASH) + ->setCookie('csrf_cookie_name', self::INVALID_CSRF_HASH); $security = $this->createMockSecurity(); $request = $this->createIncomingRequest(); @@ -123,13 +118,13 @@ public function testCSRFVerifyPostThrowsExceptionOnNoMatch(): void $security->verify($request); } - public function testCSRFVerifyPostReturnsSelfOnMatch(): void + public function testCsrfVerifyPostReturnsSelfOnMatch(): void { service('superglobals') ->setServer('REQUEST_METHOD', 'POST') ->setPost('foo', 'bar') - ->setPost('csrf_test_name', '8b9218a55906f9dcc1dc263dce7f005a') - ->setCookie('csrf_cookie_name', '8b9218a55906f9dcc1dc263dce7f005a'); + ->setPost('csrf_test_name', self::CORRECT_CSRF_HASH) + ->setCookie('csrf_cookie_name', self::CORRECT_CSRF_HASH); $security = $this->createMockSecurity(); $request = $this->createIncomingRequest(); @@ -140,67 +135,67 @@ public function testCSRFVerifyPostReturnsSelfOnMatch(): void $this->assertCount(1, service('superglobals')->getPostArray()); } - public function testCSRFVerifyHeaderThrowsExceptionOnNoMatch(): void + public function testCsrfVerifyHeaderThrowsExceptionOnNoMatch(): void { service('superglobals') ->setServer('REQUEST_METHOD', 'POST') - ->setCookie('csrf_cookie_name', '8b9218a55906f9dcc1dc263dce7f005b'); + ->setCookie('csrf_cookie_name', self::INVALID_CSRF_HASH); $security = $this->createMockSecurity(); $request = $this->createIncomingRequest(); - $request->setHeader('X-CSRF-TOKEN', '8b9218a55906f9dcc1dc263dce7f005a'); + $request->setHeader('X-CSRF-TOKEN', self::CORRECT_CSRF_HASH); $this->expectException(SecurityException::class); $security->verify($request); } - public function testCSRFVerifyHeaderReturnsSelfOnMatch(): void + public function testCsrfVerifyHeaderReturnsSelfOnMatch(): void { service('superglobals') ->setServer('REQUEST_METHOD', 'POST') ->setPost('foo', 'bar') - ->setCookie('csrf_cookie_name', '8b9218a55906f9dcc1dc263dce7f005a'); + ->setCookie('csrf_cookie_name', self::CORRECT_CSRF_HASH); $security = $this->createMockSecurity(); $request = $this->createIncomingRequest(); - $request->setHeader('X-CSRF-TOKEN', '8b9218a55906f9dcc1dc263dce7f005a'); + $request->setHeader('X-CSRF-TOKEN', self::CORRECT_CSRF_HASH); $this->assertInstanceOf(Security::class, $security->verify($request)); $this->assertLogged('info', 'CSRF token verified.'); - $this->assertCount(1, service('superglobals')->getPostArray()); + $this->assertSame(['foo' => 'bar'], service('superglobals')->getPostArray()); } - public function testCSRFVerifyJsonThrowsExceptionOnNoMatch(): void + public function testCsrfVerifyJsonThrowsExceptionOnNoMatch(): void { service('superglobals') ->setServer('REQUEST_METHOD', 'POST') - ->setCookie('csrf_cookie_name', '8b9218a55906f9dcc1dc263dce7f005b'); + ->setCookie('csrf_cookie_name', self::INVALID_CSRF_HASH); $security = $this->createMockSecurity(); $request = $this->createIncomingRequest(); $request->setBody( - '{"csrf_test_name":"8b9218a55906f9dcc1dc263dce7f005a"}', + '{"csrf_test_name":"' . self::CORRECT_CSRF_HASH . '"}', ); $this->expectException(SecurityException::class); $security->verify($request); } - public function testCSRFVerifyJsonReturnsSelfOnMatch(): void + public function testCsrfVerifyJsonReturnsSelfOnMatch(): void { service('superglobals') ->setServer('REQUEST_METHOD', 'POST') - ->setCookie('csrf_cookie_name', '8b9218a55906f9dcc1dc263dce7f005a'); + ->setCookie('csrf_cookie_name', self::CORRECT_CSRF_HASH); $security = $this->createMockSecurity(); $request = $this->createIncomingRequest(); $request->setBody( - '{"csrf_test_name":"8b9218a55906f9dcc1dc263dce7f005a","foo":"bar"}', + '{"csrf_test_name":"' . self::CORRECT_CSRF_HASH . '","foo":"bar"}', ); $this->assertInstanceOf(Security::class, $security->verify($request)); @@ -209,34 +204,34 @@ public function testCSRFVerifyJsonReturnsSelfOnMatch(): void $this->assertSame('{"foo":"bar"}', $request->getBody()); } - public function testCSRFVerifyPutBodyThrowsExceptionOnNoMatch(): void + public function testCsrfVerifyPutBodyThrowsExceptionOnNoMatch(): void { service('superglobals') ->setServer('REQUEST_METHOD', 'PUT') - ->setCookie('csrf_cookie_name', '8b9218a55906f9dcc1dc263dce7f005b'); + ->setCookie('csrf_cookie_name', self::INVALID_CSRF_HASH); $security = $this->createMockSecurity(); $request = $this->createIncomingRequest(); $request->setBody( - 'csrf_test_name=8b9218a55906f9dcc1dc263dce7f005a', + 'csrf_test_name=' . self::CORRECT_CSRF_HASH, ); $this->expectException(SecurityException::class); $security->verify($request); } - public function testCSRFVerifyPutBodyReturnsSelfOnMatch(): void + public function testCsrfVerifyPutBodyReturnsSelfOnMatch(): void { service('superglobals') ->setServer('REQUEST_METHOD', 'PUT') - ->setCookie('csrf_cookie_name', '8b9218a55906f9dcc1dc263dce7f005a'); + ->setCookie('csrf_cookie_name', self::CORRECT_CSRF_HASH); $security = $this->createMockSecurity(); $request = $this->createIncomingRequest(); $request->setBody( - 'csrf_test_name=8b9218a55906f9dcc1dc263dce7f005a&foo=bar', + 'csrf_test_name=' . self::CORRECT_CSRF_HASH . '&foo=bar', ); $this->assertInstanceOf(Security::class, $security->verify($request)); @@ -258,8 +253,8 @@ public function testRegenerateWithFalseSecurityRegenerateProperty(): void { service('superglobals') ->setServer('REQUEST_METHOD', 'POST') - ->setPost('csrf_test_name', '8b9218a55906f9dcc1dc263dce7f005a') - ->setCookie('csrf_cookie_name', '8b9218a55906f9dcc1dc263dce7f005a'); + ->setPost('csrf_test_name', self::CORRECT_CSRF_HASH) + ->setCookie('csrf_cookie_name', self::CORRECT_CSRF_HASH); $config = new SecurityConfig(); $config->regenerate = false; @@ -279,8 +274,8 @@ public function testRegenerateWithFalseSecurityRegeneratePropertyManually(): voi { service('superglobals') ->setServer('REQUEST_METHOD', 'POST') - ->setPost('csrf_test_name', '8b9218a55906f9dcc1dc263dce7f005a') - ->setCookie('csrf_cookie_name', '8b9218a55906f9dcc1dc263dce7f005a'); + ->setPost('csrf_test_name', self::CORRECT_CSRF_HASH) + ->setCookie('csrf_cookie_name', self::CORRECT_CSRF_HASH); $config = new SecurityConfig(); $config->regenerate = false; @@ -301,8 +296,8 @@ public function testRegenerateWithTrueSecurityRegenerateProperty(): void { service('superglobals') ->setServer('REQUEST_METHOD', 'POST') - ->setPost('csrf_test_name', '8b9218a55906f9dcc1dc263dce7f005a') - ->setCookie('csrf_cookie_name', '8b9218a55906f9dcc1dc263dce7f005a'); + ->setPost('csrf_test_name', self::CORRECT_CSRF_HASH) + ->setCookie('csrf_cookie_name', self::CORRECT_CSRF_HASH); $config = new SecurityConfig(); $config->regenerate = true; @@ -331,67 +326,64 @@ public function testGetters(): void public function testGetPostedTokenReturnsTokenFromPost(): void { - service('superglobals')->setPost('csrf_test_name', '8b9218a55906f9dcc1dc263dce7f005a'); + service('superglobals')->setPost('csrf_test_name', self::CORRECT_CSRF_HASH); $request = $this->createIncomingRequest(); $method = self::getPrivateMethodInvoker($this->createMockSecurity(), 'getPostedToken'); - $this->assertSame('8b9218a55906f9dcc1dc263dce7f005a', $method($request)); + $this->assertSame(self::CORRECT_CSRF_HASH, $method($request)); } public function testGetPostedTokenReturnsTokenFromHeader(): void { - $request = $this->createIncomingRequest()->setHeader('X-CSRF-TOKEN', '8b9218a55906f9dcc1dc263dce7f005a'); + $request = $this->createIncomingRequest()->setHeader('X-CSRF-TOKEN', self::CORRECT_CSRF_HASH); $method = self::getPrivateMethodInvoker($this->createMockSecurity(), 'getPostedToken'); - $this->assertSame('8b9218a55906f9dcc1dc263dce7f005a', $method($request)); + $this->assertSame(self::CORRECT_CSRF_HASH, $method($request)); } public function testGetPostedTokenReturnsTokenFromJsonBody(): void { - $jsonBody = json_encode(['csrf_test_name' => '8b9218a55906f9dcc1dc263dce7f005a']); + $jsonBody = json_encode(['csrf_test_name' => self::CORRECT_CSRF_HASH]); $request = $this->createIncomingRequest()->setBody($jsonBody); $method = self::getPrivateMethodInvoker($this->createMockSecurity(), 'getPostedToken'); - $this->assertSame('8b9218a55906f9dcc1dc263dce7f005a', $method($request)); + $this->assertSame(self::CORRECT_CSRF_HASH, $method($request)); } public function testGetPostedTokenReturnsTokenFromFormBody(): void { - $formBody = 'csrf_test_name=8b9218a55906f9dcc1dc263dce7f005a'; + $formBody = 'csrf_test_name=' . self::CORRECT_CSRF_HASH; $request = $this->createIncomingRequest()->setBody($formBody); $method = self::getPrivateMethodInvoker($this->createMockSecurity(), 'getPostedToken'); - $this->assertSame('8b9218a55906f9dcc1dc263dce7f005a', $method($request)); + $this->assertSame(self::CORRECT_CSRF_HASH, $method($request)); } #[DataProvider('provideGetPostedTokenReturnsNullForInvalidInputs')] - public function testGetPostedTokenReturnsNullForInvalidInputs(string $case, IncomingRequest $request): void + public function testGetPostedTokenReturnsNullForInvalidInputs(IncomingRequest $request): void { - $method = self::getPrivateMethodInvoker($this->createMockSecurity(), 'getPostedToken'); + $getPostedToken = self::getPrivateMethodInvoker($this->createMockSecurity(), 'getPostedToken'); - $this->assertNull( - $method($request), - sprintf('Failed asserting that %s returns null on invalid input.', $case), - ); + $this->assertNull($getPostedToken($request)); } /** - * @return iterable + * @return iterable */ public static function provideGetPostedTokenReturnsNullForInvalidInputs(): iterable { - $testCases = [ - 'empty_post' => self::createIncomingRequest(), - 'invalid_post_data' => self::createIncomingRequest()->setGlobal('post', ['csrf_test_name' => ['invalid' => 'data']]), - 'empty_header' => self::createIncomingRequest()->setHeader('X-CSRF-TOKEN', ''), - 'invalid_json_data' => self::createIncomingRequest()->setBody(json_encode(['csrf_test_name' => ['invalid' => 'data']])), - 'invalid_json' => self::createIncomingRequest()->setBody('{invalid json}'), - 'missing_token_in_body' => self::createIncomingRequest()->setBody('other=value&another=test'), - 'invalid_form_data' => self::createIncomingRequest()->setBody('csrf_test_name[]=invalid'), - ]; - - foreach ($testCases as $case => $request) { - yield $case => [$case, $request]; - } + yield 'empty_post' => [self::createIncomingRequest()]; + + yield 'invalid_post_data' => [self::createIncomingRequest()->setGlobal('post', ['csrf_test_name' => ['invalid' => 'data']])]; + + yield 'empty_header' => [self::createIncomingRequest()->setHeader('X-CSRF-TOKEN', '')]; + + yield 'invalid_json_data' => [self::createIncomingRequest()->setBody(json_encode(['csrf_test_name' => ['invalid' => 'data']]))]; + + yield 'invalid_json' => [self::createIncomingRequest()->setBody('{invalid json}')]; + + yield 'missing_token_in_body' => [self::createIncomingRequest()->setBody('other=value&another=test')]; + + yield 'invalid_form_data' => [self::createIncomingRequest()->setBody('csrf_test_name[]=invalid')]; } }