Sfoglia il codice sorgente

Fixes #23247: Carry request headers and query string into background bulk jobs (#23297)

Jeremy Stretch 2 giorni fa
parent
commit
7ed9e6e64b

+ 5 - 5
netbox/netbox/api/viewsets/mixins.py

@@ -349,11 +349,11 @@ class BackgroundOperationMixin:
             object_type=model._meta.verbose_name_plural,
             object_type=model._meta.verbose_name_plural,
         )
         )
         # Carry a serializable snapshot of the request so the worker can reconstruct it (method,
         # 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 ID, non-sensitive headers, query string, 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 = copy_safe_request(request, include_files=False)
         request_copy.user = None
         request_copy.user = None
         request_copy.COOKIES = {}
         request_copy.COOKIES = {}

+ 16 - 8
netbox/netbox/jobs.py

@@ -11,7 +11,7 @@ from django.contrib.auth import get_user_model
 from django.core.exceptions import ImproperlyConfigured, PermissionDenied, ValidationError
 from django.core.exceptions import ImproperlyConfigured, PermissionDenied, ValidationError
 from django.core.handlers.wsgi import WSGIRequest
 from django.core.handlers.wsgi import WSGIRequest
 from django.db.models import ProtectedError, RestrictedError
 from django.db.models import ProtectedError, RestrictedError
-from django.http import Http404
+from django.http import Http404, QueryDict
 from django.utils import timezone
 from django.utils import timezone
 from django.utils.functional import classproperty
 from django.utils.functional import classproperty
 from django.utils.module_loading import import_string
 from django.utils.module_loading import import_string
@@ -296,7 +296,8 @@ class AsyncAPIJob(JobRunner):
         the body, and the snapshot's host metadata (already correctly separated into
         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
         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
         absolute URLs in the captured result (serializer hyperlink fields) point at the real
-        server. The scheme is applied separately, as copy_safe_request() does not capture it.
+        server. The original headers and query string are carried as well. The scheme is applied
+        separately, as copy_safe_request() does not capture it.
         """
         """
         body = json.dumps(payload).encode('utf-8')
         body = json.dumps(payload).encode('utf-8')
         environ = {
         environ = {
@@ -312,14 +313,21 @@ class AsyncAPIJob(JobRunner):
             'SERVER_NAME': 'localhost',
             'SERVER_NAME': 'localhost',
             'SERVER_PORT': '443' if scheme == 'https' else '80',
             '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',
-        ):
+        # Carry the host 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 ('SERVER_NAME', 'SERVER_PORT'):
             if value := request_copy.META.get(key):
             if value := request_copy.META.get(key):
                 environ[key] = value
                 environ[key] = value
+        # Carry the original headers (copy_safe_request() has already dropped sensitive ones) so that
+        # request processors (e.g. a plugin selecting the active branch) see the request as received.
+        for key, value in request_copy.META.items():
+            if key.startswith('HTTP_'):
+                environ[key] = value
+        # Carry the query string, less the background flag so the worker executes the action
+        # rather than enqueuing another job.
+        query = QueryDict(request_copy.META.get('QUERY_STRING', ''), mutable=True)
+        query.pop('background', None)
+        environ['QUERY_STRING'] = query.urlencode()
 
 
         request = WSGIRequest(environ)
         request = WSGIRequest(environ)
         request.id = getattr(request_copy, 'id', None)
         request.id = getattr(request_copy, 'id', None)

+ 62 - 0
netbox/netbox/tests/test_api_background.py

@@ -9,6 +9,7 @@ inline (immediate=True) and (b) reports a worker as available for the worker-liv
 Individual tests that exercise those guards override this locally.
 Individual tests that exercise those guards override this locally.
 """
 """
 import uuid
 import uuid
+from contextlib import contextmanager
 from unittest.mock import patch
 from unittest.mock import patch
 
 
 from django.contrib.contenttypes.models import ContentType
 from django.contrib.contenttypes.models import ContentType
@@ -22,6 +23,7 @@ from core.models import Job, ObjectChange
 from dcim.api.views import RegionViewSet
 from dcim.api.views import RegionViewSet
 from dcim.models import DeviceType, Manufacturer, Region
 from dcim.models import DeviceType, Manufacturer, Region
 from netbox.jobs import AsyncAPIJob
 from netbox.jobs import AsyncAPIJob
+from netbox.registry import registry
 from users.models import ObjectPermission
 from users.models import ObjectPermission
 from utilities.request import copy_safe_request
 from utilities.request import copy_safe_request
 from utilities.testing.api import APITestCase
 from utilities.testing.api import APITestCase
@@ -480,6 +482,66 @@ class BackgroundBulkWriteTests(RQQueueTestMixin, APITestCase):
         self.assertEqual(request.META['SERVER_PORT'], '8443')
         self.assertEqual(request.META['SERVER_PORT'], '8443')
         self.assertEqual(request.scheme, 'https')
         self.assertEqual(request.scheme, 'https')
 
 
+    # ------------------------------------------------------------------ request fidelity
+
+    def test_build_request_carries_headers_and_query_string(self):
+        # Request processors (e.g. a plugin selecting the active branch) and the action itself must
+        # see the same headers and query parameters as the original request. The background flag is
+        # stripped so the action executes synchronously in the worker rather than enqueuing anew.
+        factory = RequestFactory()
+        raw_request = factory.delete(
+            '/api/dcim/regions/?background=true&_branch=abc123&omit=created',
+            data=[], content_type='application/json',
+            HTTP_X_NETBOX_BRANCH='abc123', HTTP_AUTHORIZATION='Bearer secret',
+        )
+        raw_request.user = self.user
+        request_copy = copy_safe_request(raw_request)
+
+        request = AsyncAPIJob._build_request(request_copy, payload=[], scheme='http')
+        self.assertEqual(request.headers['X-NetBox-Branch'], 'abc123')
+        self.assertNotIn('Authorization', request.headers)
+        self.assertEqual(request.GET.get('_branch'), 'abc123')
+        self.assertEqual(request.GET.get('omit'), 'created')
+        self.assertNotIn('background', request.GET)
+
+    def test_background_request_processors_see_original_request(self):
+        self.grant('delete', 'view')
+        seen = []
+
+        @contextmanager
+        def recorder(request):
+            seen.append((request.headers.get('X-Custom'), request.GET.get('custom')))
+            yield
+
+        with patch.dict(registry, {'request_processors': [*registry['request_processors'], recorder]}):
+            response = self.client.delete(
+                '/api/dcim/regions/?background=true&custom=bar',
+                [{'id': self.regions[0].pk}],
+                format='json', HTTP_X_CUSTOM='foo', **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_COMPLETED)
+        self.assertFalse(Region.objects.filter(pk=self.regions[0].pk).exists())
+        # The job runs inline here, so the processors are applied to both the originating request
+        # and the worker's reconstructed request.
+        self.assertEqual(seen, [('foo', 'bar'), ('foo', 'bar')])
+
+    def test_background_result_honors_query_parameters(self):
+        self.grant('change', 'view')
+        response = self.client.patch(
+            '/api/dcim/regions/?background=true&omit=created',
+            [{'id': self.regions[0].pk, 'description': 'omit'}],
+            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_COMPLETED)
+        self.assertIn('description', job.data['data'][0])
+        self.assertNotIn('created', job.data['data'][0])
+
     # ------------------------------------------------------------------ change logging
     # ------------------------------------------------------------------ change logging
 
 
     def test_background_update_changelog_fidelity(self):
     def test_background_update_changelog_fidelity(self):