Merge pull request #1470 from anandpdoshi/fix/private-file-with-percent-character

[fix] A percentage in file name caused 404 error in case of private file while checking permissions

frappe/core/doctype/error_snapshot/error_snapshot.json

@@ -61,7 +61,7 @@
    "bold": 0, 
    "collapsible": 0, 
    "fieldname": "evalue", 
-   "fieldtype": "Data", 
+   "fieldtype": "Small Text", 
    "hidden": 1, 
    "ignore_user_permissions": 0, 
    "in_filter": 0, 
@@ -308,7 +308,7 @@
  "issingle": 0, 
  "istable": 0, 
  "max_attachments": 0, 
- "modified": "2015-12-16 09:03:57.521251", 
+ "modified": "2015-12-21 05:07:12.840575", 
  "modified_by": "Administrator", 
  "module": "Core", 
  "name": "Error Snapshot", 

frappe/database.py

@@ -784,8 +784,19 @@ class Database:
 			self._cursor = None
 			self._conn = None
 
-	def escape(self, s):
+	def escape(self, s, percent=True):
 		"""Excape quotes and percent in given string."""
 		if isinstance(s, unicode):
 			s = (s or "").encode("utf-8")
-		return unicode(MySQLdb.escape_string(s), "utf-8").replace("%","%%").replace("`", "\\`")
+
+		s = unicode(MySQLdb.escape_string(s), "utf-8").replace("`", "\\`")
+
+		# NOTE separating % escape, because % escape should only be done when using LIKE operator
+		# or when you use python format string to generate query that already has a %s
+		# for example: sql("select name from `tabUser` where name=%s and {0}".format(conditions), something)
+		# defaulting it to True, as this is the most frequent use case
+		# ideally we shouldn't have to use ESCAPE and strive to pass values via the values argument of sql
+		if percent:
+			s = s.replace("%", "%%")
+
+		return s

frappe/desk/reportview.py

@@ -177,7 +177,7 @@ def scrub_user_tags(tagcount):
 # used in building query in queries.py
 def get_match_cond(doctype):
 	cond = DatabaseQuery(doctype).build_match_conditions()
-	return (' and ' + cond) if cond else ""
+	return ((' and ' + cond) if cond else "").replace("%", "%%")
 
 def build_match_conditions(doctype, as_condition=True):
-	return DatabaseQuery(doctype).build_match_conditions(as_condition=as_condition)
+	return (DatabaseQuery(doctype).build_match_conditions(as_condition=as_condition)).replace("%", "%%")

frappe/model/db_query.py

@@ -226,7 +226,7 @@ class DatabaseQuery(object):
 			if not isinstance(values, (list, tuple)):
 				values = values.split(",")
 
-			values = (frappe.db.escape(v.strip()) for v in values)
+			values = (frappe.db.escape(v.strip(), percent=False) for v in values)
 			values = '("{0}")'.format('", "'.join(values))
 
 			condition = 'ifnull({tname}.{fname}, "") {operator} {value}'.format(
@@ -253,9 +253,9 @@ class DatabaseQuery(object):
 					value = "" if f.value==None else f.value
 					fallback = '""'
 
-					if f.operator == "like" and isinstance(value, basestring):
+					if f.operator in ("like", "not like") and isinstance(value, basestring):
 						# because "like" uses backslash (\) for escaping
-						value = value.replace("\\", "\\\\")
+						value = value.replace("\\", "\\\\").replace("%", "%%")
 
 			else:
 				value = flt(f.value)
@@ -263,14 +263,13 @@ class DatabaseQuery(object):
 
 			# put it inside double quotes
 			if isinstance(value, basestring):
-				value = '"{0}"'.format(frappe.db.escape(value))
+				value = '"{0}"'.format(frappe.db.escape(value, percent=False))
 
 			condition = 'ifnull({tname}.{fname}, {fallback}) {operator} {value}'.format(
 				tname=tname, fname=f.fieldname, fallback=fallback, operator=f.operator,
 				value=value)
 
-		# replace % with %% to prevent python format string error
-		return condition.replace("%", "%%")
+		return condition
 
 	def get_filter(self, f):
 		"""Returns a _dict like
@@ -341,7 +340,7 @@ class DatabaseQuery(object):
 
 			if role_permissions.get("if_owner", {}).get("read"):
 				self.match_conditions.append("`tab{0}`.owner = '{1}'".format(self.doctype,
-					frappe.db.escape(frappe.session.user)))
+					frappe.db.escape(frappe.session.user, percent=False)))
 
 		if as_condition:
 			conditions = ""
@@ -358,15 +357,14 @@ class DatabaseQuery(object):
 				conditions =  "({conditions}) or ({shared_condition})".format(
 					conditions=conditions, shared_condition=self.get_share_condition())
 
-			# replace % with %% to prevent python format string error
-			return conditions.replace("%", "%%")
+			return conditions
 
 		else:
 			return self.match_filters
 
 	def get_share_condition(self):
 		return """`tab{0}`.name in ({1})""".format(self.doctype, ", ".join(["'%s'"] * len(self.shared))) % \
-			tuple([frappe.db.escape(s) for s in self.shared])
+			tuple([frappe.db.escape(s, percent=False) for s in self.shared])
 
 	def add_user_permissions(self, user_permissions, user_permission_doctypes=None):
 		user_permission_doctypes = frappe.permissions.get_user_permission_doctypes(user_permission_doctypes, user_permissions)
@@ -383,7 +381,7 @@ class DatabaseQuery(object):
 				if user_permission_values:
 					condition += """ or `tab{doctype}`.`{fieldname}` in ({values})""".format(
 						doctype=self.doctype, fieldname=df.fieldname,
-						values=", ".join([('"'+frappe.db.escape(v)+'"') for v in user_permission_values])
+						values=", ".join([('"'+frappe.db.escape(v, percent=False)+'"') for v in user_permission_values])
 					)
 				match_conditions.append("({condition})".format(condition=condition))
 

frappe/utils/error.py

@@ -128,14 +128,14 @@ def get_snapshot(exception, context=10):
 				value = pydoc.text.repr(getattr(evalue, name))
 
 				# render multilingual string properly
-				if type(value)==str and value.startswith("u'"):
+				if type(value)==str and value.startswith(b"u'"):
 					value = eval(value)
 
 				s['exception'][name] = encode(value)
 
 	# add all local values (of last frame) to the snapshot
 	for name, value in locals.items():
-		if type(value)==str and value.startswith("u'"):
+		if type(value)==str and value.startswith(b"u'"):
 			value = eval(value)
 
 		s['locals'][name] = pydoc.text.repr(value)