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
# 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"""
\xf3
@@ -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"""
+
+
+
+
+
+ """,
+ '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"""
+
+
+
+
+ """,
+ 'text/html; charset="invalid"',
+ )
+ self.assertEqual(list(encodings), ["utf-8", "cp1252"])