fix: preserve JSON body formatting when removing CSRF token

Author: gr8man

system/Security/Security.php

@@ -300,8 +300,7 @@ private function removeTokenInRequest(IncomingRequest $request): void
 
         if (is_object($json)) {
             if (property_exists($json, $tokenName)) {
-                unset($json->{$tokenName});
-                $request->setBody(json_encode($json));
+                $request->setBody($this->removeJsonMember($body, $tokenName));
             }
 
             return;
@@ -314,6 +313,199 @@ private function removeTokenInRequest(IncomingRequest $request): void
         $request->setBody(http_build_query($result));
     }
 
+    /**
+     * Removes a top-level member from a JSON object without re-encoding the
+     * document, so the original formatting of the remaining data is preserved.
+     */
+    private function removeJsonMember(string $json, string $member): string
+    {
+        $length = strlen($json);
+        $pos    = $this->skipJsonWhitespace($json, 0);
+
+        if ($pos >= $length || $json[$pos] !== '{') {
+            return $json;
+        }
+
+        $pos++;
+
+        $memberStart = null;
+        $memberEnd   = null;
+        $commaAfter  = null;
+
+        while ($pos < $length) {
+            $pos = $this->skipJsonWhitespace($json, $pos);
+
+            if ($pos >= $length || $json[$pos] === '}') {
+                break;
+            }
+
+            if ($json[$pos] !== '"') {
+                return $json;
+            }
+
+            $keyStart = $pos;
+            $pos      = $this->skipJsonString($json, $pos);
+            $key      = json_decode(substr($json, $keyStart, $pos - $keyStart));
+
+            $pos = $this->skipJsonWhitespace($json, $pos);
+
+            if ($pos >= $length || $json[$pos] !== ':') {
+                return $json;
+            }
+
+            $pos = $this->skipJsonWhitespace($json, $pos + 1);
+            $pos = $this->skipJsonValue($json, $pos);
+
+            if ($key === $member) {
+                $memberStart = $keyStart;
+                $memberEnd   = $pos;
+
+                // Drop the comma and the whitespace that follows it when there
+                // is a next member.
+                $after = $this->skipJsonWhitespace($json, $pos);
+
+                if ($after < $length && $json[$after] === ',') {
+                    $commaAfter = $this->skipJsonWhitespace($json, $after + 1);
+                }
+
+                break;
+            }
+
+            // Skip the separator comma between members.
+            $pos = $this->skipJsonWhitespace($json, $pos);
+
+            if ($pos < $length && $json[$pos] === ',') {
+                $pos++;
+            }
+        }
+
+        if ($memberStart === null) {
+            return $json;
+        }
+
+        if ($commaAfter !== null) {
+            // The member is not the last one.
+            return substr($json, 0, $memberStart) . substr($json, $commaAfter);
+        }
+
+        // The member is the last one: drop the preceding comma if there is one.
+        $before = $memberStart - 1;
+
+        while ($before >= 0 && ctype_space($json[$before])) {
+            $before--;
+        }
+
+        if ($before >= 0 && $json[$before] === ',') {
+            return substr($json, 0, $before) . substr($json, $memberEnd);
+        }
+
+        // The member is the only one: keep the surrounding whitespace clean.
+        $open = $memberStart;
+
+        while ($open > 0 && ctype_space($json[$open - 1])) {
+            $open--;
+        }
+
+        return substr($json, 0, $open) . substr($json, $memberEnd);
+    }
+
+    /**
+     * Returns the position just after the JSON string that starts at the given
+     * position (which must point to the opening quote).
+     */
+    private function skipJsonString(string $json, int $pos): int
+    {
+        $length = strlen($json);
+        $pos++;
+
+        while ($pos < $length) {
+            if ($json[$pos] === '\\') {
+                $pos += 2;
+
+                continue;
+            }
+
+            if ($json[$pos] === '"') {
+                return $pos + 1;
+            }
+
+            $pos++;
+        }
+
+        return $pos;
+    }
+
+    /**
+     * Returns the position after the JSON value that starts at the given
+     * position.
+     */
+    private function skipJsonValue(string $json, int $pos): int
+    {
+        $length = strlen($json);
+
+        if ($pos >= $length) {
+            return $pos;
+        }
+
+        $char = $json[$pos];
+
+        if ($char === '"') {
+            return $this->skipJsonString($json, $pos);
+        }
+
+        if ($char !== '{' && $char !== '[') {
+            // Number, true, false or null.
+            while ($pos < $length && ! ctype_space($json[$pos]) && $json[$pos] !== ',' && $json[$pos] !== '}' && $json[$pos] !== ']') {
+                $pos++;
+            }
+
+            return $pos;
+        }
+
+        $open  = $char;
+        $close = $char === '{' ? '}' : ']';
+        $depth = 0;
+
+        while ($pos < $length) {
+            $current = $json[$pos];
+
+            if ($current === '"') {
+                $pos = $this->skipJsonString($json, $pos);
+
+                continue;
+            }
+
+            if ($current === $open) {
+                $depth++;
+            } elseif ($current === $close) {
+                $depth--;
+
+                if ($depth === 0) {
+                    return $pos + 1;
+                }
+            }
+
+            $pos++;
+        }
+
+        return $pos;
+    }
+
+    /**
+     * Returns the position just after the whitespace starting at the given
+     * position.
+     */
+    private function skipJsonWhitespace(string $json, int $pos): int
+    {
+        $length = strlen($json);
+
+        while ($pos < $length && ctype_space($json[$pos])) {
+            $pos++;
+        }
+
+        return $pos;
+    }
+
     private function getPostedToken(IncomingRequest $request): ?string
     {
         $tokenName  = $this->config->tokenName;

tests/system/Security/SecurityTest.php

@@ -237,6 +237,104 @@ public function testCsrfVerifyHeaderWithJsonBodyStripsTokenFromBody(): void
         $this->assertSame('{"foo":"bar"}', $request->getBody());
     }
 
+    #[DataProvider('provideCsrfVerifyJsonBodyPreservesFormatting')]
+    public function testCsrfVerifyJsonBodyPreservesFormatting(string $body, string $expected): void
+    {
+        service('superglobals')
+            ->setServer('REQUEST_METHOD', 'POST')
+            ->setCookie('csrf_cookie_name', self::CORRECT_CSRF_HASH);
+
+        $security = $this->createMockSecurity();
+        $request  = $this->createIncomingRequest()->setBody($body);
+
+        $this->assertInstanceOf(Security::class, $security->verify($request));
+        $this->assertSame($expected, $request->getBody());
+    }
+
+    /**
+     * @return iterable<string, array{string, string}>
+     */
+    public static function provideCsrfVerifyJsonBodyPreservesFormatting(): iterable
+    {
+        $hash = self::CORRECT_CSRF_HASH;
+
+        yield 'pretty printed preserves indentation' => [
+            "{\n    \"csrf_test_name\": \"{$hash}\",\n    \"foo\": \"bar\"\n}",
+            "{\n    \"foo\": \"bar\"\n}",
+        ];
+
+        yield 'compact token first' => [
+            "{\"csrf_test_name\":\"{$hash}\",\"foo\":\"bar\"}",
+            '{"foo":"bar"}',
+        ];
+
+        yield 'token last keeps preceding member intact' => [
+            "{\"foo\": \"bar\", \"csrf_test_name\": \"{$hash}\"}",
+            '{"foo": "bar"}',
+        ];
+
+        yield 'token only yields empty object' => [
+            "{\"csrf_test_name\":\"{$hash}\"}",
+            '{}',
+        ];
+
+        yield 'spaces around colon and comma preserved' => [
+            "{ \"csrf_test_name\" : \"{$hash}\" , \"foo\" : \"bar\" }",
+            '{ "foo" : "bar" }',
+        ];
+
+        yield 'unicode is not escaped' => [
+            "{\"csrf_test_name\":\"{$hash}\",\"name\":\"café\"}",
+            '{"name":"café"}',
+        ];
+
+        yield 'forward slashes are not escaped' => [
+            "{\"csrf_test_name\":\"{$hash}\",\"url\":\"http://example.com/a/b\"}",
+            '{"url":"http://example.com/a/b"}',
+        ];
+
+        yield 'number representation is preserved' => [
+            "{\"csrf_test_name\":\"{$hash}\",\"price\":1.10,\"big\":12345678901234567890}",
+            '{"price":1.10,"big":12345678901234567890}',
+        ];
+
+        yield 'nested member with same name is untouched' => [
+            "{\"csrf_test_name\":\"{$hash}\",\"nested\":{\"csrf_test_name\":\"keep\"}}",
+            '{"nested":{"csrf_test_name":"keep"}}',
+        ];
+
+        yield 'key name inside a string value is untouched' => [
+            "{\"csrf_test_name\":\"{$hash}\",\"note\":\"csrf_test_name\"}",
+            '{"note":"csrf_test_name"}',
+        ];
+
+        yield 'escaped quote inside a string value' => [
+            "{\"csrf_test_name\":\"{$hash}\",\"note\":\"a\\\"b,c:1\"}",
+            '{"note":"a\"b,c:1"}',
+        ];
+    }
+
+    public function testCsrfVerifyHeaderWithPrettyJsonBodyStripsTokenPreservingFormatting(): void
+    {
+        service('superglobals')
+            ->setServer('REQUEST_METHOD', 'POST')
+            ->setCookie('csrf_cookie_name', self::CORRECT_CSRF_HASH);
+
+        $security = $this->createMockSecurity();
+        $request  = $this->createIncomingRequest();
+
+        $request->setHeader('X-CSRF-TOKEN', self::CORRECT_CSRF_HASH);
+        $request->setBody(
+            "{\n    \"csrf_test_name\": \"" . self::CORRECT_CSRF_HASH . "\",\n    \"foo\": \"bar\"\n}",
+        );
+
+        $this->assertInstanceOf(Security::class, $security->verify($request));
+        $this->assertSame(
+            "{\n    \"foo\": \"bar\"\n}",
+            $request->getBody(),
+        );
+    }
+
     public function testCsrfVerifyPutBodyThrowsExceptionOnNoMatch(): void
     {
         service('superglobals')

user_guide_src/source/changelogs/v4.7.6.rst

@@ -30,6 +30,10 @@ Deprecations
 Bugs Fixed
 **********
 
+- **Security:** Fixed a bug where removing the CSRF token from a JSON request
+  body re-encoded the whole document, losing the original formatting of the
+  remaining data. The formatting is now preserved.
+
 See the repo's
 `CHANGELOG.md <https://github.com/codeigniter4/CodeIgniter4/blob/develop/CHANGELOG.md>`_
 for a complete list of bugs fixed.

user_guide_src/source/libraries/security.rst

@@ -219,7 +219,7 @@ 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
+3. ``php://input`` (JSON request) - bear in mind that this approach is the slowest one since we have to decode JSON
 4. ``php://input`` (raw body) - for PUT, PATCH, and DELETE type of requests
 
 .. note:: ``php://input`` (raw body) is checked since v4.4.2.