From d1e4d5edffd60a20d039e0b6e6976620f264d28f Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 14 Oct 2021 12:26:03 -0400 Subject: [PATCH 1/3] Ensure each charset is attempted only once. --- synapse/rest/media/v1/preview_url_resource.py | 30 +++++++++++++++---- tests/test_preview.py | 18 +++++++++-- 2 files changed, 40 insertions(+), 8 deletions(-) diff --git a/synapse/rest/media/v1/preview_url_resource.py b/synapse/rest/media/v1/preview_url_resource.py index 7ee91a0c0534..0c3e9ef0f4e5 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 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,11 @@ def try_remove_parent_dirs(dirs: Iterable[str]) -> None: logger.debug("No media removed from url cache") +def _normalise_encoding(encoding: str) -> str: + """Use the Python codec's name as the normalised entry.""" + return codecs.lookup(encoding).name + + 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 +658,42 @@ 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")) + attempted_encodings.add(encoding) + yield encoding # TODO Support # 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 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 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", "windows-1252"): + if fallback not in attempted_encodings: + yield fallback def decode_body( diff --git a/tests/test_preview.py b/tests/test_preview.py index c6789017bc7d..6453fdf6907b 100644 --- a/tests/test_preview.py +++ b/tests/test_preview.py @@ -358,7 +358,7 @@ def test_meta_charset_underscores(self): """, "text/html", ) - self.assertEqual(list(encodings), ["Shift_JIS", "utf-8", "windows-1252"]) + self.assertEqual(list(encodings), ["shift_jis", "utf-8", "windows-1252"]) def test_xml_encoding(self): """A character encoding is found via the meta tag.""" @@ -384,7 +384,7 @@ def test_meta_xml_encoding(self): """, "text/html", ) - self.assertEqual(list(encodings), ["UTF-16", "ascii", "utf-8", "windows-1252"]) + 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.""" @@ -405,3 +405,17 @@ 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"]) + + def test_duplicates(self): + """Ensure each encoding is only attempted once.""" + encodings = get_html_media_encodings( + b""" + + + + + + """, + 'text/html; charset="UTF_8"', + ) + self.assertEqual(list(encodings), ["utf-8", "windows-1252"]) From e70a592c76a5af6eab840998efd3285df1603f26 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 14 Oct 2021 12:27:24 -0400 Subject: [PATCH 2/3] Newsfragment --- changelog.d/11089.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/11089.bugfix diff --git a/changelog.d/11089.bugfix b/changelog.d/11089.bugfix new file mode 100644 index 000000000000..dc35c86440e7 --- /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. From 76bff8a1331208ec9de93fb783f5748a91c987e2 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 14 Oct 2021 13:50:15 -0400 Subject: [PATCH 3/3] Ignore invalid character encodings. --- synapse/rest/media/v1/preview_url_resource.py | 18 ++++++----- tests/test_preview.py | 31 +++++++++++++------ 2 files changed, 33 insertions(+), 16 deletions(-) diff --git a/synapse/rest/media/v1/preview_url_resource.py b/synapse/rest/media/v1/preview_url_resource.py index 0c3e9ef0f4e5..278fd901e2bd 100644 --- a/synapse/rest/media/v1/preview_url_resource.py +++ b/synapse/rest/media/v1/preview_url_resource.py @@ -632,9 +632,12 @@ def try_remove_parent_dirs(dirs: Iterable[str]) -> None: logger.debug("No media removed from url cache") -def _normalise_encoding(encoding: str) -> str: +def _normalise_encoding(encoding: str) -> Optional[str]: """Use the Python codec's name as the normalised entry.""" - return codecs.lookup(encoding).name + try: + return codecs.lookup(encoding).name + except LookupError: + return None def get_html_media_encodings(body: bytes, content_type: Optional[str]) -> Iterable[str]: @@ -668,8 +671,9 @@ def get_html_media_encodings(body: bytes, content_type: Optional[str]) -> Iterab match = _charset_match.search(body_start) if match: encoding = _normalise_encoding(match.group(1).decode("ascii")) - attempted_encodings.add(encoding) - yield encoding + if encoding: + attempted_encodings.add(encoding) + yield encoding # TODO Support @@ -677,7 +681,7 @@ def get_html_media_encodings(body: bytes, content_type: Optional[str]) -> Iterab match = _xml_encoding_match.match(body_start) if match: encoding = _normalise_encoding(match.group(1).decode("ascii")) - if encoding not in attempted_encodings: + if encoding and encoding not in attempted_encodings: attempted_encodings.add(encoding) yield encoding @@ -686,12 +690,12 @@ def get_html_media_encodings(body: bytes, content_type: Optional[str]) -> Iterab content_match = _content_type_match.match(content_type) if content_match: encoding = _normalise_encoding(content_match.group(1)) - if encoding not in attempted_encodings: + if encoding and encoding not in attempted_encodings: attempted_encodings.add(encoding) yield encoding # Finally, fallback to UTF-8, then windows-1252. - for fallback in ("utf-8", "windows-1252"): + for fallback in ("utf-8", "cp1252"): if fallback not in attempted_encodings: yield fallback diff --git a/tests/test_preview.py b/tests/test_preview.py index 6453fdf6907b..9a576f9a4e5a 100644 --- a/tests/test_preview.py +++ b/tests/test_preview.py @@ -307,7 +307,7 @@ def test_invalid_encoding2(self): 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""" \xf3 @@ -333,7 +333,7 @@ def test_meta_charset(self): """, "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 @@ def test_meta_charset(self): """, "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 @@ def test_meta_charset_underscores(self): """, "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 @@ def test_xml_encoding(self): """, "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 @@ def test_meta_xml_encoding(self): """, "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,12 +399,12 @@ def test_content_type(self): ) 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.""" @@ -418,4 +418,17 @@ def test_duplicates(self): """, 'text/html; charset="UTF_8"', ) - self.assertEqual(list(encodings), ["utf-8", "windows-1252"]) + 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""" + + + + + """, + 'text/html; charset="invalid"', + ) + self.assertEqual(list(encodings), ["utf-8", "cp1252"])