From e2f0b49b3fa9fd87cd24ac6bdc46a94db532ba89 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <clokep@users.noreply.github.com>
Date: Thu, 14 Oct 2021 10:17:20 -0400
Subject: Attempt different character encodings when previewing a URL. (#11077)

This follows similar logic to BeautifulSoup where we attempt different
character encodings until we find one which works.
---
 tests/test_preview.py | 66 +++++++++++++++++++++++++++++++--------------------
 1 file changed, 40 insertions(+), 26 deletions(-)

(limited to 'tests/test_preview.py')

diff --git a/tests/test_preview.py b/tests/test_preview.py
index 09e017b4d9..c6789017bc 100644
--- a/tests/test_preview.py
+++ b/tests/test_preview.py
@@ -15,7 +15,7 @@
 from synapse.rest.media.v1.preview_url_resource import (
     _calc_og,
     decode_body,
-    get_html_media_encoding,
+    get_html_media_encodings,
     summarize_paragraphs,
 )
 
@@ -159,7 +159,7 @@ class CalcOgTestCase(unittest.TestCase):
         </html>
         """
 
-        tree = decode_body(html)
+        tree = decode_body(html, "http://example.com/test.html")
         og = _calc_og(tree, "http://example.com/test.html")
 
         self.assertEqual(og, {"og:title": "Foo", "og:description": "Some text."})
@@ -175,7 +175,7 @@ class CalcOgTestCase(unittest.TestCase):
         </html>
         """
 
-        tree = decode_body(html)
+        tree = decode_body(html, "http://example.com/test.html")
         og = _calc_og(tree, "http://example.com/test.html")
 
         self.assertEqual(og, {"og:title": "Foo", "og:description": "Some text."})
@@ -194,7 +194,7 @@ class CalcOgTestCase(unittest.TestCase):
         </html>
         """
 
-        tree = decode_body(html)
+        tree = decode_body(html, "http://example.com/test.html")
         og = _calc_og(tree, "http://example.com/test.html")
 
         self.assertEqual(
@@ -216,7 +216,7 @@ class CalcOgTestCase(unittest.TestCase):
         </html>
         """
 
-        tree = decode_body(html)
+        tree = decode_body(html, "http://example.com/test.html")
         og = _calc_og(tree, "http://example.com/test.html")
 
         self.assertEqual(og, {"og:title": "Foo", "og:description": "Some text."})
@@ -230,7 +230,7 @@ class CalcOgTestCase(unittest.TestCase):
         </html>
         """
 
-        tree = decode_body(html)
+        tree = decode_body(html, "http://example.com/test.html")
         og = _calc_og(tree, "http://example.com/test.html")
 
         self.assertEqual(og, {"og:title": None, "og:description": "Some text."})
@@ -245,7 +245,7 @@ class CalcOgTestCase(unittest.TestCase):
         </html>
         """
 
-        tree = decode_body(html)
+        tree = decode_body(html, "http://example.com/test.html")
         og = _calc_og(tree, "http://example.com/test.html")
 
         self.assertEqual(og, {"og:title": "Title", "og:description": "Some text."})
@@ -260,7 +260,7 @@ class CalcOgTestCase(unittest.TestCase):
         </html>
         """
 
-        tree = decode_body(html)
+        tree = decode_body(html, "http://example.com/test.html")
         og = _calc_og(tree, "http://example.com/test.html")
 
         self.assertEqual(og, {"og:title": None, "og:description": "Some text."})
@@ -268,13 +268,13 @@ class CalcOgTestCase(unittest.TestCase):
     def test_empty(self):
         """Test a body with no data in it."""
         html = b""
-        tree = decode_body(html)
+        tree = decode_body(html, "http://example.com/test.html")
         self.assertIsNone(tree)
 
     def test_no_tree(self):
         """A valid body with no tree in it."""
         html = b"\x00"
-        tree = decode_body(html)
+        tree = decode_body(html, "http://example.com/test.html")
         self.assertIsNone(tree)
 
     def test_invalid_encoding(self):
@@ -287,7 +287,7 @@ class CalcOgTestCase(unittest.TestCase):
         </body>
         </html>
         """
-        tree = decode_body(html, "invalid-encoding")
+        tree = decode_body(html, "http://example.com/test.html", "invalid-encoding")
         og = _calc_og(tree, "http://example.com/test.html")
         self.assertEqual(og, {"og:title": "Foo", "og:description": "Some text."})
 
@@ -302,15 +302,29 @@ class CalcOgTestCase(unittest.TestCase):
         </body>
         </html>
         """
-        tree = decode_body(html)
+        tree = decode_body(html, "http://example.com/test.html")
         og = _calc_og(tree, "http://example.com/test.html")
         self.assertEqual(og, {"og:title": "ÿÿ Foo", "og:description": "Some text."})
 
+    def test_windows_1252(self):
+        """A body which uses windows-1252, but doesn't declare that."""
+        html = b"""
+        <html>
+        <head><title>\xf3</title></head>
+        <body>
+        Some text.
+        </body>
+        </html>
+        """
+        tree = decode_body(html, "http://example.com/test.html")
+        og = _calc_og(tree, "http://example.com/test.html")
+        self.assertEqual(og, {"og:title": "ó", "og:description": "Some text."})
+
 
 class MediaEncodingTestCase(unittest.TestCase):
     def test_meta_charset(self):
         """A character encoding is found via the meta tag."""
-        encoding = get_html_media_encoding(
+        encodings = get_html_media_encodings(
             b"""
         <html>
         <head><meta charset="ascii">
@@ -319,10 +333,10 @@ class MediaEncodingTestCase(unittest.TestCase):
         """,
             "text/html",
         )
-        self.assertEqual(encoding, "ascii")
+        self.assertEqual(list(encodings), ["ascii", "utf-8", "windows-1252"])
 
         # A less well-formed version.
-        encoding = get_html_media_encoding(
+        encodings = get_html_media_encodings(
             b"""
         <html>
         <head>< meta charset = ascii>
@@ -331,11 +345,11 @@ class MediaEncodingTestCase(unittest.TestCase):
         """,
             "text/html",
         )
-        self.assertEqual(encoding, "ascii")
+        self.assertEqual(list(encodings), ["ascii", "utf-8", "windows-1252"])
 
     def test_meta_charset_underscores(self):
         """A character encoding contains underscore."""
-        encoding = get_html_media_encoding(
+        encodings = get_html_media_encodings(
             b"""
         <html>
         <head><meta charset="Shift_JIS">
@@ -344,11 +358,11 @@ class MediaEncodingTestCase(unittest.TestCase):
         """,
             "text/html",
         )
-        self.assertEqual(encoding, "Shift_JIS")
+        self.assertEqual(list(encodings), ["Shift_JIS", "utf-8", "windows-1252"])
 
     def test_xml_encoding(self):
         """A character encoding is found via the meta tag."""
-        encoding = get_html_media_encoding(
+        encodings = get_html_media_encodings(
             b"""
         <?xml version="1.0" encoding="ascii"?>
         <html>
@@ -356,11 +370,11 @@ class MediaEncodingTestCase(unittest.TestCase):
         """,
             "text/html",
         )
-        self.assertEqual(encoding, "ascii")
+        self.assertEqual(list(encodings), ["ascii", "utf-8", "windows-1252"])
 
     def test_meta_xml_encoding(self):
         """Meta tags take precedence over XML encoding."""
-        encoding = get_html_media_encoding(
+        encodings = get_html_media_encodings(
             b"""
         <?xml version="1.0" encoding="ascii"?>
         <html>
@@ -370,7 +384,7 @@ class MediaEncodingTestCase(unittest.TestCase):
         """,
             "text/html",
         )
-        self.assertEqual(encoding, "UTF-16")
+        self.assertEqual(list(encodings), ["UTF-16", "ascii", "utf-8", "windows-1252"])
 
     def test_content_type(self):
         """A character encoding is found via the Content-Type header."""
@@ -384,10 +398,10 @@ class MediaEncodingTestCase(unittest.TestCase):
             'text/html; charset=ascii";',
         )
         for header in headers:
-            encoding = get_html_media_encoding(b"", header)
-            self.assertEqual(encoding, "ascii")
+            encodings = get_html_media_encodings(b"", header)
+            self.assertEqual(list(encodings), ["ascii", "utf-8", "windows-1252"])
 
     def test_fallback(self):
         """A character encoding cannot be found in the body or header."""
-        encoding = get_html_media_encoding(b"", "text/html")
-        self.assertEqual(encoding, "utf-8")
+        encodings = get_html_media_encodings(b"", "text/html")
+        self.assertEqual(list(encodings), ["utf-8", "windows-1252"])
-- 
cgit 1.5.1


From efd0074ab76a9449087e38afadcc5a1b4d5a2813 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <clokep@users.noreply.github.com>
Date: Thu, 14 Oct 2021 14:51:44 -0400
Subject: Ensure each charset is attempted only once during media preview.
 (#11089)

There's no point in trying more than once since it is guaranteed to
continually fail.
---
 changelog.d/11089.bugfix                      |  1 +
 synapse/rest/media/v1/preview_url_resource.py | 34 +++++++++++++++++----
 tests/test_preview.py                         | 43 ++++++++++++++++++++++-----
 3 files changed, 64 insertions(+), 14 deletions(-)
 create mode 100644 changelog.d/11089.bugfix

(limited to 'tests/test_preview.py')

diff --git a/changelog.d/11089.bugfix b/changelog.d/11089.bugfix
new file mode 100644
index 0000000000..dc35c86440
--- /dev/null
+++ b/changelog.d/11089.bugfix
@@ -0,0 +1 @@
+Fix a long-standing bug when attempting to preview URLs which are in the `windows-1252` character encoding.
diff --git a/synapse/rest/media/v1/preview_url_resource.py b/synapse/rest/media/v1/preview_url_resource.py
index 7ee91a0c05..278fd901e2 100644
--- a/synapse/rest/media/v1/preview_url_resource.py
+++ b/synapse/rest/media/v1/preview_url_resource.py
@@ -12,6 +12,7 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
+import codecs
 import datetime
 import errno
 import fnmatch
@@ -22,7 +23,7 @@ import re
 import shutil
 import sys
 import traceback
-from typing import TYPE_CHECKING, Dict, Generator, Iterable, Optional, Tuple, Union
+from typing import TYPE_CHECKING, Dict, Generator, Iterable, Optional, Set, Tuple, Union
 from urllib import parse as urlparse
 
 import attr
@@ -631,6 +632,14 @@ class PreviewUrlResource(DirectServeJsonResource):
             logger.debug("No media removed from url cache")
 
 
+def _normalise_encoding(encoding: str) -> Optional[str]:
+    """Use the Python codec's name as the normalised entry."""
+    try:
+        return codecs.lookup(encoding).name
+    except LookupError:
+        return None
+
+
 def get_html_media_encodings(body: bytes, content_type: Optional[str]) -> Iterable[str]:
     """
     Get potential encoding of the body based on the (presumably) HTML body or the content-type header.
@@ -652,30 +661,43 @@ def get_html_media_encodings(body: bytes, content_type: Optional[str]) -> Iterab
     Returns:
         The character encoding of the body, as a string.
     """
+    # There's no point in returning an encoding more than once.
+    attempted_encodings: Set[str] = set()
+
     # Limit searches to the first 1kb, since it ought to be at the top.
     body_start = body[:1024]
 
     # Check if it has an encoding set in a meta tag.
     match = _charset_match.search(body_start)
     if match:
-        yield match.group(1).decode("ascii")
+        encoding = _normalise_encoding(match.group(1).decode("ascii"))
+        if encoding:
+            attempted_encodings.add(encoding)
+            yield encoding
 
     # TODO Support <meta http-equiv="Content-Type" content="text/html; charset=utf-8"/>
 
     # Check if it has an XML document with an encoding.
     match = _xml_encoding_match.match(body_start)
     if match:
-        yield match.group(1).decode("ascii")
+        encoding = _normalise_encoding(match.group(1).decode("ascii"))
+        if encoding and encoding not in attempted_encodings:
+            attempted_encodings.add(encoding)
+            yield encoding
 
     # Check the HTTP Content-Type header for a character set.
     if content_type:
         content_match = _content_type_match.match(content_type)
         if content_match:
-            yield content_match.group(1)
+            encoding = _normalise_encoding(content_match.group(1))
+            if encoding and encoding not in attempted_encodings:
+                attempted_encodings.add(encoding)
+                yield encoding
 
     # Finally, fallback to UTF-8, then windows-1252.
-    yield "utf-8"
-    yield "windows-1252"
+    for fallback in ("utf-8", "cp1252"):
+        if fallback not in attempted_encodings:
+            yield fallback
 
 
 def decode_body(
diff --git a/tests/test_preview.py b/tests/test_preview.py
index c6789017bc..9a576f9a4e 100644
--- a/tests/test_preview.py
+++ b/tests/test_preview.py
@@ -307,7 +307,7 @@ class CalcOgTestCase(unittest.TestCase):
         self.assertEqual(og, {"og:title": "ÿÿ Foo", "og:description": "Some text."})
 
     def test_windows_1252(self):
-        """A body which uses windows-1252, but doesn't declare that."""
+        """A body which uses cp1252, but doesn't declare that."""
         html = b"""
         <html>
         <head><title>\xf3</title></head>
@@ -333,7 +333,7 @@ class MediaEncodingTestCase(unittest.TestCase):
         """,
             "text/html",
         )
-        self.assertEqual(list(encodings), ["ascii", "utf-8", "windows-1252"])
+        self.assertEqual(list(encodings), ["ascii", "utf-8", "cp1252"])
 
         # A less well-formed version.
         encodings = get_html_media_encodings(
@@ -345,7 +345,7 @@ class MediaEncodingTestCase(unittest.TestCase):
         """,
             "text/html",
         )
-        self.assertEqual(list(encodings), ["ascii", "utf-8", "windows-1252"])
+        self.assertEqual(list(encodings), ["ascii", "utf-8", "cp1252"])
 
     def test_meta_charset_underscores(self):
         """A character encoding contains underscore."""
@@ -358,7 +358,7 @@ class MediaEncodingTestCase(unittest.TestCase):
         """,
             "text/html",
         )
-        self.assertEqual(list(encodings), ["Shift_JIS", "utf-8", "windows-1252"])
+        self.assertEqual(list(encodings), ["shift_jis", "utf-8", "cp1252"])
 
     def test_xml_encoding(self):
         """A character encoding is found via the meta tag."""
@@ -370,7 +370,7 @@ class MediaEncodingTestCase(unittest.TestCase):
         """,
             "text/html",
         )
-        self.assertEqual(list(encodings), ["ascii", "utf-8", "windows-1252"])
+        self.assertEqual(list(encodings), ["ascii", "utf-8", "cp1252"])
 
     def test_meta_xml_encoding(self):
         """Meta tags take precedence over XML encoding."""
@@ -384,7 +384,7 @@ class MediaEncodingTestCase(unittest.TestCase):
         """,
             "text/html",
         )
-        self.assertEqual(list(encodings), ["UTF-16", "ascii", "utf-8", "windows-1252"])
+        self.assertEqual(list(encodings), ["utf-16", "ascii", "utf-8", "cp1252"])
 
     def test_content_type(self):
         """A character encoding is found via the Content-Type header."""
@@ -399,9 +399,36 @@ class MediaEncodingTestCase(unittest.TestCase):
         )
         for header in headers:
             encodings = get_html_media_encodings(b"", header)
-            self.assertEqual(list(encodings), ["ascii", "utf-8", "windows-1252"])
+            self.assertEqual(list(encodings), ["ascii", "utf-8", "cp1252"])
 
     def test_fallback(self):
         """A character encoding cannot be found in the body or header."""
         encodings = get_html_media_encodings(b"", "text/html")
-        self.assertEqual(list(encodings), ["utf-8", "windows-1252"])
+        self.assertEqual(list(encodings), ["utf-8", "cp1252"])
+
+    def test_duplicates(self):
+        """Ensure each encoding is only attempted once."""
+        encodings = get_html_media_encodings(
+            b"""
+        <?xml version="1.0" encoding="utf8"?>
+        <html>
+        <head><meta charset="UTF-8">
+        </head>
+        </html>
+        """,
+            'text/html; charset="UTF_8"',
+        )
+        self.assertEqual(list(encodings), ["utf-8", "cp1252"])
+
+    def test_unknown_invalid(self):
+        """A character encoding should be ignored if it is unknown or invalid."""
+        encodings = get_html_media_encodings(
+            b"""
+        <html>
+        <head><meta charset="invalid">
+        </head>
+        </html>
+        """,
+            'text/html; charset="invalid"',
+        )
+        self.assertEqual(list(encodings), ["utf-8", "cp1252"])
-- 
cgit 1.5.1