From aa31d329aba715627e84bbb20bf028173ea5b973 Mon Sep 17 00:00:00 2001 From: Matt Westcott Date: Thu, 24 Oct 2024 13:59:02 +0100 Subject: [PATCH] Stop invalid Site hostname records from breaking preview As made famous by https://youtu.be/v3KEaMTfKg0?t=319 :-) By design, Wagtail tolerates the default Site record being left at its default value of 'localhost' up to a certain point. Ideally, that point should be when it becomes strictly necessary for Wagtail to care about hostnames (such as when setting up the second site of a multi-site installation) and it should be clear to the developer what has happened and how to fix it. In practice, that point often comes when the developer deploys their site to production, sets `DEBUG=False`, and is then required by Django to set `ALLOWED_HOSTS` to their real domain name. At this point, front-end page requests work (because the initial site record is default=True, matching any domain including the live one) but previews are broken (because the dummy request object is still formed using localhost as per the site's hostname field, which is disallowed by ALLOWED_HOSTS). This is unnecessary, and can be avoided by validating the hostname against ALLOWED_HOSTS and substituting one that _is_ allowed if necessary, as we already do for pages that don't have an associated site record. --- CHANGELOG.txt | 2 + docs/releases/6.4.md | 1 + wagtail/admin/tests/pages/test_preview.py | 65 ++++++++++++++++++++++- wagtail/models/__init__.py | 40 ++++++++++---- 4 files changed, 97 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.txt b/CHANGELOG.txt index 4050c3aadd..87d2ced65a 100644 --- a/CHANGELOG.txt +++ b/CHANGELOG.txt @@ -5,6 +5,8 @@ Changelog ~~~~~~~~~~~~~~~~ * Add the ability to apply basic Page QuerySet optimizations to `specific()` sub-queries using `select_related` & `prefetch_related` (Andy Babic) + * Increase `DATA_UPLOAD_MAX_NUMBER_FIELDS` in project template (Matt Westcott) + * Stop invalid Site hostname records from breaking preview (Matt Westcott) * 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) diff --git a/docs/releases/6.4.md b/docs/releases/6.4.md index 5a560d9f59..214f74e682 100644 --- a/docs/releases/6.4.md +++ b/docs/releases/6.4.md @@ -16,6 +16,7 @@ depth: 1 * Add the ability to apply basic Page QuerySet optimizations to `specific()` sub-queries using `select_related` & `prefetch_related`, see [](../reference/pages/queryset_reference.md) (Andy Babic) * Increase `DATA_UPLOAD_MAX_NUMBER_FIELDS` in project template (Matt Westcott) + * Stop invalid Site hostname records from breaking preview (Matt Westcott) ### Bug fixes diff --git a/wagtail/admin/tests/pages/test_preview.py b/wagtail/admin/tests/pages/test_preview.py index dcdc5b46e1..b95f3f01ce 100644 --- a/wagtail/admin/tests/pages/test_preview.py +++ b/wagtail/admin/tests/pages/test_preview.py @@ -7,7 +7,7 @@ from django.utils import timezone from freezegun import freeze_time from wagtail.admin.views.pages.preview import PreviewOnEdit -from wagtail.models import Page +from wagtail.models import Page, Site from wagtail.test.testapp.models import ( CustomPreviewSizesPage, EventCategory, @@ -228,6 +228,39 @@ class TestPreview(WagtailTestUtils, TestCase): self.assertContains(response, "
  • Parties
    • ") self.assertContains(response, "
  • Holidays
    • ") + def test_preview_on_create_with_incorrect_site_hostname(self): + # Failing to set a valid hostname in the Site record (as determined by ALLOWED_HOSTS) + # should not prevent the preview from being generated. + Site.objects.filter(is_default_site=True).update(hostname="bad.example.com") + + preview_url = reverse( + "wagtailadmin_pages:preview_on_add", + args=("tests", "eventpage", self.home_page.id), + ) + response = self.client.post(preview_url, self.post_data) + + # Check the JSON response + self.assertEqual(response.status_code, 200) + self.assertJSONEqual( + response.content.decode(), + {"is_valid": True, "is_available": True}, + ) + + # Check the user can refresh the preview + preview_session_key = "wagtail-preview-tests-eventpage-{}".format( + self.home_page.id + ) + self.assertIn(preview_session_key, self.client.session) + + response = self.client.get(preview_url) + + # Check the HTML response + self.assertEqual(response.status_code, 200) + self.assertTemplateUsed(response, "tests/event_page.html") + self.assertContains(response, "Beach party") + self.assertContains(response, "
  • Parties
    • ") + self.assertContains(response, "
  • Holidays
    • ") + def test_preview_on_edit_with_m2m_field(self): preview_url = reverse( "wagtailadmin_pages:preview_on_edit", args=(self.event_page.id,) @@ -254,6 +287,36 @@ class TestPreview(WagtailTestUtils, TestCase): self.assertContains(response, "
  • Parties
    • ") self.assertContains(response, "
  • Holidays
    • ") + def test_preview_on_edit_with_incorrect_site_hostname(self): + # Failing to set a valid hostname in the Site record (as determined by ALLOWED_HOSTS) + # should not prevent the preview from being generated. + Site.objects.filter(is_default_site=True).update(hostname="bad.example.com") + + preview_url = reverse( + "wagtailadmin_pages:preview_on_edit", args=(self.event_page.id,) + ) + response = self.client.post(preview_url, self.post_data) + + # Check the JSON response + self.assertEqual(response.status_code, 200) + self.assertJSONEqual( + response.content.decode(), + {"is_valid": True, "is_available": True}, + ) + + # Check the user can refresh the preview + preview_session_key = f"wagtail-preview-{self.event_page.id}" + self.assertIn(preview_session_key, self.client.session) + + response = self.client.get(preview_url) + + # Check the HTML response + self.assertEqual(response.status_code, 200) + self.assertTemplateUsed(response, "tests/event_page.html") + self.assertContains(response, "Beach party") + self.assertContains(response, "
  • Parties
    • ") + self.assertContains(response, "
  • Holidays
    • ") + def test_preview_on_edit_with_valid_then_invalid_data(self): preview_url = reverse( "wagtailadmin_pages:preview_on_edit", args=(self.event_page.id,) diff --git a/wagtail/models/__init__.py b/wagtail/models/__init__.py index 952c4f7c68..0a93d9ad6c 100644 --- a/wagtail/models/__init__.py +++ b/wagtail/models/__init__.py @@ -41,6 +41,7 @@ from django.db.models.expressions import OuterRef, Subquery from django.db.models.functions import Concat, Substr from django.dispatch import receiver from django.http import Http404 +from django.http.request import validate_host from django.template.response import TemplateResponse from django.urls import NoReverseMatch, reverse from django.utils import timezone @@ -725,6 +726,26 @@ class PreviewableMixin: handler.load_middleware() return handler.get_response(request) + def _get_fallback_hostname(self): + """ + Return a hostname that can be used on preview requests when the object has no + routable URL, or the real hostname is not valid according to ALLOWED_HOSTS. + """ + try: + hostname = settings.ALLOWED_HOSTS[0] + except IndexError: + # Django disallows empty ALLOWED_HOSTS outright when DEBUG=False, so we must + # have DEBUG=True. In this mode Django allows localhost amongst others. + return "localhost" + + if hostname == "*": + # Any hostname is allowed + return "localhost" + + # Hostnames beginning with a dot are domain wildcards such as ".example.com" - + # these allow example.com itself, so just strip the dot + return hostname.lstrip(".") + 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 @@ -734,20 +755,19 @@ class PreviewableMixin: if url: url_info = urlsplit(url) hostname = url_info.hostname + if not validate_host( + hostname, + settings.ALLOWED_HOSTS or [".localhost", "127.0.0.1", "[::1]"], + ): + # The hostname is not valid according to ALLOWED_HOSTS - use a fallback + hostname = self._get_fallback_hostname() + path = url_info.path port = url_info.port or (443 if url_info.scheme == "https" else 80) scheme = url_info.scheme else: - # Cannot determine a URL to this object - cobble one together based on - # whatever we find in ALLOWED_HOSTS - try: - hostname = settings.ALLOWED_HOSTS[0] - if hostname == "*": - # '*' is a valid value to find in ALLOWED_HOSTS[0], but it's not a valid domain name. - # So we pretend it isn't there. - raise IndexError - except IndexError: - hostname = "localhost" + # Cannot determine a URL to this object - cobble together an arbitrary valid one + hostname = self._get_fallback_hostname() path = "/" port = 80 scheme = "http"