Skip to content
Open
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
19 changes: 19 additions & 0 deletions app/Config/Security.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,25 @@ class Security extends BaseConfig
*/
public string $csrfProtection = 'cookie';

/**
* --------------------------------------------------------------------------
* CSRF Fetch Metadata
* --------------------------------------------------------------------------
*
* Whether to use Fetch Metadata request headers as a first-line CSRF check.
*/
public bool $csrfUseFetchMetadata = true;

/**
* --------------------------------------------------------------------------
* CSRF Allow Same Site
* --------------------------------------------------------------------------
*
* Whether requests with the Sec-Fetch-Site: same-site header should pass
* Fetch Metadata verification.
*/
public bool $csrfAllowSameSite = false;

/**
* --------------------------------------------------------------------------
* CSRF Token Randomization
Expand Down
22 changes: 21 additions & 1 deletion system/Filters/CSRF.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,13 @@
namespace CodeIgniter\Filters;

use CodeIgniter\HTTP\IncomingRequest;
use CodeIgniter\HTTP\Method;
use CodeIgniter\HTTP\RedirectResponse;
use CodeIgniter\HTTP\RequestInterface;
use CodeIgniter\HTTP\ResponseInterface;
use CodeIgniter\Security\Exceptions\SecurityException;
use CodeIgniter\Security\Security;
use Config\Security as SecurityConfig;

/**
* CSRF filter.
Expand Down Expand Up @@ -52,12 +54,19 @@ public function before(RequestInterface $request, $arguments = null)
$security->verify($request);
} catch (SecurityException $e) {
if ($security->shouldRedirect() && ! $request->isAJAX()) {
return redirect()->back()->with('error', $e->getMessage());
$response = redirect()->back()->with('error', $e->getMessage());
$this->addFetchMetadataVaryHeader($request, $response);

return $response;
}

$this->addFetchMetadataVaryHeader($request, service('response'));

throw $e;
}

$this->addFetchMetadataVaryHeader($request, service('response'));

return null;
}

Expand All @@ -70,4 +79,15 @@ public function after(RequestInterface $request, ResponseInterface $response, $a
{
return null;
}

private function addFetchMetadataVaryHeader(IncomingRequest $request, ResponseInterface $response): void
{
$config = get_object_vars(config(SecurityConfig::class));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What? The config already provides access to the properties.

$useFetchMetadata = ($config['csrfUseFetchMetadata'] ?? false) === true;
$isUnsafeMethod = in_array($request->getMethod(), [Method::POST, Method::PUT, Method::DELETE, Method::PATCH], true);

if ($useFetchMetadata && $isUnsafeMethod) {
$response->appendHeader('Vary', 'Sec-Fetch-Site');
}
}
}
64 changes: 60 additions & 4 deletions system/Security/Security.php
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,11 @@
*/
class Security implements SecurityInterface
{
public const CSRF_PROTECTION_COOKIE = 'cookie';
public const CSRF_PROTECTION_SESSION = 'session';
public const CSRF_PROTECTION_COOKIE = 'cookie';
public const CSRF_PROTECTION_SESSION = 'session';
private const FETCH_METADATA_ALLOW = 'allow';
private const FETCH_METADATA_FALLBACK = 'fallback';
private const FETCH_METADATA_REJECT = 'reject';

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

assert($request instanceof IncomingRequest);

$decision = $this->fetchMetadataDecision($request);

if ($decision === self::FETCH_METADATA_ALLOW) {
$this->removeTokenInRequest($request);

log_message('info', 'CSRF Fetch Metadata verified.');

return $this;
}

if ($decision === self::FETCH_METADATA_REJECT) {
throw SecurityException::forDisallowedAction();
}

$this->verifyToken($request);

return $this;
}

private function verifyToken(IncomingRequest $request): void
{
$postedToken = $this->getPostedToken($request);

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

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

return $this;
}

public function getHash(): ?string
Expand Down Expand Up @@ -229,6 +251,36 @@ private function isCsrfCookie(): bool
return $this->config->csrfProtection === self::CSRF_PROTECTION_COOKIE;
}

/**
* @return self::FETCH_METADATA_*
*/
private function fetchMetadataDecision(IncomingRequest $request): string
{
$config = get_object_vars($this->config);

if (($config['csrfUseFetchMetadata'] ?? false) !== true) {
return self::FETCH_METADATA_FALLBACK;
}

$fetchSite = strtolower($request->getHeaderLine('Sec-Fetch-Site'));

if ($fetchSite === 'same-origin') {
return self::FETCH_METADATA_ALLOW;
}

if ($fetchSite === 'cross-site') {
return self::FETCH_METADATA_REJECT;
}

if ($fetchSite === 'same-site') {
return ($config['csrfAllowSameSite'] ?? false) === true
? self::FETCH_METADATA_ALLOW
: self::FETCH_METADATA_REJECT;
}

return self::FETCH_METADATA_FALLBACK;
}

/**
* @phpstan-assert SessionInterface $this->session
*/
Expand Down Expand Up @@ -285,6 +337,10 @@ private function removeTokenInRequest(IncomingRequest $request): void
// If the token is found in form-encoded data, we can safely remove it.
parse_str($body, $result);

if (! array_key_exists($tokenName, $result)) {
return;
}

unset($result[$tokenName]);
$request->setBody(http_build_query($result));
}
Expand Down
91 changes: 91 additions & 0 deletions tests/system/Filters/CSRFTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,14 @@

namespace CodeIgniter\Filters;

use CodeIgniter\Config\Factories;
use CodeIgniter\HTTP\CLIRequest;
use CodeIgniter\HTTP\IncomingRequest;
use CodeIgniter\HTTP\RedirectResponse;
use CodeIgniter\HTTP\Response;
use CodeIgniter\Security\Exceptions\SecurityException;
use CodeIgniter\Test\CIUnitTestCase;
use Config\Security as SecurityConfig;
use PHPUnit\Framework\Attributes\BackupGlobals;
use PHPUnit\Framework\Attributes\Group;

Expand All @@ -37,6 +41,14 @@ protected function setUp(): void
$this->config = new \Config\Filters();
}

protected function tearDown(): void
{
parent::tearDown();

$this->resetServices();
Factories::reset('config');
}

public function testDoNotCheckCliRequest(): void
{
$this->config->globals = [
Expand Down Expand Up @@ -73,4 +85,83 @@ public function testPassGetRequest(): void
// GET request is not protected, so no SecurityException will be thrown.
$this->assertSame($this->request, $request);
}

public function testBeforeAddsVaryHeaderForFetchMetadataVerification(): void
{
$filter = new CSRF();
$request = single_service('incomingrequest', null)
->withMethod('POST')
->setHeader('Sec-Fetch-Site', 'same-origin');

$filter->before($request);

$this->assertSame('Sec-Fetch-Site', service('response')->getHeaderLine('Vary'));
}

public function testBeforeAppendsVaryHeaderForFetchMetadataVerification(): void
{
$filter = new CSRF();
$request = single_service('incomingrequest', null)
->withMethod('POST')
->setHeader('Sec-Fetch-Site', 'same-origin');
service('response')->setHeader('Vary', 'Accept-Language');

$filter->before($request);

$this->assertSame('Accept-Language, Sec-Fetch-Site', service('response')->getHeaderLine('Vary'));
}

public function testBeforeAddsVaryHeaderToRedirectResponseForFetchMetadataVerification(): void
{
$config = new SecurityConfig();
$config->redirect = true;
Factories::injectMock('config', 'Security', $config);

$filter = new CSRF();
$request = single_service('incomingrequest', null)
->withMethod('POST')
->setHeader('Sec-Fetch-Site', 'cross-site');

$response = $filter->before($request);

$this->assertInstanceOf(RedirectResponse::class, $response);
$this->assertSame('Sec-Fetch-Site', $response->getHeaderLine('Vary'));
}

public function testBeforeThrowsExceptionForRejectedFetchMetadataVerification(): void
{
$filter = new CSRF();
$request = single_service('incomingrequest', null)
->withMethod('POST')
->setHeader('Sec-Fetch-Site', 'cross-site');

try {
$filter->before($request);

$this->fail('Expected SecurityException was not thrown.');
} catch (SecurityException) {
$this->assertSame('Sec-Fetch-Site', service('response')->getHeaderLine('Vary'));
}
}

public function testBeforeDoesNotAddVaryHeaderForTokenVerification(): void
{
service('superglobals')
->setServer('REQUEST_METHOD', 'POST')
->setPost('csrf_test_name', '8b9218a55906f9dcc1dc263dce7f005a')
->setCookie('csrf_cookie_name', '8b9218a55906f9dcc1dc263dce7f005a');

$config = new SecurityConfig();
$config->csrfUseFetchMetadata = false;
Factories::injectMock('config', 'Security', $config);

$filter = new CSRF();
$request = single_service('incomingrequest', null)
->withMethod('POST')
->setHeader('Sec-Fetch-Site', 'same-origin');

$filter->before($request);

$this->assertSame('', service('response')->getHeaderLine('Vary'));
}
}
Loading
Loading