Improve PublicSuffix validation in the Domain object

Author: nyamsprod

composer.json

@@ -36,7 +36,7 @@
     "require": {
         "php": ">=7.0",
         "ext-intl": "*",
-        "psr/simple-cache": "^1"
+        "psr/simple-cache": "^1.0.1"
     },
     "require-dev": {
         "composer/composer": "^1.6",

phpstan.src.neon

@@ -1,6 +1,2 @@
 includes:
-- vendor/phpstan/phpstan-strict-rules/rules.neon
-parameters:
-    ignoreErrors:
-        - '#has invalid typehint type Psr\\SimpleCache\\DateInterval#'
-    reportUnmatchedIgnoredErrors: false
+- vendor/phpstan/phpstan-strict-rules/rules.neon

src/Domain.php

@@ -73,44 +73,47 @@ public static function __set_state(array $properties): self
     public function __construct(string $domain = null, PublicSuffix $publicSuffix = null)
     {
         list($this->domain, $this->labels) = $this->setDomain($domain);
-        $this->publicSuffix = $this->setPublicSuffix($publicSuffix);
-        $this->assertValidState();
+        $this->publicSuffix = $this->setPublicSuffix($publicSuffix ?? new PublicSuffix());
         $this->registrableDomain = $this->setRegistrableDomain();
         $this->subDomain = $this->setSubDomain();
     }
 
     /**
      * Sets the public suffix domain part.
      *
-     * @param PublicSuffix|null $publicSuffix
+     * @param PublicSuffix $publicSuffix
+     *
+     * @throws Exception If the domain can not contain a public suffix
+     * @throws Exception If the domain value is the same as the public suffix value
+     * @throws Exception If the domain can not be match with the public suffix
      *
      * @return PublicSuffix
      */
-    private function setPublicSuffix(PublicSuffix $publicSuffix = null): PublicSuffix
+    private function setPublicSuffix(PublicSuffix $publicSuffix): PublicSuffix
     {
-        if (null === $publicSuffix
-            || null === $this->domain
-            || false === strpos($this->domain, '.')
-            || count($this->labels) === count($publicSuffix)
-        ) {
+        if (null === $publicSuffix->getContent()) {
             return new PublicSuffix();
         }
 
-        return $publicSuffix;
-    }
+        if (null === $this->domain || false === strpos($this->domain, '.')) {
+            throw new Exception(sprintf('The domain `%s` can not contain a public suffix', $this->domain));
+        }
 
-    /**
-     * assert the domain internal state is valid
-     *
-     * @throws Exception if the public suffix does not match the domain
-     */
-    protected function assertValidState()
-    {
-        foreach ($this->publicSuffix as $offset => $label) {
-            if ($label !== $this->labels[$offset]) {
-                throw new Exception(sprintf('The public suffix `%s` is invalid for the domain `%s`', $this->publicSuffix->getContent(), $this->domain));
-            }
+        static $pattern = '/[^\x20-\x7f]/';
+        if (preg_match($pattern, $this->domain)) {
+            $publicSuffix = $publicSuffix->toUnicode();
+        }
+
+        $publicSuffixContent = $publicSuffix->getContent();
+        if ($this->domain === $publicSuffixContent) {
+            throw new Exception(sprintf('The public suffix `%s` can not be equal to the domain name `%s`', $publicSuffixContent, $this->domain));
+        }
+
+        if ('.'.$publicSuffixContent !== substr($this->domain, - strlen($publicSuffixContent) - 1)) {
+            throw new Exception(sprintf('The public suffix `%s` can not be assign to the domain name `%s`', $publicSuffixContent, $this->domain));
         }
+
+        return $publicSuffix;
     }
 
     /**
@@ -350,9 +353,6 @@ public function toUnicode()
      *
      * @param PublicSuffix $publicSuffix
      *
-     * @throws Exception if the domain can not contain a public suffix
-     * @throws Exception if the public suffix can not be assign to the domain name
-     *
      * @return self
      */
     public function withPublicSuffix(PublicSuffix $publicSuffix): self
@@ -361,31 +361,8 @@ public function withPublicSuffix(PublicSuffix $publicSuffix): self
             return $this;
         }
 
-        if (null === $publicSuffix->getContent()) {
-            $clone = clone $this;
-            $clone->publicSuffix = $publicSuffix;
-            $clone->registrableDomain = $clone->setRegistrableDomain();
-            $clone->subDomain = $clone->setSubDomain();
-
-            return $clone;
-        }
-
-        if (null === $this->domain || false === strpos($this->domain, '.')) {
-            throw new Exception(sprintf('The domain `%s` can not contain a public suffix', $this->domain));
-        }
-
-        static $pattern = '/[^\x20-\x7f]/';
-        if (preg_match($pattern, $this->domain)) {
-            $publicSuffix = $publicSuffix->toUnicode();
-        }
-
-        $publicSuffixContent = $publicSuffix->getContent();
-        if ($this->domain === $publicSuffixContent || '.'.$publicSuffixContent !== substr($this->domain, - strlen($publicSuffixContent) - 1)) {
-            throw new Exception(sprintf('the public suffix `%s` can not be assign to the domain name `%s`', $publicSuffixContent, $this->domain));
-        }
-
         $clone = clone $this;
-        $clone->publicSuffix = $publicSuffix;
+        $clone->publicSuffix = $clone->setPublicSuffix($publicSuffix);
         $clone->registrableDomain = $clone->setRegistrableDomain();
         $clone->subDomain = $clone->setSubDomain();
 

tests/DomainTest.php

@@ -16,46 +16,56 @@
 class DomainTest extends TestCase
 {
     /**
-     * @dataProvider invalidRegistrableDomainProvider
-     *
-     * @param string|null $domain
-     * @param string|null $publicSuffix
-     *
      * @covers ::__construct
      * @covers ::setPublicSuffix
      * @covers ::setRegistrableDomain
      * @covers ::setSubDomain
-     * @covers ::assertValidState
      * @covers ::getPublicSuffix
      * @covers ::getRegistrableDomain
      * @covers ::getSubDomain
      */
-    public function testRegistrableDomainIsNullWithFoundDomain($domain, $publicSuffix)
+    public function testRegistrableDomainIsNullWithFoundDomain()
     {
-        $domain = new Domain($domain, new PublicSuffix($publicSuffix));
+        $domain = new Domain('faketld', null);
         $this->assertNull($domain->getPublicSuffix());
         $this->assertNull($domain->getRegistrableDomain());
         $this->assertNull($domain->getSubDomain());
     }
 
-    public function invalidRegistrableDomainProvider()
-    {
-        return [
-            'domain and suffix are the same' => ['co.uk', 'co.uk'],
-            'domain has no labels' => ['faketld', 'faketld'],
-            'public suffix is null' => ['faketld', null],
-            'domain is null' => [null, 'faketld'],
-        ];
-    }
-
     /**
      * @covers ::__construct
-     * @covers ::assertValidState
+     * @covers ::setPublicSuffix
+     *
+     * @dataProvider provideWrongConstructor
+     * @param mixed $domain
+     * @param mixed $publicSuffix
      */
-    public function testConstructorThrowsExceptionOnMisMatchPublicSuffixDomain()
+    public function testConstructorThrowsExceptionOnMisMatchPublicSuffixDomain($domain, $publicSuffix)
     {
         $this->expectException(Exception::class);
-        new Domain('www.ulb.ac.be', new PublicSuffix('com'));
+        new Domain($domain, new PublicSuffix($publicSuffix));
+    }
+
+    public function provideWrongConstructor()
+    {
+        return [
+            'public suffix mismatch' => [
+                'domain' => 'www.ulb.ac.be',
+                'publicSuffix' => 'com',
+            ],
+            'domain and public suffix are the same' => [
+                'domain' => 'co.uk',
+                'publicSuffix' => 'co.uk',
+            ],
+            'domain has no labels' => [
+                'domain' => 'localhost',
+                'publicSuffix' => 'localhost',
+            ],
+            'domain is null' => [
+                'domain' => null,
+                'publicSuffix' => 'com',
+            ],
+        ];
     }
 
     /**
@@ -159,7 +169,6 @@ public function testOffsets()
      *
      * @covers ::setDomain
      * @covers ::setPublicSuffix
-     * @covers ::assertValidState
      * @covers ::setRegistrableDomain
      * @covers ::setSubDomain
      * @covers ::getDomain
@@ -259,7 +268,6 @@ public function toUnicodeProvider()
      *
      * @covers ::setDomain
      * @covers ::setPublicSuffix
-     * @covers ::assertValidState
      * @covers ::setRegistrableDomain
      * @covers ::setSubDomain
      * @covers ::getDomain
@@ -414,6 +422,10 @@ public function withPublicSuffixFailsProvider()
                 'domain' => $domain,
                 'public suffix' => new PublicSuffix('c.be'),
             ],
+            'mismatch idn public suffix' => [
+                'domain' => new Domain('www.食狮.公司.cn'),
+                'public suffix' => new PublicSuffix('cn.公司'),
+            ],
         ];
     }