From 9f5a994f709898d157f55783cfa96c5702e0634a Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Tue, 30 May 2023 13:51:18 +0530 Subject: [PATCH 1/3] fix!: improved filter validation in `Engine.get_query` --- frappe/database/database.py | 26 +++++++++++++++---- frappe/database/query.py | 15 ++++++++--- .../desk/doctype/number_card/number_card.py | 6 ++++- frappe/desk/listview.py | 7 ++++- frappe/tests/test_query.py | 7 ----- frappe/utils/goal.py | 1 + 6 files changed, 44 insertions(+), 18 deletions(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index 8b077ce4f7..aa30cd9ae9 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -812,6 +812,7 @@ class Database: fields=fields, distinct=distinct, limit=limit, + validate_filters=True, ) if isinstance(fields, str) and fields == "*": as_dict = True @@ -840,6 +841,7 @@ class Database: order_by=order_by, distinct=distinct, limit=limit, + validate_filters=True, ).run(debug=debug, run=run, as_dict=as_dict, pluck=pluck) return {} @@ -889,7 +891,12 @@ class Database: field, val, modified=modified, modified_by=modified_by, update_modified=update_modified ) - query = frappe.qb.get_query(table=dt, filters=dn, update=True) + query = frappe.qb.get_query( + table=dt, + filters=dn, + update=True, + validate_filters=True, + ) if isinstance(dn, str): frappe.clear_document_cache(dt, dn) @@ -1057,9 +1064,13 @@ class Database: cache_count = frappe.cache().get_value(f"doctype:count:{dt}") if cache_count is not None: return cache_count - count = frappe.qb.get_query(table=dt, filters=filters, fields=Count("*"), distinct=distinct).run( - debug=debug - )[0][0] + count = frappe.qb.get_query( + table=dt, + filters=filters, + fields=Count("*"), + distinct=distinct, + validate_filters=True, + ).run(debug=debug)[0][0] if not filters and cache: frappe.cache().set_value(f"doctype:count:{dt}", count, expires_in_sec=86400) return count @@ -1179,7 +1190,12 @@ class Database: Doctype name can be passed directly, it will be pre-pended with `tab`. """ filters = filters or kwargs.get("conditions") - query = frappe.qb.get_query(table=doctype, filters=filters, delete=True) + query = frappe.qb.get_query( + table=doctype, + filters=filters, + delete=True, + validate_filters=True, + ) if "debug" not in kwargs: kwargs["debug"] = debug return query.run(**kwargs) diff --git a/frappe/database/query.py b/frappe/database/query.py index 02beff9afc..eed52329d2 100644 --- a/frappe/database/query.py +++ b/frappe/database/query.py @@ -9,6 +9,7 @@ from pypika.queries import QueryBuilder, Table import frappe from frappe import _ from frappe.database.operator_map import OPERATOR_MAP +from frappe.database.schema import SPECIAL_CHAR_PATTERN from frappe.database.utils import DefaultOrderBy, get_doctype_name from frappe.query_builder import Criterion, Field, Order, functions from frappe.query_builder.functions import Function, SqlFunctions @@ -44,6 +45,8 @@ class Engine: update: bool = False, into: bool = False, delete: bool = False, + *, + validate_filters: bool = False, ) -> QueryBuilder: self.is_mariadb = frappe.db.db_type == "mariadb" self.is_postgres = frappe.db.db_type == "postgres" @@ -56,6 +59,8 @@ class Engine: self.validate_doctype() self.table = frappe.qb.DocType(table) + self.validate_filters = validate_filters + if update: self.query = frappe.qb.update(self.table) elif into: @@ -157,14 +162,16 @@ class Engine: _value = value _operator = operator - if isinstance(_field, Field): + if not isinstance(_field, str): pass - elif dynamic_field := DynamicTableField.parse(field, self.doctype): + elif not self.validate_filters and ( + dynamic_field := DynamicTableField.parse(field, self.doctype) + ): # apply implicit join if link field's field is referenced self.query = dynamic_field.apply_join(self.query) _field = dynamic_field.field - elif has_function(field): - _field = self.get_function_object(field) + elif self.validate_filters and SPECIAL_CHAR_PATTERN.search(_field): + frappe.throw(_("Invalid filter: {0}").format(_field)) elif not doctype or doctype == self.doctype: _field = self.table[field] elif doctype: diff --git a/frappe/desk/doctype/number_card/number_card.py b/frappe/desk/doctype/number_card/number_card.py index 451dc699fe..a8e4841953 100644 --- a/frappe/desk/doctype/number_card/number_card.py +++ b/frappe/desk/doctype/number_card/number_card.py @@ -202,7 +202,11 @@ def get_cards_for_user(doctype, txt, searchfield, start, page_len, filters): if txt: search_conditions = [numberCard[field].like(f"%{txt}%") for field in searchfields] - condition_query = frappe.qb.get_query(doctype, filters=filters) + condition_query = frappe.qb.get_query( + doctype, + filters=filters, + validate_filters=True, + ) return ( condition_query.select(numberCard.name, numberCard.label, numberCard.document_type) diff --git a/frappe/desk/listview.py b/frappe/desk/listview.py index 05d45ad9ac..a1db82810e 100644 --- a/frappe/desk/listview.py +++ b/frappe/desk/listview.py @@ -36,7 +36,12 @@ def get_group_by_count(doctype: str, current_filters: str, field: str) -> list[d ToDo = DocType("ToDo") User = DocType("User") count = Count("*").as_("count") - filtered_records = frappe.qb.get_query(doctype, filters=current_filters, fields=["name"]) + filtered_records = frappe.qb.get_query( + doctype, + filters=current_filters, + fields=["name"], + validate_filters=True, + ) return ( frappe.qb.from_(ToDo) diff --git a/frappe/tests/test_query.py b/frappe/tests/test_query.py index dfebf5e890..b3270c7c13 100644 --- a/frappe/tests/test_query.py +++ b/frappe/tests/test_query.py @@ -218,13 +218,6 @@ class TestQuery(FrappeTestCase): @run_only_if(db_type_is.MARIADB) def test_filters(self): - self.assertEqual( - frappe.qb.get_query( - "User", filters={"IfNull(name, " ")": ("<", Now())}, fields=["Max(name)"] - ).run(), - frappe.qb.from_("User").select(Max(Field("name"))).where(Ifnull("name", "") < Now()).run(), - ) - self.assertEqual( frappe.qb.get_query( "DocType", diff --git a/frappe/utils/goal.py b/frappe/utils/goal.py index 709fdc1644..01cd9d835e 100644 --- a/frappe/utils/goal.py +++ b/frappe/utils/goal.py @@ -31,6 +31,7 @@ def get_monthly_results( Function(aggregation, goal_field), ], filters=filters, + validate_filters=True, ) .groupby("month_year") .run() From 81d5160ac1e61681e3c8a4c1737e036023919ec3 Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Tue, 30 May 2023 17:49:51 +0530 Subject: [PATCH 2/3] test: ensure stricter filters when `validate_filters` is passed --- frappe/tests/test_query.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/frappe/tests/test_query.py b/frappe/tests/test_query.py index b3270c7c13..9242630104 100644 --- a/frappe/tests/test_query.py +++ b/frappe/tests/test_query.py @@ -251,6 +251,17 @@ class TestQuery(FrappeTestCase): ), ) + self.assertRaisesRegex( + frappe.ValidationError, + "Invalid filter", + lambda: frappe.qb.get_query( + "DocType", + fields=["name"], + filters={"permissions.role": "System Manager"}, + validate_filters=True, + ), + ) + self.assertEqual( frappe.qb.get_query( "DocType", From 1b2d1dd5678afca852661aa52406173925c73f7f Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Wed, 31 May 2023 14:20:26 +0530 Subject: [PATCH 3/3] chore: move statement to set `validate_filters` property --- frappe/database/query.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/frappe/database/query.py b/frappe/database/query.py index eed52329d2..06295d33a6 100644 --- a/frappe/database/query.py +++ b/frappe/database/query.py @@ -50,6 +50,7 @@ class Engine: ) -> QueryBuilder: self.is_mariadb = frappe.db.db_type == "mariadb" self.is_postgres = frappe.db.db_type == "postgres" + self.validate_filters = validate_filters if isinstance(table, Table): self.table = table @@ -59,8 +60,6 @@ class Engine: self.validate_doctype() self.table = frappe.qb.DocType(table) - self.validate_filters = validate_filters - if update: self.query = frappe.qb.update(self.table) elif into: