Просмотр исходного кода

Fixes #22872: Address review feedback

Validate the execution parameters actually being enqueued rather than only the
script's Meta defaults, so an explicit job_timeout or notifications supplied by a
caller (for example via the REST API) is checked and a valid explicit value is no
longer rejected because of an unused Meta default.

Make the instance argument to ScriptJob.enqueue keyword-only so the override
preserves JobRunner.enqueue's signature and cannot collide with a positional
argument in enqueue_once().

Return the REST API validation error under the non-field detail key instead of a
bare list, and chain the original exception.

Add a runscript management command test covering the invalid-Meta path.

Drop the release-note entry, which does not belong in a bug-fix PR.
Jason Novinger 12 часов назад
Родитель
Сommit
43303764ba

+ 0 - 8
docs/release-notes/version-4.6.md

@@ -1,13 +1,5 @@
 # NetBox v4.6
 
-## v4.6.10
-
-### Bug Fixes
-
-* [#22872](https://github.com/netbox-community/netbox/issues/22872) - Report an actionable error instead of a server error when a custom script declares an invalid `job_timeout` or `notifications_default` in its `Meta` class
-
----
-
 ## v4.6.9 (2026-08-25)
 
 ### Enhancements

+ 4 - 4
netbox/extras/api/views.py

@@ -418,10 +418,10 @@ class ScriptViewSet(ListModelMixin, RetrieveModelMixin, BaseViewSet):
                     notifications=input_serializer.validated_data.get('notifications'),
                 )
             except DjangoValidationError as e:
-                # The script's Meta configuration is invalid (see #22872). Surface it as a 400 rather than allowing the
-                # exception to bubble up as an HTTP 500. These are script-level config errors, not request-field errors,
-                # so report them as non-field errors.
-                raise ValidationError(e.messages)
+                # The script's execution configuration is invalid (see #22872). Surface it as a 400 rather than
+                # allowing the exception to bubble up as an HTTP 500. These are script-level config errors, not
+                # request-field errors, so report them under the non-field "detail" key.
+                raise ValidationError({'detail': e.messages}) from e
             serializer = serializers.ScriptDetailSerializer(script, context={'request': request})
 
             return Response(serializer.data)

+ 12 - 5
netbox/extras/jobs.py

@@ -8,6 +8,7 @@ from django.utils.translation import gettext as _
 from core.signals import clear_events
 from dcim.models import Device
 from extras.models import Script as ScriptModel
+from extras.scripts import _UNSET
 from netbox.context_managers import event_tracking
 from netbox.jobs import JobRunner
 from netbox.registry import registry
@@ -28,18 +29,24 @@ class ScriptJob(JobRunner):
         name = 'Run Script'
 
     @classmethod
-    def enqueue(cls, instance, *args, **kwargs):
+    def enqueue(cls, *args, instance=None, **kwargs):
         """
-        Validate the script's Meta parameters before enqueueing. This is the single choke point through which every
-        script execution passes (interactive runs, the REST API, the runscript command, event-rule actions, and
+        Validate the script's execution parameters before enqueueing. This is the single choke point through which
+        every script execution passes (interactive runs, the REST API, the runscript command, event-rule actions, and
         recurring reschedules), so validating here surfaces a misconfigured script as an actionable error rather than
         an unhandled exception at enqueue time (see #22872).
+
+        The values actually being enqueued are validated, not just the script's Meta defaults, so an explicit
+        job_timeout or notifications supplied by the caller is checked too.
         """
         script_class = getattr(instance, 'python_class', None)
         if script_class is not None:
-            script_class.validate_meta()
+            script_class.validate_meta(
+                job_timeout=kwargs.get('job_timeout', _UNSET),
+                notifications=kwargs.get('notifications', _UNSET),
+            )
 
-        return super().enqueue(instance, *args, **kwargs)
+        return super().enqueue(*args, instance=instance, **kwargs)
 
     def run_script(self, script, request, data, commit):
         """

+ 19 - 9
netbox/extras/scripts.py

@@ -46,6 +46,9 @@ __all__ = (
     'get_module_and_script',
 )
 
+# Sentinel distinguishing "argument not supplied" from an explicit None in validate_meta().
+_UNSET = object()
+
 
 #
 # Script variables
@@ -407,15 +410,20 @@ class BaseScript:
         return getattr(self.Meta, 'notifications_default', JobNotificationChoices.NOTIFICATION_ALWAYS)
 
     @classmethod
-    def validate_meta(cls):
+    def validate_meta(cls, job_timeout=_UNSET, notifications=_UNSET):
         """
-        Validate the script's execution-related Meta parameters. Raises a ValidationError if any value is invalid, so
-        that a misconfigured script surfaces an actionable error rather than an unhandled exception when the job is
-        enqueued (see #22872). Unset values fall back to valid defaults and are not rejected.
+        Validate the execution parameters used to run this script. Raises a ValidationError if any value is invalid,
+        so that a misconfigured script surfaces an actionable error rather than an unhandled exception when the job is
+        enqueued (see #22872).
+
+        The values actually enqueued are validated, not the raw Meta values: a caller may supply an explicit
+        `job_timeout` or `notifications` (e.g. via the REST API), in which case that value is checked. When a caller
+        omits a value, the corresponding Meta default is validated instead. Unset values fall back to valid defaults
+        and are not rejected.
         """
         errors = {}
 
-        job_timeout = cls.job_timeout
+        job_timeout = cls.job_timeout if job_timeout is _UNSET else job_timeout
         if job_timeout is not None:
             # parse_timeout() is what RQ applies to the timeout downstream. It raises TimeoutFormatError for
             # malformed duration strings, but a job_timeout of an unexpected type (e.g. a list) instead raises
@@ -434,12 +442,14 @@ class BaseScript:
                     "Invalid job_timeout value '{value}': must be a positive duration."
                 ).format(value=job_timeout)
 
-        notifications_default = cls.notifications_default
-        if notifications_default not in JobNotificationChoices.values():
+        # A caller may pass notifications=None to mean "use the script's default"; treat that as unset.
+        if notifications is _UNSET or notifications is None:
+            notifications = cls.notifications_default
+        if notifications not in JobNotificationChoices.values():
             valid = ', '.join(JobNotificationChoices.values())
             errors['notifications_default'] = _(
-                "Invalid notifications_default value '{value}': must be one of {valid}."
-            ).format(value=notifications_default, valid=valid)
+                "Invalid notifications value '{value}': must be one of {valid}."
+            ).format(value=notifications, valid=valid)
 
         if errors:
             raise ValidationError(errors)

+ 25 - 0
netbox/extras/tests/test_management_commands.py

@@ -412,6 +412,31 @@ class RunScriptTestCase(TestCase):
 
         self.assertEqual(enqueue.call_args.kwargs['user'], self.user)
 
+    def test_invalid_meta_raises_command_error(self):
+        """
+        A script with an invalid Meta value must fail with a clean CommandError rather than an unhandled
+        exception (#22872).
+        """
+        class BadMetaScript(Script):
+            class Meta:
+                job_timeout = 'not-a-timeout'
+
+            def run(self, data, commit):
+                return None
+
+        script_obj = SimpleNamespace(python_class=BadMetaScript)
+
+        # Note: ScriptJob.enqueue is intentionally NOT mocked here, so validate_meta() runs and raises.
+        with (
+            patch(
+                'extras.management.commands.runscript.get_module_and_script',
+                return_value=(None, script_obj),
+            ),
+            patch('extras.management.commands.runscript.logging.getLogger'),
+        ):
+            with self.assertRaises(CommandError):
+                call_command('runscript', 'test.Script', user='admin', stdout=StringIO())
+
 
 class WebhookReceiverTestCase(TestCase):
     def test_starts_http_server(self):