Merge pull request #26693 from cogk/fix-attachment-name-with-percent

fix: Fix file/email attachment name containing percent sign

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
@@ -675,10 +675,11 @@ class File(Document):
 			return self.save_file_on_filesystem()
 
 	def save_file_on_filesystem(self):
+		safe_file_name = re.sub(r"[/\\%?#]", "_", self.file_name)
 		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()
 

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,

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 <test@example.com>
+Date: Wed, 1 Jun 2024 12:00:00 +0000
+Message-ID: <test@mail.example.com>
+Subject: Test
+To: Test <test@example.com>
+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: <test_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")