Fix CsrfCounterMeasure bypass for crafted cross-site requests (#405)

The `Sec-Fetch-Site` based element decided the cross-site rejection from
the submitted `CSRFToken` value instead of the server-computed result,
so a crafted request could bypass the countermeasure in two ways:

1. An empty value nulled the element, and since it was not required its
validator was skipped entirely and the request treated as valid.
2. Any other non-empty value was coerced to the validator's bool
parameter, where every truthy string passed as safe.

The element now ignores the submitted value: it always reports a value
so the validator runs regardless of input, and the validator rejects
based solely on the `Sec-Fetch-Site` result. This restores the invariant
that a cross-site request cannot be accepted no matter what the client
sends.

Author: lippserd

src/Common/CsrfCounterMeasure.php

@@ -5,6 +5,7 @@
 use Error;
 use ipl\Html\Contract\FormElement;
 use ipl\Html\FormElement\HiddenElement;
+use ipl\Validator\CallbackValidator;
 
 trait CsrfCounterMeasure
 {
@@ -55,17 +56,17 @@ protected function createCsrfCounterMeasure($uniqueId)
     {
         $requestIsSafe = $this->requestIsSafe();
         if ($requestIsSafe !== null) {
-            return new HiddenElement('CSRFToken', [
+            return new class ('CSRFToken', [
                 'ignore' => true,
-                'value' => $requestIsSafe,
-                'validators' => ['Callback' => function (bool $requestIsSafe) {
-                    if ($requestIsSafe) {
-                        return true;
-                    }
-
-                    throw new Error('Rejecting cross-site request');
-                }]
-            ]);
+                'validators' => [
+                    new CallbackValidator(fn() => $requestIsSafe ?: throw new Error('Rejecting cross-site request')),
+                ],
+            ]) extends HiddenElement {
+                public function hasValue(): bool
+                {
+                    return true; // The validator must run even if no value was submitted
+                }
+            };
         }
 
         $hashAlgo = in_array('sha3-256', hash_algos(), true) ? 'sha3-256' : 'sha256';

tests/Common/CsrfCounterMeasureTest.php

@@ -84,6 +84,32 @@ public function testUnsafeCrossSiteRequestIsRejected(): void
         $this->makeForm()->handleRequest($this->requestMock('POST'));
     }
 
+    public function testCrossSiteRequestIsRejectedWithNonEmptyToken(): void
+    {
+        $_SERVER['HTTP_SEC_FETCH_SITE'] = 'cross-site';
+
+        $this->expectException(Error::class);
+        $this->expectExceptionMessage('Rejecting cross-site request');
+
+        $form = $this->makeForm();
+        $form->populate(['CSRFToken' => 'anything']);
+        $form->ensureAssembled();
+        $form->isValid();
+    }
+
+    public function testCrossSiteRequestIsRejectedWithEmptyToken(): void
+    {
+        $_SERVER['HTTP_SEC_FETCH_SITE'] = 'cross-site';
+
+        $this->expectException(Error::class);
+        $this->expectExceptionMessage('Rejecting cross-site request');
+
+        $form = $this->makeForm();
+        $form->populate(['CSRFToken' => '']);
+        $form->ensureAssembled();
+        $form->isValid();
+    }
+
     public function testCreateReturnsDummyElementForSafeRequest(): void
     {
         $_SERVER['HTTP_SEC_FETCH_SITE'] = 'same-origin';