Validate language & preview picture fields

Instead of saving the value of each field right into the content without any validation, it seems better to validate them.
This might sounds obvious now we say that.

diff --git a/src/Wallabag/CoreBundle/Helper/ContentProxy.php b/src/Wallabag/CoreBundle/Helper/ContentProxy.php
index d5820e6633e2250f1a53c7554046f5fc238a781f..dd9170ad2bb5c1eb4a50c3ecdaead15c5579026e 100644 (file)
--- a/src/Wallabag/CoreBundle/Helper/ContentProxy.php
+++ b/src/Wallabag/CoreBundle/Helper/ContentProxy.php
@@ -7,6 +7,9 @@ use Psr\Log\LoggerInterface;
 use Wallabag\CoreBundle\Entity\Entry;
 use Wallabag\CoreBundle\Tools\Utils;
 use Symfony\Component\HttpFoundation\File\MimeType\MimeTypeExtensionGuesser;
 use Wallabag\CoreBundle\Entity\Entry;
 use Wallabag\CoreBundle\Tools\Utils;
 use Symfony\Component\HttpFoundation\File\MimeType\MimeTypeExtensionGuesser;

+use Symfony\Component\Validator\Constraints\Language as LanguageConstraint;
+use Symfony\Component\Validator\Constraints\Url as UrlConstraint;
+use Symfony\Component\Validator\Validator\ValidatorInterface;

 
 /**
  * This kind of proxy class take care of getting the content from an url
 
 /**
  * This kind of proxy class take care of getting the content from an url


@@ -21,10 +24,11 @@ class ContentProxy
     protected $fetchingErrorMessage;
     protected $eventDispatcher;
 
     protected $fetchingErrorMessage;
     protected $eventDispatcher;
 

-    public function __construct(Graby $graby, RuleBasedTagger $tagger, LoggerInterface $logger, $fetchingErrorMessage)
+    public function __construct(Graby $graby, RuleBasedTagger $tagger, ValidatorInterface $validator, LoggerInterface $logger, $fetchingErrorMessage)

     {
         $this->graby = $graby;
         $this->tagger = $tagger;
     {
         $this->graby = $graby;
         $this->tagger = $tagger;

+        $this->validator = $validator;

         $this->logger = $logger;
         $this->mimeGuesser = new MimeTypeExtensionGuesser();
         $this->fetchingErrorMessage = $fetchingErrorMessage;
         $this->logger = $logger;
         $this->mimeGuesser = new MimeTypeExtensionGuesser();
         $this->fetchingErrorMessage = $fetchingErrorMessage;


@@ -113,7 +117,24 @@ class ContentProxy
             $entry->setHeaders($content['all_headers']);
         }
 
             $entry->setHeaders($content['all_headers']);
         }
 

-        $entry->setLanguage(isset($content['language']) ? $content['language'] : '');
+        $this->validateAndSetLanguage(
+            $entry,
+            isset($content['language']) ? $content['language'] : ''
+        );
+
+        $this->validateAndSetPreviewPicture(
+            $entry,
+            isset($content['open_graph']['og_image']) ? $content['open_graph']['og_image'] : ''
+        );
+
+        // if content is an image define as a preview too
+        if (!empty($content['content_type']) && in_array($this->mimeGuesser->guess($content['content_type']), ['jpeg', 'jpg', 'gif', 'png'], true)) {
+            $this->validateAndSetPreviewPicture(
+                $entry,
+                $content['url']
+            );
+        }
+

         $entry->setMimetype(isset($content['content_type']) ? $content['content_type'] : '');
         $entry->setReadingTime(Utils::getReadingTime($html));
 
         $entry->setMimetype(isset($content['content_type']) ? $content['content_type'] : '');
         $entry->setReadingTime(Utils::getReadingTime($html));
 


@@ -122,15 +143,6 @@ class ContentProxy
             $entry->setDomainName($domainName);
         }
 
             $entry->setDomainName($domainName);
         }
 

-        if (!empty($content['open_graph']['og_image'])) {
-            $entry->setPreviewPicture($content['open_graph']['og_image']);
-        }
-
-        // if content is an image define as a preview too
-        if (!empty($content['content_type']) && in_array($this->mimeGuesser->guess($content['content_type']), ['jpeg', 'jpg', 'gif', 'png'], true)) {
-            $entry->setPreviewPicture($content['url']);
-        }
-

         try {
             $this->tagger->tag($entry);
         } catch (\Exception $e) {
         try {
             $this->tagger->tag($entry);
         } catch (\Exception $e) {


@@ -152,4 +164,48 @@ class ContentProxy
     {
         return !empty($content['title']) && !empty($content['html']) && !empty($content['url']);
     }
     {
         return !empty($content['title']) && !empty($content['html']) && !empty($content['url']);
     }

+
+    /**
+     * Use a Symfony validator to ensure the language is well formatted.
+     *
+     * @param Entry  $entry
+     * @param string $value Language to validate
+     */
+    private function validateAndSetLanguage($entry, $value)
+    {
+        $errors = $this->validator->validate(
+            $value,
+            (new LanguageConstraint())
+        );
+
+        if (0 === count($errors)) {
+            $entry->setLanguage($value);
+
+            return;
+        }
+
+        $this->logger->warning('Language validation failed. '.(string) $errors);
+    }
+
+    /**
+     * Use a Symfony validator to ensure the preview picture is a real url.
+     *
+     * @param Entry  $entry
+     * @param string $value URL to validate
+     */
+    private function validateAndSetPreviewPicture($entry, $value)
+    {
+        $errors = $this->validator->validate(
+            $value,
+            (new UrlConstraint())
+        );
+
+        if (0 === count($errors)) {
+            $entry->setPreviewPicture($value);
+
+            return;
+        }
+
+        $this->logger->warning('PreviewPicture validation failed. '.(string) $errors);
+    }

 }
 }

diff --git a/src/Wallabag/CoreBundle/Resources/config/services.yml b/src/Wallabag/CoreBundle/Resources/config/services.yml
index a9b0d2d564a5dc7fcdcd55d99105ae19b5694295..2ae5d27f2fbd2166ab9d5913b5e7a79163e8250e 100644 (file)
--- a/src/Wallabag/CoreBundle/Resources/config/services.yml
+++ b/src/Wallabag/CoreBundle/Resources/config/services.yml
@@ -90,6 +90,7 @@ services:
         arguments:
             - "@wallabag_core.graby"
             - "@wallabag_core.rule_based_tagger"
         arguments:
             - "@wallabag_core.graby"
             - "@wallabag_core.rule_based_tagger"

+            - "@validator"

             - "@logger"
             - '%wallabag_core.fetching_error_message%'
 
             - "@logger"
             - '%wallabag_core.fetching_error_message%'
 

diff --git a/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php b/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php
index a3570125e11e39fefcd11a53aac334a9f7d9956b..95dd75ba8c3ebe9629f92ac06defe38b2a1a4bf9 100644 (file)
--- a/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php
+++ b/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php
@@ -11,6 +11,9 @@ use Wallabag\CoreBundle\Entity\Tag;
 use Wallabag\UserBundle\Entity\User;
 use Wallabag\CoreBundle\Helper\RuleBasedTagger;
 use Graby\Graby;
 use Wallabag\UserBundle\Entity\User;
 use Wallabag\CoreBundle\Helper\RuleBasedTagger;
 use Graby\Graby;

+use Symfony\Component\Validator\Validator\RecursiveValidator;
+use Symfony\Component\Validator\ConstraintViolationList;
+use Symfony\Component\Validator\ConstraintViolation;

 
 class ContentProxyTest extends \PHPUnit_Framework_TestCase
 {
 
 class ContentProxyTest extends \PHPUnit_Framework_TestCase
 {


@@ -37,7 +40,7 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase
                 'language' => '',
             ]);
 
                 'language' => '',
             ]);
 

-        $proxy = new ContentProxy($graby, $tagger, $this->getLogger(), $this->fetchingErrorMessage);
+        $proxy = new ContentProxy($graby, $tagger, $this->getValidator(), $this->getLogger(), $this->fetchingErrorMessage);

         $entry = new Entry(new User());
         $proxy->updateEntry($entry, 'http://user@:80');
 
         $entry = new Entry(new User());
         $proxy->updateEntry($entry, 'http://user@:80');
 


@@ -72,7 +75,7 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase
                 'language' => '',
             ]);
 
                 'language' => '',
             ]);
 

-        $proxy = new ContentProxy($graby, $tagger, $this->getLogger(), $this->fetchingErrorMessage);
+        $proxy = new ContentProxy($graby, $tagger, $this->getValidator(), $this->getLogger(), $this->fetchingErrorMessage);

         $entry = new Entry(new User());
         $proxy->updateEntry($entry, 'http://0.0.0.0');
 
         $entry = new Entry(new User());
         $proxy->updateEntry($entry, 'http://0.0.0.0');
 


@@ -112,7 +115,7 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase
                 ],
             ]);
 
                 ],
             ]);
 

-        $proxy = new ContentProxy($graby, $tagger, $this->getLogger(), $this->fetchingErrorMessage);
+        $proxy = new ContentProxy($graby, $tagger, $this->getValidator(), $this->getLogger(), $this->fetchingErrorMessage);

         $entry = new Entry(new User());
         $proxy->updateEntry($entry, 'http://domain.io');
 
         $entry = new Entry(new User());
         $proxy->updateEntry($entry, 'http://domain.io');
 


@@ -154,7 +157,7 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase
                 ],
             ]);
 
                 ],
             ]);
 

-        $proxy = new ContentProxy($graby, $tagger, $this->getLogger(), $this->fetchingErrorMessage);
+        $proxy = new ContentProxy($graby, $tagger, $this->getValidator(), $this->getLogger(), $this->fetchingErrorMessage);

         $entry = new Entry(new User());
         $proxy->updateEntry($entry, 'http://0.0.0.0');
 
         $entry = new Entry(new User());
         $proxy->updateEntry($entry, 'http://0.0.0.0');
 


@@ -192,18 +195,112 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase
                 'open_graph' => [
                     'og_title' => 'my OG title',
                     'og_description' => 'OG desc',
                 'open_graph' => [
                     'og_title' => 'my OG title',
                     'og_description' => 'OG desc',

-                    'og_image' => false,
+                    'og_image' => null,

                 ],
             ]);
 
                 ],
             ]);
 

-        $proxy = new ContentProxy($graby, $tagger, $this->getLogger(), $this->fetchingErrorMessage);
+        $proxy = new ContentProxy($graby, $tagger, $this->getValidator(), $this->getLogger(), $this->fetchingErrorMessage);

         $entry = new Entry(new User());
         $proxy->updateEntry($entry, 'http://0.0.0.0');
 
         $this->assertEquals('http://1.1.1.1', $entry->getUrl());
         $this->assertEquals('this is my title', $entry->getTitle());
         $this->assertContains('this is my content', $entry->getContent());
         $entry = new Entry(new User());
         $proxy->updateEntry($entry, 'http://0.0.0.0');
 
         $this->assertEquals('http://1.1.1.1', $entry->getUrl());
         $this->assertEquals('this is my title', $entry->getTitle());
         $this->assertContains('this is my content', $entry->getContent());

-        $this->assertNull($entry->getPreviewPicture());
+        $this->assertEmpty($entry->getPreviewPicture());
+        $this->assertEquals('text/html', $entry->getMimetype());
+        $this->assertEquals('fr', $entry->getLanguage());
+        $this->assertEquals('200', $entry->getHttpStatus());
+        $this->assertEquals(4.0, $entry->getReadingTime());
+        $this->assertEquals('1.1.1.1', $entry->getDomainName());
+    }
+
+    public function testWithContentAndBadLanguage()
+    {
+        $tagger = $this->getTaggerMock();
+        $tagger->expects($this->once())
+            ->method('tag');
+
+        $validator = $this->getValidator();
+        $validator->expects($this->exactly(2))
+            ->method('validate')
+            ->will($this->onConsecutiveCalls(
+                new ConstraintViolationList([new ConstraintViolation('oops', 'oops', [], 'oops', 'language', 'dontexist')]),
+                new ConstraintViolationList()
+            ));
+
+        $graby = $this->getMockBuilder('Graby\Graby')
+            ->setMethods(['fetchContent'])
+            ->disableOriginalConstructor()
+            ->getMock();
+
+        $graby->expects($this->any())
+            ->method('fetchContent')
+            ->willReturn([
+                'html' => str_repeat('this is my content', 325),
+                'title' => 'this is my title',
+                'url' => 'http://1.1.1.1',
+                'content_type' => 'text/html',
+                'language' => 'dontexist',
+                'status' => '200',
+            ]);
+
+        $proxy = new ContentProxy($graby, $tagger, $validator, $this->getLogger(), $this->fetchingErrorMessage);
+        $entry = new Entry(new User());
+        $proxy->updateEntry($entry, 'http://0.0.0.0');
+
+        $this->assertEquals('http://1.1.1.1', $entry->getUrl());
+        $this->assertEquals('this is my title', $entry->getTitle());
+        $this->assertContains('this is my content', $entry->getContent());
+        $this->assertEquals('text/html', $entry->getMimetype());
+        $this->assertEmpty($entry->getLanguage());
+        $this->assertEquals('200', $entry->getHttpStatus());
+        $this->assertEquals(4.0, $entry->getReadingTime());
+        $this->assertEquals('1.1.1.1', $entry->getDomainName());
+    }
+
+    public function testWithContentAndBadOgImage()
+    {
+        $tagger = $this->getTaggerMock();
+        $tagger->expects($this->once())
+            ->method('tag');
+
+        $validator = $this->getValidator();
+        $validator->expects($this->exactly(2))
+            ->method('validate')
+            ->will($this->onConsecutiveCalls(
+                new ConstraintViolationList(),
+                new ConstraintViolationList([new ConstraintViolation('oops', 'oops', [], 'oops', 'url', 'https://')])
+            ));
+
+        $graby = $this->getMockBuilder('Graby\Graby')
+            ->setMethods(['fetchContent'])
+            ->disableOriginalConstructor()
+            ->getMock();
+
+        $graby->expects($this->any())
+            ->method('fetchContent')
+            ->willReturn([
+                'html' => str_repeat('this is my content', 325),
+                'title' => 'this is my title',
+                'url' => 'http://1.1.1.1',
+                'content_type' => 'text/html',
+                'language' => 'fr',
+                'status' => '200',
+                'open_graph' => [
+                    'og_title' => 'my OG title',
+                    'og_description' => 'OG desc',
+                    'og_image' => 'https://',
+                ],
+            ]);
+
+        $proxy = new ContentProxy($graby, $tagger, $validator, $this->getLogger(), $this->fetchingErrorMessage);
+        $entry = new Entry(new User());
+        $proxy->updateEntry($entry, 'http://0.0.0.0');
+
+        $this->assertEquals('http://1.1.1.1', $entry->getUrl());
+        $this->assertEquals('this is my title', $entry->getTitle());
+        $this->assertContains('this is my content', $entry->getContent());
+        $this->assertEmpty($entry->getPreviewPicture());

         $this->assertEquals('text/html', $entry->getMimetype());
         $this->assertEquals('fr', $entry->getLanguage());
         $this->assertEquals('200', $entry->getHttpStatus());
         $this->assertEquals('text/html', $entry->getMimetype());
         $this->assertEquals('fr', $entry->getLanguage());
         $this->assertEquals('200', $entry->getHttpStatus());


@@ -217,7 +314,7 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase
         $tagger->expects($this->once())
             ->method('tag');
 
         $tagger->expects($this->once())
             ->method('tag');
 

-        $proxy = new ContentProxy((new Graby()), $tagger, $this->getLogger(), $this->fetchingErrorMessage);
+        $proxy = new ContentProxy((new Graby()), $tagger, $this->getValidator(), $this->getLogger(), $this->fetchingErrorMessage);

         $entry = new Entry(new User());
         $proxy->updateEntry(
             $entry,
         $entry = new Entry(new User());
         $proxy->updateEntry(
             $entry,


@@ -259,7 +356,7 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase
         $logHandler = new TestHandler();
         $logger = new Logger('test', [$logHandler]);
 
         $logHandler = new TestHandler();
         $logger = new Logger('test', [$logHandler]);
 

-        $proxy = new ContentProxy((new Graby()), $tagger, $logger, $this->fetchingErrorMessage);
+        $proxy = new ContentProxy((new Graby()), $tagger, $this->getValidator(), $logger, $this->fetchingErrorMessage);

         $entry = new Entry(new User());
         $proxy->updateEntry(
             $entry,
         $entry = new Entry(new User());
         $proxy->updateEntry(
             $entry,


@@ -294,7 +391,7 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase
         $handler = new TestHandler();
         $logger->pushHandler($handler);
 
         $handler = new TestHandler();
         $logger->pushHandler($handler);
 

-        $proxy = new ContentProxy((new Graby()), $tagger, $logger, $this->fetchingErrorMessage);
+        $proxy = new ContentProxy((new Graby()), $tagger, $this->getValidator(), $logger, $this->fetchingErrorMessage);

         $entry = new Entry(new User());
         $proxy->updateEntry(
             $entry,
         $entry = new Entry(new User());
         $proxy->updateEntry(
             $entry,


@@ -331,7 +428,7 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase
             ->method('tag')
             ->will($this->throwException(new \Exception()));
 
             ->method('tag')
             ->will($this->throwException(new \Exception()));
 

-        $proxy = new ContentProxy((new Graby()), $tagger, $this->getLogger(), $this->fetchingErrorMessage);
+        $proxy = new ContentProxy((new Graby()), $tagger, $this->getValidator(), $this->getLogger(), $this->fetchingErrorMessage);

         $entry = new Entry(new User());
         $proxy->updateEntry(
             $entry,
         $entry = new Entry(new User());
         $proxy->updateEntry(
             $entry,


@@ -371,7 +468,7 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase
         $tagger->expects($this->once())
             ->method('tag');
 
         $tagger->expects($this->once())
             ->method('tag');
 

-        $proxy = new ContentProxy((new Graby()), $tagger, $this->getLogger(), $this->fetchingErrorMessage);
+        $proxy = new ContentProxy((new Graby()), $tagger, $this->getValidator(), $this->getLogger(), $this->fetchingErrorMessage);

         $entry = new Entry(new User());
         $proxy->updateEntry(
             $entry,
         $entry = new Entry(new User());
         $proxy->updateEntry(
             $entry,


@@ -413,4 +510,12 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase
     {
         return new NullLogger();
     }
     {
         return new NullLogger();
     }

+
+    private function getValidator()
+    {
+        return $this->getMockBuilder(RecursiveValidator::class)
+            ->setMethods(['validate'])
+            ->disableOriginalConstructor()
+            ->getMock();
+    }

 }
 }