Skip to content

Commit 26e0025

Browse files
kenjissamsonasik
kenjis
and
samsonasik
authored
Fix CSRF filter does not work when set it to only post
* refactor: case the existence of config files with if statements To make it easier to know which parts to delete in the future. * docs: fix PHPDoc explanation * fix: bug that CSRF cookie is not sent just by calling csrf_hash() When the CSRF filter was set to POST method, it did not work. * refactor: replace deprecated method getHeader() * refactor: extract method * refactor: extract method * refactor: extract method * refactor: use $this->hash instead of $_COOKIE * test: fix the timing for setting superglobals * test: fix Cannot modify header information ErrorException: Cannot modify header information - headers already sent by ... * style: vendor/bin/rector process * refactor: ensure instance It becomes clear that it is `SecurityConfig`. Co-authored-by: Abdul Malik Ikhsan <samsonasik@gmail.com> * refactor: ensure instance It becomes clear that it is `CookieConfig`. Co-authored-by: Abdul Malik Ikhsan <samsonasik@gmail.com> * refactor: when $cookie is null, Cookie::setDefaults($cookie) does nothing * refactor: extract method * fix: make private extracted methods * fix: make private added property * fix: fallback to the local properties Takes care when a user removes properties in config classes. * refactor: use $request instead of $_POST Co-authored-by: Abdul Malik Ikhsan <samsonasik@gmail.com>
1 parent 2007d64 commit 26e0025

7 files changed

Lines changed: 135 additions & 59 deletions

File tree

‎app/Config/Security.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ class Security extends BaseConfig
5757
* CSRF Regenerate
5858
* --------------------------------------------------------------------------
5959
*
60-
* Regenerate CSRF Token on every request.
60+
* Regenerate CSRF Token on every submission.
6161
*
6262
* @var bool
6363
*/

‎system/Security/Security.php‎

Lines changed: 91 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
use Config\App;
1818
use Config\Cookie as CookieConfig;
1919
use Config\Security as SecurityConfig;
20+
use Config\Services;
2021

2122
/**
2223
* Class Security
@@ -77,8 +78,6 @@ class Security implements SecurityInterface
7778
* Defaults to two hours (in seconds).
7879
*
7980
* @var int
80-
*
81-
* @deprecated
8281
*/
8382
protected $expires = 7200;
8483

@@ -117,6 +116,18 @@ class Security implements SecurityInterface
117116
*/
118117
protected $samesite = Cookie::SAMESITE_LAX;
119118

119+
/**
120+
* @var RequestInterface
121+
*/
122+
private $request;
123+
124+
/**
125+
* CSRF Cookie Name without Prefix
126+
*
127+
* @var string
128+
*/
129+
private $rawCookieName;
130+
120131
/**
121132
* Constructor.
122133
*
@@ -125,27 +136,46 @@ class Security implements SecurityInterface
125136
*/
126137
public function __construct(App $config)
127138
{
128-
/** @var SecurityConfig $security */
139+
/** @var SecurityConfig|null $security */
129140
$security = config('Security');
130141

131142
// Store CSRF-related configurations
132-
$this->tokenName = $security->tokenName ?? $config->CSRFTokenName ?? $this->tokenName;
133-
$this->headerName = $security->headerName ?? $config->CSRFHeaderName ?? $this->headerName;
134-
$this->regenerate = $security->regenerate ?? $config->CSRFRegenerate ?? $this->regenerate;
135-
$rawCookieName = $security->cookieName ?? $config->CSRFCookieName ?? $this->cookieName;
143+
if ($security instanceof SecurityConfig) {
144+
$this->tokenName = $security->tokenName ?? $this->tokenName;
145+
$this->headerName = $security->headerName ?? $this->headerName;
146+
$this->regenerate = $security->regenerate ?? $this->regenerate;
147+
$this->rawCookieName = $security->cookieName ?? $this->rawCookieName;
148+
$this->expires = $security->expires ?? $this->expires;
149+
} else {
150+
// `Config/Security.php` is absence
151+
$this->tokenName = $config->CSRFTokenName ?? $this->tokenName;
152+
$this->headerName = $config->CSRFHeaderName ?? $this->headerName;
153+
$this->regenerate = $config->CSRFRegenerate ?? $this->regenerate;
154+
$this->rawCookieName = $config->CSRFCookieName ?? $this->rawCookieName;
155+
$this->expires = $config->CSRFExpire ?? $this->expires;
156+
}
136157

137-
/** @var CookieConfig $cookie */
138-
$cookie = config('Cookie');
158+
$this->configureCookie($config);
139159

140-
$cookiePrefix = $cookie->prefix ?? $config->cookiePrefix;
141-
$this->cookieName = $cookiePrefix . $rawCookieName;
160+
$this->request = Services::request();
142161

143-
$expires = $security->expires ?? $config->CSRFExpire ?? 7200;
162+
$this->generateHash();
163+
}
164+
165+
private function configureCookie(App $config): void
166+
{
167+
/** @var CookieConfig|null $cookie */
168+
$cookie = config('Cookie');
144169

145-
Cookie::setDefaults($cookie);
146-
$this->cookie = new Cookie($rawCookieName, $this->generateHash(), [
147-
'expires' => $expires === 0 ? 0 : time() + $expires,
148-
]);
170+
if ($cookie instanceof CookieConfig) {
171+
$cookiePrefix = $cookie->prefix;
172+
$this->cookieName = $cookiePrefix . $this->rawCookieName;
173+
Cookie::setDefaults($cookie);
174+
} else {
175+
// `Config/Cookie.php` is absence
176+
$cookiePrefix = $config->cookiePrefix;
177+
$this->cookieName = $cookiePrefix . $this->rawCookieName;
178+
}
149179
}
150180

151181
/**
@@ -202,26 +232,15 @@ public function verify(RequestInterface $request)
202232
return $this->sendCookie($request);
203233
}
204234

205-
// Does the token exist in POST, HEADER or optionally php:://input - json data.
206-
if ($request->hasHeader($this->headerName) && ! empty($request->getHeader($this->headerName)->getValue())) {
207-
$tokenName = $request->getHeader($this->headerName)->getValue();
208-
} else {
209-
$json = json_decode($request->getBody());
210-
211-
if (! empty($request->getBody()) && ! empty($json) && json_last_error() === JSON_ERROR_NONE) {
212-
$tokenName = $json->{$this->tokenName} ?? null;
213-
} else {
214-
$tokenName = null;
215-
}
216-
}
217-
218-
$token = $_POST[$this->tokenName] ?? $tokenName;
235+
$token = $this->getPostedToken($request);
219236

220237
// Does the tokens exist in both the POST/POSTed JSON and COOKIE arrays and match?
221-
if (! isset($token, $_COOKIE[$this->cookieName]) || ! hash_equals($token, $_COOKIE[$this->cookieName])) {
238+
if (! isset($token, $this->hash) || ! hash_equals($this->hash, $token)) {
222239
throw SecurityException::forDisallowedAction();
223240
}
224241

242+
$json = json_decode($request->getBody());
243+
225244
if (isset($_POST[$this->tokenName])) {
226245
// We kill this since we're done and we don't want to pollute the POST array.
227246
unset($_POST[$this->tokenName]);
@@ -237,14 +256,31 @@ public function verify(RequestInterface $request)
237256
unset($_COOKIE[$this->cookieName]);
238257
}
239258

240-
$this->cookie = $this->cookie->withValue($this->generateHash());
241-
$this->sendCookie($request);
259+
$this->generateHash();
242260

243261
log_message('info', 'CSRF token verified.');
244262

245263
return $this;
246264
}
247265

266+
private function getPostedToken(RequestInterface $request): ?string
267+
{
268+
// Does the token exist in POST, HEADER or optionally php:://input - json data.
269+
if ($request->hasHeader($this->headerName) && ! empty($request->header($this->headerName)->getValue())) {
270+
$tokenName = $request->header($this->headerName)->getValue();
271+
} else {
272+
$json = json_decode($request->getBody());
273+
274+
if (! empty($request->getBody()) && ! empty($json) && json_last_error() === JSON_ERROR_NONE) {
275+
$tokenName = $json->{$this->tokenName} ?? null;
276+
} else {
277+
$tokenName = null;
278+
}
279+
}
280+
281+
return $request->getPost($this->tokenName) ?? $tokenName;
282+
}
283+
248284
/**
249285
* Returns the CSRF Hash.
250286
*/
@@ -373,19 +409,37 @@ protected function generateHash(): string
373409
// We don't necessarily want to regenerate it with
374410
// each page load since a page could contain embedded
375411
// sub-pages causing this feature to fail
376-
if (isset($_COOKIE[$this->cookieName])
377-
&& is_string($_COOKIE[$this->cookieName])
378-
&& preg_match('#^[0-9a-f]{32}$#iS', $_COOKIE[$this->cookieName]) === 1
379-
) {
412+
if ($this->isHashInCookie()) {
380413
return $this->hash = $_COOKIE[$this->cookieName];
381414
}
382415

383416
$this->hash = bin2hex(random_bytes(16));
417+
418+
$this->saveHashInCookie();
384419
}
385420

386421
return $this->hash;
387422
}
388423

424+
private function isHashInCookie(): bool
425+
{
426+
return isset($_COOKIE[$this->cookieName])
427+
&& is_string($_COOKIE[$this->cookieName])
428+
&& preg_match('#^[0-9a-f]{32}$#iS', $_COOKIE[$this->cookieName]) === 1;
429+
}
430+
431+
private function saveHashInCookie()
432+
{
433+
$this->cookie = new Cookie(
434+
$this->rawCookieName,
435+
$this->hash,
436+
[
437+
'expires' => $this->expires === 0 ? 0 : time() + $this->expires,
438+
]
439+
);
440+
$this->sendCookie($this->request);
441+
}
442+
389443
/**
390444
* CSRF Send Cookie
391445
*

‎tests/system/CommonFunctionsTest.php‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
use CodeIgniter\Session\Session;
2323
use CodeIgniter\Test\CIUnitTestCase;
2424
use CodeIgniter\Test\Mock\MockIncomingRequest;
25+
use CodeIgniter\Test\Mock\MockSecurity;
2526
use CodeIgniter\Test\Mock\MockSession;
2627
use CodeIgniter\Test\TestLogger;
2728
use Config\App;
@@ -232,6 +233,8 @@ public function testAppTimezone()
232233

233234
public function testCSRFToken()
234235
{
236+
Services::injectMock('security', new MockSecurity(new App()));
237+
235238
$this->assertSame('csrf_test_name', csrf_token());
236239
}
237240

‎tests/system/CommonSingleServiceTest.php‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@
1313

1414
use CodeIgniter\Config\Services;
1515
use CodeIgniter\Test\CIUnitTestCase;
16+
use CodeIgniter\Test\Mock\MockSecurity;
17+
use Config\App;
1618
use ReflectionClass;
1719
use ReflectionMethod;
1820

@@ -26,6 +28,8 @@ final class CommonSingleServiceTest extends CIUnitTestCase
2628
*/
2729
public function testSingleServiceWithNoParamsSupplied(string $service): void
2830
{
31+
Services::injectMock('security', new MockSecurity(new App()));
32+
2933
$service1 = single_service($service);
3034
$service2 = single_service($service);
3135

@@ -90,7 +94,7 @@ public static function serviceNamesProvider(): iterable
9094
continue;
9195
}
9296

93-
yield [$name];
97+
yield $name => [$name];
9498
}
9599
}
96100
}

‎tests/system/Config/ServicesTest.php‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -397,6 +397,8 @@ public function testRouter()
397397

398398
public function testSecurity()
399399
{
400+
Services::injectMock('security', new MockSecurity(new App()));
401+
400402
$result = Services::security();
401403
$this->assertInstanceOf(Security::class, $result);
402404
}

‎tests/system/Helpers/SecurityHelperTest.php‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,9 @@
1212
namespace CodeIgniter\Helpers;
1313

1414
use CodeIgniter\Test\CIUnitTestCase;
15+
use CodeIgniter\Test\Mock\MockSecurity;
16+
use Config\App;
17+
use Tests\Support\Config\Services as Services;
1518

1619
/**
1720
* @internal
@@ -27,6 +30,8 @@ protected function setUp(): void
2730

2831
public function testSanitizeFilenameSimpleSuccess()
2932
{
33+
Services::injectMock('security', new MockSecurity(new App()));
34+
3035
$this->assertSame('hello.doc', sanitize_filename('hello.doc'));
3136
}
3237

0 commit comments

Comments
 (0)