Merge pull request #7915 from michalsn/fix/security

fix: check for CSRF token in the raw body
codeigniter4 · Sep 18, 2023 · e90be66 · e90be66
2 parents 689c933 + 46f8d5f
commit e90be66
Show file tree

Hide file tree

Showing 7 changed files with 100 additions and 16 deletions.
diff --git a/phpstan-baseline.php b/phpstan-baseline.php
@@ -2421,11 +2421,6 @@
 	'count' => 1,
 	'path' => __DIR__ . '/system/Router/RouterInterface.php',
 ];
-$ignoreErrors[] = [
-	'message' => '#^Construct empty\\(\\) is not allowed\\. Use more strict comparison\\.$#',
-	'count' => 2,
-	'path' => __DIR__ . '/system/Security/Security.php',
-];
 $ignoreErrors[] = [
 	'message' => '#^Method CodeIgniter\\\\Session\\\\Exceptions\\\\SessionException\\:\\:forEmptySavepath\\(\\) has no return type specified\\.$#',
 	'count' => 1,

diff --git a/system/Security/Security.php b/system/Security/Security.php
@@ -318,38 +318,53 @@ private function removeTokenInRequest(RequestInterface $request): void
     {
         assert($request instanceof Request);
 
-        $json = json_decode($request->getBody() ?? '');
-
         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->config->tokenName]);
             $request->setGlobal('post', $_POST);
-        } 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->config->tokenName});
-            $request->setBody(json_encode($json));
+        } 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));
+            }
         }
     }
 
     private function getPostedToken(RequestInterface $request): ?string
     {
         assert($request instanceof IncomingRequest);
 
-        // Does the token exist in POST, HEADER or optionally php:://input - json data.
+        // 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;
         }
 
-        if ($request->hasHeader($this->config->headerName) && ! empty($request->header($this->config->headerName)->getValue())) {
+        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();
         }
 
         $body = (string) $request->getBody();
-        $json = json_decode($body);
 
-        if ($body !== '' && ! empty($json) && json_last_error() === JSON_ERROR_NONE) {
-            return $json->{$this->config->tokenName} ?? null;
+        if ($body !== '') {
+            $json = json_decode($body);
+            if ($json !== null && json_last_error() === JSON_ERROR_NONE) {
+                return $json->{$this->config->tokenName} ?? null;
+            }
+
+            parse_str($body, $parsed);
+
+            return $parsed[$this->config->tokenName] ?? null;
         }
 
         return null;

diff --git a/tests/system/Security/SecurityCSRFSessionRandomizeTokenTest.php b/tests/system/Security/SecurityCSRFSessionRandomizeTokenTest.php
@@ -246,6 +246,19 @@ public function testCSRFVerifyPUTHeaderReturnsSelfOnMatch(): void
         $this->assertLogged('info', 'CSRF token verified.');
     }
 
+    public function testCSRFVerifyPUTBodyReturnsSelfOnMatch(): void
+    {
+        $_SERVER['REQUEST_METHOD'] = 'PUT';
+
+        $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent());
+        $request->setBody("csrf_test_name={$this->randomizedToken}&foo=bar");
+
+        $security = $this->createSecurity();
+
+        $this->assertInstanceOf(Security::class, $security->verify($request));
+        $this->assertLogged('info', 'CSRF token verified.');
+    }
+
     public function testCSRFVerifyJsonThrowsExceptionOnNoMatch(): void
     {
         $this->expectException(SecurityException::class);

diff --git a/tests/system/Security/SecurityCSRFSessionTest.php b/tests/system/Security/SecurityCSRFSessionTest.php
@@ -201,6 +201,19 @@ public function testCSRFVerifyPUTHeaderReturnsSelfOnMatch(): void
         $this->assertLogged('info', 'CSRF token verified.');
     }
 
+    public function testCSRFVerifyPUTBodyReturnsSelfOnMatch(): void
+    {
+        $_SERVER['REQUEST_METHOD'] = 'PUT';
+
+        $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent());
+        $request->setBody('csrf_test_name=8b9218a55906f9dcc1dc263dce7f005a&foo=bar');
+
+        $security = $this->createSecurity();
+
+        $this->assertInstanceOf(Security::class, $security->verify($request));
+        $this->assertLogged('info', 'CSRF token verified.');
+    }
+
     public function testCSRFVerifyJsonThrowsExceptionOnNoMatch(): void
     {
         $this->expectException(SecurityException::class);

diff --git a/tests/system/Security/SecurityTest.php b/tests/system/Security/SecurityTest.php
@@ -218,6 +218,50 @@ public function testCSRFVerifyJsonReturnsSelfOnMatch(): void
         $this->assertSame('{"foo":"bar"}', $request->getBody());
     }
 
+    public function testCSRFVerifyPutBodyThrowsExceptionOnNoMatch(): void
+    {
+        $_SERVER['REQUEST_METHOD']   = 'PUT';
+        $_COOKIE['csrf_cookie_name'] = '8b9218a55906f9dcc1dc263dce7f005b';
+
+        $security = $this->createMockSecurity();
+        $request  = new IncomingRequest(
+            new MockAppConfig(),
+            new URI('http://badurl.com'),
+            null,
+            new UserAgent()
+        );
+
+        $request->setBody(
+            'csrf_test_name=8b9218a55906f9dcc1dc263dce7f005a'
+        );
+
+        $this->expectException(SecurityException::class);
+        $security->verify($request);
+    }
+
+    public function testCSRFVerifyPutBodyReturnsSelfOnMatch(): void
+    {
+        $_SERVER['REQUEST_METHOD']   = 'PUT';
+        $_COOKIE['csrf_cookie_name'] = '8b9218a55906f9dcc1dc263dce7f005a';
+
+        $security = $this->createMockSecurity();
+        $request  = new IncomingRequest(
+            new MockAppConfig(),
+            new URI('http://badurl.com'),
+            null,
+            new UserAgent()
+        );
+
+        $request->setBody(
+            'csrf_test_name=8b9218a55906f9dcc1dc263dce7f005a&foo=bar'
+        );
+
+        $this->assertInstanceOf(Security::class, $security->verify($request));
+        $this->assertLogged('info', 'CSRF token verified.');
+
+        $this->assertSame('foo=bar', $request->getBody());
+    }
+
     public function testSanitizeFilename(): void
     {
         $security = $this->createMockSecurity();

diff --git a/user_guide_src/source/changelogs/v4.4.2.rst b/user_guide_src/source/changelogs/v4.4.2.rst
@@ -22,6 +22,7 @@ Changes
   command was removed. It did not work from the beginning. Also, the rollback
   command returns the database(s) state to a specified batch number and cannot
   specify only a specific database group.
+- **Security:** The presence of the CSRF token is now also checked in the raw body (not JSON format) for PUT, PATCH, and DELETE type of requests.
 
 Deprecations
 ************

diff --git a/user_guide_src/source/libraries/security.rst b/user_guide_src/source/libraries/security.rst
@@ -204,6 +204,9 @@ The order of checking the availability of the CSRF token is as follows:
 1. ``$_POST`` array
 2. HTTP header
 3. ``php://input`` (JSON request) - bear in mind that this approach is the slowest one since we have to decode JSON and then re-encode it
+4. ``php://input`` (raw body) - for PUT, PATCH, and DELETE type of requests
+
+.. note:: ``php://input`` (raw body) is checked since v4.4.2.
 
 *********************
 Other Helpful Methods