Do not error when thumbnailing invalid files (#8236)
If a file cannot be thumbnailed for some reason (e.g. the file is empty), then catch the exception and convert it to a reasonable error message for the client.
This commit is contained in:
parent
2ea1c68249
commit
b312769c0e
|
@ -0,0 +1 @@
|
|||
Fix a longstanding bug where files that could not be thumbnailed would result in an Internal Server Error.
|
|
@ -53,7 +53,7 @@ from .media_storage import MediaStorage
|
|||
from .preview_url_resource import PreviewUrlResource
|
||||
from .storage_provider import StorageProviderWrapper
|
||||
from .thumbnail_resource import ThumbnailResource
|
||||
from .thumbnailer import Thumbnailer
|
||||
from .thumbnailer import Thumbnailer, ThumbnailError
|
||||
from .upload_resource import UploadResource
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
@ -460,13 +460,30 @@ class MediaRepository:
|
|||
return t_byte_source
|
||||
|
||||
async def generate_local_exact_thumbnail(
|
||||
self, media_id, t_width, t_height, t_method, t_type, url_cache
|
||||
):
|
||||
self,
|
||||
media_id: str,
|
||||
t_width: int,
|
||||
t_height: int,
|
||||
t_method: str,
|
||||
t_type: str,
|
||||
url_cache: str,
|
||||
) -> Optional[str]:
|
||||
input_path = await self.media_storage.ensure_media_is_in_local_cache(
|
||||
FileInfo(None, media_id, url_cache=url_cache)
|
||||
)
|
||||
|
||||
thumbnailer = Thumbnailer(input_path)
|
||||
try:
|
||||
thumbnailer = Thumbnailer(input_path)
|
||||
except ThumbnailError as e:
|
||||
logger.warning(
|
||||
"Unable to generate a thumbnail for local media %s using a method of %s and type of %s: %s",
|
||||
media_id,
|
||||
t_method,
|
||||
t_type,
|
||||
e,
|
||||
)
|
||||
return None
|
||||
|
||||
t_byte_source = await defer_to_thread(
|
||||
self.hs.get_reactor(),
|
||||
self._generate_thumbnail,
|
||||
|
@ -506,14 +523,36 @@ class MediaRepository:
|
|||
|
||||
return output_path
|
||||
|
||||
# Could not generate thumbnail.
|
||||
return None
|
||||
|
||||
async def generate_remote_exact_thumbnail(
|
||||
self, server_name, file_id, media_id, t_width, t_height, t_method, t_type
|
||||
):
|
||||
self,
|
||||
server_name: str,
|
||||
file_id: str,
|
||||
media_id: str,
|
||||
t_width: int,
|
||||
t_height: int,
|
||||
t_method: str,
|
||||
t_type: str,
|
||||
) -> Optional[str]:
|
||||
input_path = await self.media_storage.ensure_media_is_in_local_cache(
|
||||
FileInfo(server_name, file_id, url_cache=False)
|
||||
)
|
||||
|
||||
thumbnailer = Thumbnailer(input_path)
|
||||
try:
|
||||
thumbnailer = Thumbnailer(input_path)
|
||||
except ThumbnailError as e:
|
||||
logger.warning(
|
||||
"Unable to generate a thumbnail for remote media %s from %s using a method of %s and type of %s: %s",
|
||||
media_id,
|
||||
server_name,
|
||||
t_method,
|
||||
t_type,
|
||||
e,
|
||||
)
|
||||
return None
|
||||
|
||||
t_byte_source = await defer_to_thread(
|
||||
self.hs.get_reactor(),
|
||||
self._generate_thumbnail,
|
||||
|
@ -559,6 +598,9 @@ class MediaRepository:
|
|||
|
||||
return output_path
|
||||
|
||||
# Could not generate thumbnail.
|
||||
return None
|
||||
|
||||
async def _generate_thumbnails(
|
||||
self,
|
||||
server_name: Optional[str],
|
||||
|
@ -590,7 +632,18 @@ class MediaRepository:
|
|||
FileInfo(server_name, file_id, url_cache=url_cache)
|
||||
)
|
||||
|
||||
thumbnailer = Thumbnailer(input_path)
|
||||
try:
|
||||
thumbnailer = Thumbnailer(input_path)
|
||||
except ThumbnailError as e:
|
||||
logger.warning(
|
||||
"Unable to generate thumbnails for remote media %s from %s using a method of %s and type of %s: %s",
|
||||
media_id,
|
||||
server_name,
|
||||
media_type,
|
||||
e,
|
||||
)
|
||||
return None
|
||||
|
||||
m_width = thumbnailer.width
|
||||
m_height = thumbnailer.height
|
||||
|
||||
|
|
|
@ -16,6 +16,7 @@
|
|||
|
||||
import logging
|
||||
|
||||
from synapse.api.errors import SynapseError
|
||||
from synapse.http.server import DirectServeJsonResource, set_cors_headers
|
||||
from synapse.http.servlet import parse_integer, parse_string
|
||||
|
||||
|
@ -173,7 +174,7 @@ class ThumbnailResource(DirectServeJsonResource):
|
|||
await respond_with_file(request, desired_type, file_path)
|
||||
else:
|
||||
logger.warning("Failed to generate thumbnail")
|
||||
respond_404(request)
|
||||
raise SynapseError(400, "Failed to generate thumbnail.")
|
||||
|
||||
async def _select_or_generate_remote_thumbnail(
|
||||
self,
|
||||
|
@ -235,7 +236,7 @@ class ThumbnailResource(DirectServeJsonResource):
|
|||
await respond_with_file(request, desired_type, file_path)
|
||||
else:
|
||||
logger.warning("Failed to generate thumbnail")
|
||||
respond_404(request)
|
||||
raise SynapseError(400, "Failed to generate thumbnail.")
|
||||
|
||||
async def _respond_remote_thumbnail(
|
||||
self, request, server_name, media_id, width, height, method, m_type
|
||||
|
|
|
@ -15,7 +15,7 @@
|
|||
import logging
|
||||
from io import BytesIO
|
||||
|
||||
from PIL import Image as Image
|
||||
from PIL import Image
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
|
@ -31,12 +31,22 @@ EXIF_TRANSPOSE_MAPPINGS = {
|
|||
}
|
||||
|
||||
|
||||
class ThumbnailError(Exception):
|
||||
"""An error occurred generating a thumbnail."""
|
||||
|
||||
|
||||
class Thumbnailer:
|
||||
|
||||
FORMATS = {"image/jpeg": "JPEG", "image/png": "PNG"}
|
||||
|
||||
def __init__(self, input_path):
|
||||
self.image = Image.open(input_path)
|
||||
try:
|
||||
self.image = Image.open(input_path)
|
||||
except OSError as e:
|
||||
# If an error occurs opening the image, a thumbnail won't be able to
|
||||
# be generated.
|
||||
raise ThumbnailError from e
|
||||
|
||||
self.width, self.height = self.image.size
|
||||
self.transpose_method = None
|
||||
try:
|
||||
|
|
|
@ -120,12 +120,13 @@ class _TestImage:
|
|||
extension = attr.ib(type=bytes)
|
||||
expected_cropped = attr.ib(type=Optional[bytes])
|
||||
expected_scaled = attr.ib(type=Optional[bytes])
|
||||
expected_found = attr.ib(default=True, type=bool)
|
||||
|
||||
|
||||
@parameterized_class(
|
||||
("test_image",),
|
||||
[
|
||||
# smol png
|
||||
# smoll png
|
||||
(
|
||||
_TestImage(
|
||||
unhexlify(
|
||||
|
@ -161,6 +162,8 @@ class _TestImage:
|
|||
None,
|
||||
),
|
||||
),
|
||||
# an empty file
|
||||
(_TestImage(b"", b"image/gif", b".gif", None, None, False,),),
|
||||
],
|
||||
)
|
||||
class MediaRepoTests(unittest.HomeserverTestCase):
|
||||
|
@ -303,12 +306,16 @@ class MediaRepoTests(unittest.HomeserverTestCase):
|
|||
self.assertEqual(headers.getRawHeaders(b"Content-Disposition"), None)
|
||||
|
||||
def test_thumbnail_crop(self):
|
||||
self._test_thumbnail("crop", self.test_image.expected_cropped)
|
||||
self._test_thumbnail(
|
||||
"crop", self.test_image.expected_cropped, self.test_image.expected_found
|
||||
)
|
||||
|
||||
def test_thumbnail_scale(self):
|
||||
self._test_thumbnail("scale", self.test_image.expected_scaled)
|
||||
self._test_thumbnail(
|
||||
"scale", self.test_image.expected_scaled, self.test_image.expected_found
|
||||
)
|
||||
|
||||
def _test_thumbnail(self, method, expected_body):
|
||||
def _test_thumbnail(self, method, expected_body, expected_found):
|
||||
params = "?width=32&height=32&method=" + method
|
||||
request, channel = self.make_request(
|
||||
"GET", self.media_id + params, shorthand=False
|
||||
|
@ -325,11 +332,23 @@ class MediaRepoTests(unittest.HomeserverTestCase):
|
|||
)
|
||||
self.pump()
|
||||
|
||||
self.assertEqual(channel.code, 200)
|
||||
if expected_body is not None:
|
||||
self.assertEqual(
|
||||
channel.result["body"], expected_body, channel.result["body"]
|
||||
)
|
||||
if expected_found:
|
||||
self.assertEqual(channel.code, 200)
|
||||
if expected_body is not None:
|
||||
self.assertEqual(
|
||||
channel.result["body"], expected_body, channel.result["body"]
|
||||
)
|
||||
else:
|
||||
# ensure that the result is at least some valid image
|
||||
Image.open(BytesIO(channel.result["body"]))
|
||||
else:
|
||||
# ensure that the result is at least some valid image
|
||||
Image.open(BytesIO(channel.result["body"]))
|
||||
# A 404 with a JSON body.
|
||||
self.assertEqual(channel.code, 404)
|
||||
self.assertEqual(
|
||||
channel.json_body,
|
||||
{
|
||||
"errcode": "M_NOT_FOUND",
|
||||
"error": "Not found [b'example.com', b'12345?width=32&height=32&method=%s']"
|
||||
% method,
|
||||
},
|
||||
)
|
||||
|
|
Loading…
Reference in New Issue