From 918efe994247853f748d879644f070202c4f3d4d Mon Sep 17 00:00:00 2001 From: Bogdan Date: Sat, 10 Oct 2026 15:27:41 +0200 Subject: [PATCH 1/2] fix: preserve JSON body formatting when removing CSRF token --- system/Security/Security.php | 196 ++++++++++++++++++- tests/system/Security/SecurityTest.php | 98 ++++++++++ user_guide_src/source/changelogs/v4.7.6.rst | 4 + user_guide_src/source/libraries/security.rst | 2 +- 4 files changed, 297 insertions(+), 3 deletions(-) diff --git a/system/Security/Security.php b/system/Security/Security.php index 4ac0de3f8ff8..72163832263f 100644 --- a/system/Security/Security.php +++ b/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; diff --git a/tests/system/Security/SecurityTest.php b/tests/system/Security/SecurityTest.php index db80d91cbd92..42d0e1a42d02 100644 --- a/tests/system/Security/SecurityTest.php +++ b/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 + */ + 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') diff --git a/user_guide_src/source/changelogs/v4.7.6.rst b/user_guide_src/source/changelogs/v4.7.6.rst index d2641f99b7cf..7ac0daafa025 100644 --- a/user_guide_src/source/changelogs/v4.7.6.rst +++ b/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 `_ for a complete list of bugs fixed. diff --git a/user_guide_src/source/libraries/security.rst b/user_guide_src/source/libraries/security.rst index 8fc49d646b1a..eef8c46fc547 100644 --- a/user_guide_src/source/libraries/security.rst +++ b/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. From f831fadc4a33bf3ed207a525d620042c06449628 Mon Sep 17 00:00:00 2001 From: Bogdan Date: Sun, 11 Oct 2026 21:45:51 +0200 Subject: [PATCH 2/2] docs: document CSRF in JSON request body limitations --- system/Security/Security.php | 196 +------------------ tests/system/Security/SecurityTest.php | 98 ---------- user_guide_src/source/changelogs/v4.7.6.rst | 4 - user_guide_src/source/libraries/security.rst | 9 +- 4 files changed, 10 insertions(+), 297 deletions(-) diff --git a/system/Security/Security.php b/system/Security/Security.php index 72163832263f..4ac0de3f8ff8 100644 --- a/system/Security/Security.php +++ b/system/Security/Security.php @@ -300,7 +300,8 @@ private function removeTokenInRequest(IncomingRequest $request): void if (is_object($json)) { if (property_exists($json, $tokenName)) { - $request->setBody($this->removeJsonMember($body, $tokenName)); + unset($json->{$tokenName}); + $request->setBody(json_encode($json)); } return; @@ -313,199 +314,6 @@ 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; diff --git a/tests/system/Security/SecurityTest.php b/tests/system/Security/SecurityTest.php index 42d0e1a42d02..db80d91cbd92 100644 --- a/tests/system/Security/SecurityTest.php +++ b/tests/system/Security/SecurityTest.php @@ -237,104 +237,6 @@ 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 - */ - 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') diff --git a/user_guide_src/source/changelogs/v4.7.6.rst b/user_guide_src/source/changelogs/v4.7.6.rst index 7ac0daafa025..d2641f99b7cf 100644 --- a/user_guide_src/source/changelogs/v4.7.6.rst +++ b/user_guide_src/source/changelogs/v4.7.6.rst @@ -30,10 +30,6 @@ 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 `_ for a complete list of bugs fixed. diff --git a/user_guide_src/source/libraries/security.rst b/user_guide_src/source/libraries/security.rst index eef8c46fc547..2e747c988ec1 100644 --- a/user_guide_src/source/libraries/security.rst +++ b/user_guide_src/source/libraries/security.rst @@ -219,11 +219,18 @@ 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 +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. +.. note:: + + If you include the CSRF token in a JSON object, removing it decodes and + re-encodes the body. This changes its formatting and may affect number + precision. To keep the JSON body unchanged, send the token in the + ``X-CSRF-TOKEN`` header and leave the CSRF field out of the body. + ********************* Other Helpful Methods *********************