Replace custom URI/URL validation with simplesamlphp/assert (#1951)

* refactor: use simplesamlphp assert for URI and URL validation

* refactor: remove unused parse() method from EngineBlock_Validator_Uri

Author: kayjoosten

composer.json

@@ -36,6 +36,7 @@
         "pimple/pimple": "^3.6",
         "ramsey/uuid": "^4.9.1",
         "robrichards/xmlseclibs": "^3.1.3",
+        "simplesamlphp/assert": "^1.9.1",
         "simplesamlphp/saml2": "^4.19",
         "symfony/asset": "^7.4",
         "symfony/console": "^7.4",

composer.lock

@@ -16,6 +16,9 @@
  * limitations under the License.
  */
 
+use SimpleSAML\Assert\Assert;
+use SimpleSAML\Assert\AssertionFailedException;
+
 class EngineBlock_Attributes_Validator_Type extends EngineBlock_Attributes_Validator_Abstract
 {
     const ERROR_ATTRIBUTE_VALIDATOR_URI      = 'error_attribute_validator_type_uri';
@@ -75,7 +78,9 @@ public function validate(array $attributes)
 
             case 'URL':
                 foreach ($attributeValues as $attributeValue) {
-                    if (filter_var($attributeValue, FILTER_VALIDATE_URL) === false) {
+                    try {
+                        Assert::validURL($attributeValue);
+                    } catch (AssertionFailedException $e) {
                         $this->_messages[] = array(
                             self::ERROR_ATTRIBUTE_VALIDATOR_URL,
                             $this->_attributeName,

library/EngineBlock/Attributes/Validator/Type.php

@@ -16,48 +16,26 @@
  * limitations under the License.
  */
 
+use SimpleSAML\Assert\Assert;
+use SimpleSAML\Assert\AssertionFailedException;
+
 /**
- * Validate URIs according to RFC-3986.
- *
- * See: http://www.rfc-editor.org/errata_search.php?rfc=3986
- *
- * Note that this is a VERY permissive regex
+ * Validate URIs using simplesamlphp/assert.
  */
 class EngineBlock_Validator_Uri
 {
-    const REGEX = '/^(([^:\/?#]+):)?(\/\/([^\/?#]*))?([^?#]*)(\?([^#]*))?(#(.*))?/';
-
     /**
      * @param string $string
      * @return bool
      */
     public function validate($uri)
     {
-        return (bool) preg_match(self::REGEX, $uri);
-    }
-
-    /**
-     * Parses the given uri with the regex, this is useful for debugging
-     *
-     * @param string $uri
-     * @return array
-     */
-    public static function parse($uri)
-    {
-        preg_match(self::REGEX, $uri, $matches);
-
-        $keys = ['match'];
-        $keys[] = 'scheme+separator';
-        $keys[] = 'scheme';
-        $keys[] = 'host+separator';
-        $keys[] = 'host';
-        $keys[] = 'path';
-        $keys[] = 'query+separator';
-        $keys[] = 'query';
-        $keys[] = 'anchor+separator';
-        $keys[] = 'anchor';
+        try {
+            Assert::validURI($uri);
+        } catch (AssertionFailedException $e) {
+            return false;
+        }
 
-        $keysMatched = array_slice($keys, 0, count($matches));
-        return array_combine($keysMatched, $matches);
+        return true;
     }
 }

library/EngineBlock/Validator/Uri.php

@@ -16,6 +16,7 @@
  * limitations under the License.
  */
 
+use PHPUnit\Framework\Attributes\DataProvider;
 use PHPUnit\Framework\TestCase;
 
 class EngineBlock_Test_TypeTest extends TestCase
@@ -26,13 +27,22 @@ class EngineBlock_Test_TypeTest extends TestCase
      * @param $options
      * @param $attributes
      */
-    #[\PHPUnit\Framework\Attributes\DataProvider('validAttributesProvider')]
+    #[DataProvider('validAttributesProvider')]
     public function testAttributeValidates($attributeName, $options, $attributes)
     {
         $validator = new EngineBlock_Attributes_Validator_Type($attributeName, $options);
         $this->assertTrue($validator->validate($attributes));
     }
 
+    #[DataProvider('invalidAttributesProvider')]
+    public function testAttributeValidationFails($attributeName, $options, $attributes, $expectedMessage)
+    {
+        $validator = new EngineBlock_Attributes_Validator_Type($attributeName, $options);
+
+        $this->assertFalse($validator->validate($attributes));
+        $this->assertSame([$expectedMessage, $attributeName, $options, $attributes[$attributeName][0]], $validator->getMessages()[0]);
+    }
+
     public static function validAttributesProvider()
     {
         return array(
@@ -59,7 +69,8 @@ public static function validAttributesProvider()
                 'options' => 'URI',
                 'attributes' => array(
                     'foo' => array(
-                        '?'
+                        '?',
+                        'urn:mace:dir:entitlement:common-lib-terms',
                     )
                 )
             ),
@@ -78,4 +89,20 @@ public static function validAttributesProvider()
             )
         );
     }
+
+    public static function invalidAttributesProvider()
+    {
+        return array(
+            array(
+                'attributeName' => 'foo',
+                'options' => 'URL',
+                'attributes' => array(
+                    'foo' => array(
+                        'mailto:test@example.org',
+                    )
+                ),
+                'expectedMessage' => EngineBlock_Attributes_Validator_Type::ERROR_ATTRIBUTE_VALIDATOR_URL,
+            ),
+        );
+    }
 }

tests/library/EngineBlock/Test/Attributes/Validator/TypeTest.php

@@ -19,9 +19,6 @@
 use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration;
 use PHPUnit\Framework\TestCase;
 
-/**
- * @todo write test which tests failing...this validator is so permissive it is VERY hard to let it fail
- */
 class EngineBlock_Test_Validator_UriTest extends TestCase
 {
     use MockeryPHPUnitIntegration;
@@ -46,7 +43,8 @@ public static function validUriProvider()
     {
         return array(
             array('http://example.com'), // Pretty standard http url
-            array('urn:mace:dir:entitlement:common-lib-terms') // Saml entitlement
+            array('urn:mace:dir:entitlement:common-lib-terms'), // Saml entitlement
+            array('?'),
         );
     }
 }