diff --git a/src/Wallabag/CoreBundle/Helper/ContentProxy.php b/src/Wallabag/CoreBundle/Helper/ContentProxy.php index d5820e663..dd9170ad2 100644 --- 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); + } } diff --git a/src/Wallabag/CoreBundle/Resources/config/services.yml b/src/Wallabag/CoreBundle/Resources/config/services.yml index a9b0d2d56..2ae5d27f2 100644 --- 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%' diff --git a/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php b/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php index a3570125e..95dd75ba8 100644 --- 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(); + } }