Middleware errors / responses during preview should be returned to the user

docs/releases/2.7.rst

@@ -28,3 +28,8 @@ Bug fixes
 
 Upgrade considerations
 ======================
+
+``Page.dummy_request`` is deprecated
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The internal ``Page.dummy_request`` method (which generates an HTTP request object simulating a real page request, for use in previews) has been deprecated, as it did not correctly handle errors generated during middleware processing. Any code that calls this method to render page previews should be updated to use the new method ``Page.make_preview_request(original_request=None, preview_mode=None)``, which builds the request and calls ``Page.serve_preview`` as a single operation.

wagtail/admin/tests/test_pages_views.py

@@ -5263,6 +5263,18 @@ def test_draft_access_authorized(self):
         # User can view
         self.assertEqual(response.status_code, 200)
 
+    def test_middleware_response_is_returned(self):
+        """
+        If middleware returns a response while serving a page preview, that response should be
+        returned back to the user
+        """
+        self.login()
+        response = self.client.get(
+            reverse('wagtailadmin_pages:view_draft', args=(self.child_page.id, )),
+            HTTP_USER_AGENT='EvilHacker'
+        )
+        self.assertEqual(response.status_code, 403)
+
 
 class TestPreview(TestCase, WagtailTestUtils):
     fixtures = ['test.json']

wagtail/admin/views/pages.py

@@ -586,7 +586,7 @@ def view_draft(request, page_id):
     perms = page.permissions_for_user(request.user)
     if not (perms.can_publish() or perms.can_edit()):
         raise PermissionDenied
-    return page.serve_preview(page.dummy_request(request), page.default_preview_mode)
+    return page.make_preview_request(request, page.default_preview_mode)
 
 
 class PreviewOnEdit(View):
@@ -646,8 +646,7 @@ def get(self, request, *args, **kwargs):
 
         form.save(commit=False)
         preview_mode = request.GET.get('mode', page.default_preview_mode)
-        return page.serve_preview(page.dummy_request(request),
-                                  preview_mode)
+        return page.make_preview_request(request, preview_mode)
 
 
 class PreviewOnCreate(PreviewOnEdit):
@@ -1055,11 +1054,9 @@ def preview_for_moderation(request, revision_id):
 
     page = revision.as_page_object()
 
-    request.revision_id = revision_id
-
-    # pass in the real user request rather than page.dummy_request(), so that request.user
-    # and request.revision_id will be picked up by the wagtail user bar
-    return page.serve_preview(request, page.default_preview_mode)
+    return page.make_preview_request(request, page.default_preview_mode, extra_request_attrs={
+        'revision_id': revision_id
+    })
 
 
 @require_POST
@@ -1180,7 +1177,7 @@ def revisions_view(request, page_id, revision_id):
     revision = get_object_or_404(page.revisions, id=revision_id)
     revision_page = revision.as_page_object()
 
-    return revision_page.serve_preview(page.dummy_request(request), page.default_preview_mode)
+    return revision_page.make_preview_request(request, page.default_preview_mode)
 
 
 def revisions_compare(request, page_id, revision_id_a, revision_id_b):

wagtail/core/models.py

@@ -3,6 +3,7 @@
 from collections import defaultdict
 from io import StringIO
 from urllib.parse import urlparse
+from warnings import warn
 
 from django.conf import settings
 from django.contrib.auth.models import Group, Permission
@@ -32,6 +33,8 @@
 from wagtail.core.url_routing import RouteResult
 from wagtail.core.utils import WAGTAIL_APPEND_SLASH, camelcase_to_underscore, resolve_model_string
 from wagtail.search import index
+from wagtail.utils.deprecation import RemovedInWagtail28Warning
+
 
 logger = logging.getLogger('wagtail.core')
 
@@ -1215,16 +1218,51 @@ def permissions_for_user(self, user):
         user_perms = UserPagePermissionsProxy(user)
         return user_perms.for_page(self)
 
-    def dummy_request(self, original_request=None, **meta):
+    def make_preview_request(self, original_request=None, preview_mode=None, extra_request_attrs=None):
         """
-        Construct a HttpRequest object that is, as far as possible, representative of ones that would
-        receive this page as a response. Used for previewing / moderation and any other place where we
+        Simulate a request to this page, by constructing a fake HttpRequest object that is (as far
+        as possible) representative of a real request to this page's front-end URL, and invoking
+        serve_preview with that request (and the given preview_mode).
+
+        Used for previewing / moderation and any other place where we
         want to display a view of this page in the admin interface without going through the regular
         page routing logic.
 
         If you pass in a real request object as original_request, additional information (e.g. client IP, cookies)
         will be included in the dummy request.
         """
+        dummy_meta = self._get_dummy_headers(original_request)
+        request = WSGIRequest(dummy_meta)
+
+        # Add a flag to let middleware know that this is a dummy request.
+        request.is_dummy = True
+
+        if extra_request_attrs:
+            for k, v in extra_request_attrs.items():
+                setattr(request, k, v)
+
+        page = self
+
+        # Build a custom django.core.handlers.BaseHandler subclass that invokes serve_preview as
+        # the eventual view function called at the end of the middleware chain, rather than going
+        # through the URL resolver
+        class Handler(BaseHandler):
+            def _get_response(self, request):
+                response = page.serve_preview(request, preview_mode)
+                if hasattr(response, 'render') and callable(response.render):
+                    response = response.render()
+                return response
+
+        # Invoke this custom handler.
+        handler = Handler()
+        handler.load_middleware()
+        return handler.get_response(request)
+
+    def _get_dummy_headers(self, original_request=None):
+        """
+        Return a dict of META information to be included in a faked HttpRequest object to pass to
+        serve_preview.
+        """
         url = self.full_url
         if url:
             url_info = urlparse(url)
@@ -1278,6 +1316,16 @@ def dummy_request(self, original_request=None, **meta):
                 if header in original_request.META:
                     dummy_values[header] = original_request.META[header]
 
+        return dummy_values
+
+    def dummy_request(self, original_request=None, **meta):
+        warn(
+            "Page.dummy_request is deprecated. Use Page.make_preview_request instead",
+            category=RemovedInWagtail28Warning
+        )
+
+        dummy_values = self._get_dummy_headers(original_request)
+
         # Add additional custom metadata sent by the caller.
         dummy_values.update(**meta)
 

wagtail/core/tests/test_page_model.py

@@ -1434,13 +1434,14 @@ def test_delete_content_type(self):
         self.assertEqual(event_index.content_type, ContentType.objects.get_for_model(Page))
 
 
-@override_settings(ALLOWED_HOSTS=['localhost'])
-class TestDummyRequest(TestCase):
+class TestMakePreviewRequest(TestCase):
     fixtures = ['test.json']
 
-    def test_dummy_request_for_accessible_page(self):
+    def test_make_preview_request_for_accessible_page(self):
         event_index = Page.objects.get(url_path='/home/events/')
-        request = event_index.dummy_request()
+        response = event_index.make_preview_request()
+        self.assertEqual(response.status_code, 200)
+        request = response.context_data['request']
 
         # request should have the correct path and hostname for this page
         self.assertEqual(request.path, '/events/')
@@ -1461,11 +1462,13 @@ def test_dummy_request_for_accessible_page(self):
         self.assertIn('wsgi.multiprocess', request.META)
         self.assertIn('wsgi.run_once', request.META)
 
-    def test_dummy_request_for_accessible_page_https(self):
+    def test_make_preview_request_for_accessible_page_https(self):
         Site.objects.update(port=443)
 
         event_index = Page.objects.get(url_path='/home/events/')
-        request = event_index.dummy_request()
+        response = event_index.make_preview_request()
+        self.assertEqual(response.status_code, 200)
+        request = response.context_data['request']
 
         # request should have the correct path and hostname for this page
         self.assertEqual(request.path, '/events/')
@@ -1486,11 +1489,13 @@ def test_dummy_request_for_accessible_page_https(self):
         self.assertIn('wsgi.multiprocess', request.META)
         self.assertIn('wsgi.run_once', request.META)
 
-    def test_dummy_request_for_accessible_page_non_standard_port(self):
+    def test_make_preview_request_for_accessible_page_non_standard_port(self):
         Site.objects.update(port=8888)
 
         event_index = Page.objects.get(url_path='/home/events/')
-        request = event_index.dummy_request()
+        response = event_index.make_preview_request()
+        self.assertEqual(response.status_code, 200)
+        request = response.context_data['request']
 
         # request should have the correct path and hostname for this page
         self.assertEqual(request.path, '/events/')
@@ -1511,7 +1516,7 @@ def test_dummy_request_for_accessible_page_non_standard_port(self):
         self.assertIn('wsgi.multiprocess', request.META)
         self.assertIn('wsgi.run_once', request.META)
 
-    def test_dummy_request_for_accessible_page_with_original_request(self):
+    def test_make_preview_request_for_accessible_page_with_original_request(self):
         event_index = Page.objects.get(url_path='/home/events/')
         original_headers = {
             'REMOTE_ADDR': '192.168.0.1',
@@ -1522,7 +1527,9 @@ def test_dummy_request_for_accessible_page_with_original_request(self):
         }
         factory = RequestFactory(**original_headers)
         original_request = factory.get('/home/events/')
-        request = event_index.dummy_request(original_request)
+        response = event_index.make_preview_request(original_request)
+        self.assertEqual(response.status_code, 200)
+        request = response.context_data['request']
 
         # request should have the all the special headers we set in original_request
         self.assertEqual(request.META['REMOTE_ADDR'], original_request.META['REMOTE_ADDR'])
@@ -1547,9 +1554,11 @@ def test_dummy_request_for_accessible_page_with_original_request(self):
         self.assertIn('wsgi.run_once', request.META)
 
     @override_settings(ALLOWED_HOSTS=['production.example.com'])
-    def test_dummy_request_for_inaccessible_page_should_use_valid_host(self):
+    def test_make_preview_request_for_inaccessible_page_should_use_valid_host(self):
         root_page = Page.objects.get(url_path='/')
-        request = root_page.dummy_request()
+        response = root_page.make_preview_request()
+        self.assertEqual(response.status_code, 200)
+        request = response.context_data['request']
 
         # in the absence of an actual Site record where we can access this page,
         # dummy_request should still provide a hostname that Django's host header
@@ -1559,7 +1568,9 @@ def test_dummy_request_for_inaccessible_page_should_use_valid_host(self):
     @override_settings(ALLOWED_HOSTS=['*'])
     def test_dummy_request_for_inaccessible_page_with_wildcard_allowed_hosts(self):
         root_page = Page.objects.get(url_path='/')
-        request = root_page.dummy_request()
+        response = root_page.make_preview_request()
+        self.assertEqual(response.status_code, 200)
+        request = response.context_data['request']
 
         # '*' is not a valid hostname, so ensure that we replace it with something sensible
         self.assertNotEqual(request.META['HTTP_HOST'], '*')

wagtail/tests/middleware.py

@@ -0,0 +1,8 @@
+from django.http import HttpResponseForbidden
+from django.utils.deprecation import MiddlewareMixin
+
+
+class BlockDodgyUserAgentMiddleware(MiddlewareMixin):
+    def process_request(self, request):
+        if not request.path.startswith('/admin/') and request.META.get('HTTP_USER_AGENT') == 'EvilHacker':
+            return HttpResponseForbidden("Forbidden")

wagtail/tests/settings.py

@@ -94,6 +94,7 @@
     'django.contrib.messages.middleware.MessageMiddleware',
     'django.middleware.clickjacking.XFrameOptionsMiddleware',
 
+    'wagtail.tests.middleware.BlockDodgyUserAgentMiddleware',
     'wagtail.core.middleware.SiteMiddleware',
     'wagtail.contrib.redirects.middleware.RedirectMiddleware',
 )
@@ -157,6 +158,7 @@
     'django.contrib.auth.hashers.MD5PasswordHasher',  # don't use the intentionally slow default password hasher
 )
 
+ALLOWED_HOSTS = ['localhost', 'testserver']
 
 WAGTAILSEARCH_BACKENDS = {
     'default': {