From 60ec324956164eaa95709dde48226a19b2b4035b Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 14 Jun 2022 16:40:41 +0530 Subject: [PATCH 1/4] fix: Remove unwanted blacklist over fields A field like 'count(`tabBOM Update Log`.name) as total_count' split by whitespace loses it's meaning. Tried splitting it meaningfully but didn't get the point of this tbh and stopped. I'm not sure what the code before was trying to do and with what set of inputs. Imagine the following fields: [count(`tabBOM Update Log`.name) as total_count, `tabBOM Update Log`.name as update_name, `tabBOM Update Log`.name, `tabBOM Update as Log`.name, tabBOM.name, name], I couldn't see what the previous check was trying to protect - hence, didn't add any equivalent functionality. --- frappe/model/db_query.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py index fe52818235..91c70f5b97 100644 --- a/frappe/model/db_query.py +++ b/frappe/model/db_query.py @@ -378,9 +378,6 @@ class DatabaseQuery(object): for field in self.fields: if sub_query_regex.match(field): - if any(keyword in field.lower().split() for keyword in blacklisted_keywords): - _raise_exception() - if any(f"({keyword}" in field.lower() for keyword in blacklisted_keywords): _raise_exception() From 678eebe4fb8531413b88865f3e1bea162758910e Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 14 Jun 2022 17:16:34 +0530 Subject: [PATCH 2/4] perf: Limit re internal cache to avoid caching patterns twice --- frappe/__init__.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/frappe/__init__.py b/frappe/__init__.py index 7c71bc8815..c4f4b2a690 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -15,6 +15,7 @@ import importlib import inspect import json import os +import re import warnings from typing import TYPE_CHECKING, Any, Dict, List, Optional, Union @@ -45,6 +46,10 @@ STANDARD_USERS = ("Guest", "Administrator") _dev_server = int(sbool(os.environ.get("DEV_SERVER", False))) _qb_patched = False +re._MAXCACHE = ( + 50 # reduced from default 512 given we are already maintaining this on parent worker +) + if _dev_server: warnings.simplefilter("always", DeprecationWarning) From 9b4db43b84ead21a135fa0ce29e810306c01f9b5 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 14 Jun 2022 17:17:22 +0530 Subject: [PATCH 3/4] perf(db_query): Maintain compiled pattern globally --- frappe/model/db_query.py | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py index 91c70f5b97..82913db98d 100644 --- a/frappe/model/db_query.py +++ b/frappe/model/db_query.py @@ -40,6 +40,16 @@ CAST_VARCHAR_PATTERN = re.compile( r"([`\"]?tab[\w`\" -]+\.[`\"]?name[`\"]?)(?!\w)", flags=re.IGNORECASE ) ORDER_BY_PATTERN = re.compile(r"\ order\ by\ |\ asc|\ ASC|\ desc|\ DESC", flags=re.IGNORECASE) +SUB_QUERY_PATTERN = re.compile("^.*[,();@].*") +IS_QUERY_PATTERN = re.compile(r"^(select|delete|update|drop|create)\s") +IS_QUERY_PREDICATE_PATTERN = re.compile( + r"\s*[0-9a-zA-z]*\s*( from | group by | order by | where | join )" +) +FIELD_QUOTE_PATTERN = re.compile(r"[0-9a-zA-Z]+\s*'") +FIELD_COMMA_PATTERN = re.compile(r"[0-9a-zA-Z]+\s*,") +STRICT_FIELD_PATTERN = re.compile(r".*/\*.*") +STRICT_UNION_PATTERN = re.compile(r".*\s(union).*\s") +ORDER_GROUP_PATTERN = re.compile(r".*[^a-z0-9-_ ,`'\"\.\(\)].*") class DatabaseQuery(object): @@ -343,8 +353,6 @@ class DatabaseQuery(object): As field contains `,` and mysql function `version()`, with the help of regex the system will filter out this field. """ - - sub_query_regex = re.compile("^.*[,();@].*") blacklisted_keywords = ["select", "create", "insert", "delete", "drop", "update", "case", "show"] blacklisted_functions = [ "concat", @@ -368,16 +376,14 @@ class DatabaseQuery(object): frappe.throw(_("Use of sub-query or function is restricted"), frappe.DataError) def _is_query(field): - if re.compile(r"^(select|delete|update|drop|create)\s").match(field): + if IS_QUERY_PATTERN.match(field): _raise_exception() - elif re.compile(r"\s*[0-9a-zA-z]*\s*( from | group by | order by | where | join )").match( - field - ): + elif IS_QUERY_PREDICATE_PATTERN.match(field): _raise_exception() for field in self.fields: - if sub_query_regex.match(field): + if SUB_QUERY_PATTERN.match(field): if any(f"({keyword}" in field.lower() for keyword in blacklisted_keywords): _raise_exception() @@ -388,19 +394,19 @@ class DatabaseQuery(object): # prevent access to global variables _raise_exception() - if re.compile(r"[0-9a-zA-Z]+\s*'").match(field): + if FIELD_QUOTE_PATTERN.match(field): _raise_exception() - if re.compile(r"[0-9a-zA-Z]+\s*,").match(field): + if FIELD_COMMA_PATTERN.match(field): _raise_exception() _is_query(field) if self.strict: - if re.compile(r".*/\*.*").match(field): + if STRICT_FIELD_PATTERN.match(field): frappe.throw(_("Illegal SQL Query")) - if re.compile(r".*\s(union).*\s").match(field.lower()): + if STRICT_UNION_PATTERN.match(field.lower()): frappe.throw(_("Illegal SQL Query")) def extract_tables(self): @@ -907,7 +913,7 @@ class DatabaseQuery(object): if "select" in _lower and "from" in _lower: frappe.throw(_("Cannot use sub-query in order by")) - if re.compile(r".*[^a-z0-9-_ ,`'\"\.\(\)].*").match(_lower): + if ORDER_GROUP_PATTERN.match(_lower): frappe.throw(_("Illegal SQL Query")) for field in parameters.split(","): From ec2aadbf5a685591dd8f90bb8812d34fc4167c66 Mon Sep 17 00:00:00 2001 From: Faris Ansari Date: Tue, 14 Jun 2022 18:15:48 +0530 Subject: [PATCH 4/4] chore: typo --- frappe/public/js/frappe/ui/toolbar/about.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/public/js/frappe/ui/toolbar/about.js b/frappe/public/js/frappe/ui/toolbar/about.js index 47dc7ee851..464c7c4787 100644 --- a/frappe/public/js/frappe/ui/toolbar/about.js +++ b/frappe/public/js/frappe/ui/toolbar/about.js @@ -19,7 +19,7 @@ frappe.ui.misc.about = function() {

Installed Apps

\
Loading versions...
\
\ -

© Frappe Technologies Pvt. Ltd and contributors

\ +

© Frappe Technologies Pvt. Ltd. and contributors

\ ", frappe.app)); frappe.ui.misc.about_dialog = d;