From 5b955a255beb0d7703801bafdffab4d95c95223f Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Sun, 19 Jul 2020 19:15:30 +0100 Subject: [PATCH 01/12] Check media_type and upload_name before persisting file --- synapse/rest/media/v1/media_repository.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/synapse/rest/media/v1/media_repository.py b/synapse/rest/media/v1/media_repository.py index 45628c07b401..fc3a96342db7 100644 --- a/synapse/rest/media/v1/media_repository.py +++ b/synapse/rest/media/v1/media_repository.py @@ -149,6 +149,13 @@ async def create_content( Returns: Deferred[str]: The mxc url of the stored content """ + + if not isinstance(media_type, str): + raise SynapseError(400, "Invalid parameter media_type", Codes.INVALID_PARAM) + + if not isinstance(upload_name, str): + raise SynapseError(400, "Invalid parameter upload_name", Codes.INVALID_PARAM) + media_id = random_string(24) file_info = FileInfo(server_name=None, file_id=media_id) From 1f96b413ebfdc219975a8b35796f92d3b7fe4326 Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Sun, 19 Jul 2020 19:17:46 +0100 Subject: [PATCH 02/12] Create 7905.bugfix --- changelog.d/7905.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/7905.bugfix diff --git a/changelog.d/7905.bugfix b/changelog.d/7905.bugfix new file mode 100644 index 000000000000..73d8efd8cd0e --- /dev/null +++ b/changelog.d/7905.bugfix @@ -0,0 +1 @@ +Fix synapse storing a media file with an invalid media_type or upload_name. From 2bba403cba388b87232bbb518acfd5bcf214dc8e Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Mon, 20 Jul 2020 13:20:18 +0100 Subject: [PATCH 03/12] Fix lint --- synapse/rest/media/v1/media_repository.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/synapse/rest/media/v1/media_repository.py b/synapse/rest/media/v1/media_repository.py index fc3a96342db7..362c966fa1c1 100644 --- a/synapse/rest/media/v1/media_repository.py +++ b/synapse/rest/media/v1/media_repository.py @@ -25,6 +25,7 @@ from twisted.web.resource import Resource from synapse.api.errors import ( + Codes, FederationDeniedError, HttpResponseException, NotFoundError, @@ -149,12 +150,14 @@ async def create_content( Returns: Deferred[str]: The mxc url of the stored content """ - + if not isinstance(media_type, str): raise SynapseError(400, "Invalid parameter media_type", Codes.INVALID_PARAM) if not isinstance(upload_name, str): - raise SynapseError(400, "Invalid parameter upload_name", Codes.INVALID_PARAM) + raise SynapseError( + 400, "Invalid parameter upload_name", Codes.INVALID_PARAM + ) media_id = random_string(24) From 64d51866c4cf1c070d2cecfa1e48f77ce4647416 Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Mon, 20 Jul 2020 13:39:36 +0100 Subject: [PATCH 04/12] Update docstring, allow upload_name to be optional --- synapse/rest/media/v1/media_repository.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/rest/media/v1/media_repository.py b/synapse/rest/media/v1/media_repository.py index 362c966fa1c1..cba53ae6cdf8 100644 --- a/synapse/rest/media/v1/media_repository.py +++ b/synapse/rest/media/v1/media_repository.py @@ -142,7 +142,7 @@ async def create_content( Args: media_type(str): The content type of the file - upload_name(str): The name of the file + upload_name(str|None): The name of the file content: A file like object that is the content to store content_length(int): The length of the content auth_user(str): The user_id of the uploader @@ -154,7 +154,7 @@ async def create_content( if not isinstance(media_type, str): raise SynapseError(400, "Invalid parameter media_type", Codes.INVALID_PARAM) - if not isinstance(upload_name, str): + if upload_name and not isinstance(upload_name, str): raise SynapseError( 400, "Invalid parameter upload_name", Codes.INVALID_PARAM ) From 97db88c234d3568c4d9dfdff23532b2e6fe4c64c Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Fri, 31 Jul 2020 15:13:45 +0100 Subject: [PATCH 05/12] Do not raise Errors at the handler level --- synapse/rest/media/v1/media_repository.py | 8 -------- 1 file changed, 8 deletions(-) diff --git a/synapse/rest/media/v1/media_repository.py b/synapse/rest/media/v1/media_repository.py index cba53ae6cdf8..af960a9d21b9 100644 --- a/synapse/rest/media/v1/media_repository.py +++ b/synapse/rest/media/v1/media_repository.py @@ -151,14 +151,6 @@ async def create_content( Deferred[str]: The mxc url of the stored content """ - if not isinstance(media_type, str): - raise SynapseError(400, "Invalid parameter media_type", Codes.INVALID_PARAM) - - if upload_name and not isinstance(upload_name, str): - raise SynapseError( - 400, "Invalid parameter upload_name", Codes.INVALID_PARAM - ) - media_id = random_string(24) file_info = FileInfo(server_name=None, file_id=media_id) From cff75a8fbb1b55cb7925bc627ca32763f1638bd3 Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Fri, 31 Jul 2020 15:14:34 +0100 Subject: [PATCH 06/12] Check if upload_name is empty --- synapse/rest/media/v1/upload_resource.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/synapse/rest/media/v1/upload_resource.py b/synapse/rest/media/v1/upload_resource.py index 3ebf7a68e673..f87636100470 100644 --- a/synapse/rest/media/v1/upload_resource.py +++ b/synapse/rest/media/v1/upload_resource.py @@ -62,6 +62,10 @@ async def _async_render_POST(self, request): raise SynapseError( msg="Invalid UTF-8 filename parameter: %r" % (upload_name), code=400 ) + + # upload_name might be an empty byte string + if len(upload_name) === 0: + upload_name = None headers = request.requestHeaders From cfd2a98139b71a6a39c6f7149f49c3dca7fef3b7 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 28 Sep 2020 17:17:42 -0400 Subject: [PATCH 07/12] Be Pythonic. --- synapse/rest/media/v1/upload_resource.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/rest/media/v1/upload_resource.py b/synapse/rest/media/v1/upload_resource.py index f87636100470..f8ce3e4d24c5 100644 --- a/synapse/rest/media/v1/upload_resource.py +++ b/synapse/rest/media/v1/upload_resource.py @@ -64,7 +64,7 @@ async def _async_render_POST(self, request): ) # upload_name might be an empty byte string - if len(upload_name) === 0: + if not len(upload_name): upload_name = None headers = request.requestHeaders From ae842e1e4413054cd84c18b6e9064ea9b1704899 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 28 Sep 2020 17:30:23 -0400 Subject: [PATCH 08/12] More lint. --- synapse/rest/media/v1/media_repository.py | 1 - synapse/rest/media/v1/upload_resource.py | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/synapse/rest/media/v1/media_repository.py b/synapse/rest/media/v1/media_repository.py index 5dc373d2abc6..76f5b8f9c70d 100644 --- a/synapse/rest/media/v1/media_repository.py +++ b/synapse/rest/media/v1/media_repository.py @@ -26,7 +26,6 @@ from twisted.web.resource import Resource from synapse.api.errors import ( - Codes, FederationDeniedError, HttpResponseException, NotFoundError, diff --git a/synapse/rest/media/v1/upload_resource.py b/synapse/rest/media/v1/upload_resource.py index f8ce3e4d24c5..28a8654c0e4d 100644 --- a/synapse/rest/media/v1/upload_resource.py +++ b/synapse/rest/media/v1/upload_resource.py @@ -62,7 +62,7 @@ async def _async_render_POST(self, request): raise SynapseError( msg="Invalid UTF-8 filename parameter: %r" % (upload_name), code=400 ) - + # upload_name might be an empty byte string if not len(upload_name): upload_name = None From 7ee568b8e746acad8ba989d8565a47a5df8dd758 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 28 Sep 2020 17:37:08 -0400 Subject: [PATCH 09/12] Update docstring. --- synapse/rest/media/v1/media_repository.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/synapse/rest/media/v1/media_repository.py b/synapse/rest/media/v1/media_repository.py index 76f5b8f9c70d..fb80b02bc36c 100644 --- a/synapse/rest/media/v1/media_repository.py +++ b/synapse/rest/media/v1/media_repository.py @@ -147,7 +147,8 @@ async def create_content( """Store uploaded content for a local user and return the mxc URL Args: - media_type(str): The content type of the file + media_type: The content type of the file. + upload_name: The name of the file, if provided. content: A file like object that is the content to store content_length: The length of the content auth_user: The user_id of the uploader From 8f1f1198c47f9cf3fba3eaa269768a7f5332c47a Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 29 Sep 2020 10:15:23 -0400 Subject: [PATCH 10/12] Fix handling of none. --- synapse/rest/media/v1/upload_resource.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/rest/media/v1/upload_resource.py b/synapse/rest/media/v1/upload_resource.py index 28a8654c0e4d..d76f7389e101 100644 --- a/synapse/rest/media/v1/upload_resource.py +++ b/synapse/rest/media/v1/upload_resource.py @@ -63,8 +63,8 @@ async def _async_render_POST(self, request): msg="Invalid UTF-8 filename parameter: %r" % (upload_name), code=400 ) - # upload_name might be an empty byte string - if not len(upload_name): + # If the name is falsey (e.g. an empty byte string) ensure it is None. + else: upload_name = None headers = request.requestHeaders From 9b2a1cb5eb2b9719ed7e190981de4f29cb12658e Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Tue, 29 Sep 2020 15:50:31 +0100 Subject: [PATCH 11/12] Update changelog --- changelog.d/7905.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/7905.bugfix b/changelog.d/7905.bugfix index 73d8efd8cd0e..8ed9e1313ca3 100644 --- a/changelog.d/7905.bugfix +++ b/changelog.d/7905.bugfix @@ -1 +1 @@ -Fix synapse storing a media file with an invalid media_type or upload_name. +Fix synapse storing a media file with an invalid upload_name. From 0dc79d3fa6c7c720bc625d76e81af7a98d17ab61 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 29 Sep 2020 12:15:01 -0400 Subject: [PATCH 12/12] Update changelog. --- changelog.d/7905.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/7905.bugfix b/changelog.d/7905.bugfix index 8ed9e1313ca3..e60e62441210 100644 --- a/changelog.d/7905.bugfix +++ b/changelog.d/7905.bugfix @@ -1 +1 @@ -Fix synapse storing a media file with an invalid upload_name. +Fix a longstanding bug when storing a media file with an empty `upload_name`.