From 3c5a91162ba7f214d28dea109d49b8f903dffe5f Mon Sep 17 00:00:00 2001 From: Martin Burggraf Date: Sun, 30 Aug 2026 13:47:54 +0200 Subject: [PATCH] Fix empty error responses and test hygiene in script run API - Replace the '_' prefix heuristic for filtering ScriptForm errors with an explicit exclusion list (EXEC_PARAM_FIELDS). The heuristic also stripped Django's NON_FIELD_ERRORS ('__all__'), so a pure form-level error (e.g. ScriptForm.clean()'s "Scheduled time must be in the future.") resulted in an empty 400 response body - Nest script-variable errors under 'data' so they can't collide with ScriptInputSerializer's own top-level fields (commit, interval, ...) - Use input_serializer.is_valid(raise_exception=True) for consistency with the rest of the method - Clarify the "script class could not be loaded" error message - Move EXEC_PARAM_FIELDS to extras/scripts.py as the single source of truth for both the API view and the runscript command; drop prepare_script_form from extras.scripts.__all__ as internal plumbing - Fix runscript only popping 3 of 4 internal exec fields from cleaned_data, leaking '_notifications' into the script's own data - Restore Script.python_class via patch.object()/addCleanup() in ScriptRunExecutionTestCase instead of a permanent override - Add test coverage for schedule_at/interval forwarding and for rejecting a nonexistent object ID --- netbox/extras/api/views.py | 26 ++++++---- .../extras/management/commands/runscript.py | 13 ++--- netbox/extras/scripts.py | 18 ++++++- netbox/extras/tests/test_api.py | 49 ++++++++++++++++--- 4 files changed, 82 insertions(+), 24 deletions(-) diff --git a/netbox/extras/api/views.py b/netbox/extras/api/views.py index 0841c4e21..aa6f001a2 100644 --- a/netbox/extras/api/views.py +++ b/netbox/extras/api/views.py @@ -1,8 +1,8 @@ +from django.core.exceptions import NON_FIELD_ERRORS 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, extend_schema_view -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,7 +16,7 @@ from core.choices import ManagedFileRootPathChoices from extras import filtersets from extras.jobs import ScriptJob from extras.models import * -from extras.scripts import prepare_script_form +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 @@ -367,8 +367,7 @@ class ScriptViewSet(ModelViewSet): if not any_workers_for_queue('default'): raise RQWorkerNotRunningException() - if not input_serializer.is_valid(): - return Response(input_serializer.errors, status=status.HTTP_400_BAD_REQUEST) + input_serializer.is_valid(raise_exception=True) validated = input_serializer.validated_data @@ -380,17 +379,26 @@ class ScriptViewSet(ModelViewSet): script_class = script.python_class if not script_class: - raise ValidationError({'script': _('Script class could not be loaded; cannot determine job timeout.')}) + raise ValidationError({'script': _('Unable to load the script class.')}) script_instance = script_class() form = prepare_script_form(script_instance, payload, files=request.FILES) if not form.is_valid(): - # remove internal fields (_commit etc.) from API error message - errors = {k: v for k, v in form.errors.items() if not k.startswith('_')} - raise ValidationError(errors) + # Exec params (_commit etc.) are validated separately via ScriptInputSerializer; + # exclude them explicitly rather than via a '_' prefix, which would also strip + # Django's NON_FIELD_ERRORS key ('__all__') and any script var legitimately + # named with a leading underscore. + errors = {k: v for k, v in form.errors.items() if k not in EXEC_PARAM_FIELDS} + if not errors: + # Only a non-field error remains (e.g. ScriptForm.clean()'s "Scheduled + # time must be in the future."). Surface it instead of an empty body. + errors = {NON_FIELD_ERRORS: form.errors.get(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 ('_commit', '_schedule_at', '_interval', '_notifications'): + for k in EXEC_PARAM_FIELDS: data.pop(k, None) ScriptJob.enqueue( diff --git a/netbox/extras/management/commands/runscript.py b/netbox/extras/management/commands/runscript.py index ef3a46fac..f1b77e901 100644 --- a/netbox/extras/management/commands/runscript.py +++ b/netbox/extras/management/commands/runscript.py @@ -6,7 +6,7 @@ import uuid 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 @@ -81,17 +81,18 @@ 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') + # Remove exec-parameter fields from ScriptForm before passing data to the script. + # (Previously missed '_notifications', which leaked into the script's own data.) + cleaned_data = form.cleaned_data.copy() + for key in EXEC_PARAM_FIELDS: + cleaned_data.pop(key, None) # Execute the script. job = ScriptJob.enqueue( instance=script_obj, user=user, immediate=True, - data=form.cleaned_data, + data=cleaned_data, request=NetBoxFakeRequest({ 'META': {}, 'COOKIES': {}, diff --git a/netbox/extras/scripts.py b/netbox/extras/scripts.py index 09d427b8c..c27c72ff8 100644 --- a/netbox/extras/scripts.py +++ b/netbox/extras/scripts.py @@ -41,9 +41,13 @@ __all__ = ( 'StringVar', 'TextVar', 'get_module_and_script', - 'prepare_script_form', ) +# 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') + # # Script variables @@ -664,7 +668,17 @@ def prepare_script_form(script_instance, data, files=None): declared variable's `default` value into `data` when the caller omitted it. Used by both the UI (extras/views.py) and the REST API (extras/api/views.py) so the - two entry points share one contract and can't drift apart again. + two entry points share one contract and can't drift apart again. `runscript` deliberately + stays on the plain `as_form()` call, since its own test suite exercises it with bare + script doubles that only implement `as_form()`, not the full Script/`_get_vars()` API. + + Note: `script_instance` must already be an *instance* (e.g. `script.python_class()`), + not the class itself. + + `data` is copied via `.copy()` rather than coerced with `dict(...)`, so a QueryDict + (as submitted by the UI form) keeps its multi-value semantics -- collapsing it to a + plain dict would silently drop all but the last value for a MultiObjectVar's + multi-select field. """ data = data.copy() if data is not None else {} for name, var in script_instance._get_vars().items(): diff --git a/netbox/extras/tests/test_api.py b/netbox/extras/tests/test_api.py index 039dc7766..e8467e9c2 100644 --- a/netbox/extras/tests/test_api.py +++ b/netbox/extras/tests/test_api.py @@ -1447,11 +1447,11 @@ class ScriptTestCase(APITestCase): class ScriptRunExecutionTestCase(APITestCase): """ Exercises ScriptViewSet.post() end-to-end (real request -> real serializer -> real - form -> real ScriptJob.enqueue() call), covering the regressions raised in review of - PR #22861: execution parameters must be taken from the validated request rather than - the form's own defaults, ObjectVar/MultiObjectVar values must be converted from raw - IDs to model instances/querysets, and declared defaults must be back-filled for - variables the client omits. + form -> real ScriptJob.enqueue() call): execution parameters (commit, schedule_at, + interval, notifications) must be taken from the validated request rather than the + form's own defaults; ObjectVar/MultiObjectVar values must be converted from raw IDs + to model instances/querysets (see #22750); and declared defaults must be back-filled + for variables the client omits. """ class TestScriptClass(PythonClass): @@ -1486,8 +1486,11 @@ class ScriptRunExecutionTestCase(APITestCase): super().setUp() self.add_permissions('extras.run_script') - # Monkey-patch the Script model to return our TestScriptClass above - Script.python_class = self.TestScriptClass + # 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. @@ -1526,6 +1529,25 @@ class ScriptRunExecutionTestCase(APITestCase): 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 = { @@ -1565,6 +1587,19 @@ class ScriptRunExecutionTestCase(APITestCase): 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() + class CreatedUpdatedFilterTestCase(APITestCase):