From 6e0b522ae3c27b1a334c93c55a6ffa7fd3a60cb6 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 21 Aug 2023 17:25:47 +0530 Subject: [PATCH] refactor!: Disable server scripts by default - Move the config to bench level and not site level because, server script "threat model" requires consent from a bench owner and not individual site. - While this is a breaking change which people may not like, we believe it's essential to improve security model of Frappe. --- frappe/__init__.py | 31 +++++++++++++------ frappe/core/doctype/report/test_report.py | 5 +++ .../server_script/test_server_script.py | 7 +++-- .../system_console/test_system_console.py | 5 +++ frappe/tests/test_boot.py | 7 +++++ frappe/tests/test_safe_exec.py | 12 ++++++- frappe/tests/utils.py | 16 ++++++++++ frappe/utils/safe_exec.py | 12 +++---- 8 files changed, 75 insertions(+), 20 deletions(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index b8a48b9d96..62292c9718 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -302,19 +302,13 @@ def connect_replica() -> bool: def get_site_config(sites_path: str | None = None, site_path: str | None = None) -> dict[str, Any]: """Returns `site_config.json` combined with `sites/common_site_config.json`. `site_config` is a set of site wide settings like database name, password, email etc.""" - config = {} + config = _dict() sites_path = sites_path or getattr(local, "sites_path", None) site_path = site_path or getattr(local, "site_path", None) if sites_path: - common_site_config = os.path.join(sites_path, "common_site_config.json") - if os.path.exists(common_site_config): - try: - config.update(get_file_json(common_site_config)) - except Exception as error: - click.secho("common_site_config.json is invalid", fg="red") - print(error) + config.update(get_common_site_config(sites_path)) if site_path: site_config = os.path.join(site_path, "site_config.json") @@ -348,7 +342,26 @@ def get_site_config(sites_path: str | None = None, site_path: str | None = None) os.environ.get("FRAPPE_DB_PORT") or config.get("db_port") or db_default_ports(config["db_type"]) ) - return _dict(config) + return config + + +def get_common_site_config(sites_path: str | None = None) -> dict[str, Any]: + """Returns common site config as dictionary. + + This is useful for: + - checking configuration which should only be allowed in common site config + - When no site context is present and fallback is required. + """ + sites_path = sites_path or getattr(local, "sites_path", None) + + common_site_config = os.path.join(sites_path, "common_site_config.json") + if os.path.exists(common_site_config): + try: + return _dict(get_file_json(common_site_config)) + except Exception as error: + click.secho("common_site_config.json is invalid", fg="red") + print(error) + return _dict() def get_conf(site: str | None = None) -> dict[str, Any]: diff --git a/frappe/core/doctype/report/test_report.py b/frappe/core/doctype/report/test_report.py index 670b6b7410..4f9c229ab8 100644 --- a/frappe/core/doctype/report/test_report.py +++ b/frappe/core/doctype/report/test_report.py @@ -18,6 +18,11 @@ test_dependencies = ["User"] class TestReport(FrappeTestCase): + @classmethod + def setUpClass(cls) -> None: + cls.enable_safe_exec() + return super().setUpClass() + def test_report_builder(self): if frappe.db.exists("Report", "User Activity Report"): frappe.delete_doc("Report", "User Activity Report") diff --git a/frappe/core/doctype/server_script/test_server_script.py b/frappe/core/doctype/server_script/test_server_script.py index af1352f02b..b83d1edda4 100644 --- a/frappe/core/doctype/server_script/test_server_script.py +++ b/frappe/core/doctype/server_script/test_server_script.py @@ -97,8 +97,9 @@ class TestServerScript(FrappeTestCase): script_doc = frappe.get_doc(doctype="Server Script") script_doc.update(script) script_doc.insert() - + cls.enable_safe_exec() frappe.db.commit() + return super().setUpClass() @classmethod def tearDownClass(cls): @@ -269,13 +270,13 @@ frappe.qb.from_(todo).select(todo.name).where(todo.name == "{todo.name}").run() site = frappe.utils.get_site_url(frappe.local.site) client = FrappeClient(site) - # Exhaust rate limti + # Exhaust rate limit for _ in range(5): client.get_api(script1.api_method) self.assertRaises(FrappeException, client.get_api, script1.api_method) - # Exhaust rate limti + # Exhaust rate limit for _ in range(5): client.get_api(script2.api_method) diff --git a/frappe/desk/doctype/system_console/test_system_console.py b/frappe/desk/doctype/system_console/test_system_console.py index ade8704813..08a8b83708 100644 --- a/frappe/desk/doctype/system_console/test_system_console.py +++ b/frappe/desk/doctype/system_console/test_system_console.py @@ -5,6 +5,11 @@ from frappe.tests.utils import FrappeTestCase class TestSystemConsole(FrappeTestCase): + @classmethod + def setUpClass(cls) -> None: + cls.enable_safe_exec() + return super().setUpClass() + def test_system_console(self): system_console = frappe.get_doc("System Console") system_console.console = 'log("hello")' diff --git a/frappe/tests/test_boot.py b/frappe/tests/test_boot.py index 8ad2a94aeb..91a188e6ae 100644 --- a/frappe/tests/test_boot.py +++ b/frappe/tests/test_boot.py @@ -27,6 +27,13 @@ class TestBootData(FrappeTestCase): unseen_notes = [d.title for d in get_unseen_notes()] self.assertListEqual(unseen_notes, []) + +class TestPermissionQueries(FrappeTestCase): + @classmethod + def setUpClass(cls) -> None: + cls.enable_safe_exec() + return super().setUpClass() + def test_get_user_pages_or_reports_with_permission_query(self): # Create a ToDo custom report with admin user frappe.set_user("Administrator") diff --git a/frappe/tests/test_safe_exec.py b/frappe/tests/test_safe_exec.py index 49f4974148..ba38cc93e9 100644 --- a/frappe/tests/test_safe_exec.py +++ b/frappe/tests/test_safe_exec.py @@ -2,10 +2,15 @@ import types import frappe from frappe.tests.utils import FrappeTestCase -from frappe.utils.safe_exec import get_safe_globals, safe_exec +from frappe.utils.safe_exec import ServerScriptNotEnabled, get_safe_globals, safe_exec class TestSafeExec(FrappeTestCase): + @classmethod + def setUpClass(cls) -> None: + cls.enable_safe_exec() + return super().setUpClass() + def test_import_fails(self): self.assertRaises(ImportError, safe_exec, "import os") @@ -115,3 +120,8 @@ class TestSafeExec(FrappeTestCase): # dont Allow modifying _dict class self.assertRaises(Exception, safe_exec, "_dict.x = 1") + + +class TestNoSafeExec(FrappeTestCase): + def test_safe_exec_disabled_by_default(self): + self.assertRaises(ServerScriptNotEnabled, safe_exec, "pass") diff --git a/frappe/tests/utils.py b/frappe/tests/utils.py index a98801c503..38be668f6c 100644 --- a/frappe/tests/utils.py +++ b/frappe/tests/utils.py @@ -1,5 +1,6 @@ import copy import datetime +import os import signal import unittest from collections.abc import Sequence @@ -113,6 +114,21 @@ class FrappeTestCase(unittest.TestCase): finally: frappe.db.sql = orig_sql + @classmethod + def enable_safe_exec(cls) -> None: + """Enable safe exec and disable them after test case is completed.""" + from frappe.installer import update_site_config + from frappe.utils.safe_exec import SAFE_EXEC_CONFIG_KEY + + cls._common_conf = os.path.join(frappe.local.sites_path, "common_site_config.json") + update_site_config(SAFE_EXEC_CONFIG_KEY, 1, validate=False, site_config_path=cls._common_conf) + + cls.addClassCleanup( + lambda: update_site_config( + SAFE_EXEC_CONFIG_KEY, 0, validate=False, site_config_path=cls._common_conf + ) + ) + class MockedRequestTestCase(FrappeTestCase): def setUp(self): diff --git a/frappe/utils/safe_exec.py b/frappe/utils/safe_exec.py index f38cb3d4b7..1eb6617cda 100644 --- a/frappe/utils/safe_exec.py +++ b/frappe/utils/safe_exec.py @@ -35,6 +35,8 @@ class ServerScriptNotEnabled(frappe.PermissionError): ARGUMENT_NOT_SET = object() +SAFE_EXEC_CONFIG_KEY = "server_script_enabled" + class NamespaceDict(frappe._dict): """Raise AttributeError if function not found in namespace""" @@ -59,15 +61,11 @@ class FrappeTransformer(RestrictingNodeTransformer): def safe_exec(script, _globals=None, _locals=None, restrict_commit_rollback=False): - # server scripts can be disabled via site_config.json - # they are enabled by default - if "server_script_enabled" in frappe.conf: - enabled = frappe.conf.server_script_enabled - else: - enabled = True + # server scripts can only be enabled via common_site_config.json + enabled = frappe.get_common_site_config().get(SAFE_EXEC_CONFIG_KEY) if not enabled: - frappe.throw(_("Please Enable Server Scripts"), ServerScriptNotEnabled) + frappe.throw(_("Please Enable Server Scripts From Bench Config."), ServerScriptNotEnabled) # build globals exec_globals = get_safe_globals()