diff --git a/system/Security/Security.php b/system/Security/Security.php index 5d86dbbe9984..574da8d50dc1 100644 --- a/system/Security/Security.php +++ b/system/Security/Security.php @@ -307,13 +307,13 @@ private function getPostedToken(RequestInterface $request): ?string // Does the token exist in POST, HEADER or optionally php:://input - json data or PUT, DELETE, PATCH - raw data. if ($tokenValue = $request->getPost($this->config->tokenName)) { - return $tokenValue; + return is_string($tokenValue) ? $tokenValue : null; } - if ($request->hasHeader($this->config->headerName) - && $request->header($this->config->headerName)->getValue() !== '' - && $request->header($this->config->headerName)->getValue() !== []) { - return $request->header($this->config->headerName)->getValue(); + if ($request->hasHeader($this->config->headerName)) { + $tokenValue = $request->header($this->config->headerName)->getValue(); + + return (is_string($tokenValue) && $tokenValue !== '') ? $tokenValue : null; } $body = (string) $request->getBody(); @@ -321,12 +321,15 @@ private function getPostedToken(RequestInterface $request): ?string if ($body !== '') { $json = json_decode($body); if ($json !== null && json_last_error() === JSON_ERROR_NONE) { - return $json->{$this->config->tokenName} ?? null; + $tokenValue = $json->{$this->config->tokenName} ?? null; + + return is_string($tokenValue) ? $tokenValue : null; } parse_str($body, $parsed); + $tokenValue = $parsed[$this->config->tokenName] ?? null; - return $parsed[$this->config->tokenName] ?? null; + return is_string($tokenValue) ? $tokenValue : null; } return null; diff --git a/tests/system/Security/SecurityTest.php b/tests/system/Security/SecurityTest.php index fa7d29e775c3..2f3aea06470b 100644 --- a/tests/system/Security/SecurityTest.php +++ b/tests/system/Security/SecurityTest.php @@ -24,6 +24,7 @@ use CodeIgniter\Test\Mock\MockSecurity; use Config\Security as SecurityConfig; use PHPUnit\Framework\Attributes\BackupGlobals; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Group; /** @@ -42,13 +43,23 @@ protected function setUp(): void $this->resetServices(); } - private function createMockSecurity(?SecurityConfig $config = null): MockSecurity + private static function createMockSecurity(SecurityConfig $config = new SecurityConfig()): MockSecurity { - $config ??= new SecurityConfig(); - return new MockSecurity($config); } + private static function createIncomingRequest(): IncomingRequest + { + $config = new MockAppConfig(); + + return new IncomingRequest( + $config, + new SiteURI($config), + null, + new UserAgent(), + ); + } + public function testBasicConfigIsSaved(): void { $security = $this->createMockSecurity(); @@ -108,18 +119,6 @@ public function testCSRFVerifyPostThrowsExceptionOnNoMatch(): void $security->verify($request); } - private function createIncomingRequest(): IncomingRequest - { - $config = new MockAppConfig(); - - return new IncomingRequest( - $config, - new SiteURI($config), - null, - new UserAgent(), - ); - } - public function testCSRFVerifyPostReturnsSelfOnMatch(): void { $_SERVER['REQUEST_METHOD'] = 'POST'; @@ -315,4 +314,73 @@ public function testGetters(): void $this->assertIsString($security->getCookieName()); $this->assertIsBool($security->shouldRedirect()); } + + public function testGetPostedTokenReturnsTokenFromPost(): void + { + $_POST['csrf_test_name'] = '8b9218a55906f9dcc1dc263dce7f005a'; + $request = $this->createIncomingRequest(); + $method = $this->getPrivateMethodInvoker($this->createMockSecurity(), 'getPostedToken'); + + $this->assertSame('8b9218a55906f9dcc1dc263dce7f005a', $method($request)); + } + + public function testGetPostedTokenReturnsTokenFromHeader(): void + { + $_POST = []; + $request = $this->createIncomingRequest()->setHeader('X-CSRF-TOKEN', '8b9218a55906f9dcc1dc263dce7f005a'); + $method = $this->getPrivateMethodInvoker($this->createMockSecurity(), 'getPostedToken'); + + $this->assertSame('8b9218a55906f9dcc1dc263dce7f005a', $method($request)); + } + + public function testGetPostedTokenReturnsTokenFromJsonBody(): void + { + $_POST = []; + $jsonBody = json_encode(['csrf_test_name' => '8b9218a55906f9dcc1dc263dce7f005a']); + $request = $this->createIncomingRequest()->setBody($jsonBody); + $method = $this->getPrivateMethodInvoker($this->createMockSecurity(), 'getPostedToken'); + + $this->assertSame('8b9218a55906f9dcc1dc263dce7f005a', $method($request)); + } + + public function testGetPostedTokenReturnsTokenFromFormBody(): void + { + $_POST = []; + $formBody = 'csrf_test_name=8b9218a55906f9dcc1dc263dce7f005a'; + $request = $this->createIncomingRequest()->setBody($formBody); + $method = $this->getPrivateMethodInvoker($this->createMockSecurity(), 'getPostedToken'); + + $this->assertSame('8b9218a55906f9dcc1dc263dce7f005a', $method($request)); + } + + #[DataProvider('provideGetPostedTokenReturnsNullForInvalidInputs')] + public function testGetPostedTokenReturnsNullForInvalidInputs(string $case, IncomingRequest $request): void + { + $method = $this->getPrivateMethodInvoker($this->createMockSecurity(), 'getPostedToken'); + + $this->assertNull( + $method($request), + sprintf('Failed asserting that %s returns null on invalid input.', $case), + ); + } + + /** + * @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]; + } + } } diff --git a/user_guide_src/source/changelogs/v4.5.8.rst b/user_guide_src/source/changelogs/v4.5.8.rst index e62944df9334..910912ae5dc4 100644 --- a/user_guide_src/source/changelogs/v4.5.8.rst +++ b/user_guide_src/source/changelogs/v4.5.8.rst @@ -39,6 +39,7 @@ Bugs Fixed ********** - **Database:** Fixed a bug where ``Builder::affectedRows()`` threw an error when the previous query call failed in ``Postgre`` and ``SQLSRV`` drivers. +- **Security:** Fixed a bug where the CSRF token validation could fail on malformed input, causing a generic HTTP 500 status code instead of handling the input gracefully. See the repo's `CHANGELOG.md `_