fix: use is_file_path_valid instead of is_safe_path (frappe#18316) (

frappe#18642)

* fix: use `validate_file_path` instead of `is_safe_path`

* test: specify `is_private` for file with private URL

(cherry picked from commit d35aa6c)

Co-authored-by: Sagar Vora <sagar@resilient.tech>

frappe/core/doctype/file/file.py

@@ -16,7 +16,6 @@
 from frappe import _
 from frappe.model.document import Document
 from frappe.utils import call_hook_method, cint, get_files_path, get_hook_method, get_url
-from frappe.utils.file_manager import is_safe_path
 from frappe.utils.image import optimize_image, strip_exif_data
 
 from .exceptions import AttachmentLimitReached, FolderNotEmpty, MaxFileSizeReachedError
@@ -87,9 +86,9 @@ def validate(self):
 			self.handle_is_private_changed()
 
 		if not self.is_folder:
-			self.validate_file_path()
-			self.validate_file_url()
-			self.validate_file_on_disk()
+			# get_full_path validates file URL and name
+			full_path = self.get_full_path()
+			self.validate_file_on_disk(full_path)
 
 		self.file_size = frappe.form_dict.file_size or self.file_size
 
@@ -140,16 +139,12 @@ def get_name_based_on_parent_folder(self) -> str | None:
 	def get_successors(self):
 		return frappe.get_all("File", filters={"folder": self.name}, pluck="name")
 
-	def validate_file_path(self):
-		if self.is_remote_file:
-			return
+	def is_file_path_valid(self, file_path):
+		"""Return True if file path is a valid path for a local file"""
 
 		base_path = os.path.realpath(get_files_path(is_private=self.is_private))
-		if not os.path.realpath(self.get_full_path()).startswith(base_path):
-			frappe.throw(
-				_("The File URL you've entered is incorrect"),
-				title=_("Invalid File URL"),
-			)
+		if os.path.realpath(file_path).startswith(base_path):
+			return True
 
 	def validate_file_url(self):
 		if self.is_remote_file or not self.file_url:
@@ -276,9 +271,11 @@ def set_folder_name(self):
 		elif not self.is_home_folder:
 			self.folder = "Home"
 
-	def validate_file_on_disk(self):
+	def validate_file_on_disk(self, full_path=None):
 		"""Validates existence file"""
-		full_path = self.get_full_path()
+
+		if full_path is None:
+			full_path = self.get_full_path()
 
 		if full_path.startswith(URL_PREFIXES):
 			return True
@@ -458,6 +455,9 @@ def get_full_path(self):
 		if "/files/" in file_path and file_path.startswith(site_url):
 			file_path = file_path.split(site_url, 1)[1]
 
+		if file_path.startswith(URL_PREFIXES):
+			return file_path
+
 		if "/" not in file_path:
 			if self.is_private:
 				file_path = f"/private/files/{file_path}"
@@ -470,13 +470,10 @@ def get_full_path(self):
 		elif file_path.startswith("/files/"):
 			file_path = get_files_path(*file_path.split("/files/", 1)[1].split("/"))
 
-		elif file_path.startswith(URL_PREFIXES):
-			pass
-
 		elif not self.file_url:
 			frappe.throw(_("There is some problem with the file url: {0}").format(file_path))
 
-		if not is_safe_path(file_path):
+		if not self.is_file_path_valid(file_path):
 			frappe.throw(_("Cannot access file path {0}").format(file_path))
 
 		if os.path.sep in self.file_name:

frappe/core/doctype/file/test_file.py

@@ -433,6 +433,7 @@ def test_file_url_validation(self):
 		self.assertRaisesRegex(IOError, "does not exist", test_file.validate)
 
 		test_file.file_url = None
+		test_file.is_private = 1
 		test_file.file_name = "/private/files/_file"
 		self.assertRaisesRegex(ValidationError, "File name cannot have", test_file.validate)
 

frappe/utils/file_manager.py

@@ -449,15 +449,3 @@ def add_attachments(doctype, name, attachments):
 			files.append(f)
 
 	return files
-
-
-def is_safe_path(path: str) -> bool:
-	if path.startswith(("http://", "https://")):
-		return True
-
-	basedir = frappe.get_site_path()
-	# ref: https://docs.python.org/3/library/os.path.html#os.path.commonpath
-	matchpath = os.path.realpath(os.path.abspath(path))
-	basedir = os.path.realpath(os.path.abspath(basedir))
-
-	return basedir == os.path.commonpath((basedir, matchpath))