From aa9f0d365c8e8f6e9f8a8e3d1b416adbf11a2ddd Mon Sep 17 00:00:00 2001 From: Corentin Flr <10946971+cogk@users.noreply.github.com> Date: Wed, 5 Jun 2024 17:22:48 +0200 Subject: [PATCH 1/5] fix(File): Unquote file_url only once --- frappe/core/doctype/file/file.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frappe/core/doctype/file/file.py b/frappe/core/doctype/file/file.py index 13435d5d47..c5b218f42f 100755 --- a/frappe/core/doctype/file/file.py +++ b/frappe/core/doctype/file/file.py @@ -90,6 +90,9 @@ class File(Document): self.name = frappe.generate_hash(length=10) def before_insert(self): + # Ensure correct formatting and type + self.file_url = unquote(self.file_url) if self.file_url else "" + self.set_folder_name() self.set_is_private() self.set_file_name() @@ -117,9 +120,6 @@ class File(Document): if self.is_folder: return - # Ensure correct formatting and type - self.file_url = unquote(self.file_url) if self.file_url else "" - self.validate_attachment_references() # when dict is passed to get_doc for creation of new_doc, is_new returns None From b23f24a4fc19a23f0b246a97c875855816e05906 Mon Sep 17 00:00:00 2001 From: Corentin Flr <10946971+cogk@users.noreply.github.com> Date: Wed, 5 Jun 2024 17:24:18 +0200 Subject: [PATCH 2/5] fix(File): Disallow percent sign in file_name --- frappe/core/doctype/file/file.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/frappe/core/doctype/file/file.py b/frappe/core/doctype/file/file.py index c5b218f42f..e2c8228ae2 100755 --- a/frappe/core/doctype/file/file.py +++ b/frappe/core/doctype/file/file.py @@ -675,10 +675,11 @@ class File(Document): return self.save_file_on_filesystem() def save_file_on_filesystem(self): + safe_file_name = self.file_name.replace("%", "_") if self.is_private: - self.file_url = f"/private/files/{self.file_name}" + self.file_url = f"/private/files/{safe_file_name}" else: - self.file_url = f"/files/{self.file_name}" + self.file_url = f"/files/{safe_file_name}" fpath = self.write_file() From 71f111e26796e728b745fffc64e3c96642b27c51 Mon Sep 17 00:00:00 2001 From: Corentin Flr <10946971+cogk@users.noreply.github.com> Date: Wed, 5 Jun 2024 17:24:39 +0200 Subject: [PATCH 3/5] fix(InboundEmail): Unquote percent-encoded file names --- frappe/email/receive.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frappe/email/receive.py b/frappe/email/receive.py index d7f0ed0721..0227f726e8 100644 --- a/frappe/email/receive.py +++ b/frappe/email/receive.py @@ -12,6 +12,7 @@ import ssl from contextlib import suppress from email.errors import HeaderParseError from email.header import decode_header +from urllib.parse import unquote import _socket import chardet @@ -566,7 +567,7 @@ class Email: _file = frappe.get_doc( { "doctype": "File", - "file_name": attachment["fname"], + "file_name": unquote(attachment["fname"]), "attached_to_doctype": doc.doctype, "attached_to_name": doc.name, "is_private": 1, From 6b7c0007fee4256a54c5a2727180ff44d1960802 Mon Sep 17 00:00:00 2001 From: Corentin Flr <10946971+cogk@users.noreply.github.com> Date: Wed, 5 Jun 2024 18:09:39 +0200 Subject: [PATCH 4/5] test(File): Add test for attachments with percent-encoded file names --- frappe/email/test_email_attachments.py | 89 ++++++++++++++++++++++++++ 1 file changed, 89 insertions(+) create mode 100644 frappe/email/test_email_attachments.py diff --git a/frappe/email/test_email_attachments.py b/frappe/email/test_email_attachments.py new file mode 100644 index 0000000000..9fe8ea6e9b --- /dev/null +++ b/frappe/email/test_email_attachments.py @@ -0,0 +1,89 @@ +from typing import TYPE_CHECKING +from urllib.parse import quote + +import requests + +import frappe +from frappe.email.receive import InboundMail +from frappe.tests.utils import FrappeTestCase +from frappe.utils import get_url + +if TYPE_CHECKING: + from frappe.core.doctype.file.file import File + +EMAIL_CONTENT = """ +Delivered-To: test@example.com +From: Test +Date: Wed, 1 Jun 2024 12:00:00 +0000 +Message-ID: +Subject: Test +To: Test +Content-Type: multipart/mixed; boundary="ABC" + +--ABC +Content-Type: application/pdf; name="t%C3%A9st%2542.txt" +Content-Disposition: attachment; filename="t%C3%A9st%2542.txt" +Content-Transfer-Encoding: base64 +Content-ID: +X-Attachment-Id: test_content_id + +YWJjZGVmZ2hpamtsbW5vcF9hdHRhY2htZW50 +--ABC-- +""".strip() + + +class TestEmailAttachments(FrappeTestCase): + def test_email_attachment_percent_encoded(self): + email_account = frappe._dict({"email_id": "receive@example.com"}) + mail = InboundMail(EMAIL_CONTENT, email_account) + communication = mail.process() + file: "File" = frappe.get_last_doc( + "File", + { + "attached_to_doctype": communication.doctype, + "attached_to_name": communication.name, + }, + ) # type: ignore + self.assertEqual(file.file_name, "tést%42.txt") + file.save() + self.assertEqual(file.file_name, "tést%42.txt") + + def test_file_with_percent_in_filename(self): + def make_and_check_file(index: int, literal_file_name: str, disk_file_name: str): + content = "abcdefghijklmnop_attachment" + file: "File" = frappe.new_doc("File") # type: ignore + file.update( + { + "file_name": literal_file_name, + "is_private": 0, + "content": f"{content}{index}", + } + ) + file.save() + + # Check that the file name is kept as is + self.assertEqual(file.file_name, literal_file_name) + + # Check that the file URL is not encoded + self.assertEqual(file.file_url, "/files/" + disk_file_name) + + # Try to download the file, first quoting it + # To download a file named "test%42.txt", we would need to request "test%2542.txt", which is confusing + # Instead, we should request "test_42.txt", which is the actual file name + assert file.file_url + + res = requests.get(get_url(quote(file.file_url))) + res.raise_for_status() + + res = requests.get(get_url("/files/" + disk_file_name)) + res.raise_for_status() + + # Saving again should not change the file_name or file_url + values = file.as_dict() + file.save() + self.assertEqual(file.file_name, literal_file_name) + self.assertEqual(file.file_url, values["file_url"]) + + make_and_check_file(1, "1test%2542.txt", "1test_2542.txt") + make_and_check_file(2, "2test%42.txt", "2test_42.txt") + make_and_check_file(3, "3test*.txt", "3test*.txt") From 772df163cfa581565401f33a6d2563e49c848ae8 Mon Sep 17 00:00:00 2001 From: Corentin Forler Date: Mon, 16 Sep 2024 13:43:37 +0200 Subject: [PATCH 5/5] fix(file): Remove illegal chars in file_name - / and \ cannot work as file names - % is not allowed verbatim in URLs - ? and # are not allowed because confused with query/hash separators --- frappe/core/doctype/file/file.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/core/doctype/file/file.py b/frappe/core/doctype/file/file.py index e2c8228ae2..56492790dd 100755 --- a/frappe/core/doctype/file/file.py +++ b/frappe/core/doctype/file/file.py @@ -675,7 +675,7 @@ class File(Document): return self.save_file_on_filesystem() def save_file_on_filesystem(self): - safe_file_name = self.file_name.replace("%", "_") + safe_file_name = re.sub(r"[/\\%?#]", "_", self.file_name) if self.is_private: self.file_url = f"/private/files/{safe_file_name}" else: