diff --git a/netbox/extras/constants.py b/netbox/extras/constants.py index c6f4bb50b..a0a33936b 100644 --- a/netbox/extras/constants.py +++ b/netbox/extras/constants.py @@ -28,6 +28,11 @@ IMAGE_ATTACHMENT_IMAGE_FORMATS = { # Template Export DEFAULT_MIME_TYPE = 'text/plain; charset=utf-8' +# Scripts +# Prefix applied to dynamically-loaded script/report module names so that a script whose filename +# matches a core app or package (e.g. "circuits.py") cannot shadow it in sys.modules. +SCRIPT_MODULE_NAME_PREFIX = '_netbox_script_module_' + # Webhooks HTTP_CONTENT_TYPE_JSON = 'application/json' diff --git a/netbox/extras/models/mixins.py b/netbox/extras/models/mixins.py index 7eee9ac3c..a8e714c7e 100644 --- a/netbox/extras/models/mixins.py +++ b/netbox/extras/models/mixins.py @@ -12,7 +12,7 @@ from django.utils.module_loading import import_string from django.utils.translation import gettext_lazy as _ from core.models import ObjectType -from extras.constants import DEFAULT_MIME_TYPE, JINJA_ENV_PARAMS_ALLOWED +from extras.constants import DEFAULT_MIME_TYPE, JINJA_ENV_PARAMS_ALLOWED, SCRIPT_MODULE_NAME_PREFIX from extras.utils import filename_from_model, filename_from_object from utilities.jinja2 import render_jinja2 @@ -68,12 +68,16 @@ class PythonModuleMixin: Load the module using importlib, but use a custom loader to use django-storages instead of the file system. """ - spec = importlib.util.spec_from_file_location(self.python_name, self.name) + # Load the module under a namespaced name rather than its bare name. Using the bare name + # (e.g. "circuits") would replace the like-named core app package in sys.modules, breaking + # app and migration graph resolution. + module_name = f'{SCRIPT_MODULE_NAME_PREFIX}{self.python_name}' + spec = importlib.util.spec_from_file_location(module_name, self.name) if spec is None: raise ModuleNotFoundError(f"Could not find module: {self.python_name}") loader = CustomStoragesLoader(self.name) module = importlib.util.module_from_spec(spec) - sys.modules[self.python_name] = module + sys.modules[module_name] = module loader.exec_module(module) return module diff --git a/netbox/extras/scripts.py b/netbox/extras/scripts.py index 95ca00d56..e0799834c 100644 --- a/netbox/extras/scripts.py +++ b/netbox/extras/scripts.py @@ -12,6 +12,7 @@ from django.utils.translation import gettext as _ from core.choices import JobNotificationChoices from extras.choices import LogLevelChoices +from extras.constants import SCRIPT_MODULE_NAME_PREFIX from extras.models import ScriptModule from ipam.formfields import IPAddressFormField, IPNetworkFormField from ipam.validators import MaxPrefixLengthValidator, MinPrefixLengthValidator, prefix_validator @@ -349,7 +350,12 @@ class BaseScript: @classproperty def module(self): - return self.__module__ + # Strip the internal prefix applied when the module is loaded (see #22566) so that + # user-facing names (full_name, logger namespaces) reflect the original script filename. + name = self.__module__ + if name.startswith(SCRIPT_MODULE_NAME_PREFIX): + name = name[len(SCRIPT_MODULE_NAME_PREFIX):] + return name @classproperty def class_name(self): @@ -361,7 +367,7 @@ class BaseScript: @classmethod def root_module(cls): - return cls.__module__.split(".")[0] + return cls.module.split(".")[0] # Author-defined attributes diff --git a/netbox/extras/tests/test_scripts.py b/netbox/extras/tests/test_scripts.py index fab047484..01bc84407 100644 --- a/netbox/extras/tests/test_scripts.py +++ b/netbox/extras/tests/test_scripts.py @@ -1,11 +1,16 @@ +import io +import sys from datetime import UTC, date, datetime from decimal import Decimal +from unittest.mock import patch from django.core.files.uploadedfile import SimpleUploadedFile from django.test import TestCase from netaddr import IPAddress, IPNetwork from dcim.models import DeviceRole +from extras.constants import SCRIPT_MODULE_NAME_PREFIX +from extras.models import ScriptModule from extras.scripts import * CHOICES = ( @@ -388,3 +393,49 @@ class ScriptVariablesTestCase(TestCase): self.assertEqual(form.cleaned_data['var1'], input_datetime) # Validate required=False works for this Var type self.assertEqual(form.cleaned_data['var2'], None) + + +class ScriptModuleLoadingTestCase(TestCase): + + def test_module_does_not_shadow_core_app(self): + """ + Loading a custom script whose filename matches a core app label must not replace that + app's package in sys.modules. Regression test for issue #22566. + """ + import circuits # The real core app package + + script_content = ( + b"from extras.scripts import Script\n\n\n" + b"class TestScript(Script):\n pass\n" + ) + + class _Storage: + def open(self, name, mode='rb'): + return io.BytesIO(script_content) + + module = ScriptModule(file_root='scripts', file_path='circuits.py') + namespaced_key = f'{SCRIPT_MODULE_NAME_PREFIX}circuits' + self.addCleanup(lambda: sys.modules.pop(namespaced_key, None)) + + with patch('extras.models.mixins.storages') as mock_storages: + mock_storages.__getitem__.return_value = _Storage() + loaded = module.get_module() + + # The script module is registered under the private, namespaced key, and its own + # __name__ matches that key (i.e. sys.modules[module.__name__] resolves to the module) + self.assertIs(sys.modules[namespaced_key], loaded) + self.assertEqual(loaded.__name__, namespaced_key) + + # The namespacing must not leak into the derived script name stored in the database + self.assertEqual(next(iter(module.module_scripts)), 'TestScript') + + # Nor into the user-facing names exposed on the Script class (used for logger + # namespaces, page headers, etc.): these must reflect the original filename. + script_class = loaded.TestScript + self.assertEqual(script_class.module, 'circuits') + self.assertEqual(script_class.full_name, 'circuits.TestScript') + self.assertEqual(script_class.root_module(), 'circuits') + + # The real circuits app must be untouched and remain an importable package + self.assertIs(sys.modules['circuits'], circuits) + self.assertTrue(hasattr(circuits, '__path__'))