Catch redirects that omit a destination link or point to a page with no routable URL (#4836)

Fixes #4815

CHANGELOG.txt

@@ -17,6 +17,7 @@ Changelog
  * Fix: Additional fields on custom document models now show on the multiple document upload view (Robert Rollins, Sergey Fedoseev)
  * Fix: Help text is partially hidden when using a combination of BooleanField and FieldPanel in page model (Dzianis Sheka)
  * Fix: Allow custom logos of any height in the admin menu (Meteor0id)
+ * Fix: Redirects now return 404 when destination is unspecified or a page with no site (Hillary Jeffrey)
 
 
 2.3 LTS (23.10.2018)

CONTRIBUTORS.rst

@@ -333,6 +333,7 @@ Contributors
 * Fedor Selitsky
 * Seb Brown
 * Noah B Johnson
+* Hillary Jeffrey
 
 Translators
 ===========

docs/releases/2.4.rst

@@ -41,6 +41,7 @@ Bug fixes
  * Document chooser now displays more useful help message when there are no documents in Wagtail document library (gmmoraes, Stas Rudakou)
  * Allow custom logos of any height in the admin menu (Meteor0id)
  * Users without the edit permission no longer see "Edit" links in list of pages waiting for moderation (Justin Focus, Fedor Selitsky)
+ * Redirects now return 404 when destination is unspecified or a page with no site (Hillary Jeffrey)
 
 
 Upgrade considerations

wagtail/contrib/redirects/middleware.py

@@ -57,6 +57,9 @@ class RedirectMiddleware(MiddlewareMixin):
             if redirect is None:
                 return response
 
+        if redirect.link is None:
+            return response
+
         if redirect.is_permanent:
             return http.HttpResponsePermanentRedirect(redirect.link)
         else:

wagtail/contrib/redirects/models.py

@@ -34,9 +34,11 @@ class Redirect(models.Model):
     def link(self):
         if self.redirect_page:
             return self.redirect_page.url
-        else:
+        elif self.redirect_link:
             return self.redirect_link
 
+        return None
+
     def get_is_permanent_display(self):
         if self.is_permanent:
             return _("permanent")

wagtail/contrib/redirects/tests.py

@@ -176,6 +176,21 @@ class TestRedirects(TestCase):
         response = self.client.get('/xmas/', HTTP_HOST='localhost')
         self.assertEqual(response.status_code, 404)
 
+    def test_redirect_without_page_or_link_target(self):
+        models.Redirect.objects.create(old_path='/xmas/', redirect_link='')
+
+        # the redirect has been created but has no target and should 404
+        response = self.client.get('/xmas/', HTTP_HOST='localhost')
+        self.assertEqual(response.status_code, 404)
+
+    def test_redirect_to_page_without_site(self):
+        siteless_page = Page.objects.get(url_path='/does-not-exist/')
+        models.Redirect.objects.create(old_path='/xmas', redirect_page=siteless_page)
+
+        # the redirect's destination page doesn't have a site so the redirect should 404
+        response = self.client.get('/xmas/', HTTP_HOST='localhost')
+        self.assertEqual(response.status_code, 404)
+
     def test_duplicate_redirects_when_match_is_for_generic(self):
         contact_page = Page.objects.get(url_path='/home/contact-us/')
         site = Site.objects.create(hostname='other.example.com', port=80, root_page=contact_page)

wagtail/tests/testapp/fixtures/test.json

@@ -5,7 +5,7 @@
     "fields": {
         "title": "Root",
         "draft_title": "Root",
-        "numchild": 1,
+        "numchild": 2,
         "show_in_menus": false,
         "live": true,
         "depth": 1,
@@ -594,6 +594,23 @@
     }
 },
 
+{
+    "pk": 20,
+    "model": "wagtailcore.page",
+    "fields": {
+        "title": "This page doesn't get served",
+        "draft_title": "This page doesn't get served",
+        "numchild": 0,
+        "show_in_menus": false,
+        "live": true,
+        "depth": 2,
+        "content_type": ["wagtailcore", "page"],
+        "path": "00010002",
+        "url_path": "/does-not-exist/",
+        "slug": "does-not-exist"
+    }
+},
+
 
 {
     "pk": 1,