Skip to content

Commit b192a17

Browse files
committed
fix: check for CSRF token in the raw body
1 parent 1ff3e6e commit b192a17

File tree

5 files changed

+93
-10
lines changed

5 files changed

+93
-10
lines changed

system/Security/Security.php

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -318,24 +318,30 @@ private function removeTokenInRequest(RequestInterface $request): void
318318
{
319319
assert($request instanceof Request);
320320

321-
$json = json_decode($request->getBody() ?? '');
322-
323321
if (isset($_POST[$this->config->tokenName])) {
324322
// We kill this since we're done and we don't want to pollute the POST array.
325323
unset($_POST[$this->config->tokenName]);
326324
$request->setGlobal('post', $_POST);
327-
} elseif (isset($json->{$this->config->tokenName})) {
328-
// We kill this since we're done and we don't want to pollute the JSON data.
329-
unset($json->{$this->config->tokenName});
330-
$request->setBody(json_encode($json));
325+
} else {
326+
$body = $request->getBody() ?? '';
327+
if (! empty($json = json_decode($body)) && json_last_error() === JSON_ERROR_NONE) {
328+
// We kill this since we're done and we don't want to pollute the JSON data.
329+
unset($json->{$this->config->tokenName});
330+
$request->setBody(json_encode($json));
331+
} else {
332+
parse_str($body, $parsed);
333+
// We kill this since we're done and we don't want to pollute the BODY data.
334+
unset($parsed[$this->config->tokenName]);
335+
$request->setBody(http_build_query($parsed));
336+
}
331337
}
332338
}
333339

334340
private function getPostedToken(RequestInterface $request): ?string
335341
{
336342
assert($request instanceof IncomingRequest);
337343

338-
// Does the token exist in POST, HEADER or optionally php:://input - json data.
344+
// Does the token exist in POST, HEADER or optionally php:://input - json data or PUT, DELETE, PATCH - raw data.
339345

340346
if ($tokenValue = $request->getPost($this->config->tokenName)) {
341347
return $tokenValue;
@@ -346,10 +352,15 @@ private function getPostedToken(RequestInterface $request): ?string
346352
}
347353

348354
$body = (string) $request->getBody();
349-
$json = json_decode($body);
350355

351-
if ($body !== '' && ! empty($json) && json_last_error() === JSON_ERROR_NONE) {
352-
return $json->{$this->config->tokenName} ?? null;
356+
if ($body !== '') {
357+
if (! empty($json = json_decode($body)) && json_last_error() === JSON_ERROR_NONE) {
358+
return $json->{$this->config->tokenName} ?? null;
359+
}
360+
361+
parse_str($body, $parsed);
362+
363+
return $parsed[$this->config->tokenName] ?? null;
353364
}
354365

355366
return null;

tests/system/Security/SecurityCSRFSessionRandomizeTokenTest.php

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -246,6 +246,19 @@ public function testCSRFVerifyPUTHeaderReturnsSelfOnMatch(): void
246246
$this->assertLogged('info', 'CSRF token verified.');
247247
}
248248

249+
public function testCSRFVerifyPUTBodyReturnsSelfOnMatch(): void
250+
{
251+
$_SERVER['REQUEST_METHOD'] = 'PUT';
252+
253+
$request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent());
254+
$request->setBody("csrf_test_name={$this->randomizedToken}&foo=bar");
255+
256+
$security = $this->createSecurity();
257+
258+
$this->assertInstanceOf(Security::class, $security->verify($request));
259+
$this->assertLogged('info', 'CSRF token verified.');
260+
}
261+
249262
public function testCSRFVerifyJsonThrowsExceptionOnNoMatch(): void
250263
{
251264
$this->expectException(SecurityException::class);

tests/system/Security/SecurityCSRFSessionTest.php

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,19 @@ public function testCSRFVerifyPUTHeaderReturnsSelfOnMatch(): void
201201
$this->assertLogged('info', 'CSRF token verified.');
202202
}
203203

204+
public function testCSRFVerifyPUTBodyReturnsSelfOnMatch(): void
205+
{
206+
$_SERVER['REQUEST_METHOD'] = 'PUT';
207+
208+
$request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent());
209+
$request->setBody('csrf_test_name=8b9218a55906f9dcc1dc263dce7f005a&foo=bar');
210+
211+
$security = $this->createSecurity();
212+
213+
$this->assertInstanceOf(Security::class, $security->verify($request));
214+
$this->assertLogged('info', 'CSRF token verified.');
215+
}
216+
204217
public function testCSRFVerifyJsonThrowsExceptionOnNoMatch(): void
205218
{
206219
$this->expectException(SecurityException::class);

tests/system/Security/SecurityTest.php

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -218,6 +218,50 @@ public function testCSRFVerifyJsonReturnsSelfOnMatch(): void
218218
$this->assertSame('{"foo":"bar"}', $request->getBody());
219219
}
220220

221+
public function testCSRFVerifyPutBodyThrowsExceptionOnNoMatch(): void
222+
{
223+
$_SERVER['REQUEST_METHOD'] = 'PUT';
224+
$_COOKIE['csrf_cookie_name'] = '8b9218a55906f9dcc1dc263dce7f005b';
225+
226+
$security = $this->createMockSecurity();
227+
$request = new IncomingRequest(
228+
new MockAppConfig(),
229+
new URI('http://badurl.com'),
230+
null,
231+
new UserAgent()
232+
);
233+
234+
$request->setBody(
235+
'csrf_test_name=8b9218a55906f9dcc1dc263dce7f005a'
236+
);
237+
238+
$this->expectException(SecurityException::class);
239+
$security->verify($request);
240+
}
241+
242+
public function testCSRFVerifyPutBodyReturnsSelfOnMatch(): void
243+
{
244+
$_SERVER['REQUEST_METHOD'] = 'PUT';
245+
$_COOKIE['csrf_cookie_name'] = '8b9218a55906f9dcc1dc263dce7f005a';
246+
247+
$security = $this->createMockSecurity();
248+
$request = new IncomingRequest(
249+
new MockAppConfig(),
250+
new URI('http://badurl.com'),
251+
null,
252+
new UserAgent()
253+
);
254+
255+
$request->setBody(
256+
'csrf_test_name=8b9218a55906f9dcc1dc263dce7f005a&foo=bar'
257+
);
258+
259+
$this->assertInstanceOf(Security::class, $security->verify($request));
260+
$this->assertLogged('info', 'CSRF token verified.');
261+
262+
$this->assertSame('foo=bar', $request->getBody());
263+
}
264+
221265
public function testSanitizeFilename(): void
222266
{
223267
$security = $this->createMockSecurity();

user_guide_src/source/changelogs/v4.4.2.rst

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@ Deprecations
2929
Bugs Fixed
3030
**********
3131

32+
- **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.
33+
3234
See the repo's
3335
`CHANGELOG.md <https://github.com/codeigniter4/CodeIgniter4/blob/develop/CHANGELOG.md>`_
3436
for a complete list of bugs fixed.

0 commit comments

Comments
 (0)