fix: ensure csrf token is string (#9365)

* fix: ensure csrf token is string * fix: ensure csrf token is string * handle request php://input * handle request php://input * wip * wip * wip * wip Co-authored-by: Michal Sniatala <[email protected]> * wip Co-authored-by: Michal Sniatala <[email protected]> * Update user_guide_src/source/changelogs/v4.5.8.rst * Use data providers --------- Co-authored-by: Michal Sniatala <[email protected]> Co-authored-by: John Paul E. Balandan, CPA <[email protected]>
codeigniter4 · Jan 18, 2025 · 97a6d66 · 97a6d66
1 parent 5f8aa24
commit 97a6d66
Show file tree

Hide file tree

Showing 3 changed files with 94 additions and 22 deletions.
diff --git a/system/Security/Security.php b/system/Security/Security.php
@@ -307,26 +307,29 @@ 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();
 
         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
@@ -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<string, array{string, IncomingRequest}>
+     */
+    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
@@ -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 <https://github.com/codeigniter4/CodeIgniter4/blob/develop/CHANGELOG.md>`_