Fixes #22750: Validate Custom Script input and resolve object IDs in the REST API (#23119)

Validate REST script input before enqueueing jobs. Resolve ObjectVar
IDs to model instances and MultiObjectVar IDs to querysets, returning
HTTP 400 with errors nested under data when validation fails.

Share form preparation between the API and UI, including multi-value
defaults, while keeping validation out of the job runner to preserve
other execution paths. Exclude only known execution fields from script
data and prevent _notifications from leaking into CLI script input.

Document the REST compatibility changes, including required-field
validation and discarded undeclared keys. Add regression coverage for
object resolution, defaults, validation errors, and execution options.

Co-authored-by: Martin Burggraf <martin.burggraf@netclusive.com>

docs/customization/custom-scripts.md

@@ -301,6 +301,9 @@ All custom script variables support the following default options:
 * `required` - Indicates whether the field is mandatory (all fields are required by default)
 * `widget` - The class of form widget to use (see the [Django documentation](https://docs.djangoproject.com/en/stable/ref/forms/widgets/))
 
+!!! warning "Reserved variable names"
+    The names `_commit`, `_schedule_at`, `_interval`, and `_notifications` are reserved for the execution parameters which NetBox renders alongside a script's own fields. A variable declared with one of these names shadows its execution parameter, and its value is not passed to `run()`. Choose a different name.
+
 ### StringVar
 
 Stores a string of characters (i.e. text). Options include:
@@ -546,6 +549,9 @@ http://netbox/api/extras/scripts/example.MyReport/ \
 
 Optionally `schedule_at` can be passed in the form data with a datetime string to schedule a script at the specified date and time.
 
+!!! note
+    Script input submitted through the REST API is validated against the variables declared by the script. Missing required variables or invalid values result in an HTTP 400 response, and undeclared keys are discarded rather than passed to `run()`. Existing API clients that relied on the previous pass-through behavior may need to update their requests. Scripts declaring a `FileVar` must be run via a `multipart/form-data` request, passing `data` as a JSON string alongside the uploaded file.
+
 ### Via the CLI
 
 Scripts can be run on the CLI by invoking the management command:

netbox/extras/api/serializers_/scripts.py

@@ -188,6 +188,16 @@ class ScriptInputSerializer(serializers.Serializer):
         if script and script.python_class:
             self.fields['notifications'].default = script.python_class.notifications_default
 
+    def validate_data(self, value):
+        """
+        Validates that the script input is an object mapping variable names to values.
+        """
+        if not isinstance(value, dict):
+            raise serializers.ValidationError(
+                _('Invalid data payload; expected an object mapping variable names to values.')
+            )
+        return value
+
     def validate_schedule_at(self, value):
         """
         Validates the specified schedule time for a script execution.

netbox/extras/api/views.py

@@ -1,9 +1,9 @@
+from django.core.exceptions import NON_FIELD_ERRORS
 from django.core.exceptions import ValidationError as DjangoValidationError
 from django.http import Http404
 from django.shortcuts import get_object_or_404
 from django.utils.translation import gettext_lazy as _
 from drf_spectacular.utils import OpenApiResponse, OpenApiTypes, extend_schema
-from rest_framework import status
 from rest_framework.decorators import action
 from rest_framework.exceptions import PermissionDenied, ValidationError
 from rest_framework.generics import RetrieveUpdateDestroyAPIView
@@ -16,6 +16,7 @@ from core.choices import ManagedFileRootPathChoices
 from extras import filtersets
 from extras.jobs import ScriptJob
 from extras.models import *
+from extras.scripts import EXEC_PARAM_FIELDS, prepare_script_form
 from netbox.api.authentication import IsAuthenticatedOrLoginNotRequired, TokenWritePermission
 from netbox.api.features import SyncedDataMixin
 from netbox.api.metadata import ContentTypeMetadata
@@ -404,29 +405,56 @@ class ScriptViewSet(ListModelMixin, RetrieveModelMixin, BaseViewSet):
         if not any_workers_for_queue('default'):
             raise RQWorkerNotRunningException()
 
-        if input_serializer.is_valid():
-            try:
-                ScriptJob.enqueue(
-                    instance=script,
-                    user=request.user,
-                    data=input_serializer.data['data'],
-                    request=copy_safe_request(request),
-                    commit=input_serializer.data['commit'],
-                    job_timeout=script.python_class.job_timeout,
-                    schedule_at=input_serializer.validated_data.get('schedule_at'),
-                    interval=input_serializer.validated_data.get('interval'),
-                    notifications=input_serializer.validated_data.get('notifications'),
-                )
-            except DjangoValidationError as e:
-                # 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)
-
-        return Response(input_serializer.errors, status=status.HTTP_400_BAD_REQUEST)
+        input_serializer.is_valid(raise_exception=True)
+
+        validated = input_serializer.validated_data
+
+        payload = validated['data']
+
+        # Guaranteed non-None by the is_executable check above
+        script_class = script.python_class
+        script_instance = script_class()
+
+        form = prepare_script_form(script_instance, payload, files=request.FILES)
+        if not form.is_valid():
+            # Exec params are validated separately via ScriptInputSerializer. Excluded by name
+            # rather than by '_' prefix, which would also strip Django's NON_FIELD_ERRORS
+            # key ('__all__').
+            errors = {k: v for k, v in form.errors.items() if k not in EXEC_PARAM_FIELDS}
+            if not errors:
+                # Every error was on an exec-param field, which a client can bind by naming one
+                # in 'data' (e.g. {"_interval": "abc"}). NON_FIELD_ERRORS is never among them --
+                # the filter above retains '__all__' -- so there is nothing to re-surface here;
+                # report a generic message rather than an empty body.
+                errors = {NON_FIELD_ERRORS: [_('Invalid script input.')]}
+            # Nest under 'data' so script-variable errors can't collide with the
+            # serializer's own top-level fields (commit, schedule_at, interval, ...).
+            raise ValidationError({'data': errors})
+
+        data = form.cleaned_data.copy()
+        for k in EXEC_PARAM_FIELDS:
+            data.pop(k, None)
+
+        try:
+            ScriptJob.enqueue(
+                instance=script,
+                user=request.user,
+                data=data,
+                request=copy_safe_request(request),
+                commit=validated.get('commit'),
+                job_timeout=script_class.job_timeout,
+                schedule_at=validated.get('schedule_at'),
+                interval=validated.get('interval'),
+                notifications=validated.get('notifications'),
+            )
+        except DjangoValidationError as e:
+            # 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)
 
 
 #

netbox/extras/management/commands/runscript.py

@@ -7,7 +7,7 @@ from django.core.exceptions import ValidationError
 from django.core.management.base import BaseCommand, CommandError
 
 from extras.jobs import ScriptJob
-from extras.scripts import get_module_and_script
+from extras.scripts import EXEC_PARAM_FIELDS, get_module_and_script
 from users.models import User
 from utilities.request import NetBoxFakeRequest
 
@@ -82,11 +82,11 @@ class Command(BaseCommand):
                     logger.error(f'\t{field}: {error.get("message")}')
             raise CommandError()
 
-        # Remove extra fields from ScriptForm before passing data to script
-        form.cleaned_data.pop('_schedule_at')
-        form.cleaned_data.pop('_interval')
-        form.cleaned_data.pop('_commit')
-        notifications = form.cleaned_data.pop('_notifications')
+        # Remove exec-parameter fields from ScriptForm before passing data to the script
+        cleaned_data = form.cleaned_data.copy()
+        notifications = cleaned_data.pop('_notifications')
+        for key in EXEC_PARAM_FIELDS:
+            cleaned_data.pop(key, None)
 
         # Execute the script.
         try:
@@ -94,7 +94,7 @@ class Command(BaseCommand):
                 instance=script_obj,
                 user=user,
                 immediate=True,
-                data=form.cleaned_data,
+                data=cleaned_data,
                 notifications=notifications,
                 request=NetBoxFakeRequest({
                     'META': {},

netbox/extras/scripts.py

@@ -46,6 +46,11 @@ __all__ = (
     'get_module_and_script',
 )
 
+# Internal ScriptForm fields used to carry execution parameters (see ScriptForm in
+# extras/forms/scripts.py). These are validated/sourced separately from the script's own
+# declared variables and must never be treated as script data or surfaced as script errors.
+EXEC_PARAM_FIELDS = ('_commit', '_schedule_at', '_interval', '_notifications')
+
 # Sentinel distinguishing "argument not supplied" from an explicit None in validate_meta().
 _UNSET = object()
 
@@ -706,3 +711,27 @@ def get_module_and_script(module_name, script_name):
     module = ScriptModule.objects.get(file_path=f'{module_name}.py')
     script = module.scripts.get(name=script_name)
     return module, script
+
+
+def prepare_script_form(script_instance, data, files=None):
+    """
+    Return a bound ScriptForm for the given Script instance, back-filling the declared
+    `default` of any variable omitted from `data`.
+
+    `data` is copied rather than coerced to a plain dict, so a QueryDict retains the
+    multi-value semantics a MultiObjectVar's multi-select field depends on.
+    """
+    data = data.copy() if data is not None else {}
+    for name, var in script_instance._get_vars().items():
+        if name in data:
+            continue
+        if (initial := var.field_attrs.get('initial')) is None:
+            continue
+        if isinstance(initial, (list, tuple)) and hasattr(data, 'setlist'):
+            # Assigning a list to a QueryDict stores it as a single nested value, which a
+            # multi-select widget reads back as one bogus choice. Set the values individually
+            # so a MultiChoiceVar/MultiObjectVar default binds as it does for a plain dict.
+            data.setlist(name, list(initial))
+        else:
+            data[name] = initial
+    return script_instance.as_form(data=data, files=files)

netbox/extras/tests/test_api.py

@@ -13,14 +13,14 @@ from django.urls import reverse
 from django.utils.timezone import make_aware, now
 from rest_framework import status
 
-from core.choices import ManagedFileRootPathChoices
+from core.choices import JobNotificationChoices, ManagedFileRootPathChoices
 from core.events import *
 from core.models import DataFile, DataSource, Job, ObjectType
 from dcim.models import Device, DeviceRole, DeviceType, Location, Manufacturer, Rack, RackRole, Site
 from extras.api.serializers import EventRuleSerializer
 from extras.choices import *
 from extras.models import *
-from extras.scripts import BooleanVar, IntegerVar, StringVar
+from extras.scripts import BooleanVar, IntegerVar, MultiObjectVar, ObjectVar, StringVar
 from extras.scripts import Script as PythonClass
 from netbox.event_rules import EventRuleAction, register_event_rule_action
 from netbox.registry import registry
@@ -1890,6 +1890,208 @@ class ScriptTestCase(APITestCase):
                 self.assertHttpStatus(response, status.HTTP_404_NOT_FOUND)
 
 
+class ScriptRunExecutionTestCase(APITestCase):
+    """
+    Exercises ScriptViewSet.run() end-to-end: request -> serializer -> form -> enqueue.
+    """
+
+    class TestScriptClass(PythonClass):
+        class Meta:
+            name = 'Test run script'
+
+        site = ObjectVar(model=Site)
+        sites = MultiObjectVar(model=Site, required=False)
+        label = StringVar(default='hello')
+        flag = BooleanVar(default=True)
+
+        def run(self, data, commit=True):
+            return 'ok'
+
+    @classmethod
+    def setUpTestData(cls):
+        cls.sites = [
+            Site.objects.create(name=f'Test Site {i}', slug=f'test-site-{i}') for i in range(1, 3)
+        ]
+        with patch.object(ScriptModule, 'sync_classes'):
+            module = ScriptModule.objects.create(
+                file_root=ManagedFileRootPathChoices.SCRIPTS,
+                file_path='run_script.py',
+            )
+        script = Script.objects.create(
+            module=module,
+            name='Test run script',
+            is_executable=True,
+        )
+        cls.url = reverse('extras-api:script-detail', kwargs={'pk': script.pk})
+
+    def setUp(self):
+        super().setUp()
+        self.add_permissions('extras.run_script')
+
+        # Monkey-patch the Script model to return our TestScriptClass above, restoring
+        # the real property afterwards so later tests aren't left with our stub.
+        python_class_patch = patch.object(Script, 'python_class', new=self.TestScriptClass)
+        python_class_patch.start()
+        self.addCleanup(python_class_patch.stop)
+
+        # The script-run endpoint gates on a live RQ worker. Tests run without one, so
+        # bypass the check to exercise validation and the enqueue path.
+        worker_patch = patch('extras.api.views.any_workers_for_queue', return_value=True)
+        worker_patch.start()
+        self.addCleanup(worker_patch.stop)
+
+    @patch('extras.jobs.ScriptJob.enqueue')
+    def test_run_forwards_commit_value(self, mock_enqueue):
+        for commit_value in (True, False):
+            with self.subTest(commit=commit_value):
+                mock_enqueue.reset_mock()
+                payload = {'data': {'site': self.sites[0].pk}, 'commit': commit_value}
+
+                response = self.client.post(self.url, payload, format='json', **self.header)
+
+                self.assertHttpStatus(response, status.HTTP_200_OK)
+                mock_enqueue.assert_called_once()
+                self.assertIs(mock_enqueue.call_args.kwargs['commit'], commit_value)
+
+    @patch('extras.jobs.ScriptJob.enqueue')
+    def test_run_forwards_notifications_value(self, mock_enqueue):
+        # Regression: ScriptForm.clean() overwrites an empty '_notifications' with the
+        # field's own initial, so a client-supplied value never reached ScriptJob.enqueue.
+        payload = {
+            'data': {'site': self.sites[0].pk},
+            'commit': True,
+            'notifications': JobNotificationChoices.NOTIFICATION_NEVER,
+        }
+
+        response = self.client.post(self.url, payload, format='json', **self.header)
+
+        self.assertHttpStatus(response, status.HTTP_200_OK)
+        self.assertEqual(
+            mock_enqueue.call_args.kwargs['notifications'],
+            JobNotificationChoices.NOTIFICATION_NEVER,
+        )
+
+    @patch('extras.jobs.ScriptJob.enqueue')
+    def test_run_forwards_schedule_at_and_interval(self, mock_enqueue):
+        # Regression: schedule_at/interval were likewise read from the form (always
+        # absent there) instead of the validated request
+        schedule_at = now() + datetime.timedelta(hours=1)
+        payload = {
+            'data': {'site': self.sites[0].pk},
+            'commit': True,
+            'schedule_at': schedule_at,
+            'interval': 60,
+        }
+
+        response = self.client.post(self.url, payload, format='json', **self.header)
+
+        self.assertHttpStatus(response, status.HTTP_200_OK)
+        kwargs = mock_enqueue.call_args.kwargs
+        self.assertEqual(kwargs['schedule_at'], schedule_at)
+        self.assertEqual(kwargs['interval'], 60)
+
+    @patch('extras.jobs.ScriptJob.enqueue')
+    def test_run_converts_objectvar_and_multiobjectvar_ids(self, mock_enqueue):
+        payload = {
+            'data': {
+                'site': self.sites[0].pk,
+                'sites': [site.pk for site in self.sites],
+            },
+            'commit': True,
+        }
+
+        response = self.client.post(self.url, payload, format='json', **self.header)
+
+        self.assertHttpStatus(response, status.HTTP_200_OK)
+        data = mock_enqueue.call_args.kwargs['data']
+        self.assertEqual(data['site'], self.sites[0])
+        self.assertEqual(
+            set(data['sites'].values_list('pk', flat=True)),
+            {site.pk for site in self.sites},
+        )
+
+    @patch('extras.jobs.ScriptJob.enqueue')
+    def test_run_backfills_default_for_omitted_required_var(self, mock_enqueue):
+        # Regression: required vars declaring `default=` were not back-filled before
+        # binding the form on the API path (unlike the UI path), so they 400'd even
+        # though the client legitimately omitted them.
+        payload = {'data': {'site': self.sites[0].pk}, 'commit': True}  # 'label' omitted
+
+        response = self.client.post(self.url, payload, format='json', **self.header)
+
+        self.assertHttpStatus(response, status.HTTP_200_OK)
+        self.assertEqual(mock_enqueue.call_args.kwargs['data']['label'], 'hello')
+
+    def test_run_rejects_non_dict_payload(self):
+        payload = {'data': 'not-a-dict', 'commit': True}
+
+        response = self.client.post(self.url, payload, format='json', **self.header)
+
+        self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST)
+
+    @patch('extras.jobs.ScriptJob.enqueue')
+    def test_run_rejects_nonexistent_object_id(self, mock_enqueue):
+        # This is the primary new failure mode introduced by converting raw IDs to model
+        # instances: a PK that doesn't resolve must 400 cleanly, not enqueue a broken job
+        # or raise an unhandled DoesNotExist.
+        nonexistent_pk = Site.objects.order_by('-pk').first().pk + 1000
+        payload = {'data': {'site': nonexistent_pk}, 'commit': True}
+
+        response = self.client.post(self.url, payload, format='json', **self.header)
+
+        self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST)
+        mock_enqueue.assert_not_called()
+
+    @patch('extras.jobs.ScriptJob.enqueue')
+    def test_run_ignores_undeclared_keys(self, mock_enqueue):
+        # Binding 'data' to the script's form means keys which don't correspond to a declared
+        # variable are dropped rather than forwarded to run(). This is the contract documented
+        # under "Running Custom Scripts > Via the API"; pin it so it can't regress silently.
+        payload = {
+            'data': {'site': self.sites[0].pk, 'bogus': 'ignored', 'id': 99},
+            'commit': True,
+        }
+
+        response = self.client.post(self.url, payload, format='json', **self.header)
+
+        self.assertHttpStatus(response, status.HTTP_200_OK)
+        data = mock_enqueue.call_args.kwargs['data']
+        self.assertEqual(data['site'], self.sites[0])
+        self.assertNotIn('bogus', data)
+        self.assertNotIn('id', data)
+
+    @patch('extras.jobs.ScriptJob.enqueue')
+    def test_run_rejects_omitted_required_var(self, mock_enqueue):
+        # 'site' is required and declares no default, so unlike 'label' it cannot be
+        # back-filled: omitting it must 400 rather than enqueue a job that fails at runtime.
+        payload = {'data': {'label': 'hi'}, 'commit': True}
+
+        response = self.client.post(self.url, payload, format='json', **self.header)
+
+        self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST)
+        self.assertIn('site', response.data['data'])
+        mock_enqueue.assert_not_called()
+
+    @patch('extras.jobs.ScriptJob.enqueue')
+    def test_run_resolves_booleanvar_default_and_explicit_values(self, mock_enqueue):
+        # BooleanVar renders as a checkbox, and CheckboxInput reads a missing key as False.
+        # An omitted BooleanVar must therefore pick up its declared default, while an
+        # explicitly supplied False must not be overwritten by that default.
+        for case, supplied, expected in (
+            ('omitted', {}, True),
+            ('explicit False', {'flag': False}, False),
+            ('explicit True', {'flag': True}, True),
+        ):
+            with self.subTest(case=case):
+                mock_enqueue.reset_mock()
+                payload = {'data': {'site': self.sites[0].pk, **supplied}, 'commit': True}
+
+                response = self.client.post(self.url, payload, format='json', **self.header)
+
+                self.assertHttpStatus(response, status.HTTP_200_OK)
+                self.assertIs(mock_enqueue.call_args.kwargs['data']['flag'], expected)
+
+
 class CreatedUpdatedFilterTestCase(APITestCase):
 
     @classmethod

netbox/extras/tests/test_views.py

@@ -14,7 +14,7 @@ from core.models import Job, ObjectType
 from dcim.models import DeviceType, Manufacturer, Site
 from extras.choices import *
 from extras.models import *
-from extras.scripts import BooleanVar, IntegerVar
+from extras.scripts import BooleanVar, IntegerVar, MultiChoiceVar, StringVar
 from extras.scripts import Script as PythonClass
 from users.models import Group, ObjectPermission, User
 from utilities.testing import TestCase, ViewTestCases
@@ -1279,6 +1279,62 @@ class ScriptModuleCreateViewTestCase(TestCase):
         self.assertEqual(response.context['return_url'], reverse('extras:script_list'))
 
 
+class ScriptDefaultBackfillTestCase(TestCase):
+    """
+    The UI and the REST API now share prepare_script_form(), so the UI's back-filling of
+    declared defaults (previously inline in ScriptView.post()) must keep working after
+    that logic moved into the helper.
+    """
+    user_permissions = ['extras.view_script', 'extras.run_script']
+
+    class TestScriptClass(PythonClass):
+        class Meta:
+            name = 'Backfill test'
+            commit_default = False
+
+        label = StringVar(default='hello')
+        flag = BooleanVar(default=True)
+        picks = MultiChoiceVar(choices=(('a', 'A'), ('b', 'B'), ('c', 'C')), default=['a', 'b'])
+
+        def run(self, data, commit):
+            return 'Complete'
+
+    @classmethod
+    def setUpTestData(cls):
+        with patch.object(ScriptModule, 'sync_classes'):
+            module = ScriptModule.objects.create(
+                file_root=ManagedFileRootPathChoices.SCRIPTS,
+                file_path='backfill_script.py',
+            )
+        cls.script = Script.objects.create(module=module, name='Backfill test', is_executable=True)
+
+    def setUp(self):
+        super().setUp()
+        python_class_patch = patch.object(Script, 'python_class', new=self.TestScriptClass)
+        python_class_patch.start()
+        self.addCleanup(python_class_patch.stop)
+
+    @tag('regression')
+    def test_ui_backfills_declared_defaults(self):
+        url = reverse('extras:script', kwargs={'pk': self.script.pk})
+
+        with (
+            patch('extras.views.any_workers_for_queue', return_value=True),
+            patch('extras.jobs.ScriptJob.enqueue') as mock_enqueue,
+        ):
+            mock_enqueue.return_value.pk = 1
+            response = self.client.post(url, {'_commit': 'true'})
+
+        self.assertEqual(response.status_code, 302)
+        data = mock_enqueue.call_args.kwargs['data']
+        self.assertEqual(data['label'], 'hello')
+        self.assertIs(data['flag'], True)
+        # A multi-value default must be set on the QueryDict with setlist(): a plain
+        # assignment stores the list as one nested value, which the multi-select widget
+        # then rejects as a single invalid choice.
+        self.assertEqual(data['picks'], ['a', 'b'])
+
+
 class ScriptValidationErrorTestCase(TestCase):
     user_permissions = ['extras.view_script', 'extras.run_script']
 

netbox/extras/views.py

@@ -24,6 +24,7 @@ from dcim.models import Device, DeviceRole, Platform
 from extras.choices import LogLevelChoices
 from extras.dashboard.forms import DashboardWidgetAddForm, DashboardWidgetForm
 from extras.dashboard.utils import get_widget_class
+from extras.scripts import prepare_script_form
 from extras.utils import SharedObjectViewMixin
 from netbox.object_actions import *
 from netbox.ui import layout
@@ -1739,13 +1740,7 @@ class ScriptView(BaseScriptView):
                 'script': script,
             })
 
-        # Populate missing variables with their default values, if defined
-        post_data = request.POST.copy()
-        for name, var in script_class._get_vars().items():
-            if name not in post_data and (initial := var.field_attrs.get('initial')) is not None:
-                post_data[name] = initial
-
-        form = script_class.as_form(post_data, request.FILES)
+        form = prepare_script_form(script_class, request.POST, request.FILES)
 
         # Allow execution only if RQ worker process is running
         if not any_workers_for_queue('default'):