From 2124f668db4a7c19e3ed8357dad887711b3d3c35 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 30 Jun 2015 09:33:48 +0100 Subject: [PATCH 1/5] Add Content-Disposition headers to media repo v1 downloads --- synapse/rest/media/v1/base_resource.py | 23 ++++++++++++++++++---- synapse/rest/media/v1/download_resource.py | 8 ++++++-- synapse/rest/media/v1/upload_resource.py | 6 +++++- 3 files changed, 30 insertions(+), 7 deletions(-) diff --git a/synapse/rest/media/v1/base_resource.py b/synapse/rest/media/v1/base_resource.py index 6c83a9478..04410ab82 100644 --- a/synapse/rest/media/v1/base_resource.py +++ b/synapse/rest/media/v1/base_resource.py @@ -30,6 +30,7 @@ from synapse.util.async import ObservableDeferred import os +import cgi import logging logger = logging.getLogger(__name__) @@ -37,7 +38,9 @@ logger = logging.getLogger(__name__) def parse_media_id(request): try: - server_name, media_id = request.postpath + # This allows users to append e.g. /test.png to the URL. Useful for + # clients that parse the URL to see content type. + server_name, media_id = request.postpath[:2] return (server_name, media_id) except: raise SynapseError( @@ -128,12 +131,19 @@ class BaseMediaResource(Resource): media_type = headers["Content-Type"][0] time_now_ms = self.clock.time_msec() + content_disposition = headers.get("Content-Disposition", None) + if content_disposition: + _, params = cgi.parse_header(content_disposition[0],) + upload_name = params.get("filename", None) + else: + upload_name = None + yield self.store.store_cached_remote_media( origin=server_name, media_id=media_id, media_type=media_type, time_now_ms=self.clock.time_msec(), - upload_name=None, + upload_name=upload_name, media_length=length, filesystem_id=file_id, ) @@ -144,7 +154,7 @@ class BaseMediaResource(Resource): media_info = { "media_type": media_type, "media_length": length, - "upload_name": None, + "upload_name": upload_name, "created_ts": time_now_ms, "filesystem_id": file_id, } @@ -157,11 +167,16 @@ class BaseMediaResource(Resource): @defer.inlineCallbacks def _respond_with_file(self, request, media_type, file_path, - file_size=None): + file_size=None, upload_name=None): logger.debug("Responding with %r", file_path) if os.path.isfile(file_path): request.setHeader(b"Content-Type", media_type.encode("UTF-8")) + if upload_name: + request.setHeader( + b"Content-Disposition", + b"inline; filename=%s" % (upload_name.encode("utf-8"),), + ) # cache for at least a day. # XXX: we might want to turn this off for data we don't want to diff --git a/synapse/rest/media/v1/download_resource.py b/synapse/rest/media/v1/download_resource.py index 0fe6abf64..2af48ab62 100644 --- a/synapse/rest/media/v1/download_resource.py +++ b/synapse/rest/media/v1/download_resource.py @@ -47,10 +47,12 @@ class DownloadResource(BaseMediaResource): media_type = media_info["media_type"] media_length = media_info["media_length"] + upload_name = media_info["upload_name"] file_path = self.filepaths.local_media_filepath(media_id) yield self._respond_with_file( - request, media_type, file_path, media_length + request, media_type, file_path, media_length, + upload_name=upload_name, ) @defer.inlineCallbacks @@ -60,11 +62,13 @@ class DownloadResource(BaseMediaResource): media_type = media_info["media_type"] media_length = media_info["media_length"] filesystem_id = media_info["filesystem_id"] + upload_name = media_info["upload_name"] file_path = self.filepaths.remote_media_filepath( server_name, filesystem_id ) yield self._respond_with_file( - request, media_type, file_path, media_length + request, media_type, file_path, media_length, + upload_name=upload_name, ) diff --git a/synapse/rest/media/v1/upload_resource.py b/synapse/rest/media/v1/upload_resource.py index cc571976a..92e855a44 100644 --- a/synapse/rest/media/v1/upload_resource.py +++ b/synapse/rest/media/v1/upload_resource.py @@ -84,6 +84,10 @@ class UploadResource(BaseMediaResource): code=413, ) + upload_name = request.args.get("filename", None) + if upload_name: + upload_name = upload_name[0] + headers = request.requestHeaders if headers.hasHeader("Content-Type"): @@ -99,7 +103,7 @@ class UploadResource(BaseMediaResource): # TODO(markjh): parse content-dispostion content_uri = yield self.create_content( - media_type, None, request.content.read(), + media_type, upload_name, request.content.read(), content_length, auth_user ) From 9beaedd1642673d36428dd796dda62f18a937c2a Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 30 Jun 2015 10:31:59 +0100 Subject: [PATCH 2/5] Enforce ascii filenames for uploads --- synapse/rest/media/v1/base_resource.py | 3 +++ synapse/rest/media/v1/upload_resource.py | 4 +++- synapse/util/stringutils.py | 9 +++++++++ 3 files changed, 15 insertions(+), 1 deletion(-) diff --git a/synapse/rest/media/v1/base_resource.py b/synapse/rest/media/v1/base_resource.py index 04410ab82..1b7517e2f 100644 --- a/synapse/rest/media/v1/base_resource.py +++ b/synapse/rest/media/v1/base_resource.py @@ -27,6 +27,7 @@ from twisted.web.resource import Resource from twisted.protocols.basic import FileSender from synapse.util.async import ObservableDeferred +from synapse.util.stringutils import is_ascii import os @@ -135,6 +136,8 @@ class BaseMediaResource(Resource): if content_disposition: _, params = cgi.parse_header(content_disposition[0],) upload_name = params.get("filename", None) + if upload_name and not is_ascii(upload_name): + upload_name = None else: upload_name = None diff --git a/synapse/rest/media/v1/upload_resource.py b/synapse/rest/media/v1/upload_resource.py index 92e855a44..cdd1d44e0 100644 --- a/synapse/rest/media/v1/upload_resource.py +++ b/synapse/rest/media/v1/upload_resource.py @@ -15,7 +15,7 @@ from synapse.http.server import respond_with_json, request_handler -from synapse.util.stringutils import random_string +from synapse.util.stringutils import random_string, is_ascii from synapse.api.errors import SynapseError from twisted.web.server import NOT_DONE_YET @@ -87,6 +87,8 @@ class UploadResource(BaseMediaResource): upload_name = request.args.get("filename", None) if upload_name: upload_name = upload_name[0] + if upload_name and not is_ascii(upload_name): + raise SynapseError(400, "filename must be ascii") headers = request.requestHeaders diff --git a/synapse/util/stringutils.py b/synapse/util/stringutils.py index 52e66beae..7a1e96af3 100644 --- a/synapse/util/stringutils.py +++ b/synapse/util/stringutils.py @@ -33,3 +33,12 @@ def random_string_with_symbols(length): return ''.join( random.choice(_string_with_symbols) for _ in xrange(length) ) + + +def is_ascii(s): + try: + s.encode("ascii") + except UnicodeDecodeError: + return False + else: + return True From 12b83f1a0d1e666b8dc629358a904613b23aac11 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 3 Jul 2015 11:24:55 +0100 Subject: [PATCH 3/5] If user supplies filename in URL when downloading from media repo, use that name in Content Disposition --- synapse/rest/media/v1/base_resource.py | 5 ++++- synapse/rest/media/v1/download_resource.py | 16 +++++++++------- synapse/rest/media/v1/thumbnail_resource.py | 2 +- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/synapse/rest/media/v1/base_resource.py b/synapse/rest/media/v1/base_resource.py index 1b7517e2f..c43ae0314 100644 --- a/synapse/rest/media/v1/base_resource.py +++ b/synapse/rest/media/v1/base_resource.py @@ -42,7 +42,10 @@ def parse_media_id(request): # This allows users to append e.g. /test.png to the URL. Useful for # clients that parse the URL to see content type. server_name, media_id = request.postpath[:2] - return (server_name, media_id) + if len(request.postpath) > 2 and is_ascii(request.postpath[-1]): + return server_name, media_id, request.postpath[-1] + else: + return server_name, media_id, None except: raise SynapseError( 404, diff --git a/synapse/rest/media/v1/download_resource.py b/synapse/rest/media/v1/download_resource.py index 2af48ab62..ab384e538 100644 --- a/synapse/rest/media/v1/download_resource.py +++ b/synapse/rest/media/v1/download_resource.py @@ -32,14 +32,16 @@ class DownloadResource(BaseMediaResource): @request_handler @defer.inlineCallbacks def _async_render_GET(self, request): - server_name, media_id = parse_media_id(request) + server_name, media_id, name = parse_media_id(request) if server_name == self.server_name: - yield self._respond_local_file(request, media_id) + yield self._respond_local_file(request, media_id, name) else: - yield self._respond_remote_file(request, server_name, media_id) + yield self._respond_remote_file( + request, server_name, media_id, name + ) @defer.inlineCallbacks - def _respond_local_file(self, request, media_id): + def _respond_local_file(self, request, media_id, name): media_info = yield self.store.get_local_media(media_id) if not media_info: self._respond_404(request) @@ -47,7 +49,7 @@ class DownloadResource(BaseMediaResource): media_type = media_info["media_type"] media_length = media_info["media_length"] - upload_name = media_info["upload_name"] + upload_name = name if name else media_info["upload_name"] file_path = self.filepaths.local_media_filepath(media_id) yield self._respond_with_file( @@ -56,13 +58,13 @@ class DownloadResource(BaseMediaResource): ) @defer.inlineCallbacks - def _respond_remote_file(self, request, server_name, media_id): + def _respond_remote_file(self, request, server_name, media_id, name): media_info = yield self._get_remote_media(server_name, media_id) media_type = media_info["media_type"] media_length = media_info["media_length"] filesystem_id = media_info["filesystem_id"] - upload_name = media_info["upload_name"] + upload_name = name if name else media_info["upload_name"] file_path = self.filepaths.remote_media_filepath( server_name, filesystem_id diff --git a/synapse/rest/media/v1/thumbnail_resource.py b/synapse/rest/media/v1/thumbnail_resource.py index 1dadd880b..4a9b6d8ee 100644 --- a/synapse/rest/media/v1/thumbnail_resource.py +++ b/synapse/rest/media/v1/thumbnail_resource.py @@ -36,7 +36,7 @@ class ThumbnailResource(BaseMediaResource): @request_handler @defer.inlineCallbacks def _async_render_GET(self, request): - server_name, media_id = parse_media_id(request) + server_name, media_id, _ = parse_media_id(request) width = parse_integer(request, "width") height = parse_integer(request, "height") method = parse_string(request, "method", "scale") From 65e69dec8b202e3f03063b18ebde55ceee69e2e7 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 6 Jul 2015 09:31:40 +0100 Subject: [PATCH 4/5] Don't explode if we don't recognize one of the event_ids in the backfill request --- synapse/storage/event_federation.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/synapse/storage/event_federation.py b/synapse/storage/event_federation.py index ace745953..c71019d93 100644 --- a/synapse/storage/event_federation.py +++ b/synapse/storage/event_federation.py @@ -408,10 +408,12 @@ class EventFederationStore(SQLBaseStore): keyvalues={ "event_id": event_id, }, - retcol="depth" + retcol="depth", + allow_none=True, ) - queue.put((-depth, event_id)) + if depth: + queue.put((-depth, event_id)) while not queue.empty() and len(event_results) < limit: try: From a7dcbfe430f31a6e9fc6b5920ddff39eb1b1e3b4 Mon Sep 17 00:00:00 2001 From: Mark Haines Date: Mon, 6 Jul 2015 16:47:17 +0100 Subject: [PATCH 5/5] Don't 500 if a group is missing from the regex --- synapse/http/server.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/http/server.py b/synapse/http/server.py index 807ff95c6..b60e905a6 100644 --- a/synapse/http/server.py +++ b/synapse/http/server.py @@ -207,7 +207,7 @@ class JsonResource(HttpServer, resource.Resource): incoming_requests_counter.inc(request.method, servlet_classname) args = [ - urllib.unquote(u).decode("UTF-8") for u in m.groups() + urllib.unquote(u).decode("UTF-8") if u else u for u in m.groups() ] callback_return = yield callback(request, *args)