From 575d32ec35f136fa2f98541113f804bee3107b20 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 24 Nov 2022 15:22:12 +0530 Subject: [PATCH 1/4] fix(db_query): Space resilient matching --- frappe/model/db_query.py | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py index e689f91ddd..5026b04999 100644 --- a/frappe/model/db_query.py +++ b/frappe/model/db_query.py @@ -384,14 +384,19 @@ class DatabaseQuery: _raise_exception() for field in self.fields: + lower_field = field.lower() + function = lower_field.split("(", 1)[0].rstrip() + + if function in blacklisted_functions: + frappe.throw(_("Use of function {0} in field is restricted").format(function)) + if SUB_QUERY_PATTERN.match(field): - if any(f"({keyword}" in field.lower() for keyword in blacklisted_keywords): - _raise_exception() + if field[0] == "(": + subquery_token = lower_field[1:].lstrip().split(" ", 1)[0] + if subquery_token in blacklisted_keywords: + _raise_exception() - if any(f"{keyword}(" in field.lower() for keyword in blacklisted_functions): - _raise_exception() - - if "@" in field.lower(): + if "@" in lower_field: # prevent access to global variables _raise_exception() @@ -407,7 +412,7 @@ class DatabaseQuery: if STRICT_FIELD_PATTERN.match(field): frappe.throw(_("Illegal SQL Query")) - if STRICT_UNION_PATTERN.match(field.lower()): + if STRICT_UNION_PATTERN.match(lower_field): frappe.throw(_("Illegal SQL Query")) def extract_tables(self): From 1f913248aa61958be77a97221b1db8b43210aba8 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 24 Nov 2022 15:30:38 +0530 Subject: [PATCH 2/4] test: Add more tests for illegal subquery and fn usage --- frappe/tests/test_db_query.py | 37 +++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/frappe/tests/test_db_query.py b/frappe/tests/test_db_query.py index 162d3e9d8a..79bfacdcf9 100644 --- a/frappe/tests/test_db_query.py +++ b/frappe/tests/test_db_query.py @@ -418,6 +418,43 @@ class TestReportview(FrappeTestCase): ) self.assertTrue("date_diff" in data[0]) + with self.assertRaises(frappe.ValidationError): + DatabaseQuery("DocType").execute( + fields=["name", "issingle", "if (issingle=1, (select name from tabUser), count(name))"], + limit_start=0, + limit_page_length=1, + ) + + with self.assertRaises(frappe.ValidationError): + DatabaseQuery("DocType").execute( + fields=["name", "issingle", "if(issingle=1, (select name from tabUser), count(name))"], + limit_start=0, + limit_page_length=1, + ) + + with self.assertRaises(frappe.ValidationError): + DatabaseQuery("DocType").execute( + fields=[ + "name", + "issingle", + "( select name from `tabUser` where `tabDocType`.owner = `tabUser`.name )", + ], + limit_start=0, + limit_page_length=1, + ignore_permissions=True, + ) + + with self.assertRaises(frappe.ValidationError): + DatabaseQuery("DocType").execute( + fields=[ + "name", + "issingle", + "(select name from `tabUser` where `tabDocType`.owner = `tabUser`.name )", + ], + limit_start=0, + limit_page_length=1, + ) + def test_nested_permission(self): frappe.set_user("Administrator") create_nested_doctype() From 1a5e5f546b7151ec5994cc4631e697c44b355880 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 24 Nov 2022 16:21:39 +0530 Subject: [PATCH 3/4] fix: Move function check inside subquery --- frappe/model/db_query.py | 10 ++++++---- frappe/tests/test_db_query.py | 8 ++++---- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py index 5026b04999..f56e777ad9 100644 --- a/frappe/model/db_query.py +++ b/frappe/model/db_query.py @@ -385,10 +385,6 @@ class DatabaseQuery: for field in self.fields: lower_field = field.lower() - function = lower_field.split("(", 1)[0].rstrip() - - if function in blacklisted_functions: - frappe.throw(_("Use of function {0} in field is restricted").format(function)) if SUB_QUERY_PATTERN.match(field): if field[0] == "(": @@ -396,6 +392,12 @@ class DatabaseQuery: if subquery_token in blacklisted_keywords: _raise_exception() + function = lower_field.split("(", 1)[0].rstrip() + if function in blacklisted_functions: + frappe.throw( + _("Use of function {0} in field is restricted").format(function), exc=frappe.DataError + ) + if "@" in lower_field: # prevent access to global variables _raise_exception() diff --git a/frappe/tests/test_db_query.py b/frappe/tests/test_db_query.py index 79bfacdcf9..52f18f5963 100644 --- a/frappe/tests/test_db_query.py +++ b/frappe/tests/test_db_query.py @@ -418,21 +418,21 @@ class TestReportview(FrappeTestCase): ) self.assertTrue("date_diff" in data[0]) - with self.assertRaises(frappe.ValidationError): + with self.assertRaises(frappe.DataError): DatabaseQuery("DocType").execute( fields=["name", "issingle", "if (issingle=1, (select name from tabUser), count(name))"], limit_start=0, limit_page_length=1, ) - with self.assertRaises(frappe.ValidationError): + with self.assertRaises(frappe.DataError): DatabaseQuery("DocType").execute( fields=["name", "issingle", "if(issingle=1, (select name from tabUser), count(name))"], limit_start=0, limit_page_length=1, ) - with self.assertRaises(frappe.ValidationError): + with self.assertRaises(frappe.DataError): DatabaseQuery("DocType").execute( fields=[ "name", @@ -444,7 +444,7 @@ class TestReportview(FrappeTestCase): ignore_permissions=True, ) - with self.assertRaises(frappe.ValidationError): + with self.assertRaises(frappe.DataError): DatabaseQuery("DocType").execute( fields=[ "name", From 35827af1729f4d05cdb277d367849439cbd90e13 Mon Sep 17 00:00:00 2001 From: gavin Date: Fri, 25 Nov 2022 12:39:11 +0530 Subject: [PATCH 4/4] fix: Strip white spaces on lower cased field value Co-authored-by: Ankush Menat --- frappe/model/db_query.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py index f56e777ad9..4b9318ab03 100644 --- a/frappe/model/db_query.py +++ b/frappe/model/db_query.py @@ -384,10 +384,10 @@ class DatabaseQuery: _raise_exception() for field in self.fields: - lower_field = field.lower() + lower_field = field.lower().strip() if SUB_QUERY_PATTERN.match(field): - if field[0] == "(": + if lower_field[0] == "(": subquery_token = lower_field[1:].lstrip().split(" ", 1)[0] if subquery_token in blacklisted_keywords: _raise_exception()