Fix Missing Cache-Control Headers for Password-Protected Pages (#12465)

Fixes #12455

CHANGELOG.txt

@@ -13,6 +13,7 @@ Changelog
  * Add the ability to restrict what types of requests a Pages supports via `allowed_http_methods` (Andy Babic)
  * Allow plain strings in panel definitions as shorthand for `FieldPanel` / `InlinePanel` (Matt Westcott)
  * Only allow selection of valid new parents within the copy Page view (Mauro Soche)
+ * Add `on_serve_page` hook to modify the serving chain of pages (Krystian Magdziarz, Dawid Bugajewski)
  * Fix: Improve handling of translations for bulk page action confirmation messages (Matt Westcott)
  * Fix: Ensure custom rich text feature icons are correctly handled when provided as a list of SVG paths (Temidayo Azeez, Joel William, LB (Ben) Johnston)
  * Fix: Ensure manual edits to `StreamField` values do not throw an error (Stefan Hammer)
@@ -32,6 +33,7 @@ Changelog
  * Fix: Ensure `MultipleChooserPanel` using images or documents work when nested within an `InlinePanel` when no other choosers are in use within the model (Elhussein Almasri)
  * Fix: Ensure `MultipleChooserPanel` works after doing a search in the page chooser modal (Matt Westcott)
  * Fix: Ensure new `ListBlock` instances get created with unique IDs in the admin client for accessibility and mini-map element references (Srishti Jaiswal)
+ * Fix: Return never-cache HTTP headers when serving pages and documents with view restrictions (Krystian Magdziarz, Dawid Bugajewski)
  * Docs: Move the model reference page from reference/pages to the references section as it covers all Wagtail core models (Srishti Jaiswal)
  * Docs: Move the panels reference page from references/pages to the references section as panels are available for any model editing, merge panels API into this page (Srishti Jaiswal)
  * Docs: Move the tags documentation to standalone advanced topic, instead of being inside the reference/pages section (Srishti Jaiswal)

CONTRIBUTORS.md

@@ -860,6 +860,7 @@
 * Alex Fulcher
 * Harsh Dange
 * Mauro Soche
+* Krystian Magdziarz
 
 ## Translators
 

docs/reference/hooks.md

@@ -1131,6 +1131,40 @@ def block_googlebot(page, request, serve_args, serve_kwargs):
         return HttpResponse("<h1>bad googlebot no cookie</h1>")
 ```
 
+(on_serve_page)=
+
+### `on_serve_page`
+
+Called when Wagtail is serving a page, after `before_serve_page` but before the page's `serve()` method is called. Unlike `before_serve_page`, this hook allows you to modify the serving chain rather than just returning an alternative response.
+
+The callable passed to this hook must accept a function as its argument and return a new function that will be used in its place. The passed-in function will be the next callable in the serving chain.
+
+For example, to add custom cache headers to the response:
+
+```python
+from wagtail import hooks
+
+@hooks.register('on_serve_page')
+def add_custom_headers(next_serve_page):
+    def wrapper(page, request, args, kwargs):
+        response = next_serve_page(page, request, args, kwargs)
+        response['Custom-Header'] = 'value'
+        return response
+    return wrapper
+```
+
+Parameters passed to the function:
+- `page` - the Page object being served
+- `request` - the request object
+- `args` - positional arguments that will be passed to the page's serve method
+- `kwargs` - keyword arguments that will be passed to the page's serve method
+
+This hook is particularly useful for:
+- Adding/modifying response headers
+- Implementing access restrictions
+- Modifying the response content
+- Adding logging or monitoring
+
 ## Document serving
 
 (before_serve_document)=

docs/releases/6.4.md

@@ -22,6 +22,7 @@ depth: 1
  * Add the ability to restrict what types of requests a Pages supports via [`allowed_http_methods`](#wagtail.models.Page.allowed_http_methods) (Andy Babic)
  * Allow plain strings in panel definitions as shorthand for `FieldPanel` / `InlinePanel` (Matt Westcott)
  * Only allow selection of valid new parents within the copy Page view (Mauro Soche)
+ * Add [`on_serve_page`](on_serve_page) hook to modify the serving chain of pages (Krystian Magdziarz, Dawid Bugajewski)
 
 ### Bug fixes
 
@@ -44,6 +45,7 @@ depth: 1
  * Ensure `MultipleChooserPanel` using images or documents work when nested within an `InlinePanel` when no other choosers are in use within the model (Elhussein Almasri)
  * Ensure `MultipleChooserPanel` works after doing a search in the page chooser modal (Matt Westcott)
  * Ensure new `ListBlock` instances get created with unique IDs in the admin client for accessibility and mini-map element references (Srishti Jaiswal)
+ * Return never-cache HTTP headers when serving pages and documents with view restrictions (Krystian Magdziarz, Dawid Bugajewski)
 
 ### Documentation
 

wagtail/documents/wagtail_hooks.py

@@ -3,6 +3,7 @@ from warnings import warn
 from django.conf import settings
 from django.template.response import TemplateResponse
 from django.urls import include, path, reverse, reverse_lazy
+from django.utils.cache import add_never_cache_headers
 from django.utils.translation import gettext, ngettext
 from django.utils.translation import gettext_lazy as _
 
@@ -200,7 +201,11 @@ def check_view_restrictions(document, request):
                     )
 
                 context = {"form": form, "action_url": action_url}
-                return TemplateResponse(request, password_required_template, context)
+                response = TemplateResponse(
+                    request, password_required_template, context
+                )
+                add_never_cache_headers(response)
+                return response
 
             elif restriction.restriction_type in [
                 BaseViewRestriction.LOGIN,

wagtail/tests/test_hooks.py

@@ -1,7 +1,14 @@
-from django.test import TestCase
+from unittest import mock
+
+from django.contrib.sessions.middleware import SessionMiddleware
+from django.http import HttpResponse
+from django.test import RequestFactory, TestCase
 
 from wagtail import hooks
+from wagtail.models import Page, PageViewRestriction
 from wagtail.test.utils import WagtailTestUtils
+from wagtail.views import serve, serve_chain
+from wagtail.wagtail_hooks import check_view_restrictions
 
 
 def test_hook():
@@ -34,3 +41,161 @@ class TestLoginView(WagtailTestUtils, TestCase):
         with self.register_hook("test_hook_name", after_hook, order=1):
             hook_fns = hooks.get_hooks("test_hook_name")
             self.assertEqual(hook_fns, [test_hook, after_hook])
+
+
+class TestServeHooks(WagtailTestUtils, TestCase):
+    fixtures = ["test.json"]
+
+    def setUp(self):
+        self.page = Page.objects.get(id=2)
+        self.request = RequestFactory().get("/test/")
+        middleware = SessionMiddleware(lambda x: None)
+        middleware.process_request(self.request)
+        self.request.session.save()
+
+    def test_serve_chain_order(self):
+        order_calls = []
+
+        def hook_1(next_fn):
+            def wrapper(page, request, *args, **kwargs):
+                order_calls.append(1)
+                return next_fn(page, request, *args, **kwargs)
+
+            return wrapper
+
+        def hook_2(next_fn):
+            def wrapper(page, request, *args, **kwargs):
+                order_calls.append(2)
+                return next_fn(page, request, *args, **kwargs)
+
+            return wrapper
+
+        def hook_3(next_fn):
+            def wrapper(page, request, *args, **kwargs):
+                order_calls.append(3)
+                return next_fn(page, request, *args, **kwargs)
+
+            return wrapper
+
+        with self.register_hook("on_serve_page", hook_1):
+            with self.register_hook("on_serve_page", hook_2):
+                with self.register_hook("on_serve_page", hook_3):
+                    serve(self.request, self.page.url)
+
+                    self.assertEqual(order_calls, [1, 2, 3])
+
+    def test_serve_chain_modification(self):
+        def hook_modifier(next_fn):
+            def wrapper(page, request, *args, **kwargs):
+                response = next_fn(page, request, *args, **kwargs)
+                response.content = b"Modified content"
+                return response
+
+            return wrapper
+
+        with self.register_hook("on_serve_page", hook_modifier):
+            response = serve(self.request, self.page.url)
+            self.assertEqual(response.content, b"Modified content")
+
+    def test_serve_chain_halt_execution(self):
+        def hook_halt(next_fn):
+            def wrapper(page, request, *args, **kwargs):
+                return HttpResponse("Halted")
+
+            return wrapper
+
+        with self.register_hook("on_serve_page", hook_halt):
+            response = serve(self.request, self.page.url)
+            self.assertEqual(response.content, b"Halted")
+
+    def test_serve_chain_view_restriction(self):
+        restriction = PageViewRestriction.objects.create(
+            page=self.page,
+            restriction_type=PageViewRestriction.PASSWORD,
+            password="password",
+        )
+
+        with self.register_hook("on_serve_page", check_view_restrictions):
+            response = self.client.get(self.page.url)
+            self.assertEqual(response.status_code, 200)
+            self.assertTemplateUsed(response, "wagtailcore/password_required.html")
+
+        restriction.delete()
+
+    def test_serve_always_called_last(self):
+        hook_calls = []
+        serve_called = []
+
+        def hook_1(next_fn):
+            def wrapper(page, request, *args, **kwargs):
+                hook_calls.append(1)
+                return next_fn(page, request, *args, **kwargs)
+
+            return wrapper
+
+        def hook_2(next_fn):
+            def wrapper(page, request, *args, **kwargs):
+                hook_calls.append(2)
+                return next_fn(page, request, *args, **kwargs)
+
+            return wrapper
+
+        def hook_3(next_fn):
+            def wrapper(page, request, *args, **kwargs):
+                hook_calls.append(3)
+                return next_fn(page, request, *args, **kwargs)
+
+            return wrapper
+
+        original_serve_chain = serve_chain
+
+        def mock_serve_chain(page, request, *args, **kwargs):
+            serve_called.append(True)
+            return original_serve_chain(page, request, *args, **kwargs)
+
+        with mock.patch("wagtail.views.serve_chain", mock_serve_chain):
+            with self.register_hook("on_serve_page", hook_1):
+                with self.register_hook("on_serve_page", hook_2):
+                    with self.register_hook("on_serve_page", hook_3):
+                        serve(self.request, self.page.url)
+
+                        self.assertEqual(hook_calls, [1, 2, 3])
+                        self.assertEqual(len(serve_called), 1)
+                        self.assertTrue(serve_called[0])
+
+    def test_check_view_restrictions_receives_correct_parameters(self):
+        received_params = []
+
+        def hook_spy(next_fn):
+            def wrapper(page, request, *args, **kwargs):
+                received_params.append(
+                    {"page": page, "request": request, "args": args, "kwargs": kwargs}
+                )
+                return next_fn(page, request, *args, **kwargs)
+
+            return wrapper
+
+        self.assertIsNotNone(self.page, "Test page should not be None")
+        self.assertIsNotNone(self.request, "Test request should not be None")
+
+        with self.register_hook("on_serve_page", hook_spy):
+            route_result = Page.route_for_request(self.request, self.page.url)
+
+            self.assertIsNotNone(route_result, "route_result should not be None")
+
+            if route_result:
+                page, args, kwargs = route_result
+                serve(self.request, self.page.url)
+
+                self.assertEqual(len(received_params), 1)
+
+                params = received_params[0]
+
+                self.assertIsNotNone(params["page"], "Hook received None as page")
+                self.assertIsNotNone(params["request"], "Hook received None as request")
+
+                self.assertEqual(params["page"], self.page)
+                self.assertEqual(params["request"], self.request)
+
+                self.assertEqual(params["args"], ([], {}))
+                self.assertEqual(params["kwargs"], {})

wagtail/tests/test_page_model.py

@@ -10,6 +10,7 @@ from django.core.exceptions import ValidationError
 from django.http import Http404
 from django.test import Client, TestCase, override_settings
 from django.test.client import RequestFactory
+from django.urls import reverse
 from django.utils import timezone, translation
 from freezegun import freeze_time
 
@@ -4000,3 +4001,64 @@ class TestPageCachedParentObjExists(TestCase):
             "_cached_parent_obj_exists",
             "Page.get_parent() (treebeard) no longer uses _cached_parent_obj to cache the parent object",
         )
+
+
+class TestPageServeWithPasswordRestriction(TestCase, WagtailTestUtils):
+    def setUp(self):
+        self.root_page = Page.objects.get(id=2)
+        self.test_page = Page(
+            title="Test Page",
+            slug="test",
+        )
+        self.root_page.add_child(instance=self.test_page)
+
+        self.password_restriction = PageViewRestriction.objects.create(
+            page=self.test_page,
+            restriction_type=PageViewRestriction.PASSWORD,
+            password="password123",
+        )
+
+    def test_page_with_password_restriction_authenticated_has_cache_headers(self):
+        auth_url = reverse(
+            "wagtailcore_authenticate_with_password",
+            args=[self.password_restriction.id, self.test_page.id],
+        )
+
+        post_response = self.client.post(
+            auth_url,
+            {
+                "password": "password123",
+                "return_url": "/test/",
+            },
+        )
+
+        self.assertRedirects(post_response, "/test/")
+
+        response = self.client.get("/test/")
+
+        self.assertTrue("Cache-Control" in response)
+        self.assertIn("max-age=0", response["Cache-Control"])
+        self.assertIn("no-cache", response["Cache-Control"])
+        self.assertIn("no-store", response["Cache-Control"])
+        self.assertIn("must-revalidate", response["Cache-Control"])
+        self.assertIn("private", response["Cache-Control"])
+        self.assertTrue("Expires" in response)
+
+    def test_page_with_password_restriction_has_cache_headers(self):
+        response = self.client.get("/test/")
+
+        self.assertTrue("Cache-Control" in response)
+        self.assertIn("max-age=0", response["Cache-Control"])
+        self.assertIn("no-cache", response["Cache-Control"])
+        self.assertIn("no-store", response["Cache-Control"])
+        self.assertIn("must-revalidate", response["Cache-Control"])
+        self.assertIn("private", response["Cache-Control"])
+        self.assertTrue("Expires" in response)
+
+    def test_page_without_password_restriction_has_no_cache_headers(self):
+        self.password_restriction.delete()
+
+        response = self.client.get("/test/")
+
+        self.assertFalse("Cache-Control" in response)
+        self.assertFalse("Expires" in response)

wagtail/tests/test_page_privacy.py

@@ -231,3 +231,13 @@ class TestPagePrivacy(WagtailTestUtils, TestCase):
         response = self.client.get("/secret-login-plans/")
         self.assertEqual(response.status_code, 200)
         self.assertContains(response, "<title>Secret login plans</title>")
+
+    def test_password_protected_page_headers(self):
+        response = self.client.get("/secret-plans/")
+        self.assertEqual(
+            response.templates[0].name, "wagtailcore/password_required.html"
+        )
+        self.assertIn("no-cache", response["Cache-Control"])
+        self.assertIn("no-store", response["Cache-Control"])
+        self.assertIn("must-revalidate", response["Cache-Control"])
+        self.assertIn("max-age=0", response["Cache-Control"])

wagtail/views.py

@@ -9,6 +9,10 @@ from wagtail.forms import PasswordViewRestrictionForm
 from wagtail.models import Page, PageViewRestriction
 
 
+def serve_chain(page, request, args, kwargs):
+    return page.serve(request, *args, **kwargs)
+
+
 def serve(request, path):
     route_result = Page.route_for_request(request, path)
     if route_result is None:
@@ -16,12 +20,16 @@ def serve(request, path):
     else:
         page, args, kwargs = route_result
 
+    on_serve_chain = serve_chain
+    for fn in reversed(hooks.get_hooks("on_serve_page")):
+        on_serve_chain = fn(on_serve_chain)
+
     for fn in hooks.get_hooks("before_serve_page"):
         result = fn(page, request, args, kwargs)
         if isinstance(result, HttpResponse):
             return result
 
-    return page.serve(request, *args, **kwargs)
+    return on_serve_chain(page, request, args, kwargs)
 
 
 def authenticate_with_password(request, page_view_restriction_id, page_id):

wagtail/wagtail_hooks.py

@@ -5,6 +5,7 @@ from django.contrib.auth.models import Permission
 from django.contrib.auth.views import redirect_to_login
 from django.db import models
 from django.urls import reverse
+from django.utils.cache import add_never_cache_headers
 from django.utils.text import capfirst
 from django.utils.translation import gettext_lazy as _
 from django.utils.translation import ngettext
@@ -28,35 +29,51 @@ def require_wagtail_login(next):
     return redirect_to_login(next, login_url)
 
 
-@hooks.register("before_serve_page")
-def check_view_restrictions(page, request, serve_args, serve_kwargs):
-    """
-    Check whether there are any view restrictions on this page which are
-    not fulfilled by the given request object. If there are, return an
-    HttpResponse that will notify the user of that restriction (and possibly
-    include a password / login form that will allow them to proceed). If
-    there are no such restrictions, return None
-    """
-    for restriction in page.get_view_restrictions():
-        if not restriction.accept_request(request):
-            if restriction.restriction_type == PageViewRestriction.PASSWORD:
-                from wagtail.forms import PasswordViewRestrictionForm
+@hooks.register("on_serve_page")
+def check_view_restrictions(callback):
+    def inner(page, request, serve_args, serve_kwargs):
+        """
+        Check whether there are any view restrictions on this page which are
+        not fulfilled by the given request object. If there are, return an
+        HttpResponse that will notify the user of that restriction (and possibly
+        include a password / login form that will allow them to proceed). If
+        there are no such restrictions, return None
+        """
+        restrictions = page.get_view_restrictions()
+        response = None
+        for restriction in restrictions:
+            if not restriction.accept_request(request):
+                if restriction.restriction_type == PageViewRestriction.PASSWORD:
+                    from wagtail.forms import PasswordViewRestrictionForm
 
-                form = PasswordViewRestrictionForm(
-                    instance=restriction,
-                    initial={"return_url": request.get_full_path()},
-                )
-                action_url = reverse(
-                    "wagtailcore_authenticate_with_password",
-                    args=[restriction.id, page.id],
-                )
-                return page.serve_password_required_response(request, form, action_url)
+                    form = PasswordViewRestrictionForm(
+                        instance=restriction,
+                        initial={"return_url": request.get_full_path()},
+                    )
+                    action_url = reverse(
+                        "wagtailcore_authenticate_with_password",
+                        args=[restriction.id, page.id],
+                    )
 
-            elif restriction.restriction_type in [
-                PageViewRestriction.LOGIN,
-                PageViewRestriction.GROUPS,
-            ]:
-                return require_wagtail_login(next=request.get_full_path())
+                    response = page.serve_password_required_response(
+                        request, form, action_url
+                    )
+                    add_never_cache_headers(response)
+                    return response
+                elif restriction.restriction_type in [
+                    PageViewRestriction.LOGIN,
+                    PageViewRestriction.GROUPS,
+                ]:
+                    response = require_wagtail_login(next=request.get_full_path())
+                    add_never_cache_headers(response)
+                    return response
+
+        response = callback(page, request, serve_args, serve_kwargs)
+        if restrictions:
+            add_never_cache_headers(response)
+        return response
+
+    return inner
 
 
 @hooks.register("register_rich_text_features")