From 57699a54b176ca31f5ea7a9eb0841f09df819651 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Fri, 8 Dec 2023 15:01:13 +0530 Subject: [PATCH] fix: Show server script name in traceback (#23676) * fix: Show server script name in traceback * chore: typo Co-authored-by: Sagar Vora --------- Co-authored-by: Sagar Vora --- frappe/core/doctype/report/report.py | 2 +- .../doctype/server_script/server_script.py | 13 ++++++--- .../doctype/system_console/system_console.py | 2 +- frappe/query_builder/utils.py | 27 ++++++++++--------- frappe/recorder.py | 4 ++- frappe/utils/error.py | 7 ++--- frappe/utils/safe_exec.py | 16 +++++++++-- frappe/website/doctype/web_page/web_page.py | 2 +- 8 files changed, 47 insertions(+), 26 deletions(-) diff --git a/frappe/core/doctype/report/report.py b/frappe/core/doctype/report/report.py index f43bc65f6b..ad51b0415c 100644 --- a/frappe/core/doctype/report/report.py +++ b/frappe/core/doctype/report/report.py @@ -184,7 +184,7 @@ class Report(Document): def execute_script(self, filters): # server script loc = {"filters": frappe._dict(filters), "data": None, "result": None} - safe_exec(self.report_script, None, loc) + safe_exec(self.report_script, None, loc, script_filename=f"Report {self.name}") if loc["data"]: return loc["data"] else: diff --git a/frappe/core/doctype/server_script/server_script.py b/frappe/core/doctype/server_script/server_script.py index a58e50dddc..e19bdc681d 100644 --- a/frappe/core/doctype/server_script/server_script.py +++ b/frappe/core/doctype/server_script/server_script.py @@ -155,7 +155,12 @@ class ServerScript(Document): Args: doc (Document): Executes script with for a certain document's events """ - safe_exec(self.script, _locals={"doc": doc}, restrict_commit_rollback=True) + safe_exec( + self.script, + _locals={"doc": doc}, + restrict_commit_rollback=True, + script_filename=self.name, + ) def execute_scheduled_method(self): """Specific to Scheduled Jobs via Server Scripts @@ -166,7 +171,7 @@ class ServerScript(Document): if self.script_type != "Scheduler Event": raise frappe.DoesNotExistError - safe_exec(self.script) + safe_exec(self.script, script_filename=self.name) def get_permission_query_conditions(self, user: str) -> list[str]: """Specific to Permission Query Server Scripts @@ -178,7 +183,7 @@ class ServerScript(Document): list: Returns list of conditions defined by rules in self.script """ locals = {"user": user, "conditions": ""} - safe_exec(self.script, None, locals) + safe_exec(self.script, None, locals, script_filename=self.name) if locals["conditions"]: return locals["conditions"] @@ -278,7 +283,7 @@ def execute_api_server_script(script=None, *args, **kwargs): raise frappe.PermissionError # output can be stored in flags - _globals, _locals = safe_exec(script.script) + _globals, _locals = safe_exec(script.script, script_filename=script.name) return _globals.frappe.flags diff --git a/frappe/desk/doctype/system_console/system_console.py b/frappe/desk/doctype/system_console/system_console.py index 4969f5a04a..f34f750f9c 100644 --- a/frappe/desk/doctype/system_console/system_console.py +++ b/frappe/desk/doctype/system_console/system_console.py @@ -28,7 +28,7 @@ class SystemConsole(Document): try: frappe.local.debug_log = [] if self.type == "Python": - safe_exec(self.console) + safe_exec(self.console, script_filename="System Console") self.output = "\n".join(frappe.debug_log) elif self.type == "SQL": self.output = frappe.as_json(read_sql(self.console, as_dict=1)) diff --git a/frappe/query_builder/utils.py b/frappe/query_builder/utils.py index c7000a0409..f4ab85ac6b 100644 --- a/frappe/query_builder/utils.py +++ b/frappe/query_builder/utils.py @@ -107,6 +107,8 @@ def patch_query_execute(): def prepare_query(query): import inspect + from frappe.utils.safe_exec import SERVER_SCRIPT_FILE_PREFIX + param_collector = NamedParameterWrapper() query = query.get_sql(param_wrapper=param_collector) if frappe.flags.in_safe_exec: @@ -114,21 +116,20 @@ def patch_query_execute(): if not check_safe_sql_query(query, throw=False): callstack = inspect.stack() - if len(callstack) >= 3 and ".py" in callstack[2].filename: - # ignore any query builder methods called from python files - # assumption is that those functions are whitelisted already. - # since query objects are patched everywhere any query.run() - # will have callstack like this: - # frame0: this function prepare_query() - # frame1: execute_query() - # frame2: frame that called `query.run()` - # - # if frame2 is server script is set as the filename - # it shouldn't be allowed. - pass - else: + # This check is required because QB can execute from anywhere and we can not + # reliably provide a safe version for it in server scripts. + + # since query objects are patched everywhere any query.run() + # will have callstack like this: + # frame0: this function prepare_query() + # frame1: execute_query() + # frame2: frame that called `query.run()` + # + # if frame2 is server script is set as the filename it shouldn't be allowed. + if len(callstack) >= 3 and SERVER_SCRIPT_FILE_PREFIX in callstack[2].filename: raise frappe.PermissionError("Only SELECT SQL allowed in scripting") + return query, param_collector.get_parameters() builder_class = frappe.qb._BuilderClasss diff --git a/frappe/recorder.py b/frappe/recorder.py index 68356c732e..9e67ca9ff0 100644 --- a/frappe/recorder.py +++ b/frappe/recorder.py @@ -41,11 +41,13 @@ def sql(*args, **kwargs): def get_current_stack_frames(): + from frappe.utils.safe_exec import SERVER_SCRIPT_FILE_PREFIX + try: current = inspect.currentframe() frames = inspect.getouterframes(current, context=10) for frame, filename, lineno, function, context, index in list(reversed(frames))[:-2]: - if "/apps/" in filename or "" in filename: + if "/apps/" in filename or SERVER_SCRIPT_FILE_PREFIX in filename: yield { "filename": TRACEBACK_PATH_PATTERN.sub("", filename), "lineno": lineno, diff --git a/frappe/utils/error.py b/frappe/utils/error.py index 50432a01d5..6cee88598e 100644 --- a/frappe/utils/error.py +++ b/frappe/utils/error.py @@ -146,19 +146,20 @@ def guess_exception_source(exception: str) -> str | None: - For unhandled exception last python file from apps folder is responsible. - For frappe.throws the exception source is possibly present after skipping frappe.throw frames - - For server script the file name is `` + - For server script the file name contains SERVER_SCRIPT_FILE_PREFIX """ + from frappe.utils.safe_exec import SERVER_SCRIPT_FILE_PREFIX + with suppress(Exception): installed_apps = frappe.get_installed_apps() app_priority = {app: installed_apps.index(app) for app in installed_apps} APP_NAME_REGEX = re.compile(r".*File.*apps/(?P\w+)/\1/") - SERVER_SCRIPT_FRAME = re.compile(r".*") apps = Counter() for line in reversed(exception.splitlines()): - if SERVER_SCRIPT_FRAME.match(line): + if SERVER_SCRIPT_FILE_PREFIX in line: return "Server Script" if matches := APP_NAME_REGEX.match(line): diff --git a/frappe/utils/safe_exec.py b/frappe/utils/safe_exec.py index 6bdf34bc77..a9f457a0a6 100644 --- a/frappe/utils/safe_exec.py +++ b/frappe/utils/safe_exec.py @@ -37,6 +37,7 @@ class ServerScriptNotEnabled(frappe.PermissionError): ARGUMENT_NOT_SET = object() SAFE_EXEC_CONFIG_KEY = "server_script_enabled" +SERVER_SCRIPT_FILE_PREFIX = "" class NamespaceDict(frappe._dict): @@ -76,7 +77,14 @@ def is_safe_exec_enabled() -> bool: return bool(frappe.get_common_site_config().get(SAFE_EXEC_CONFIG_KEY)) -def safe_exec(script, _globals=None, _locals=None, restrict_commit_rollback=False): +def safe_exec( + script: str, + _globals: dict | None = None, + _locals: dict | None = None, + *, + restrict_commit_rollback: bool = False, + script_filename: str | None = None, +): if not is_safe_exec_enabled(): msg = _("Server Scripts are disabled. Please enable server scripts from bench configuration.") @@ -95,10 +103,14 @@ def safe_exec(script, _globals=None, _locals=None, restrict_commit_rollback=Fals exec_globals.frappe.db.pop("rollback", None) exec_globals.frappe.db.pop("add_index", None) + filename = SERVER_SCRIPT_FILE_PREFIX + if script_filename: + filename += f": {frappe.scrub(script_filename)}" + with safe_exec_flags(), patched_qb(): # execute script compiled by RestrictedPython exec( - compile_restricted(script, filename="", policy=FrappeTransformer), + compile_restricted(script, filename=filename, policy=FrappeTransformer), exec_globals, _locals, ) diff --git a/frappe/website/doctype/web_page/web_page.py b/frappe/website/doctype/web_page/web_page.py index 5bcc237e60..12f86f44c2 100644 --- a/frappe/website/doctype/web_page/web_page.py +++ b/frappe/website/doctype/web_page/web_page.py @@ -78,7 +78,7 @@ class WebPage(WebsiteGenerator): if self.context_script: _locals = dict(context=frappe._dict()) - safe_exec(self.context_script, None, _locals) + safe_exec(self.context_script, None, _locals, script_filename=f"web page {self.name}") context.update(_locals["context"]) self.render_dynamic(context)