CsrfCounterMeasure: Only reject unsafe requests upon validation

src/Common/CsrfCounterMeasure.php

@@ -6,7 +6,7 @@
 use ipl\Html\Contract\FormElement;
 use ipl\Html\FormElement\HiddenElement;
 
 trait CsrfCounterMeasure
 {
     /** @var ?string The ID of the CSRF form element */
     private ?string $csrfCounterMeasureId = null;
@@ -49,23 +49,21 @@
      *
      * @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 @@
      * 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

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;
+    }
 }