From d76a5a6d60b6ee0d1f7efd0c8a70204f821ed99e Mon Sep 17 00:00:00 2001 From: Tobi823 Date: Tue, 18 Sep 2018 15:04:19 +0200 Subject: Bugfix: Sanitize the title of a saved webpage from invalid UTF-8 characters --- src/Wallabag/CoreBundle/Helper/ContentProxy.php | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/Wallabag/CoreBundle/Helper/ContentProxy.php b/src/Wallabag/CoreBundle/Helper/ContentProxy.php index 3fe31c2c..2628af19 100644 --- a/src/Wallabag/CoreBundle/Helper/ContentProxy.php +++ b/src/Wallabag/CoreBundle/Helper/ContentProxy.php @@ -53,6 +53,7 @@ class ContentProxy if ((empty($content) || false === $this->validateContent($content)) && false === $disableContentUpdate) { $fetchedContent = $this->graby->fetchContent($url); + $fetchedContent['title'] = $this->sanitizeUTF8Text($fetchedContent['title']); // when content is imported, we have information in $content // in case fetching content goes bad, we'll keep the imported information instead of overriding them @@ -68,6 +69,28 @@ class ContentProxy $this->stockEntry($entry, $content); } + /** + * Remove invalid UTF-8 characters from the given string in following steps: + * - try to interpret the given string as ISO-8859-1, convert it to UTF-8 and return it (if its valid) + * - simply remove every invalid UTF-8 character and return the result (https://stackoverflow.com/a/1433665) + * @param String $rawText + * @return string + */ + private function sanitizeUTF8Text(String $rawText) { + if (mb_check_encoding($rawText, 'utf-8')) { + return $rawText; // return because its valid utf-8 text + } + + // we assume that $text is encoded in ISO-8859-1 (and not the similar Windows-1252 or other encoding) + $convertedText = utf8_encode($rawText); + if (mb_check_encoding($convertedText, 'utf-8')) { + return $convertedText; + } + + // last resort: simply remove invalid UTF-8 character because $rawText can have some every exotic encoding + return iconv("UTF-8", "UTF-8//IGNORE", $rawText); + } + /** * Use a Symfony validator to ensure the language is well formatted. * -- cgit v1.2.3 From 8648f0c00534e8af83b2a5451269d79906db6c16 Mon Sep 17 00:00:00 2001 From: Tobi823 Date: Wed, 19 Sep 2018 11:03:42 +0200 Subject: Remove type declaration for PHP 5 compatibility --- src/Wallabag/CoreBundle/Helper/ContentProxy.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Wallabag/CoreBundle/Helper/ContentProxy.php b/src/Wallabag/CoreBundle/Helper/ContentProxy.php index 2628af19..29259bbd 100644 --- a/src/Wallabag/CoreBundle/Helper/ContentProxy.php +++ b/src/Wallabag/CoreBundle/Helper/ContentProxy.php @@ -76,7 +76,7 @@ class ContentProxy * @param String $rawText * @return string */ - private function sanitizeUTF8Text(String $rawText) { + private function sanitizeUTF8Text($rawText) { if (mb_check_encoding($rawText, 'utf-8')) { return $rawText; // return because its valid utf-8 text } -- cgit v1.2.3 From f80f16dfc858ec90da76daacd405b0cfdaa32f74 Mon Sep 17 00:00:00 2001 From: Tobi823 Date: Wed, 19 Sep 2018 12:30:26 +0200 Subject: Try to detect the character encoding in PDFs and try to translate the title from the PDF to UTF-8 --- src/Wallabag/CoreBundle/Helper/ContentProxy.php | 46 ++++++++++++++++++------- 1 file changed, 33 insertions(+), 13 deletions(-) diff --git a/src/Wallabag/CoreBundle/Helper/ContentProxy.php b/src/Wallabag/CoreBundle/Helper/ContentProxy.php index 29259bbd..fab05268 100644 --- a/src/Wallabag/CoreBundle/Helper/ContentProxy.php +++ b/src/Wallabag/CoreBundle/Helper/ContentProxy.php @@ -53,7 +53,7 @@ class ContentProxy if ((empty($content) || false === $this->validateContent($content)) && false === $disableContentUpdate) { $fetchedContent = $this->graby->fetchContent($url); - $fetchedContent['title'] = $this->sanitizeUTF8Text($fetchedContent['title']); + $fetchedContent['title'] = $this->sanitizeContentTitle($fetchedContent['title'], $fetchedContent['content_type']); // when content is imported, we have information in $content // in case fetching content goes bad, we'll keep the imported information instead of overriding them @@ -70,24 +70,44 @@ class ContentProxy } /** - * Remove invalid UTF-8 characters from the given string in following steps: - * - try to interpret the given string as ISO-8859-1, convert it to UTF-8 and return it (if its valid) - * - simply remove every invalid UTF-8 character and return the result (https://stackoverflow.com/a/1433665) - * @param String $rawText + * Try to sanitize the title of the fetched content from wrong character encodings and invalid UTF-8 character. + * @param $title + * @param $contentType * @return string */ - private function sanitizeUTF8Text($rawText) { - if (mb_check_encoding($rawText, 'utf-8')) { - return $rawText; // return because its valid utf-8 text + private function sanitizeContentTitle($title, $contentType) { + if ('application/pdf' === $contentType) { + $convertedTitle = $this->convertPdfEncodingToUTF8($title); + return $this->sanitizeUTF8Text($convertedTitle); } + return $this->sanitizeUTF8Text($title); + } - // we assume that $text is encoded in ISO-8859-1 (and not the similar Windows-1252 or other encoding) - $convertedText = utf8_encode($rawText); - if (mb_check_encoding($convertedText, 'utf-8')) { - return $convertedText; + /** + * If the title from the fetched content comes from a PDF, then its very possible that the character encoding is not + * UTF-8. This methods tries to identify the character encoding and translate the title to UTF-8. + * @param $title + * @return string (maybe contains invalid UTF-8 character) + */ + private function convertPdfEncodingToUTF8($title) { + // first try UTF-16 (then UTF-8) because its easier to detect its present/absence + foreach (array('UTF-16BE', 'UTF-16LE', 'UTF-8', 'WINDOWS-1252') as $encoding) { + if (mb_check_encoding($title, $encoding)) { + return mb_convert_encoding($title, 'UTF-8', $encoding); + } } + return $title; + } - // last resort: simply remove invalid UTF-8 character because $rawText can have some every exotic encoding + /** + * Remove invalid UTF-8 characters from the given string. + * @param String $rawText + * @return string + */ + private function sanitizeUTF8Text($rawText) { + if (mb_check_encoding($rawText, 'UTF-8')) { + return $rawText; + } return iconv("UTF-8", "UTF-8//IGNORE", $rawText); } -- cgit v1.2.3 From c01d9532920ec5a298bb347dbb83a078d36d4841 Mon Sep 17 00:00:00 2001 From: Tobi823 Date: Wed, 19 Sep 2018 13:59:07 +0200 Subject: Add tests for logic Try to translate the title of a PDF from UTF-8 (then UTF-16BE, then WINDOWS-1252) to UTF-8 --- src/Wallabag/CoreBundle/Helper/ContentProxy.php | 4 +- .../CoreBundle/Helper/ContentProxyTest.php | 236 +++++++++++++++++++++ 2 files changed, 238 insertions(+), 2 deletions(-) diff --git a/src/Wallabag/CoreBundle/Helper/ContentProxy.php b/src/Wallabag/CoreBundle/Helper/ContentProxy.php index fab05268..50090100 100644 --- a/src/Wallabag/CoreBundle/Helper/ContentProxy.php +++ b/src/Wallabag/CoreBundle/Helper/ContentProxy.php @@ -90,8 +90,8 @@ class ContentProxy * @return string (maybe contains invalid UTF-8 character) */ private function convertPdfEncodingToUTF8($title) { - // first try UTF-16 (then UTF-8) because its easier to detect its present/absence - foreach (array('UTF-16BE', 'UTF-16LE', 'UTF-8', 'WINDOWS-1252') as $encoding) { + // first try UTF-8 because its easier to detect its present/absence + foreach (array('UTF-8', 'UTF-16BE', 'WINDOWS-1252') as $encoding) { if (mb_check_encoding($title, $encoding)) { return mb_convert_encoding($title, 'UTF-8', $encoding); } diff --git a/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php b/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php index 51df8de1..9d8098ef 100644 --- a/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php +++ b/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php @@ -531,6 +531,242 @@ class ContentProxyTest extends TestCase $this->assertSame('1.1.1.1', $entry->getDomainName()); } + public function testWebsiteWithValidUTF8Title_doNothing() + { + // You can use https://www.online-toolz.com/tools/text-hex-convertor.php to convert UTF-8 text <=> hex + // See http://graphemica.com for more info about the characters + // '😻ℤz' (U+1F63B or F09F98BB; U+2124 or E284A4; U+007A or 7A) in hexadecimal and UTF-8 + $actualTitle = $this->hexToStr('F09F98BB' . 'E284A4' . '7A'); + + $tagger = $this->getTaggerMock(); + $tagger->expects($this->once()) + ->method('tag'); + + $graby = $this->getMockBuilder('Graby\Graby') + ->setMethods(['fetchContent']) + ->disableOriginalConstructor() + ->getMock(); + + $graby->expects($this->any()) + ->method('fetchContent') + ->willReturn([ + 'html' => false, + 'title' => $actualTitle, + 'url' => '', + 'content_type' => 'text/html', + 'language' => '', + ]); + + $proxy = new ContentProxy($graby, $tagger, $this->getValidator(), $this->getLogger(), $this->fetchingErrorMessage); + $entry = new Entry(new User()); + $proxy->updateEntry($entry, 'http://0.0.0.0'); + + // '😻ℤz' (U+1F63B or F09F98BB; U+2124 or E284A4; U+007A or 7A) in hexadecimal and UTF-8 + $expectedTitle = 'F09F98BB' . 'E284A4' . '7A'; + $this->assertSame($expectedTitle, $this->strToHex($entry->getTitle())); + } + + public function testWebsiteWithInvalidUTF8Title_removeInvalidCharacter() + { + // See http://graphemica.com for more info about the characters + // 'a€b' (61;80;62) in hexadecimal and WINDOWS-1252 - but 80 is a invalid UTF-8 character. + // The correct UTF-8 € character (U+20AC) is E282AC + $actualTitle = $this->hexToStr('61' . '80' . '62'); + + $tagger = $this->getTaggerMock(); + $tagger->expects($this->once()) + ->method('tag'); + + $graby = $this->getMockBuilder('Graby\Graby') + ->setMethods(['fetchContent']) + ->disableOriginalConstructor() + ->getMock(); + + $graby->expects($this->any()) + ->method('fetchContent') + ->willReturn([ + 'html' => false, + 'title' => $actualTitle, + 'url' => '', + 'content_type' => 'text/html', + 'language' => '', + ]); + + $proxy = new ContentProxy($graby, $tagger, $this->getValidator(), $this->getLogger(), $this->fetchingErrorMessage); + $entry = new Entry(new User()); + $proxy->updateEntry($entry, 'http://0.0.0.0'); + + // 'ab' (61;62) because all invalid UTF-8 character (like 80) are removed + $expectedTitle = '61' . '62'; + $this->assertSame($expectedTitle, $this->strToHex($entry->getTitle())); + } + + public function testPdfWithUTF16BETitle_convertToUTF8() + { + // See http://graphemica.com for more info about the characters + // '😻' (U+1F63B;D83DDE3B) in hexadecimal and as UTF16BE + $actualTitle = $this->hexToStr('D83DDE3B'); + + $tagger = $this->getTaggerMock(); + $tagger->expects($this->once()) + ->method('tag'); + + $graby = $this->getMockBuilder('Graby\Graby') + ->setMethods(['fetchContent']) + ->disableOriginalConstructor() + ->getMock(); + + $graby->expects($this->any()) + ->method('fetchContent') + ->willReturn([ + 'html' => false, + 'title' => $actualTitle, + 'url' => '', + 'content_type' => 'application/pdf', + 'language' => '', + ]); + + $proxy = new ContentProxy($graby, $tagger, $this->getValidator(), $this->getLogger(), $this->fetchingErrorMessage); + $entry = new Entry(new User()); + $proxy->updateEntry($entry, 'http://0.0.0.0'); + + // '😻' (U+1F63B or F09F98BB) in hexadecimal and UTF-8 + $expectedTitle = 'F09F98BB'; + $this->assertSame($expectedTitle, $this->strToHex($entry->getTitle())); + } + + public function testPdfWithUTF8Title_doNothing() + { + // See http://graphemica.com for more info about the characters + // '😻' (U+1F63B;D83DDE3B) in hexadecimal and as UTF8 + $actualTitle = $this->hexToStr('F09F98BB'); + + $tagger = $this->getTaggerMock(); + $tagger->expects($this->once()) + ->method('tag'); + + $graby = $this->getMockBuilder('Graby\Graby') + ->setMethods(['fetchContent']) + ->disableOriginalConstructor() + ->getMock(); + + $graby->expects($this->any()) + ->method('fetchContent') + ->willReturn([ + 'html' => false, + 'title' => $actualTitle, + 'url' => '', + 'content_type' => 'application/pdf', + 'language' => '', + ]); + + $proxy = new ContentProxy($graby, $tagger, $this->getValidator(), $this->getLogger(), $this->fetchingErrorMessage); + $entry = new Entry(new User()); + $proxy->updateEntry($entry, 'http://0.0.0.0'); + + // '😻' (U+1F63B or F09F98BB) in hexadecimal and UTF-8 + $expectedTitle = 'F09F98BB'; + $this->assertSame($expectedTitle, $this->strToHex($entry->getTitle())); + } + + public function testPdfWithWINDOWS1252Title_convertToUTF8() + { + // See http://graphemica.com for more info about the characters + // '€' (80) in hexadecimal and WINDOWS-1252 + $actualTitle = $this->hexToStr('80'); + + $tagger = $this->getTaggerMock(); + $tagger->expects($this->once()) + ->method('tag'); + + $graby = $this->getMockBuilder('Graby\Graby') + ->setMethods(['fetchContent']) + ->disableOriginalConstructor() + ->getMock(); + + $graby->expects($this->any()) + ->method('fetchContent') + ->willReturn([ + 'html' => false, + 'title' => $actualTitle, + 'url' => '', + 'content_type' => 'application/pdf', + 'language' => '', + ]); + + $proxy = new ContentProxy($graby, $tagger, $this->getValidator(), $this->getLogger(), $this->fetchingErrorMessage); + $entry = new Entry(new User()); + $proxy->updateEntry($entry, 'http://0.0.0.0'); + + // '€' (U+20AC or E282AC) in hexadecimal and UTF-8 + $expectedTitle = 'E282AC'; + $this->assertSame($expectedTitle, $this->strToHex($entry->getTitle())); + } + + public function testPdfWithInvalidCharacterInTitle_removeInvalidCharacter() + { + // See http://graphemica.com for more info about the characters + // '😻ℤ�z' (U+1F63B or F09F98BB; U+2124 or E284A4; invalid character 81; U+007A or 7A) in hexadecimal and UTF-8 + // 0x81 is not a valid character for UTF16, UTF8 and WINDOWS-1252 + $actualTitle = $this->hexToStr('F09F98BB' . 'E284A4' . '81' . '7A'); + + $tagger = $this->getTaggerMock(); + $tagger->expects($this->once()) + ->method('tag'); + + $graby = $this->getMockBuilder('Graby\Graby') + ->setMethods(['fetchContent']) + ->disableOriginalConstructor() + ->getMock(); + + $graby->expects($this->any()) + ->method('fetchContent') + ->willReturn([ + 'html' => false, + 'title' => $actualTitle, + 'url' => '', + 'content_type' => 'application/pdf', + 'language' => '', + ]); + + $proxy = new ContentProxy($graby, $tagger, $this->getValidator(), $this->getLogger(), $this->fetchingErrorMessage); + $entry = new Entry(new User()); + $proxy->updateEntry($entry, 'http://0.0.0.0'); + + // '😻ℤz' (U+1F63B or F09F98BB; U+2124 or E284A4; U+007A or 7A) in hexadecimal and UTF-8 + // the 0x81 (represented by �) is invalid for UTF16, UTF8 and WINDOWS-1252 and is removed + $expectedTitle = 'F09F98BB' . 'E284A4' . '7A'; + $this->assertSame($expectedTitle, $this->strToHex($entry->getTitle())); + } + + /** + * https://stackoverflow.com/a/18506801 + * @param $string + * @return string + */ + function strToHex($string){ + $hex = ''; + for ($i=0; $igetMockBuilder(RuleBasedTagger::class) -- cgit v1.2.3 From 7a65c2017bf4dd47414df27d0a07829580392c96 Mon Sep 17 00:00:00 2001 From: Tobi823 Date: Fri, 21 Sep 2018 13:23:39 +0200 Subject: Override the value of the given parameter ($title) with the (hopefully) correct (to UTF-8) converted PDF title --- src/Wallabag/CoreBundle/Helper/ContentProxy.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Wallabag/CoreBundle/Helper/ContentProxy.php b/src/Wallabag/CoreBundle/Helper/ContentProxy.php index 50090100..ce82f6bc 100644 --- a/src/Wallabag/CoreBundle/Helper/ContentProxy.php +++ b/src/Wallabag/CoreBundle/Helper/ContentProxy.php @@ -77,8 +77,7 @@ class ContentProxy */ private function sanitizeContentTitle($title, $contentType) { if ('application/pdf' === $contentType) { - $convertedTitle = $this->convertPdfEncodingToUTF8($title); - return $this->sanitizeUTF8Text($convertedTitle); + $title = $this->convertPdfEncodingToUTF8($title); } return $this->sanitizeUTF8Text($title); } -- cgit v1.2.3 From d64139d8123ac88c8ba1b427c3ee3637b6ea1c96 Mon Sep 17 00:00:00 2001 From: Tobi823 Date: Fri, 21 Sep 2018 13:31:28 +0200 Subject: Make helper methods strToHex and hexToStr in ContentProxyTest.php private to prevent misusage (from outside this class) --- tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php b/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php index 9d8098ef..5f10f482 100644 --- a/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php +++ b/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php @@ -744,7 +744,7 @@ class ContentProxyTest extends TestCase * @param $string * @return string */ - function strToHex($string){ + private function strToHex($string){ $hex = ''; for ($i=0; $i Date: Sun, 23 Sep 2018 22:20:43 +0200 Subject: Run php-cs-fixer for fixing coding standard issues --- src/Wallabag/CoreBundle/Helper/ContentProxy.php | 94 ++++++++++++++----------- 1 file changed, 53 insertions(+), 41 deletions(-) diff --git a/src/Wallabag/CoreBundle/Helper/ContentProxy.php b/src/Wallabag/CoreBundle/Helper/ContentProxy.php index ce82f6bc..d4ea608f 100644 --- a/src/Wallabag/CoreBundle/Helper/ContentProxy.php +++ b/src/Wallabag/CoreBundle/Helper/ContentProxy.php @@ -69,47 +69,6 @@ class ContentProxy $this->stockEntry($entry, $content); } - /** - * Try to sanitize the title of the fetched content from wrong character encodings and invalid UTF-8 character. - * @param $title - * @param $contentType - * @return string - */ - private function sanitizeContentTitle($title, $contentType) { - if ('application/pdf' === $contentType) { - $title = $this->convertPdfEncodingToUTF8($title); - } - return $this->sanitizeUTF8Text($title); - } - - /** - * If the title from the fetched content comes from a PDF, then its very possible that the character encoding is not - * UTF-8. This methods tries to identify the character encoding and translate the title to UTF-8. - * @param $title - * @return string (maybe contains invalid UTF-8 character) - */ - private function convertPdfEncodingToUTF8($title) { - // first try UTF-8 because its easier to detect its present/absence - foreach (array('UTF-8', 'UTF-16BE', 'WINDOWS-1252') as $encoding) { - if (mb_check_encoding($title, $encoding)) { - return mb_convert_encoding($title, 'UTF-8', $encoding); - } - } - return $title; - } - - /** - * Remove invalid UTF-8 characters from the given string. - * @param String $rawText - * @return string - */ - private function sanitizeUTF8Text($rawText) { - if (mb_check_encoding($rawText, 'UTF-8')) { - return $rawText; - } - return iconv("UTF-8", "UTF-8//IGNORE", $rawText); - } - /** * Use a Symfony validator to ensure the language is well formatted. * @@ -218,6 +177,59 @@ class ContentProxy $entry->setTitle($path); } + /** + * Try to sanitize the title of the fetched content from wrong character encodings and invalid UTF-8 character. + * + * @param $title + * @param $contentType + * + * @return string + */ + private function sanitizeContentTitle($title, $contentType) + { + if ('application/pdf' === $contentType) { + $title = $this->convertPdfEncodingToUTF8($title); + } + + return $this->sanitizeUTF8Text($title); + } + + /** + * If the title from the fetched content comes from a PDF, then its very possible that the character encoding is not + * UTF-8. This methods tries to identify the character encoding and translate the title to UTF-8. + * + * @param $title + * + * @return string (maybe contains invalid UTF-8 character) + */ + private function convertPdfEncodingToUTF8($title) + { + // first try UTF-8 because its easier to detect its present/absence + foreach (['UTF-8', 'UTF-16BE', 'WINDOWS-1252'] as $encoding) { + if (mb_check_encoding($title, $encoding)) { + return mb_convert_encoding($title, 'UTF-8', $encoding); + } + } + + return $title; + } + + /** + * Remove invalid UTF-8 characters from the given string. + * + * @param string $rawText + * + * @return string + */ + private function sanitizeUTF8Text($rawText) + { + if (mb_check_encoding($rawText, 'UTF-8')) { + return $rawText; + } + + return iconv('UTF-8', 'UTF-8//IGNORE', $rawText); + } + /** * Stock entry with fetched or imported content. * Will fall back to OpenGraph data if available. -- cgit v1.2.3 From 28cc645b93a3505f39f8b5655e5f860544c023b4 Mon Sep 17 00:00:00 2001 From: Tobi823 Date: Sun, 23 Sep 2018 23:42:05 +0200 Subject: Run php-cs-fixer for fixing coding standard issues (on ContentProxyTest) --- .../CoreBundle/Helper/ContentProxyTest.php | 30 ++++++++++++++-------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php b/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php index 5f10f482..3f3c60d0 100644 --- a/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php +++ b/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php @@ -740,30 +740,38 @@ class ContentProxyTest extends TestCase } /** - * https://stackoverflow.com/a/18506801 + * https://stackoverflow.com/a/18506801. + * * @param $string + * * @return string */ - private function strToHex($string){ + private function strToHex($string) + { $hex = ''; - for ($i=0; $i