#21992: Additional cleanup

docs/integrations/rest-api.md

@@ -744,7 +744,7 @@ http://netbox/api/dcim/sites/ \
 
 Bulk write operations (creating, updating, or deleting multiple objects via a model's list endpoint) can optionally be processed as a [background job](../features/background-jobs.md) rather than synchronously. This is useful for large batches that would otherwise hold the connection open long enough to risk a proxy or gateway timeout.
 
-To request background processing, append the `background=true` query parameter to a bulk write request. NetBox validates the request immediately and, if it is well-formed and authorized, enqueues a job and returns an `HTTP 202 Accepted` response containing the job's ID and URL. The actual write is performed later by a worker, running the same logic (and preserving the same all-or-none transaction semantics) as the synchronous path.
+To request background processing, append the `background=true` query parameter to a bulk write request. NetBox enqueues a job and returns an `HTTP 202 Accepted` response containing the job's ID and URL. The actual write is performed later by a worker, running the same logic (and preserving the same all-or-none transaction semantics) as the synchronous path. Note that the request payload is **not** validated before the job is enqueued; validation is deferred to the worker (see below).
 
 ```no-highlight
 curl -s -X PATCH \
@@ -779,7 +779,7 @@ Poll the job's URL to track its progress. When the job reaches a terminal status
 
 A failed job records the equivalent error response, for instance `{"status_code": 400, "data": {"slug": ["This field may not be blank."]}}`, with a short summary also placed in the job's `error` field.
 
-A `202` response indicates that the request was accepted and queued, not that it succeeded: object-level validation and the database write occur when the job runs. Always inspect the job's final status to confirm the outcome. Because the result is stored on the job, any user permitted to view jobs (`core.view_job`, subject to object permissions) can read the serialized objects it contains.
+A `202` response indicates that the request was accepted and queued, not that it succeeded: validation (including malformed or invalid payloads) and the database write all occur when the job runs. A rejected payload is therefore reported as a failed job rather than a synchronous error response. Always inspect the job's final status to confirm the outcome. Because the result is stored on the job, any user permitted to view jobs (`core.view_job`, subject to object permissions) can read the serialized objects it contains.
 
 Background processing applies only to bulk operations (a JSON list) on a model's list endpoint. For a single-object write the `background` parameter is ignored and the request is processed synchronously. It cannot be combined with an [`If-Match`](#if-match) precondition (which cannot be evaluated reliably once execution is deferred); such a request is rejected with an `HTTP 400` response. If no background worker is running to service the queue, the request is rejected with an `HTTP 503` response rather than enqueuing a job that would never run.
 

netbox/netbox/api/viewsets/__init__.py

@@ -224,8 +224,8 @@ class NetBoxModelViewSet(
 
         This mirrors the except clauses in dispatch(); it is also called by the background
         job runner (netbox.jobs.AsyncAPIJob), which executes action methods directly and so
-        bypasses dispatch(). NOTE: dispatch() does not yet call this helper itself — see the
-        PR description for the proposed consolidation.
+        bypasses dispatch(). NOTE: dispatch() does not yet call this helper itself; the two
+        should be consolidated into a single source of truth in a future change.
         """
         logger = logging.getLogger(f'netbox.api.views.{self.__class__.__name__}')
         if isinstance(exc, (ProtectedError, RestrictedError)):
@@ -245,9 +245,9 @@ class NetBoxModelViewSet(
     # Creates
 
     def create(self, request, *args, **kwargs):
-        # If background processing was requested for a bulk (list) create, validate and enqueue.
-        # Single-object creates always run synchronously.
-        if (response := self._maybe_background_bulk_create(request)) is not None:
+        # If background processing was requested for a bulk (list) create, enqueue a job and
+        # return immediately. Single-object creates always run synchronously.
+        if (response := self._handle_background_request(request, 'create')) is not None:
             return response
 
         serializer = self.get_serializer(data=request.data)

netbox/netbox/api/viewsets/mixins.py

@@ -13,6 +13,7 @@ from netbox.api.serializers import BulkOperationSerializer
 from netbox.api.serializers.bulk import get_bulk_update_serializer_class
 from netbox.jobs import AsyncAPIJob
 from utilities.exceptions import RQWorkerNotRunningException
+from utilities.request import copy_safe_request
 from utilities.rqworker import any_workers_for_queue
 
 __all__ = (
@@ -29,10 +30,10 @@ __all__ = (
 class BackgroundOperationMixin:
     """
     Enable optional background processing of REST API bulk write operations. When a write
-    request to a list endpoint includes ``?background=true``, the bulk action validates the
-    payload synchronously, enqueues an ``AsyncAPIJob`` to perform the work, and immediately
-    returns ``202 Accepted`` with the job's ID and polling URL. The actual write runs in a
-    worker via the same action method, so behavior is identical to the synchronous path.
+    request to a list endpoint includes ``?background=true``, the bulk action enqueues an
+    ``AsyncAPIJob`` to perform the work and immediately returns ``202 Accepted`` with the
+    job's ID and polling URL. The actual write (including validation) runs in a worker via
+    the same action method, so behavior is identical to the synchronous path.
 
     This mixin overrides no framework methods; the bulk action methods call its helpers.
     """
@@ -43,22 +44,19 @@ class BackgroundOperationMixin:
             return False
         return request.query_params.get('background', '').lower() == 'true'
 
-    def _maybe_background_bulk_create(self, request):
+    def _handle_background_request(self, request, action, action_kwargs=None):
         """
-        Shared entry point for the create() overrides. If background processing was requested
-        for a bulk (list) create, validate the payload synchronously and return a 202 Response;
-        otherwise return None so the caller proceeds with synchronous creation.
+        Shared entry point for the bulk write actions. If background processing was requested
+        for a bulk (list) operation, enqueue an AsyncAPIJob and return a 202 Response; otherwise
+        return None so the caller proceeds synchronously.
+
+        Validation is intentionally deferred to the worker (which runs the same action method),
+        so it is not performed twice and the request returns promptly regardless of batch size.
         """
         if not (isinstance(request.data, list) and self._background_requested(request)):
             return None
 
-        # Validate synchronously before enqueuing so a malformed payload is rejected with a
-        # 400 now, rather than producing a 202 for work that can never succeed. (Constraints
-        # that depend on other items in the batch are still evaluated when the job runs.)
-        serializer = self.get_serializer(data=request.data, many=True)
-        serializer.is_valid(raise_exception=True)
-
-        return self._enqueue_bulk_job(request, 'create', payload=list(request.data))
+        return self._enqueue_bulk_job(request, action, payload=list(request.data), action_kwargs=action_kwargs)
 
     def _enqueue_bulk_job(self, request, action, payload, action_kwargs=None):
         """
@@ -85,6 +83,16 @@ class BackgroundOperationMixin:
             verb=verb,
             object_type=model._meta.verbose_name_plural,
         )
+        # Carry a serializable snapshot of the request so the worker can reconstruct it (method,
+        # request ID, and host metadata for absolute URLs in the captured result). The scheme is
+        # passed separately, as copy_safe_request() does not capture it. The worker re-fetches the
+        # user by PK and bypasses authentication entirely, so it reads neither the copied user nor
+        # cookies; drop both so no User instance or session data is pickled into the job payload
+        # for the lifetime of the job.
+        request_copy = copy_safe_request(request, include_files=False)
+        request_copy.user = None
+        request_copy.COOKIES = {}
+
         job = AsyncAPIJob.enqueue(
             name=job_name,
             user=request.user,
@@ -92,13 +100,9 @@ class BackgroundOperationMixin:
             action=action,
             payload=payload,
             user_pk=request.user.pk,
-            request_id=str(getattr(request, 'id', '')),
-            method=request.method,
             action_kwargs=action_kwargs or {},
-            # Carry the request's scheme/host so URLs in the captured result are absolute
-            # and followable (the worker has no real request to derive them from).
+            request=request_copy,
             scheme=request.scheme,
-            host=request.get_host(),
         )
 
         job_url = reverse('core-api:job-detail', kwargs={'pk': job.pk}, request=request)
@@ -152,11 +156,12 @@ class SequentialBulkCreatesMixin:
     appropriately.
     """
     def create(self, request, *args, **kwargs):
-        # If background processing was requested for a bulk (list) create, validate and enqueue.
-        # _maybe_background_bulk_create() comes from BackgroundOperationMixin; fall back to "no
-        # background" so this mixin remains usable on its own (e.g. in custom viewsets).
-        maybe_background = getattr(self, '_maybe_background_bulk_create', lambda request: None)
-        if (response := maybe_background(request)) is not None:
+        # If background processing was requested for a bulk (list) create, enqueue a job and
+        # return immediately. _handle_background_request() comes from BackgroundOperationMixin;
+        # fall back to "no background" so this mixin remains usable on its own (e.g. in custom
+        # viewsets).
+        handle_background = getattr(self, '_handle_background_request', lambda *a, **kw: None)
+        if (response := handle_background(request, 'create')) is not None:
             return response
 
         with transaction.atomic(using=router.db_for_write(self.queryset.model)):
@@ -199,17 +204,19 @@ class BulkUpdateModelMixin:
 
     def bulk_update(self, request, *args, **kwargs):
         partial = kwargs.pop('partial', False)
+
+        # If background processing was requested, enqueue a job and return immediately (before
+        # any validation, which is deferred to the worker). _handle_background_request() comes
+        # from BackgroundOperationMixin; fall back to "no background" so this mixin remains
+        # usable on its own (e.g. in custom viewsets).
+        handle_background = getattr(self, '_handle_background_request', lambda *a, **kw: None)
+        action = 'bulk_partial_update' if partial else 'bulk_update'
+        if (response := handle_background(request, action)) is not None:
+            return response
+
         serializer = BulkOperationSerializer(data=request.data, many=True)
         serializer.is_valid(raise_exception=True)
 
-        # If background processing was requested, enqueue a job and return immediately.
-        # The payload is captured here, before the request.data mutation below.
-        # _background_requested() comes from BackgroundOperationMixin; fall back to "no
-        # background" so this mixin remains usable on its own (e.g. in custom viewsets).
-        if getattr(self, '_background_requested', lambda request: False)(request):
-            action = 'bulk_partial_update' if partial else 'bulk_update'
-            return self._enqueue_bulk_job(request, action, payload=list(request.data))
-
         qs = self.get_bulk_update_queryset().filter(
             pk__in=[o['id'] for o in serializer.data]
         )
@@ -275,15 +282,17 @@ class BulkDestroyModelMixin:
         return self.get_queryset()
 
     def bulk_destroy(self, request, *args, **kwargs):
+        # If background processing was requested, enqueue a job and return immediately (before
+        # any validation, which is deferred to the worker). _handle_background_request() comes
+        # from BackgroundOperationMixin; fall back to "no background" so this mixin remains
+        # usable on its own (e.g. in custom viewsets).
+        handle_background = getattr(self, '_handle_background_request', lambda *a, **kw: None)
+        if (response := handle_background(request, 'bulk_destroy')) is not None:
+            return response
+
         serializer = BulkOperationSerializer(data=request.data, many=True)
         serializer.is_valid(raise_exception=True)
 
-        # If background processing was requested, enqueue a job and return immediately.
-        # _background_requested() comes from BackgroundOperationMixin; fall back to "no
-        # background" so this mixin remains usable on its own (e.g. in custom viewsets).
-        if getattr(self, '_background_requested', lambda request: False)(request):
-            return self._enqueue_bulk_job(request, 'bulk_destroy', payload=list(request.data))
-
         qs = self.get_bulk_destroy_queryset().filter(
             pk__in=[o['id'] for o in serializer.validated_data]
         )

netbox/netbox/jobs.py

@@ -6,7 +6,6 @@ from abc import ABC, abstractmethod
 from datetime import timedelta
 from io import BytesIO
 from pathlib import Path
-from urllib.parse import urlsplit
 
 from django.contrib.auth import get_user_model
 from django.core.exceptions import ImproperlyConfigured, PermissionDenied
@@ -269,41 +268,52 @@ class AsyncAPIJob(JobRunner):
         name = 'Async API Request'
 
     @staticmethod
-    def _build_request(payload, method, request_id, scheme, host):
+    def _build_request(request_copy, payload, scheme):
         """
-        Reconstruct a minimal WSGIRequest carrying the original JSON payload. DRF's Request
-        wrapper requires a real HttpRequest, and the original scheme/host are applied so that
+        Reconstruct a real WSGIRequest from a copy_safe_request() snapshot, injecting the JSON
+        payload as the request body. DRF's Request wrapper requires a real HttpRequest to parse
+        the body, and the snapshot's host metadata (already correctly separated into
+        SERVER_NAME/SERVER_PORT/HTTP_HOST by the original WSGI layer) is carried verbatim so that
         absolute URLs in the captured result (serializer hyperlink fields) point at the real
-        server. The host is passed via HTTP_HOST verbatim (request.get_host() already formats
-        it correctly, including bracketed IPv6, and Django's get_host() prefers it);
-        SERVER_NAME/SERVER_PORT are populated only to satisfy WSGI, parsed via urlsplit so
-        host:port and [::1]:port split correctly.
+        server. The scheme is applied separately, as copy_safe_request() does not capture it.
         """
-        parsed_host = urlsplit(f'//{host}')
         body = json.dumps(payload).encode('utf-8')
-        request = WSGIRequest({
-            'REQUEST_METHOD': method.upper(),
-            'PATH_INFO': '/',
+        environ = {
+            'REQUEST_METHOD': request_copy.method,
+            'PATH_INFO': getattr(request_copy, 'path', '/') or '/',
             'CONTENT_TYPE': 'application/json',
             'CONTENT_LENGTH': str(len(body)),
             'wsgi.input': BytesIO(body),
             'wsgi.url_scheme': scheme,
-            'HTTP_HOST': host,
-            'SERVER_NAME': parsed_host.hostname or 'localhost',
-            'SERVER_PORT': str(parsed_host.port) if parsed_host.port else ('443' if scheme == 'https' else '80'),
             'SERVER_PROTOCOL': 'HTTP/1.1',
-        })
-        request.id = request_id
+            # Sensible defaults (a real WSGI layer always sets these); overridden below by the
+            # snapshot's host metadata when present.
+            'SERVER_NAME': 'localhost',
+            'SERVER_PORT': '443' if scheme == 'https' else '80',
+        }
+        # Carry the host/forwarding metadata from the safe request copy (no host:port parsing
+        # needed: these were already split correctly when the original request was received).
+        for key in (
+            'HTTP_HOST', 'SERVER_NAME', 'SERVER_PORT',
+            'HTTP_X_FORWARDED_HOST', 'HTTP_X_FORWARDED_PORT', 'HTTP_X_FORWARDED_PROTO',
+        ):
+            if value := request_copy.META.get(key):
+                environ[key] = value
+
+        request = WSGIRequest(environ)
+        request.id = getattr(request_copy, 'id', None)
         return request
 
     def run(
-        self, viewset_class, action, payload, user_pk, request_id, method,
-        action_kwargs=None, scheme='http', host='localhost', **kwargs
+        self, viewset_class, action, payload, user_pk, request,
+        action_kwargs=None, scheme='http', **kwargs
     ):
         # Imported here to avoid a circular import (netbox.api.viewsets imports from this module).
         from netbox.api.viewsets import HTTP_ACTIONS
 
         action_kwargs = action_kwargs or {}
+        method = request.method
+        request_id = getattr(request, 'id', '') or ''
         viewset_class = import_string(viewset_class)
 
         # Re-fetch the requesting user. If the user no longer exists or is inactive, fail
@@ -320,7 +330,7 @@ class AsyncAPIJob(JobRunner):
             self.job.save()
             raise JobFailed()
 
-        django_request = self._build_request(payload, method, request_id, scheme, host)
+        django_request = self._build_request(request, payload, scheme)
 
         # Instantiate the viewset and apply the minimal scaffolding that DRF's as_view()
         # normally sets, so initialize_request() can wire up parsers, etc.

netbox/netbox/tests/test_api_background.py

@@ -12,6 +12,7 @@ import uuid
 from unittest.mock import patch
 
 from django.contrib.contenttypes.models import ContentType
+from django.test import RequestFactory
 from rest_framework import status
 
 from core.choices import JobStatusChoices
@@ -19,6 +20,7 @@ from core.exceptions import JobFailed
 from core.models import Job, ObjectChange
 from dcim.models import DeviceType, Manufacturer, Region
 from users.models import ObjectPermission
+from utilities.request import copy_safe_request
 from utilities.testing.api import APITestCase
 from utilities.testing.mixins import RQQueueTestMixin
 
@@ -99,6 +101,25 @@ class BackgroundBulkWriteTests(RQQueueTestMixin, APITestCase):
         for obj in job.data['data']:
             self.assertTrue(obj['url'].startswith('http://testserver/'), obj['url'])
 
+    def test_background_bulk_create_invalid_deferred(self):
+        # Validation is deferred to the worker: an invalid bulk create is accepted with 202 and
+        # fails at execution time (capturing the 400 in job.data), rather than being rejected
+        # synchronously. All-or-nothing semantics still leave nothing persisted.
+        self.grant('add', 'view')
+        payload = [
+            {'name': 'Region A', 'slug': 'region-a'},
+            {'name': 'Region B', 'slug': ''},  # invalid: blank slug
+        ]
+        response = self.client.post(
+            '/api/dcim/regions/?background=true', payload, format='json', **self.header
+        )
+        self.assertEqual(response.status_code, status.HTTP_202_ACCEPTED)
+        job = Job.objects.get(pk=response.data['job']['id'])
+        self.assertEqual(job.status, JobStatusChoices.STATUS_FAILED)
+        self.assertEqual(job.data['status_code'], status.HTTP_400_BAD_REQUEST)
+        self.assertTrue(job.error)
+        self.assertFalse(Region.objects.filter(slug='region-a').exists())
+
     # ------------------------------------------------------------------ update
 
     def test_background_bulk_update_patch(self):
@@ -206,16 +227,21 @@ class BackgroundBulkWriteTests(RQQueueTestMixin, APITestCase):
 
     # ------------------------------------------------------------------ contract guards
 
-    def test_synchronous_rejection_no_job(self):
-        # Malformed payload (missing required id) must 400 synchronously, with no Job created.
+    def test_invalid_payload_deferred_to_job(self):
+        # Validation is deferred to the worker: a malformed payload (missing required id) is
+        # accepted with 202, and the job fails at execution time capturing the 400 in job.data
+        # (rather than being rejected synchronously).
         self.grant('change', 'view')
         response = self.client.patch(
             '/api/dcim/regions/?background=true',
             [{'description': 'no id'}],
             format='json', **self.header
         )
-        self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
-        self.assertEqual(Job.objects.count(), 0)
+        self.assertEqual(response.status_code, status.HTTP_202_ACCEPTED)
+        job = Job.objects.get(pk=response.data['job']['id'])
+        self.assertEqual(job.status, JobStatusChoices.STATUS_FAILED)
+        self.assertEqual(job.data['status_code'], status.HTTP_400_BAD_REQUEST)
+        self.assertTrue(job.error)
 
     def test_if_match_with_background_rejected(self):
         self.grant('change', 'view')
@@ -255,6 +281,31 @@ class BackgroundBulkWriteTests(RQQueueTestMixin, APITestCase):
         self.assertEqual(response.status_code, status.HTTP_201_CREATED)
         self.assertEqual(Job.objects.count(), 0)
 
+    def test_background_request_snapshot_excludes_cookies(self):
+        # The request snapshot pickled into the job payload must not carry session cookies: the
+        # worker bypasses authentication and never reads them, so retaining them would be a
+        # needless exposure of session data in Redis.
+        self.grant('change', 'view')
+        self.client.cookies['sessionid'] = 'super-secret-session'
+
+        captured = {}
+        import netbox.jobs as jobs_module
+        orig_enqueue = jobs_module.AsyncAPIJob.enqueue.__func__
+
+        def _capture(cls, *args, **kwargs):
+            captured.update(kwargs)
+            return orig_enqueue(cls, *args, **kwargs)
+
+        with patch.object(jobs_module.AsyncAPIJob, 'enqueue', classmethod(_capture)):
+            self.client.patch(
+                '/api/dcim/regions/?background=true',
+                [{'id': self.regions[0].pk, 'description': 'x'}],
+                format='json', **self.header,
+            )
+
+        self.assertIn('request', captured)
+        self.assertEqual(captured['request'].COOKIES, {})
+
     # ------------------------------------------------------------------ enqueue scheduling
 
     def test_non_immediate_enqueue_creates_pending_job(self):
@@ -289,6 +340,11 @@ class BackgroundBulkWriteTests(RQQueueTestMixin, APITestCase):
         self.user.is_active = False
         self.user.save()
 
+        factory = RequestFactory()
+        raw_request = factory.patch('/api/dcim/regions/', data=[], content_type='application/json')
+        raw_request.user = self.user
+        request_copy = copy_safe_request(raw_request)
+
         job = Job.objects.create(name='Bulk update regions', user=self.user, job_id=uuid.uuid4())
         with self.assertRaises(JobFailed):
             AsyncAPIJob(job).run(
@@ -296,8 +352,8 @@ class BackgroundBulkWriteTests(RQQueueTestMixin, APITestCase):
                 action='bulk_update',
                 payload=[{'id': self.regions[0].pk, 'description': 'x'}],
                 user_pk=self.user.pk,
-                request_id='',
-                method='PATCH',
+                request=request_copy,
+                scheme='http',
                 action_kwargs={'partial': True},
             )
         job.refresh_from_db()
@@ -308,16 +364,21 @@ class BackgroundBulkWriteTests(RQQueueTestMixin, APITestCase):
     # ------------------------------------------------------------------ host parsing
 
     def test_ipv6_host_builds_correct_request(self):
-        # A bracketed IPv6 host:port must not be split on its inner colons. Assert directly on
-        # the request the worker reconstructs: get_host() must round-trip the bracketed host,
-        # and SERVER_NAME/SERVER_PORT must be the IPv6 literal and port (a host.partition(':')
-        # implementation would yield SERVER_NAME='[' here).
+        # A bracketed IPv6 host:port must round-trip through the request snapshot without being
+        # split on its inner colons. copy_safe_request() carries SERVER_NAME/SERVER_PORT/HTTP_HOST
+        # verbatim (already separated when the original request was received), so _build_request()
+        # needs no host parsing of its own.
         from netbox.jobs import AsyncAPIJob
 
-        request = AsyncAPIJob._build_request(
-            payload=[], method='PATCH', request_id=str(uuid.uuid4()),
-            scheme='https', host='[::1]:8443',
+        factory = RequestFactory()
+        raw_request = factory.patch(
+            '/api/dcim/regions/', data=[], content_type='application/json',
+            SERVER_NAME='::1', SERVER_PORT='8443', HTTP_HOST='[::1]:8443',
         )
+        raw_request.user = self.user
+        request_copy = copy_safe_request(raw_request)
+
+        request = AsyncAPIJob._build_request(request_copy, payload=[], scheme='https')
         self.assertEqual(request.get_host(), '[::1]:8443')
         self.assertEqual(request.META['SERVER_NAME'], '::1')
         self.assertEqual(request.META['SERVER_PORT'], '8443')