diff --git a/src/Common/CsrfCounterMeasure.php b/src/Common/CsrfCounterMeasure.php index cda68dff..07f38e77 100644 --- a/src/Common/CsrfCounterMeasure.php +++ b/src/Common/CsrfCounterMeasure.php @@ -49,23 +49,21 @@ public function disableCsrfCounterMeasure(): void * * @return FormElement * - * @throws Error If {@see requestIsSafe()} returns false - * * @deprecated Use {@see addCsrfCounterMeasure()} instead */ protected function createCsrfCounterMeasure($uniqueId) { $requestIsSafe = $this->requestIsSafe(); - if ($requestIsSafe !== null) { - if (! $requestIsSafe) { - throw new Error('Rejecting cross-site request'); - } - return new HiddenElement('CSRFToken', [ - 'ignore' => true, - 'validators' => ['Callback' => function () { - return true; + 'ignore' => true, + 'value' => $requestIsSafe, + 'validators' => ['Callback' => function (bool $requestIsSafe) { + if ($requestIsSafe) { + return true; + } + + throw new Error('Rejecting cross-site request'); }] ]); } @@ -134,21 +132,13 @@ protected function addCsrfCounterMeasure(?string $uniqueId = null): void * Get whether the request is safe from CSRF based on the Sec-Fetch-Site header * * Returns true if the header indicates a same-origin request or a user-originated operation (e.g. typing a URL), - * both of which cannot be forged by a cross-site attacker. Returns false if the header indicates a cross-site - * request. Returns null in two cases where a token-based fallback should be used instead: when the request uses an - * RFC 9110 safe method (GET, HEAD, OPTIONS, TRACE), which cannot carry CSRF side effects by definition, or when - * the header is absent, indicating a legacy browser that must be validated via the CSRF token. - * - * @see https://datatracker.ietf.org/doc/html/rfc9110#section-9.2.1 + * both of which cannot be forged by a cross-site attacker. Returns null if the header is absent, + * in which case a CSRF token must be validated. False indicates a cross-site request. * * @return ?bool */ protected function requestIsSafe(): ?bool { - if (in_array($_SERVER['REQUEST_METHOD'] ?? null, ['GET', 'HEAD', 'OPTIONS', 'TRACE'], true)) { - return null; - } - return match ($_SERVER['HTTP_SEC_FETCH_SITE'] ?? null) { 'same-origin' => true, // same scheme, host and port 'none' => true, // a user-originated operation diff --git a/tests/Common/CsrfCounterMeasureTest.php b/tests/Common/CsrfCounterMeasureTest.php index 1ada6115..c72eea69 100644 --- a/tests/Common/CsrfCounterMeasureTest.php +++ b/tests/Common/CsrfCounterMeasureTest.php @@ -6,9 +6,12 @@ use ipl\Html\Contract\FormElement; use ipl\Html\Form; use ipl\Html\FormElement\HiddenElement; +use ipl\Stdlib\Contract\Validator; use ipl\Tests\Web\TestCase; use ipl\Web\Common\CsrfCounterMeasure; use PHPUnit\Framework\Attributes\DataProvider; +use Psr\Http\Message\ServerRequestInterface; +use Psr\Http\Message\UriInterface; class CsrfCounterMeasureTest extends TestCase { @@ -71,45 +74,48 @@ public function testInvalidToken() $token->isValid(); } - public static function safeHeaderValueProvider(): array - { - return [ - 'same-origin' => ['same-origin'], - 'none' => ['none'], - ]; - } - - public function testAddThrowsForCrossSiteRequest(): void + public function testUnsafeCrossSiteRequestIsRejected(): void { $_SERVER['HTTP_SEC_FETCH_SITE'] = 'cross-site'; $this->expectException(Error::class); $this->expectExceptionMessage('Rejecting cross-site request'); - $this->makeForm()->ensureAssembled(); + $this->makeForm()->handleRequest($this->requestMock('POST')); } - #[DataProvider('safeHeaderValueProvider')] - public function testCreateReturnsDummyElementForSafeRequest(string $headerValue): void + public function testCreateReturnsDummyElementForSafeRequest(): void { - $_SERVER['HTTP_SEC_FETCH_SITE'] = $headerValue; + $_SERVER['HTTP_SEC_FETCH_SITE'] = 'same-origin'; $element = $this->makeForm()->callCreate('uniqueId'); - $this->assertInstanceOf(HiddenElement::class, $element); - $this->assertNotCount(0, $element->getValidators(), 'Dummy element must have at least one validator'); - $element->setValue('garbage'); - $this->assertTrue($element->isValid(), 'Dummy element should accept any value without validation'); + + $validatorMock = $this->createMock(Validator::class); + $validatorMock->expects($this->once()) + ->method('isValid') + ->willReturn(true); + $element->getValidators()->add($validatorMock); + + $this->assertTrue($element->isValid(), 'Dummy element must be successfully validated'); } - public function testCreateThrowsForCrossSiteRequest(): void + public function testCreateReturnsElementWithTokenForIndistinguishableRequests(): void { - $_SERVER['HTTP_SEC_FETCH_SITE'] = 'cross-site'; + $_SERVER['HTTP_SEC_FETCH_SITE'] = 'none'; - $this->expectException(Error::class); - $this->expectExceptionMessage('Rejecting cross-site request'); + $element = $this->makeForm()->callCreate('uniqueId'); + $this->assertInstanceOf(HiddenElement::class, $element); - $this->makeForm()->callCreate('uniqueId'); + $element->setValue(base64_encode('seed') . '|' . hash('sha3-256', 'uniqueIdseed')); + + $validatorMock = $this->createMock(Validator::class); + $validatorMock->expects($this->once()) + ->method('isValid') + ->willReturn(true); + $element->getValidators()->add($validatorMock); + + $this->assertTrue($element->isValid(), 'Dummy element must be successfully validated'); } public static function safeMethodProvider(): array @@ -123,30 +129,29 @@ public static function safeMethodProvider(): array } #[DataProvider('safeMethodProvider')] - public function testCreateDoesNotThrowForSafeMethodWithCrossSiteHeader(string $method): void + public function testSafeCrossSiteRequestIsNotValidated(string $method): void { - $_SERVER['REQUEST_METHOD'] = $method; $_SERVER['HTTP_SEC_FETCH_SITE'] = 'cross-site'; - $this->makeForm()->callCreate('uniqueId'); - $this->addToAssertionCount(1); + $flag = false; + + $form = $this->makeForm(); + $form->on(Form::ON_REQUEST, function () use (&$flag) { + $flag = true; + }); + $form->handleRequest($this->requestMock($method)); + + $this->assertTrue($flag, 'Form did not emit ON_REQUEST event for method ' . $method); } - #[DataProvider('safeMethodProvider')] - public function testCreateReturnsTokenElementForSafeMethod(string $method): void + public function testValidationThrowsForCrossSiteRequests(): void { - $_SERVER['REQUEST_METHOD'] = $method; $_SERVER['HTTP_SEC_FETCH_SITE'] = 'cross-site'; - $element = $this->makeForm()->callCreate('uniqueId'); - - // The token element must reject invalid values; a dummy element would accept them. - $element->setValue('garbage'); - $this->expectException(Error::class); - $this->expectExceptionMessage('Invalid CSRF token provided'); + $this->expectExceptionMessage('Rejecting cross-site request'); - $element->isValid(); + $this->makeForm()->isValid(); } private function createElement(): FormElement @@ -181,4 +186,18 @@ protected function assemble(): void } }; } + + private function requestMock(string $method): ServerRequestInterface + { + $mock = $this->createMock(ServerRequestInterface::class); + $mock->method('getMethod')->willReturn($method); + $mock->method('getParsedBody')->willReturn([]); + $mock->method('getUploadedFiles')->willReturn([]); + $mock->method('getUri')->willReturn($this->createConfiguredMock( + UriInterface::class, + ['getQuery' => ''] + )); + + return $mock; + } }