Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: check for CSRF token in the raw body #7915

Merged
merged 7 commits into from
Sep 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions phpstan-baseline.php
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
37 changes: 26 additions & 11 deletions system/Security/Security.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
13 changes: 13 additions & 0 deletions tests/system/Security/SecurityCSRFSessionRandomizeTokenTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
13 changes: 13 additions & 0 deletions tests/system/Security/SecurityCSRFSessionTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
44 changes: 44 additions & 0 deletions tests/system/Security/SecurityTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
1 change: 1 addition & 0 deletions user_guide_src/source/changelogs/v4.4.2.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
************
Expand Down
3 changes: 3 additions & 0 deletions user_guide_src/source/libraries/security.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
michalsn marked this conversation as resolved.
Show resolved Hide resolved

.. note:: ``php://input`` (raw body) is checked since v4.4.2.

*********************
Other Helpful Methods
Expand Down
Loading