Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Show server script name in traceback #23676

Merged
merged 2 commits into from
Dec 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion frappe/core/doctype/report/report.py
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ def execute_module(self, filters):
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:
Expand Down
13 changes: 9 additions & 4 deletions frappe/core/doctype/server_script/server_script.py
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,12 @@ def execute_doc(self, doc: 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
Expand All @@ -166,7 +171,7 @@ def execute_scheduled_method(self):
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
Expand All @@ -178,7 +183,7 @@ def get_permission_query_conditions(self, user: str) -> list[str]:
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"]

Expand Down Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion frappe/desk/doctype/system_console/system_console.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ def run(self):
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))
Expand Down
29 changes: 15 additions & 14 deletions frappe/query_builder/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,28 +107,29 @@ def execute_child_queries(queries, result):
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:
from frappe.utils.safe_exec import check_safe_sql_query

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 <serverscript> 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 <serverscript> is set as the filename it shouldn't be allowed.
if len(callstack) >= 3 and SERVER_SCRIPT_FILE_PREFIX in callstack[2].filename:
ankush marked this conversation as resolved.
Show resolved Hide resolved
raise frappe.PermissionError("Only SELECT SQL allowed in scripting")

return query, param_collector.get_parameters()

builder_class = frappe.qb._BuilderClasss
Expand Down
4 changes: 3 additions & 1 deletion frappe/recorder.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 "<serverscript>" in filename:
if "/apps/" in filename or SERVER_SCRIPT_FILE_PREFIX in filename:
yield {
"filename": TRACEBACK_PATH_PATTERN.sub("", filename),
"lineno": lineno,
Expand Down
7 changes: 4 additions & 3 deletions frappe/utils/error.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 `<serverscript>`
- 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<app_name>\w+)/\1/")
SERVER_SCRIPT_FRAME = re.compile(r".*<serverscript>")

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):
Expand Down
16 changes: 14 additions & 2 deletions frappe/utils/safe_exec.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ class ServerScriptNotEnabled(frappe.PermissionError):
ARGUMENT_NOT_SET = object()

SAFE_EXEC_CONFIG_KEY = "server_script_enabled"
SERVER_SCRIPT_FILE_PREFIX = "<serverscript>"


class NamespaceDict(frappe._dict):
Expand Down Expand Up @@ -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.")
Expand All @@ -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="<serverscript>", policy=FrappeTransformer),
compile_restricted(script, filename=filename, policy=FrappeTransformer),
exec_globals,
_locals,
)
Expand Down
2 changes: 1 addition & 1 deletion frappe/website/doctype/web_page/web_page.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ def get_context(self, context):

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)
Expand Down
Loading