diff --git a/phpstan-baseline.php b/phpstan-baseline.php index 106de3723ef0..d95d38fb3c6d 100644 --- 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 index 648856321363..4ba665639f36 100644 --- a/system/Security/Security.php +++ b/system/Security/Security.php @@ -318,16 +318,23 @@ 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)); + } } } @@ -335,21 +342,29 @@ 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 index 2a148c902e0f..7c8af63040f4 100644 --- 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 index a8f784a5464b..1d701520488c 100644 --- 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 index 972115de6a7b..ae4953607662 100644 --- 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 index 3866a7664ee6..19f2a4f1782c 100644 --- 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 index bc5777f759fa..6c39cbefdb5e 100644 --- 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