From b4e43257c3e0e15bd171e080711927a8724b111e Mon Sep 17 00:00:00 2001 From: Shariq Ansari <30859809+shariquerik@users.noreply.github.com> Date: Wed, 4 May 2022 19:07:51 +0530 Subject: [PATCH] fix: bad query if user has ' in the email address (#16796) --- frappe/core/doctype/user/test_records.json | 7 ++++ .../dashboard_settings/dashboard_settings.py | 2 +- .../desk/doctype/kanban_board/kanban_board.py | 4 +- frappe/desk/doctype/note/note.py | 2 +- .../notification_log/notification_log.py | 2 +- .../notification_settings.py | 2 +- .../desk/doctype/number_card/number_card.py | 38 ++++++------------- frappe/tests/test_db_query.py | 23 +++++++++++ .../workflow_action/workflow_action.py | 9 +++-- 9 files changed, 54 insertions(+), 35 deletions(-) diff --git a/frappe/core/doctype/user/test_records.json b/frappe/core/doctype/user/test_records.json index 21fe3ff69d..9d1bf0efd4 100644 --- a/frappe/core/doctype/user/test_records.json +++ b/frappe/core/doctype/user/test_records.json @@ -45,6 +45,13 @@ "new_password": "Eastern_43A1W", "enabled": 1 }, + { + "doctype": "User", + "email": "test'5@example.com", + "first_name": "_Test'5", + "new_password": "Eastern_43A1W", + "enabled": 1 + }, { "doctype": "User", "email": "testperm@example.com", diff --git a/frappe/desk/doctype/dashboard_settings/dashboard_settings.py b/frappe/desk/doctype/dashboard_settings/dashboard_settings.py index d43476ad7d..01c3a87f20 100644 --- a/frappe/desk/doctype/dashboard_settings/dashboard_settings.py +++ b/frappe/desk/doctype/dashboard_settings/dashboard_settings.py @@ -28,7 +28,7 @@ def get_permission_query_conditions(user): if not user: user = frappe.session.user - return """(`tabDashboard Settings`.name = '{user}')""".format(user=user) + return """(`tabDashboard Settings`.name = {user})""".format(user=frappe.db.escape(user)) @frappe.whitelist() diff --git a/frappe/desk/doctype/kanban_board/kanban_board.py b/frappe/desk/doctype/kanban_board/kanban_board.py index 381f71438c..bc47bbbaf4 100644 --- a/frappe/desk/doctype/kanban_board/kanban_board.py +++ b/frappe/desk/doctype/kanban_board/kanban_board.py @@ -34,7 +34,9 @@ def get_permission_query_conditions(user): if user == "Administrator": return "" - return """(`tabKanban Board`.private=0 or `tabKanban Board`.owner='{user}')""".format(user=user) + return """(`tabKanban Board`.private=0 or `tabKanban Board`.owner={user})""".format( + user=frappe.db.escape(user) + ) def has_permission(doc, ptype, user): diff --git a/frappe/desk/doctype/note/note.py b/frappe/desk/doctype/note/note.py index de019d9898..d67ecda594 100644 --- a/frappe/desk/doctype/note/note.py +++ b/frappe/desk/doctype/note/note.py @@ -38,7 +38,7 @@ def get_permission_query_conditions(user): if user == "Administrator": return "" - return """(`tabNote`.public=1 or `tabNote`.owner="{user}")""".format(user=user) + return """(`tabNote`.public=1 or `tabNote`.owner={user})""".format(user=frappe.db.escape(user)) def has_permission(doc, ptype, user): diff --git a/frappe/desk/doctype/notification_log/notification_log.py b/frappe/desk/doctype/notification_log/notification_log.py index 011f3e22ff..9e7ed7d9fe 100644 --- a/frappe/desk/doctype/notification_log/notification_log.py +++ b/frappe/desk/doctype/notification_log/notification_log.py @@ -30,7 +30,7 @@ def get_permission_query_conditions(for_user): if for_user == "Administrator": return - return """(`tabNotification Log`.for_user = '{user}')""".format(user=for_user) + return """(`tabNotification Log`.for_user = {user})""".format(user=frappe.db.escape(for_user)) def get_title(doctype, docname, title_field=None): diff --git a/frappe/desk/doctype/notification_settings/notification_settings.py b/frappe/desk/doctype/notification_settings/notification_settings.py index bbb4a62154..ee425e154b 100644 --- a/frappe/desk/doctype/notification_settings/notification_settings.py +++ b/frappe/desk/doctype/notification_settings/notification_settings.py @@ -81,7 +81,7 @@ def get_permission_query_conditions(user): if "System Manager" in roles: return """(`tabNotification Settings`.name != 'Administrator')""" - return """(`tabNotification Settings`.name = '{user}')""".format(user=user) + return """(`tabNotification Settings`.name = {user})""".format(user=frappe.db.escape(user)) @frappe.whitelist() diff --git a/frappe/desk/doctype/number_card/number_card.py b/frappe/desk/doctype/number_card/number_card.py index e1b2b19026..d6d4f00b69 100644 --- a/frappe/desk/doctype/number_card/number_card.py +++ b/frappe/desk/doctype/number_card/number_card.py @@ -8,6 +8,8 @@ from frappe.config import get_modules_from_all_apps_for_user from frappe.model.document import Document from frappe.model.naming import append_number_if_name_exists from frappe.modules.export_file import export_to_files +from frappe.query_builder import Criterion +from frappe.query_builder.utils import DocType from frappe.utils import cint @@ -190,36 +192,18 @@ def get_cards_for_user(doctype, txt, searchfield, start, page_len, filters): if not frappe.db.exists("DocType", doctype): return + numberCard = DocType("Number Card") + if txt: - for field in searchfields: - search_conditions.append( - "`tab{doctype}`.`{field}` like %(txt)s".format(field=field, doctype=doctype, txt=txt) - ) + search_conditions = [numberCard[field].like("%{txt}%".format(txt=txt)) for field in searchfields] - search_conditions = " or ".join(search_conditions) + condition_query = frappe.db.query.build_conditions(doctype, filters) - search_conditions = "and (" + search_conditions + ")" if search_conditions else "" - conditions, values = frappe.db.build_conditions(filters) - values["txt"] = "%" + txt + "%" - - return frappe.db.sql( - """select - `tabNumber Card`.name, `tabNumber Card`.label, `tabNumber Card`.document_type - from - `tabNumber Card` - where - {conditions} and - (`tabNumber Card`.owner = '{user}' or - `tabNumber Card`.is_public = 1) - {search_conditions} - """.format( - filters=filters, - user=frappe.session.user, - search_conditions=search_conditions, - conditions=conditions, - ), - values, - ) + return ( + condition_query.select(numberCard.name, numberCard.label, numberCard.document_type) + .where((numberCard.owner == frappe.session.user) | (numberCard.is_public == 1)) + .where(Criterion.any(search_conditions)) + ).run() @frappe.whitelist() diff --git a/frappe/tests/test_db_query.py b/frappe/tests/test_db_query.py index 19a8c445f8..dd67d68cd2 100644 --- a/frappe/tests/test_db_query.py +++ b/frappe/tests/test_db_query.py @@ -692,6 +692,29 @@ class TestReportview(unittest.TestCase): dt.delete() table_dt.delete() + def test_permission_query_condition(self): + from frappe.desk.doctype.dashboard_settings.dashboard_settings import create_dashboard_settings + + self.doctype = "Dashboard Settings" + self.user = "test'5@example.com" + + permission_query_conditions = DatabaseQuery.get_permission_query_conditions(self) + + create_dashboard_settings(self.user) + + dashboard_settings = frappe.db.sql( + """ + SELECT name + FROM `tabDashboard Settings` + WHERE {condition} + """.format( + condition=permission_query_conditions + ), + as_dict=1, + )[0] + + self.assertTrue(dashboard_settings) + def add_child_table_to_blog_post(): child_table = frappe.get_doc( diff --git a/frappe/workflow/doctype/workflow_action/workflow_action.py b/frappe/workflow/doctype/workflow_action/workflow_action.py index abbcda5c4c..7b54df2d51 100644 --- a/frappe/workflow/doctype/workflow_action/workflow_action.py +++ b/frappe/workflow/doctype/workflow_action/workflow_action.py @@ -53,9 +53,12 @@ def get_permission_query_conditions(user): .where(WorkflowActionPermittedRole.role.isin(roles)) ).get_sql() - return f"""(`tabWorkflow Action`.`name` in ({permitted_workflow_actions}) - or `tabWorkflow Action`.`user`='{user}') - and `tabWorkflow Action`.`status`='Open'""" + return """(`tabWorkflow Action`.`name` in ({permitted_workflow_actions}) + or `tabWorkflow Action`.`user`={user}) + and `tabWorkflow Action`.`status`='Open' + """.format( + permitted_workflow_actions=permitted_workflow_actions, user=frappe.db.escape(user) + ) def has_permission(doc, user):