fix: check for CSRF token in the raw body

michalsn · Sep 9, 2023 · 3c95f0c · 3c95f0c
1 parent 64819ee
commit 3c95f0c
Show file tree

Hide file tree

Showing 5 changed files with 93 additions and 10 deletions.
diff --git a/system/Security/Security.php b/system/Security/Security.php
@@ -318,24 +318,30 @@ 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() ?? '';
+            if (! empty($json = json_decode($body)) && 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;
@@ -346,10 +352,15 @@ private function getPostedToken(RequestInterface $request): ?string
         }
 
         $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 !== '') {
+            if (! empty($json = json_decode($body)) && 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
@@ -24,6 +24,8 @@ Deprecations
 Bugs Fixed
 **********
 
+- **Security:** Fixed a bug where the CSRF token wasn't checked if we sent it in the raw body (not JSON format) for PUT, PATCH, and DELETE requests.
+
 See the repo's
 `CHANGELOG.md <https://github.com/codeigniter4/CodeIgniter4/blob/develop/CHANGELOG.md>`_
 for a complete list of bugs fixed.