diff --git a/frappe/__init__.py b/frappe/__init__.py index 85aa4e660a..2ee63ff47c 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -14,7 +14,7 @@ import os, sys, importlib, inspect, json from .exceptions import * from .utils.jinja import get_jenv, get_template, render_template, get_email_from_template -__version__ = '10.1.37' +__version__ = '10.1.38' __title__ = "Frappe Framework" local = Local() diff --git a/frappe/desk/search.py b/frappe/desk/search.py index 8527076afe..67e52be218 100644 --- a/frappe/desk/search.py +++ b/frappe/desk/search.py @@ -8,6 +8,31 @@ from frappe.utils import cstr, unique from frappe import _ from six import string_types + +def sanitize_searchfield(searchfield): + blacklisted_keywords = ['select', 'delete', 'drop', 'update', 'case', 'and', 'or', 'like'] + + def _raise_exception(): + frappe.throw(_('Invalid Search Field'), frappe.DataError) + + if len(searchfield) >= 3: + + # to avoid 1=1 + if '=' in searchfield: + _raise_exception() + + # in mysql -- is used for commenting the query + elif ' --' in searchfield: + _raise_exception() + + # to avoid and, or and like + elif any(' {0} '.format(keyword) in searchfield.split() for keyword in blacklisted_keywords): + _raise_exception() + + # to avoid select, delete, drop, update and case + elif any(keyword in searchfield.split() for keyword in blacklisted_keywords): + _raise_exception() + # this is called by the Link Field @frappe.whitelist() def search_link(doctype, txt, query=None, filters=None, page_length=20, searchfield=None): @@ -24,6 +49,9 @@ def search_widget(doctype, txt, query=None, searchfield=None, start=0, meta = frappe.get_meta(doctype) + if searchfield: + sanitize_searchfield(searchfield) + if not searchfield: searchfield = "name" diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py index dd0b386570..9413980506 100644 --- a/frappe/model/db_query.py +++ b/frappe/model/db_query.py @@ -140,7 +140,7 @@ class DatabaseQuery(object): if self.or_conditions: args.conditions += (' or ' if args.conditions else "") + \ - ' or '.join(self.or_conditions) + ' or '.join(self.or_conditions) self.set_field_tables() @@ -190,7 +190,8 @@ class DatabaseQuery(object): As field contains `,` and mysql function `version()`, with the help of regex the system will filter out this field. ''' - regex = re.compile('^.*[,();].*') + + sub_query_regex = re.compile("^.*[,();].*") blacklisted_keywords = ['select', 'create', 'insert', 'delete', 'drop', 'update', 'case'] blacklisted_functions = ['concat', 'concat_ws', 'if', 'ifnull', 'nullif', 'coalesce', 'connection_id', 'current_user', 'database', 'last_insert_id', 'session_user', @@ -200,14 +201,22 @@ class DatabaseQuery(object): frappe.throw(_('Cannot use sub-query or function in fields'), frappe.DataError) for field in self.fields: - if regex.match(field): - if any(keyword in field.lower() for keyword in blacklisted_keywords): + if sub_query_regex.match(field): + if any(keyword in field.lower().split() for keyword in blacklisted_keywords): _raise_exception() - if any("{0}(".format(keyword) in field.lower() \ - for keyword in blacklisted_functions): + if any("({0}".format(keyword) in field.lower() for keyword in blacklisted_keywords): _raise_exception() + if any("{0}(".format(keyword) in field.lower() for keyword in blacklisted_functions): + _raise_exception() + + if re.compile("[a-zA-Z]+\s*'").match(field): + _raise_exception() + + if re.compile('[a-zA-Z]+\s*,').match(field): + _raise_exception() + def extract_tables(self): """extract tables from fields""" self.tables = ['`tab' + self.doctype + '`'] diff --git a/frappe/model/naming.py b/frappe/model/naming.py index d3be62e88e..6c909f133f 100644 --- a/frappe/model/naming.py +++ b/frappe/model/naming.py @@ -202,7 +202,7 @@ def append_number_if_name_exists(doctype, value, fieldname='name', separator='-' filters.update({fieldname: value}) exists = frappe.db.exists(doctype, filters) - regex = '^{value}{separator}[[:digit:]]$'.format(value=re.escape(value), separator=separator) + regex = '^{value}{separator}\d+$'.format(value=re.escape(value), separator=separator) if exists: last = frappe.db.sql("""select {fieldname} from `tab{doctype}` where {fieldname} regexp %s diff --git a/frappe/public/css/bootstrap.css b/frappe/public/css/bootstrap.css index 3bcdd13eb9..c4db43f199 100644 --- a/frappe/public/css/bootstrap.css +++ b/frappe/public/css/bootstrap.css @@ -193,9 +193,9 @@ th { *, *:before, *:after { - color: #000 !important; + color: #000; + background: transparent; text-shadow: none !important; - background: transparent !important; -webkit-box-shadow: none !important; box-shadow: none !important; } @@ -254,11 +254,11 @@ th { } .table td, .table th { - background-color: #fff !important; + background-color: #fff; } .table-bordered th, .table-bordered td { - border: 1px solid #ddd !important; + border: 1px solid #ddd; } } @font-face { diff --git a/frappe/tests/test_db_query.py b/frappe/tests/test_db_query.py index 484a309534..07d47ca722 100644 --- a/frappe/tests/test_db_query.py +++ b/frappe/tests/test_db_query.py @@ -117,6 +117,12 @@ class TestReportview(unittest.TestCase): fields=["name", "issingle, IF(issingle=1, (SELECT name from tabUser), count(*))"], limit_start=0, limit_page_length=1) + self.assertRaises(frappe.DataError, DatabaseQuery("DocType").execute, + fields=["name", "issingle ''"],limit_start=0, limit_page_length=1) + + self.assertRaises(frappe.DataError, DatabaseQuery("DocType").execute, + fields=["name", "issingle,'"],limit_start=0, limit_page_length=1) + data = DatabaseQuery("DocType").execute(fields=["name", "issingle", "count(name)"], limit_start=0, limit_page_length=1) self.assertTrue('count(name)' in data[0]) @@ -133,6 +139,48 @@ class TestReportview(unittest.TestCase): "datediff(modified, creation) as date_diff"], limit_start=0, limit_page_length=1) self.assertTrue('date_diff' in data[0]) + def test_filter_sanitizer(self): + self.assertRaises(frappe.DataError, DatabaseQuery("DocType").execute, + fields=["name"], filters={'istable,': 1}, limit_start=0, limit_page_length=1) + + self.assertRaises(frappe.DataError, DatabaseQuery("DocType").execute, + fields=["name"], filters={'editable_grid,': 1}, or_filters={'istable,': 1}, + limit_start=0, limit_page_length=1) + + self.assertRaises(frappe.DataError, DatabaseQuery("DocType").execute, + fields=["name"], filters={'editable_grid,': 1}, + or_filters=[['DocType', 'istable,', '=', 1]], + limit_start=0, limit_page_length=1) + + self.assertRaises(frappe.DataError, DatabaseQuery("DocType").execute, + fields=["name"], filters={'editable_grid,': 1}, + or_filters=[['DocType', 'istable', '=', 1], ['DocType', 'beta and 1=1', '=', 0]], + limit_start=0, limit_page_length=1) + + data = DatabaseQuery("DocType").execute(fields=["name", "issingle"], + filters={'editable_grid': 1, 'module': 'Core'}, + or_filters=[['DocType', 'istable', '=', 1]], + limit_start=0, limit_page_length=1) + self.assertEquals('DocField', data[0]['name']) + + data = DatabaseQuery("DocType").execute(fields=["name"], + filters={'issingle': 1}, or_filters=[['DocType', 'module', '=', 'Core']], + limit_start=0, limit_page_length=1) + self.assertEquals('User Permission for Page and Report', data[0]['name']) + + data = DatabaseQuery("DocType").execute(fields=["name"], + filters={'track_changes': 1, 'module': 'Core'}, + limit_start=0, limit_page_length=4) + self.assertEquals(['File', 'Version', 'Data Import', 'Domain Settings'], + [d['name'] for d in data]) + + data = DatabaseQuery("DocType").execute(fields=["name"], + filters=[['DocType', 'ifnull(track_changes, 0)', '=', 0], + ['DocType', 'module', '=', 'Core']], + limit_start=0, limit_page_length=4) + self.assertEquals(['DocField', 'User Permission for Page and Report', 'SMS Settings', 'Block Module'], + [d['name'] for d in data]) + def create_event(subject="_Test Event", starts_on=None): """ create a test event """ @@ -145,4 +193,4 @@ def create_event(subject="_Test Event", starts_on=None): "starts_on": get_datetime(starts_on), }).insert(ignore_permissions=True) - return event \ No newline at end of file + return event diff --git a/frappe/tests/test_search.py b/frappe/tests/test_search.py new file mode 100644 index 0000000000..ee4dd3cc53 --- /dev/null +++ b/frappe/tests/test_search.py @@ -0,0 +1,28 @@ +# Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors +# MIT License. See license.txt + +from __future__ import unicode_literals +import unittest +import frappe +from frappe.desk.search import search_link + + +class TestSearch(unittest.TestCase): + def test_search_field_sanitizer(self): + # pass + search_link('DocType', 'User', query=None, filters=None, page_length=20, searchfield='name') + result = frappe.response['results'][0] + self.assertTrue('User' in result['value']) + + #raise exception on injection + self.assertRaises(frappe.DataError, + search_link, 'DocType', 'Customer', query=None, filters=None, + page_length=20, searchfield='1=1') + + self.assertRaises(frappe.DataError, + search_link, 'DocType', 'Customer', query=None, filters=None, + page_length=20, searchfield='select * from tabSessions) --') + + self.assertRaises(frappe.DataError, + search_link, 'DocType', 'Customer', query=None, filters=None, + page_length=20, searchfield='name or (select * from tabSessions)') diff --git a/frappe/utils/data.py b/frappe/utils/data.py index 86fef0b61c..b0b70bd644 100644 --- a/frappe/utils/data.py +++ b/frappe/utils/data.py @@ -801,6 +801,8 @@ def get_filter(doctype, f): f = frappe._dict(doctype=f[0], fieldname=f[1], operator=f[2], value=f[3]) + sanitize_column(f.fieldname) + if not f.operator: # if operator is missing f.operator = "=" @@ -840,6 +842,27 @@ def make_filter_dict(filters): return _filter +def sanitize_column(column_name): + from frappe import _ + regex = re.compile("^.*[,'();].*") + blacklisted_keywords = ['select', 'create', 'insert', 'delete', 'drop', 'update', 'case', 'and', 'or'] + + def _raise_exception(): + frappe.throw(_("Invalid field name {0}").format(column_name), frappe.DataError) + + if 'ifnull' in column_name: + if regex.match(column_name): + # to avoid and, or + if any(' {0} '.format(keyword) in column_name.split() for keyword in blacklisted_keywords): + _raise_exception() + + # to avoid select, delete, drop, update and case + elif any(keyword in column_name.split() for keyword in blacklisted_keywords): + _raise_exception() + + elif regex.match(column_name): + _raise_exception() + def scrub_urls(html): html = expand_relative_urls(html) # encoding should be responsibility of the composer diff --git a/frappe/utils/global_search.py b/frappe/utils/global_search.py index 5811d78057..0f640083f7 100644 --- a/frappe/utils/global_search.py +++ b/frappe/utils/global_search.py @@ -8,9 +8,9 @@ import re import redis from frappe.utils import cint, strip_html_tags from frappe.model.base_document import get_controller +from frappe.model.db_schema import varchar_len from six import text_type - def setup_global_search_table(): """ Creates __global_seach table @@ -19,17 +19,16 @@ def setup_global_search_table(): if not '__global_search' in frappe.db.get_tables(): frappe.db.sql('''create table __global_search( doctype varchar(100), - name varchar(140), - title varchar(140), + name varchar({varchar_len}), + title varchar({varchar_len}), content text, fulltext(content), - route varchar(140), + route varchar({varchar_len}), published int(1) not null default 0, unique `doctype_name` (doctype, name)) COLLATE=utf8mb4_unicode_ci ENGINE=MyISAM - CHARACTER SET=utf8mb4''') - + CHARACTER SET=utf8mb4'''.format(varchar_len=varchar_len)) def reset(): """ @@ -142,8 +141,8 @@ def rebuild_for_doctype(doctype): "name": frappe.db.escape(doc.name), "content": frappe.db.escape(' ||| '.join(content or '')), "published": published, - "title": frappe.db.escape(title or ''), - "route": frappe.db.escape(route or '') + "title": frappe.db.escape(title or '')[:int(varchar_len)], + "route": frappe.db.escape(route or '')[:int(varchar_len)] }) if all_contents: insert_values_for_multiple_docs(all_contents) @@ -259,7 +258,8 @@ def update_global_search(doc): frappe.flags.update_global_search.append( dict(doctype=doc.doctype, name=doc.name, content=' ||| '.join(content or ''), - published=published, title=doc.get_title(), route=doc.get('route'))) + published=published, title=doc.get_title()[:int(varchar_len)], route=doc.get('route'))) + enqueue_global_search()