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.
This commit is contained in:
parent
fe820ae8c8
commit
6e0b522ae3
8 changed files with 75 additions and 20 deletions
|
|
@ -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]:
|
||||
|
|
|
|||
|
|
@ -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")
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
||||
|
|
|
|||
|
|
@ -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")'
|
||||
|
|
|
|||
|
|
@ -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")
|
||||
|
|
|
|||
|
|
@ -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")
|
||||
|
|
|
|||
|
|
@ -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):
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue