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.
2024-11-26 11:01:04 +00:00 · 2017-06-08 21:51:46 +02:00 · 2017-06-08 21:51:46 +02:00 · 0d349ea670
commit 0d349ea670
parent 3f474025d8
3 changed files with 185 additions and 23 deletions
--- 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 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
@ -21,10 +24,11 @@ class ContentProxy
    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->validator = $validator;
        $this->logger = $logger;
        $this->mimeGuesser = new MimeTypeExtensionGuesser();
        $this->fetchingErrorMessage = $fetchingErrorMessage;
@ -113,7 +117,24 @@ class ContentProxy
            $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));

@ -122,15 +143,6 @@ class ContentProxy
            $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) {
@ -152,4 +164,48 @@ class ContentProxy
    {
        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);
+    }
 }
--- 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"
+            - "@validator"
            - "@logger"
            - '%wallabag_core.fetching_error_message%'

--- 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 Symfony\Component\Validator\Validator\RecursiveValidator;
+use Symfony\Component\Validator\ConstraintViolationList;
+use Symfony\Component\Validator\ConstraintViolation;

 class ContentProxyTest extends \PHPUnit_Framework_TestCase
 {
@ -37,7 +40,7 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase
                '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');

@ -72,7 +75,7 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase
                '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');

@ -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');

@ -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');

@ -192,18 +195,112 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase
                '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());
-        $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());
@ -217,7 +314,7 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase
        $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,
@ -259,7 +356,7 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase
        $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,
@ -294,7 +391,7 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase
        $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,
@ -331,7 +428,7 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase
            ->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,
@ -371,7 +468,7 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase
        $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,
@ -413,4 +510,12 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase
    {
        return new NullLogger();
    }
+
+    private function getValidator()
+    {
+        return $this->getMockBuilder(RecursiveValidator::class)
+            ->setMethods(['validate'])
+            ->disableOriginalConstructor()
+            ->getMock();
+    }
 }