Skip to content

Commit 800a72b

Browse files
memleakdmemleakd
authored
feat: add Fetch Metadata CSRF protection (#10221)
1 parent 3c079f5 commit 800a72b

10 files changed

Lines changed: 462 additions & 5 deletions

File tree

‎app/Config/Security.php‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,25 @@ class Security extends BaseConfig
1717
*/
1818
public string $csrfProtection = 'cookie';
1919

20+
/**
21+
* --------------------------------------------------------------------------
22+
* CSRF Fetch Metadata
23+
* --------------------------------------------------------------------------
24+
*
25+
* Whether to use Fetch Metadata request headers as a first-line CSRF check.
26+
*/
27+
public bool $csrfFetchMetadata = true;
28+
29+
/**
30+
* --------------------------------------------------------------------------
31+
* CSRF Fetch Metadata Reject Same Site
32+
* --------------------------------------------------------------------------
33+
*
34+
* Whether requests with the Sec-Fetch-Site: same-site header should be
35+
* rejected instead of falling back to token verification.
36+
*/
37+
public bool $csrfFetchMetadataRejectSameSite = false;
38+
2039
/**
2140
* --------------------------------------------------------------------------
2241
* CSRF Token Randomization

‎system/Filters/CSRF.php‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
namespace CodeIgniter\Filters;
1515

1616
use CodeIgniter\HTTP\IncomingRequest;
17+
use CodeIgniter\HTTP\Method;
1718
use CodeIgniter\HTTP\RedirectResponse;
1819
use CodeIgniter\HTTP\RequestInterface;
1920
use CodeIgniter\HTTP\ResponseInterface;
@@ -52,12 +53,19 @@ public function before(RequestInterface $request, $arguments = null)
5253
$security->verify($request);
5354
} catch (SecurityException $e) {
5455
if ($security->shouldRedirect() && ! $request->isAJAX()) {
55-
return redirect()->back()->with('error', $e->getMessage());
56+
$response = redirect()->back()->with('error', $e->getMessage());
57+
$this->addFetchMetadataVaryHeader($request, $response, $security);
58+
59+
return $response;
5660
}
5761

62+
$this->addFetchMetadataVaryHeader($request, service('response'), $security);
63+
5864
throw $e;
5965
}
6066

67+
$this->addFetchMetadataVaryHeader($request, service('response'), $security);
68+
6169
return null;
6270
}
6371

@@ -70,4 +78,13 @@ public function after(RequestInterface $request, ResponseInterface $response, $a
7078
{
7179
return null;
7280
}
81+
82+
private function addFetchMetadataVaryHeader(IncomingRequest $request, ResponseInterface $response, Security $security): void
83+
{
84+
$isUnsafeMethod = in_array($request->getMethod(), [Method::POST, Method::PUT, Method::DELETE, Method::PATCH], true);
85+
86+
if ($security->shouldUseFetchMetadata() && $isUnsafeMethod) {
87+
$response->appendHeader('Vary', 'Sec-Fetch-Site');
88+
}
89+
}
7390
}

‎system/Security/Security.php‎

Lines changed: 63 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,11 @@
3535
*/
3636
class Security implements SecurityInterface
3737
{
38-
public const CSRF_PROTECTION_COOKIE = 'cookie';
39-
public const CSRF_PROTECTION_SESSION = 'session';
38+
public const CSRF_PROTECTION_COOKIE = 'cookie';
39+
public const CSRF_PROTECTION_SESSION = 'session';
40+
private const FETCH_METADATA_ALLOW = 'allow';
41+
private const FETCH_METADATA_FALLBACK = 'fallback';
42+
private const FETCH_METADATA_REJECT = 'reject';
4043

4144
/**
4245
* CSRF hash length in bytes.
@@ -118,6 +121,27 @@ public function verify(RequestInterface $request): static
118121

119122
assert($request instanceof IncomingRequest);
120123

124+
$decision = $this->fetchMetadataDecision($request);
125+
126+
if ($decision === self::FETCH_METADATA_ALLOW) {
127+
$this->removeTokenInRequest($request);
128+
129+
log_message('info', 'CSRF Fetch Metadata verified.');
130+
131+
return $this;
132+
}
133+
134+
if ($decision === self::FETCH_METADATA_REJECT) {
135+
throw SecurityException::forDisallowedAction();
136+
}
137+
138+
$this->verifyToken($request);
139+
140+
return $this;
141+
}
142+
143+
private function verifyToken(IncomingRequest $request): void
144+
{
121145
$postedToken = $this->getPostedToken($request);
122146

123147
try {
@@ -139,8 +163,6 @@ public function verify(RequestInterface $request): static
139163
}
140164

141165
log_message('info', 'CSRF token verified.');
142-
143-
return $this;
144166
}
145167

146168
public function getHash(): ?string
@@ -170,6 +192,11 @@ public function shouldRedirect(): bool
170192
return $this->config->redirect;
171193
}
172194

195+
public function shouldUseFetchMetadata(): bool
196+
{
197+
return $this->config->csrfFetchMetadata ?? false; // @phpstan-ignore nullCoalesce.initializedProperty
198+
}
199+
173200
/**
174201
* @phpstan-assert string $this->hash
175202
*/
@@ -229,6 +256,34 @@ private function isCsrfCookie(): bool
229256
return $this->config->csrfProtection === self::CSRF_PROTECTION_COOKIE;
230257
}
231258

259+
/**
260+
* @return self::FETCH_METADATA_*
261+
*/
262+
private function fetchMetadataDecision(IncomingRequest $request): string
263+
{
264+
if (! $this->shouldUseFetchMetadata()) {
265+
return self::FETCH_METADATA_FALLBACK;
266+
}
267+
268+
$fetchSite = strtolower($request->getHeaderLine('Sec-Fetch-Site'));
269+
270+
if ($fetchSite === 'same-origin') {
271+
return self::FETCH_METADATA_ALLOW;
272+
}
273+
274+
if ($fetchSite === 'cross-site') {
275+
return self::FETCH_METADATA_REJECT;
276+
}
277+
278+
if ($fetchSite === 'same-site') {
279+
return ($this->config->csrfFetchMetadataRejectSameSite ?? false) // @phpstan-ignore nullCoalesce.initializedProperty
280+
? self::FETCH_METADATA_REJECT
281+
: self::FETCH_METADATA_FALLBACK;
282+
}
283+
284+
return self::FETCH_METADATA_FALLBACK;
285+
}
286+
232287
/**
233288
* @phpstan-assert SessionInterface $this->session
234289
*/
@@ -285,6 +340,10 @@ private function removeTokenInRequest(IncomingRequest $request): void
285340
// If the token is found in form-encoded data, we can safely remove it.
286341
parse_str($body, $result);
287342

343+
if (! array_key_exists($tokenName, $result)) {
344+
return;
345+
}
346+
288347
unset($result[$tokenName]);
289348
$request->setBody(http_build_query($result));
290349
}

‎tests/system/Filters/CSRFTest.php‎

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,16 @@
1313

1414
namespace CodeIgniter\Filters;
1515

16+
use CodeIgniter\Config\Factories;
17+
use CodeIgniter\Config\Services;
1618
use CodeIgniter\HTTP\CLIRequest;
1719
use CodeIgniter\HTTP\IncomingRequest;
20+
use CodeIgniter\HTTP\RedirectResponse;
1821
use CodeIgniter\HTTP\Response;
22+
use CodeIgniter\Security\Exceptions\SecurityException;
1923
use CodeIgniter\Test\CIUnitTestCase;
24+
use CodeIgniter\Test\Mock\MockSecurity;
25+
use Config\Security as SecurityConfig;
2026
use PHPUnit\Framework\Attributes\BackupGlobals;
2127
use PHPUnit\Framework\Attributes\Group;
2228

@@ -37,6 +43,14 @@ protected function setUp(): void
3743
$this->config = new \Config\Filters();
3844
}
3945

46+
protected function tearDown(): void
47+
{
48+
parent::tearDown();
49+
50+
$this->resetServices();
51+
Factories::reset('config');
52+
}
53+
4054
public function testDoNotCheckCliRequest(): void
4155
{
4256
$this->config->globals = [
@@ -73,4 +87,99 @@ public function testPassGetRequest(): void
7387
// GET request is not protected, so no SecurityException will be thrown.
7488
$this->assertSame($this->request, $request);
7589
}
90+
91+
public function testBeforeAddsVaryHeaderForFetchMetadataVerification(): void
92+
{
93+
$filter = new CSRF();
94+
$request = single_service('incomingrequest', null)
95+
->withMethod('POST')
96+
->setHeader('Sec-Fetch-Site', 'same-origin');
97+
98+
$filter->before($request);
99+
100+
$this->assertSame('Sec-Fetch-Site', service('response')->getHeaderLine('Vary'));
101+
}
102+
103+
public function testBeforeAddsVaryHeaderWhenFetchMetadataFallsBackToToken(): void
104+
{
105+
service('superglobals')
106+
->setServer('REQUEST_METHOD', 'POST')
107+
->setPost('csrf_test_name', '8b9218a55906f9dcc1dc263dce7f005a')
108+
->setCookie('csrf_cookie_name', '8b9218a55906f9dcc1dc263dce7f005a');
109+
110+
$filter = new CSRF();
111+
$request = single_service('incomingrequest', null)
112+
->withMethod('POST');
113+
114+
$filter->before($request);
115+
116+
$this->assertSame('Sec-Fetch-Site', service('response')->getHeaderLine('Vary'));
117+
}
118+
119+
public function testBeforeAppendsVaryHeaderForFetchMetadataVerification(): void
120+
{
121+
$filter = new CSRF();
122+
$request = single_service('incomingrequest', null)
123+
->withMethod('POST')
124+
->setHeader('Sec-Fetch-Site', 'same-origin');
125+
service('response')->setHeader('Vary', 'Accept-Language');
126+
127+
$filter->before($request);
128+
129+
$this->assertSame('Accept-Language, Sec-Fetch-Site', service('response')->getHeaderLine('Vary'));
130+
}
131+
132+
public function testBeforeAddsVaryHeaderToRedirectResponseForFetchMetadataVerification(): void
133+
{
134+
$config = new SecurityConfig();
135+
$config->redirect = true;
136+
Factories::injectMock('config', 'Security', $config);
137+
138+
$filter = new CSRF();
139+
$request = single_service('incomingrequest', null)
140+
->withMethod('POST')
141+
->setHeader('Sec-Fetch-Site', 'cross-site');
142+
143+
$response = $filter->before($request);
144+
145+
$this->assertInstanceOf(RedirectResponse::class, $response);
146+
$this->assertSame('Sec-Fetch-Site', $response->getHeaderLine('Vary'));
147+
}
148+
149+
public function testBeforeThrowsExceptionForRejectedFetchMetadataVerification(): void
150+
{
151+
$filter = new CSRF();
152+
$request = single_service('incomingrequest', null)
153+
->withMethod('POST')
154+
->setHeader('Sec-Fetch-Site', 'cross-site');
155+
156+
try {
157+
$filter->before($request);
158+
159+
$this->fail('Expected SecurityException was not thrown.');
160+
} catch (SecurityException) {
161+
$this->assertSame('Sec-Fetch-Site', service('response')->getHeaderLine('Vary'));
162+
}
163+
}
164+
165+
public function testBeforeUsesSecurityServiceConfigForVaryHeader(): void
166+
{
167+
service('superglobals')
168+
->setServer('REQUEST_METHOD', 'POST')
169+
->setPost('csrf_test_name', '8b9218a55906f9dcc1dc263dce7f005a')
170+
->setCookie('csrf_cookie_name', '8b9218a55906f9dcc1dc263dce7f005a');
171+
172+
$config = new SecurityConfig();
173+
$config->csrfFetchMetadata = false;
174+
Services::injectMock('security', new MockSecurity($config));
175+
176+
$filter = new CSRF();
177+
$request = single_service('incomingrequest', null)
178+
->withMethod('POST')
179+
->setHeader('Sec-Fetch-Site', 'same-origin');
180+
181+
$filter->before($request);
182+
183+
$this->assertSame('', service('response')->getHeaderLine('Vary'));
184+
}
76185
}

0 commit comments

Comments
 (0)