Skip to content

Commit cf9fb5e

Browse files
paulbalandanpaulbalandan
committed
Refactor Security class
1 parent eff12b0 commit cf9fb5e

File tree

4 files changed

+98
-88
lines changed

4 files changed

+98
-88
lines changed

‎rector.php‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,6 @@
149149
__DIR__ . '/system/HTTP/CURLRequest.php',
150150
__DIR__ . '/system/HTTP/DownloadResponse.php',
151151
__DIR__ . '/system/HTTP/IncomingRequest.php',
152-
__DIR__ . '/system/Security/Security.php',
153152
__DIR__ . '/system/Session/Session.php',
154153
],
155154

‎system/Security/Security.php‎

Lines changed: 67 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,9 @@
2121
use CodeIgniter\HTTP\RequestInterface;
2222
use CodeIgniter\I18n\Time;
2323
use CodeIgniter\Security\Exceptions\SecurityException;
24-
use CodeIgniter\Session\Session;
24+
use CodeIgniter\Session\SessionInterface;
2525
use Config\Cookie as CookieConfig;
2626
use Config\Security as SecurityConfig;
27-
use ErrorException;
2827
use JsonException;
2928
use SensitiveParameter;
3029

@@ -38,7 +37,16 @@ class Security implements SecurityInterface
3837
{
3938
public const CSRF_PROTECTION_COOKIE = 'cookie';
4039
public const CSRF_PROTECTION_SESSION = 'session';
41-
protected const CSRF_HASH_BYTES = 16;
40+
41+
/**
42+
* CSRF hash length in bytes.
43+
*/
44+
protected const CSRF_HASH_BYTES = 16;
45+
46+
/**
47+
* CSRF hash length in hexadecimal characters.
48+
*/
49+
protected const CSRF_HASH_HEX = self::CSRF_HASH_BYTES * 2;
4250

4351
/**
4452
* CSRF Hash (without randomization)
@@ -51,6 +59,8 @@ class Security implements SecurityInterface
5159

5260
/**
5361
* @var Cookie
62+
*
63+
* @deprecated v4.8.0 Use service('response')->getCookie() instead.
5464
*/
5565
protected $cookie;
5666

@@ -63,17 +73,12 @@ class Security implements SecurityInterface
6373
*/
6474
protected $cookieName = 'csrf_cookie_name';
6575

66-
private readonly IncomingRequest $request;
67-
6876
/**
6977
* CSRF Cookie Name without Prefix
7078
*/
7179
private ?string $rawCookieName = null;
7280

73-
/**
74-
* Session instance.
75-
*/
76-
private ?Session $session = null;
81+
private ?SessionInterface $session = null;
7782

7883
/**
7984
* CSRF Hash in Request Cookie
@@ -83,25 +88,17 @@ class Security implements SecurityInterface
8388
*/
8489
private ?string $hashInCookie = null;
8590

86-
protected SecurityConfig $config;
87-
88-
public function __construct(SecurityConfig $config)
91+
public function __construct(protected SecurityConfig $config)
8992
{
90-
$this->config = $config;
91-
9293
$this->rawCookieName = $config->cookieName;
9394

94-
if ($this->isCSRFCookie()) {
95-
$cookie = config(CookieConfig::class);
96-
97-
$this->configureCookie($cookie);
95+
if ($this->isCsrfCookie()) {
96+
$this->configureCookie(config(CookieConfig::class));
9897
} else {
99-
// Session based CSRF protection
10098
$this->configureSession();
10199
}
102100

103-
$this->request = service('request');
104-
$this->hashInCookie = $this->request->getCookie($this->cookieName);
101+
$this->hashInCookie = service('request')->getCookie($this->cookieName);
105102

106103
$this->restoreHash();
107104

@@ -148,7 +145,9 @@ public function verify(RequestInterface $request): static
148145

149146
public function getHash(): ?string
150147
{
151-
return $this->config->tokenRandomize ? $this->randomize($this->hash) : $this->hash;
148+
return $this->config->tokenRandomize && isset($this->hash)
149+
? $this->randomize($this->hash)
150+
: $this->hash;
152151
}
153152

154153
public function getTokenName(): string
@@ -171,14 +170,16 @@ public function shouldRedirect(): bool
171170
return $this->config->redirect;
172171
}
173172

173+
/**
174+
* @phpstan-assert string $this->hash
175+
*/
174176
public function generateHash(): string
175177
{
176178
$this->hash = bin2hex(random_bytes(static::CSRF_HASH_BYTES));
177179

178-
if ($this->isCSRFCookie()) {
180+
if ($this->isCsrfCookie()) {
179181
$this->saveHashInCookie();
180182
} else {
181-
// Session based CSRF protection
182183
$this->saveHashInSession();
183184
}
184185

@@ -207,30 +208,39 @@ protected function randomize(string $hash): string
207208
*/
208209
protected function derandomize(#[SensitiveParameter] string $token): string
209210
{
210-
$key = substr($token, -static::CSRF_HASH_BYTES * 2);
211-
$value = substr($token, 0, static::CSRF_HASH_BYTES * 2);
211+
// The token should be in the format of `randomizedHash` + `key`,
212+
// where both `randomizedHash` and `key` are hex strings of length CSRF_HASH_HEX.
213+
if (strlen($token) !== self::CSRF_HASH_HEX * 2) {
214+
throw new InvalidArgumentException('Invalid CSRF token.');
215+
}
212216

213-
try {
214-
return bin2hex((string) hex2bin($value) ^ (string) hex2bin($key));
215-
} catch (ErrorException $e) {
216-
throw new InvalidArgumentException($e->getMessage(), $e->getCode(), $e);
217+
$keyBinary = hex2bin(substr($token, -self::CSRF_HASH_HEX));
218+
$hashBinary = hex2bin(substr($token, 0, self::CSRF_HASH_HEX));
219+
220+
if ($hashBinary === false || $keyBinary === false) {
221+
throw new InvalidArgumentException('Invalid CSRF token.');
217222
}
223+
224+
return bin2hex($hashBinary ^ $keyBinary);
218225
}
219226

220-
private function isCSRFCookie(): bool
227+
private function isCsrfCookie(): bool
221228
{
222229
return $this->config->csrfProtection === self::CSRF_PROTECTION_COOKIE;
223230
}
224231

232+
/**
233+
* @phpstan-assert SessionInterface $this->session
234+
*/
225235
private function configureSession(): void
226236
{
227237
$this->session = service('session');
228238
}
229239

230240
private function configureCookie(CookieConfig $cookie): void
231241
{
232-
$cookiePrefix = $cookie->prefix;
233-
$this->cookieName = $cookiePrefix . $this->rawCookieName;
242+
$this->cookieName = $cookie->prefix . $this->rawCookieName;
243+
234244
Cookie::setDefaults($cookie);
235245
}
236246

@@ -343,13 +353,16 @@ private function isNonEmptyTokenString(mixed $token): bool
343353
*/
344354
private function restoreHash(): void
345355
{
346-
if ($this->isCSRFCookie()) {
347-
if ($this->isHashInCookie()) {
348-
$this->hash = $this->hashInCookie;
349-
}
350-
} elseif ($this->session->has($this->config->tokenName)) {
351-
// Session based CSRF protection
352-
$this->hash = $this->session->get($this->config->tokenName);
356+
if ($this->isCsrfCookie()) {
357+
$this->hash = $this->isHashInCookie() ? $this->hashInCookie : null;
358+
359+
return;
360+
}
361+
362+
$tokenName = $this->config->tokenName;
363+
364+
if ($this->session instanceof SessionInterface && $this->session->has($tokenName)) {
365+
$this->hash = $this->session->get($tokenName);
353366
}
354367
}
355368

@@ -359,28 +372,33 @@ private function isHashInCookie(): bool
359372
return false;
360373
}
361374

362-
$length = static::CSRF_HASH_BYTES * 2;
363-
$pattern = '#^[0-9a-f]{' . $length . '}$#iS';
375+
if (strlen($this->hashInCookie) !== self::CSRF_HASH_HEX) {
376+
return false;
377+
}
364378

365-
return preg_match($pattern, $this->hashInCookie) === 1;
379+
return ctype_xdigit($this->hashInCookie);
366380
}
367381

368382
private function saveHashInCookie(): void
369383
{
370-
$this->cookie = new Cookie(
384+
$expires = $this->config->expires === 0 ? 0 : Time::now()->getTimestamp() + $this->config->expires;
385+
386+
$cookie = new Cookie(
371387
$this->rawCookieName,
372388
$this->hash,
373-
[
374-
'expires' => $this->config->expires === 0 ? 0 : Time::now()->getTimestamp() + $this->config->expires,
375-
],
389+
compact('expires'),
376390
);
377391

378-
$response = service('response');
379-
$response->setCookie($this->cookie);
392+
service('response')->setCookie($cookie);
393+
394+
// For backward compatibility, we also set the cookie value to $this->cookie property.
395+
// @todo v4.8.0 Remove $this->cookie property and its usages.
396+
$this->cookie = $cookie;
380397
}
381398

382399
private function saveHashInSession(): void
383400
{
401+
assert($this->session instanceof SessionInterface);
384402
$this->session->set($this->config->tokenName, $this->hash);
385403
}
386404
}

‎tests/system/Security/SecurityCSRFCookieRandomizeTokenTest.php‎

Lines changed: 29 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515

1616
use CodeIgniter\Config\Factories;
1717
use CodeIgniter\Config\Services;
18-
use CodeIgniter\Cookie\Cookie;
1918
use CodeIgniter\HTTP\IncomingRequest;
2019
use CodeIgniter\HTTP\SiteURI;
2120
use CodeIgniter\HTTP\UserAgent;
@@ -32,66 +31,59 @@
3231
#[Group('Others')]
3332
final class SecurityCSRFCookieRandomizeTokenTest extends CIUnitTestCase
3433
{
35-
/**
36-
* @var string CSRF protection hash
37-
*/
38-
private string $hash = '8b9218a55906f9dcc1dc263dce7f005a';
39-
40-
/**
41-
* @var string CSRF randomized token
42-
*/
43-
private string $randomizedToken = '8bc70b67c91494e815c7d2219c1ae0ab005513c290126d34d41bf41c5265e0f1';
44-
45-
private SecurityConfig $config;
34+
private const CSRF_PROTECTION_HASH = '8b9218a55906f9dcc1dc263dce7f005a';
35+
private const CSRF_RANDOMIZED_TOKEN = '8bc70b67c91494e815c7d2219c1ae0ab005513c290126d34d41bf41c5265e0f1';
4636

4737
protected function setUp(): void
4838
{
4939
parent::setUp();
5040

51-
Services::injectMock('superglobals', new Superglobals(null, null, null, []));
41+
Services::injectMock('superglobals', new Superglobals(post: [], cookie: []));
42+
43+
$config = new SecurityConfig();
44+
$config->csrfProtection = Security::CSRF_PROTECTION_COOKIE;
45+
$config->tokenRandomize = true;
46+
Factories::injectMock('config', 'Security', $config);
47+
48+
$security = new MockSecurity($config);
49+
service('superglobals')->setCookie($security->getCookieName(), self::CSRF_PROTECTION_HASH);
5250

53-
$this->config = new SecurityConfig();
54-
$this->config->csrfProtection = Security::CSRF_PROTECTION_COOKIE;
55-
$this->config->tokenRandomize = true;
56-
Factories::injectMock('config', 'Security', $this->config);
51+
$this->resetServices();
52+
}
5753

58-
// Set Cookie value
59-
$security = new MockSecurity($this->config);
60-
service('superglobals')->setCookie($security->getCookieName(), $this->hash);
54+
protected function tearDown(): void
55+
{
56+
parent::tearDown();
6157

6258
$this->resetServices();
59+
Factories::reset('config');
6360
}
6461

6562
public function testTokenIsReadFromCookie(): void
6663
{
67-
$security = new MockSecurity($this->config);
64+
$security = new MockSecurity(config('Security'));
6865

69-
$this->assertSame(
70-
$this->randomizedToken,
71-
$security->getHash(),
72-
);
66+
$this->assertSame(self::CSRF_RANDOMIZED_TOKEN, $security->getHash());
7367
}
7468

75-
public function testCSRFVerifySetNewCookie(): void
69+
public function testCsrfVerifySetNewCookie(): void
7670
{
77-
service('superglobals')->setServer('REQUEST_METHOD', 'POST');
78-
service('superglobals')->setPost('foo', 'bar');
79-
service('superglobals')->setPost('csrf_test_name', $this->randomizedToken);
71+
service('superglobals')
72+
->setServer('REQUEST_METHOD', 'POST')
73+
->setPost('foo', 'bar')
74+
->setPost('csrf_test_name', self::CSRF_RANDOMIZED_TOKEN);
8075

8176
$config = new MockAppConfig();
8277
$request = new IncomingRequest($config, new SiteURI($config), null, new UserAgent());
8378

84-
$security = new Security($this->config);
79+
$security = new Security(config('Security'));
8580

8681
$this->assertInstanceOf(Security::class, $security->verify($request));
8782
$this->assertLogged('info', 'CSRF token verified.');
88-
$this->assertCount(1, service('superglobals')->getPostArray());
89-
90-
/** @var Cookie $cookie */
91-
$cookie = $this->getPrivateProperty($security, 'cookie');
92-
$newHash = $cookie->getValue();
83+
$this->assertSame(['foo' => 'bar'], service('superglobals')->getPostArray());
9384

94-
$this->assertNotSame($this->hash, $newHash);
95-
$this->assertSame(32, strlen($newHash));
85+
$cookieHash = service('response')->getCookie($security->getCookieName())->getValue();
86+
$this->assertNotSame(self::CSRF_PROTECTION_HASH, $cookieHash);
87+
$this->assertSame(32, strlen($cookieHash));
9688
}
9789
}

‎user_guide_src/source/changelogs/v4.8.0.rst‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,8 @@ Behavior Changes
2626
Interface Changes
2727
=================
2828

29-
**NOTE:** If you've implemented your own classes that implement these interfaces from scratch, you will need to update your implementations to include the new methods to ensure compatibility.
29+
**NOTE:** If you've implemented your own classes that implement these interfaces from scratch, you will need to
30+
update your implementations to include the new methods or method changes to ensure compatibility.
3031

3132
- **Security:** The ``SecurityInterface``'s ``verify()`` method now has a native return type of ``static``.
3233

0 commit comments

Comments
 (0)