Strip Unicode NULL chars when normalizing paths

After migrating a Wagtail-based site from MySQL to Postgres, we
noticed that malicious requests to the site that included percent-
encoded Unicode NULLs (`%00`) raised a `ValueError` exception that we
hadn't seen when using MySQL: `A string literal cannot contain NUL
(0x00) characters.` This appears to relate to `psycopg2`'s decision to
raise an exception in these situations, as discussed here:

    https://github.com/psycopg/psycopg2/issues/420

While newer versions of Django appear to provide some field validation
that addresses these characters, it doesn't look like Wagtail's
redirect middleware is making use of those validators, and so it seemed
reasonable to clean these characters in the context of 'normalizing'
the paths before looking for corresponding redirects -- especially
since a quick investigation on the internet suggests that U+0000 in
URLs can be used as a means of attack, and also since RFC 3986 says:

   Note, however, that the "%00" percent-encoding (NUL) may require
   special handling and should be rejected if the application is not
   expecting to receive raw data within a component.

CHANGELOG.txt

@@ -24,6 +24,7 @@ Changelog
  * Fix: Remove duplicate border radius of avatars (Benjamin Thurm)
  * Fix: Site.get_site_root_paths() preferring other sites over the default when some sites share the same root_page (Andy Babic)
  * Fix: Pages with missing model definitions no longer crash the API (Abdulmalik Abdulwahab)
+ * Fix: Strip null characters from paths when checking for redirects (Andrew Crewdson)
 
 
 2.1.1 (xx.xx.xxxx) - IN DEVELOPMENT

CONTRIBUTORS.rst

@@ -308,6 +308,7 @@ Contributors
 * Daniele Procida
 * Catherine Farman
 * Abdulmalik Abdulwahab
+* Andrew Crewdson
 
 Translators
 ===========

docs/releases/2.2.rst

@@ -37,6 +37,7 @@ Bug fixes
  * Remove duplicate border radius of avatars (Benjamin Thurm)
  * Site.get_site_root_paths() preferring other sites over the default when some sites share the same root_page (Andy Babic)
  * Pages with missing model definitions no longer crash the API (Abdulmalik Abdulwahab)
+ * Strip null characters from paths when checking for redirects (Andrew Crewdson)
 
 Upgrade considerations
 ======================

wagtail/contrib/redirects/models.py

@@ -71,8 +71,12 @@ class Redirect(models.Model):
         parameters_components = parameters.split(';')
         parameters = ';'.join(sorted(parameters_components))
 
-        # Query string components must be sorted alphabetically
         query_string = url_parsed[4]
+
+        # Strip Unicode NULLs (U+0000) for safety and Postgres compatibility
+        query_string = query_string.replace('%00', '')
+
+        # Query string components must be sorted alphabetically
         query_string_components = query_string.split('&')
         query_string = '&'.join(sorted(query_string_components))
 

wagtail/contrib/redirects/tests.py

@@ -78,6 +78,8 @@ class TestRedirects(TestCase):
 
         self.assertEqual('/', normalise_path('/'))  # '/' should stay '/'
 
+        self.assertEqual('/foo?bar=baz', normalise_path('/foo?bar=baz%00'))  # Strip NULLs
+
         # Normalise some rubbish to make sure it doesn't crash
         normalise_path('This is not a URL')
         normalise_path('//////hello/world')