From 4d9be26ada8669b97ea4a5cfc6bfcabaedf0976c Mon Sep 17 00:00:00 2001 From: Daizy Modi Date: Wed, 14 Dec 2022 16:05:56 +0530 Subject: [PATCH 1/3] fix: use stricter regex for `sanitize_searchfield` --- frappe/desk/search.py | 35 ++++------------------------------- 1 file changed, 4 insertions(+), 31 deletions(-) diff --git a/frappe/desk/search.py b/frappe/desk/search.py index 4843219179..446f842a0b 100644 --- a/frappe/desk/search.py +++ b/frappe/desk/search.py @@ -7,45 +7,18 @@ import re import frappe from frappe import _, is_whitelisted +from frappe.database.schema import SPECIAL_CHAR_PATTERN from frappe.permissions import has_permission from frappe.utils import cint, cstr, unique def sanitize_searchfield(searchfield): - blacklisted_keywords = ["select", "delete", "drop", "update", "case", "and", "or", "like"] + if not searchfield: + return - def _raise_exception(searchfield): + if SPECIAL_CHAR_PATTERN.search(searchfield): frappe.throw(_("Invalid Search Field {0}").format(searchfield), frappe.DataError) - if len(searchfield) == 1: - # do not allow special characters to pass as searchfields - regex = re.compile(r'^.*[=;*,\'"$\-+%#@()_].*') - if regex.match(searchfield): - _raise_exception(searchfield) - - if len(searchfield) >= 3: - - # to avoid 1=1 - if "=" in searchfield: - _raise_exception(searchfield) - - # in mysql -- is used for commenting the query - elif " --" in searchfield: - _raise_exception(searchfield) - - # to avoid and, or and like - elif any(f" {keyword} " in searchfield.split() for keyword in blacklisted_keywords): - _raise_exception(searchfield) - - # to avoid select, delete, drop, update and case - elif any(keyword in searchfield.split() for keyword in blacklisted_keywords): - _raise_exception(searchfield) - - else: - regex = re.compile(r'^.*[=;*,\'"$\-+%#@()].*') - if any(regex.match(f) for f in searchfield.split()): - _raise_exception(searchfield) - # this is called by the Link Field @frappe.whitelist() From 3e824a9ea59534c0d00e513569631aee0273e916 Mon Sep 17 00:00:00 2001 From: Daizy Modi Date: Wed, 14 Dec 2022 16:47:23 +0530 Subject: [PATCH 2/3] test: test case for `sanitize_searchfield` --- frappe/tests/test_search.py | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/frappe/tests/test_search.py b/frappe/tests/test_search.py index e010d2abc0..9dbf13b729 100644 --- a/frappe/tests/test_search.py +++ b/frappe/tests/test_search.py @@ -1,10 +1,16 @@ # Copyright (c) 2021, Frappe Technologies Pvt. Ltd. and Contributors # License: MIT. See LICENSE +import re import frappe from frappe.app import make_form_dict -from frappe.desk.search import get_names_for_mentions, search_link, search_widget +from frappe.desk.search import ( + get_names_for_mentions, + sanitize_searchfield, + search_link, + search_widget, +) from frappe.tests.utils import FrappeTestCase from frappe.utils import set_request from frappe.website.serve import get_response @@ -179,6 +185,32 @@ class TestSearch(FrappeTestCase): search_link("User", "user@random", searchfield="name") self.assertListEqual(frappe.response["results"], []) + def test_sanitize_searchfield(self): + # should raise error if searchfield is injectable + self.assertRaisesRegex( + frappe.DataError, + re.compile(r"^(Invalid Search Field .*)$"), + sanitize_searchfield, + "1=1", + ) + + # should raise error if searchfield is special character + self.assertRaisesRegex( + frappe.DataError, + re.compile(r"^(Invalid Search Field .*)$"), + sanitize_searchfield, + ";", + ) + + self.assertRaisesRegex( + frappe.DataError, + re.compile(r"^(Invalid Search Field .*)$"), + sanitize_searchfield, + "name or (select * from tabSessions)", + ) + + sanitize_searchfield("name") + @frappe.validate_and_sanitize_search_inputs def get_data(doctype, txt, searchfield, start, page_len, filters): From 9a8dbc42a78c4bce5b94babe36fd5f7eac83006b Mon Sep 17 00:00:00 2001 From: Daizy Modi Date: Wed, 14 Dec 2022 17:01:19 +0530 Subject: [PATCH 3/3] fix: added a case with backticks --- frappe/tests/test_search.py | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-) diff --git a/frappe/tests/test_search.py b/frappe/tests/test_search.py index 9dbf13b729..5d98d6f49f 100644 --- a/frappe/tests/test_search.py +++ b/frappe/tests/test_search.py @@ -186,28 +186,13 @@ class TestSearch(FrappeTestCase): self.assertListEqual(frappe.response["results"], []) def test_sanitize_searchfield(self): - # should raise error if searchfield is injectable - self.assertRaisesRegex( - frappe.DataError, - re.compile(r"^(Invalid Search Field .*)$"), - sanitize_searchfield, - "1=1", - ) - - # should raise error if searchfield is special character - self.assertRaisesRegex( - frappe.DataError, - re.compile(r"^(Invalid Search Field .*)$"), - sanitize_searchfield, - ";", - ) - - self.assertRaisesRegex( - frappe.DataError, - re.compile(r"^(Invalid Search Field .*)$"), - sanitize_searchfield, - "name or (select * from tabSessions)", - ) + for searchfield in ("1=1", "name or (select * from tabSessions)", ";", "`tabSessions`"): + self.assertRaisesRegex( + frappe.DataError, + re.compile(r"^(Invalid Search Field .*)$"), + sanitize_searchfield, + searchfield, + ) sanitize_searchfield("name")