From fe3fc5210f0bb334a679ed420152af1c862c0239 Mon Sep 17 00:00:00 2001 From: Alasdair Nicol Date: Wed, 16 Sep 2015 23:07:39 +0100 Subject: [PATCH] Fixed #23813 -- Added checks for common URL pattern errors Thanks jwa and lamby for the suggestions, and timgraham and jarshwah for their reviews. --- django/core/checks/__init__.py | 2 + django/core/checks/registry.py | 1 + django/core/checks/urls.py | 87 +++++++++++++++++++++++ django/core/urlresolvers.py | 2 +- docs/ref/checks.txt | 16 +++++ docs/releases/1.9.txt | 2 + tests/check_framework/test_urls.py | 37 ++++++++++ tests/check_framework/urls_include.py | 7 ++ tests/check_framework/urls_name.py | 13 ++++ tests/check_framework/urls_no_warnings.py | 15 ++++ tests/check_framework/urls_slash.py | 13 ++++ 11 files changed, 194 insertions(+), 1 deletion(-) create mode 100644 django/core/checks/urls.py create mode 100644 tests/check_framework/test_urls.py create mode 100644 tests/check_framework/urls_include.py create mode 100644 tests/check_framework/urls_name.py create mode 100644 tests/check_framework/urls_no_warnings.py create mode 100644 tests/check_framework/urls_slash.py diff --git a/django/core/checks/__init__.py b/django/core/checks/__init__.py index dec2446ae2..cd21744327 100644 --- a/django/core/checks/__init__.py +++ b/django/core/checks/__init__.py @@ -15,6 +15,8 @@ import django.core.checks.security.base # NOQA isort:skip import django.core.checks.security.csrf # NOQA isort:skip import django.core.checks.security.sessions # NOQA isort:skip import django.core.checks.templates # NOQA isort:skip +import django.core.checks.urls # NOQA isort:skip + __all__ = [ 'CheckMessage', diff --git a/django/core/checks/registry.py b/django/core/checks/registry.py index 96df429dc9..3e3d45fff8 100644 --- a/django/core/checks/registry.py +++ b/django/core/checks/registry.py @@ -17,6 +17,7 @@ class Tags(object): security = 'security' signals = 'signals' templates = 'templates' + urls = 'urls' class CheckRegistry(object): diff --git a/django/core/checks/urls.py b/django/core/checks/urls.py new file mode 100644 index 0000000000..a0d5b4a825 --- /dev/null +++ b/django/core/checks/urls.py @@ -0,0 +1,87 @@ +from __future__ import unicode_literals + +from . import Tags, Warning, register + + +@register(Tags.urls) +def check_url_config(app_configs, **kwargs): + from django.core.urlresolvers import get_resolver + resolver = get_resolver() + return check_resolver(resolver) + + +def check_resolver(resolver): + """ + Recursively check the resolver + """ + from django.core.urlresolvers import RegexURLPattern, RegexURLResolver + warnings = [] + for pattern in resolver.url_patterns: + if isinstance(pattern, RegexURLResolver): + warnings.extend(check_include_trailing_dollar(pattern)) + # Check resolver recursively + warnings.extend(check_resolver(pattern)) + elif isinstance(pattern, RegexURLPattern): + warnings.extend(check_pattern_name(pattern)) + + warnings.extend(check_pattern_startswith_slash(pattern)) + + return warnings + + +def describe_pattern(pattern): + """ + Formats the URL pattern for display in warning messages + """ + description = "'{}'".format(pattern.regex.pattern) + if getattr(pattern, 'name', False): + description += " [name='{}']".format(pattern.name) + return description + + +def check_include_trailing_dollar(pattern): + """ + Checks that include is not used with a regex ending with a dollar. + """ + regex_pattern = pattern.regex.pattern + if regex_pattern.endswith('$') and not regex_pattern.endswith('\$'): + warning = Warning( + "Your URL pattern {} uses include with a regex ending with a '$'. " + "Remove the dollar from the regex to avoid problems including " + "URLs.".format(describe_pattern(pattern)), + id="urls.W001", + ) + return [warning] + else: + return [] + + +def check_pattern_startswith_slash(pattern): + """ + Checks that the pattern does not begin with a forward slash + """ + regex_pattern = pattern.regex.pattern + if regex_pattern.startswith('/') or regex_pattern.startswith('^/'): + warning = Warning( + "Your URL pattern {} has a regex beginning with a '/'. " + "Remove this slash as it is unnecessary.".format(describe_pattern(pattern)), + id="urls.W002", + ) + return [warning] + else: + return [] + + +def check_pattern_name(pattern): + """ + Checks that the pattern name does not contain a colon + """ + if pattern.name is not None and ":" in pattern.name: + warning = Warning( + "Your URL pattern {} has a name including a ':'. Remove the colon, to " + "avoid ambiguous namespace references.".format(describe_pattern(pattern)), + id="urls.W003", + ) + return [warning] + else: + return [] diff --git a/django/core/urlresolvers.py b/django/core/urlresolvers.py index e462c343dc..3554878865 100644 --- a/django/core/urlresolvers.py +++ b/django/core/urlresolvers.py @@ -146,7 +146,7 @@ def get_callable(lookup_view, can_fail=False): @lru_cache.lru_cache(maxsize=None) -def get_resolver(urlconf): +def get_resolver(urlconf=None): if urlconf is None: from django.conf import settings urlconf = settings.ROOT_URLCONF diff --git a/docs/ref/checks.txt b/docs/ref/checks.txt index 39f3a06089..208e3fb81f 100644 --- a/docs/ref/checks.txt +++ b/docs/ref/checks.txt @@ -81,6 +81,7 @@ Django's system checks are organized using the following tags: * ``security``: Checks security related configuration. * ``templates``: Checks template related configuration. * ``caches``: Checks cache related configuration. +* ``urls``: Checks URL configuration. Some checks may be registered with multiple tags. @@ -580,3 +581,18 @@ configured: * **caches.E001**: You must define a ``'default'`` cache in your :setting:`CACHES` setting. + +URLs +---- + +The following checks are performed on your URL configuration: + +* **urls.W001**: Your URL pattern ```` uses + :func:`~django.conf.urls.include` with a ``regex`` ending with a + ``$``. Remove the dollar from the ``regex`` to avoid problems + including URLs. +* **urls.W002**: Your URL pattern ```` has a ``regex`` + beginning with a ``/``. Remove this slash as it is unnecessary. +* **urls.W003**: Your URL pattern ```` has a ``name`` + including a ``:``. Remove the colon, to avoid ambiguous namespace + references. diff --git a/docs/releases/1.9.txt b/docs/releases/1.9.txt index 18fa8ef6bf..f1eba56a1e 100644 --- a/docs/releases/1.9.txt +++ b/docs/releases/1.9.txt @@ -641,6 +641,8 @@ URLs of (, ) as the first argument to :func:`~django.conf.urls.include`. +* System checks have been added for common URL pattern mistakes. + Validators ^^^^^^^^^^ diff --git a/tests/check_framework/test_urls.py b/tests/check_framework/test_urls.py new file mode 100644 index 0000000000..2a05146e77 --- /dev/null +++ b/tests/check_framework/test_urls.py @@ -0,0 +1,37 @@ +from django.core.checks.urls import check_url_config +from django.test import SimpleTestCase +from django.test.utils import override_settings + + +class CheckUrlsTest(SimpleTestCase): + @override_settings(ROOT_URLCONF='check_framework.urls_no_warnings') + def test_include_no_warnings(self): + result = check_url_config(None) + self.assertEqual(result, []) + + @override_settings(ROOT_URLCONF='check_framework.urls_include') + def test_include_with_dollar(self): + result = check_url_config(None) + self.assertEqual(len(result), 1) + warning = result[0] + self.assertEqual(warning.id, 'urls.W001') + expected_msg = "Your URL pattern '^include-with-dollar$' uses include with a regex ending with a '$'." + self.assertIn(expected_msg, warning.msg) + + @override_settings(ROOT_URLCONF='check_framework.urls_slash') + def test_url_beginning_with_slash(self): + result = check_url_config(None) + self.assertEqual(len(result), 1) + warning = result[0] + self.assertEqual(warning.id, 'urls.W002') + expected_msg = "Your URL pattern '/starting-with-slash/$' has a regex beginning with a '/'" + self.assertIn(expected_msg, warning.msg) + + @override_settings(ROOT_URLCONF='check_framework.urls_name') + def test_url_pattern_name_with_colon(self): + result = check_url_config(None) + self.assertEqual(len(result), 1) + warning = result[0] + self.assertEqual(warning.id, 'urls.W003') + expected_msg = "Your URL pattern '^$' [name='name_with:colon'] has a name including a ':'." + self.assertIn(expected_msg, warning.msg) diff --git a/tests/check_framework/urls_include.py b/tests/check_framework/urls_include.py new file mode 100644 index 0000000000..5bb94c9688 --- /dev/null +++ b/tests/check_framework/urls_include.py @@ -0,0 +1,7 @@ +from django.conf.urls import include, url + +urlpatterns = [ + url(r'^', include([ + url(r'^include-with-dollar$', include([])), + ])), +] diff --git a/tests/check_framework/urls_name.py b/tests/check_framework/urls_name.py new file mode 100644 index 0000000000..9662532851 --- /dev/null +++ b/tests/check_framework/urls_name.py @@ -0,0 +1,13 @@ +from django.conf.urls import include, url +from django.http import HttpResponse + + +def view(request): + return HttpResponse('') + + +urlpatterns = [ + url('^', include([ + url(r'^$', view, name='name_with:colon'), + ])), +] diff --git a/tests/check_framework/urls_no_warnings.py b/tests/check_framework/urls_no_warnings.py new file mode 100644 index 0000000000..657306f1ef --- /dev/null +++ b/tests/check_framework/urls_no_warnings.py @@ -0,0 +1,15 @@ +from django.conf.urls import include, url +from django.http import HttpResponse + + +def view(request): + return HttpResponse('') + + +urlpatterns = [ + url(r'^foo/', view, name='foo'), + # This dollar is ok as it is escaped + url(r'^\$', include([ + url(r'^bar/$', view, name='bar'), + ])), +] diff --git a/tests/check_framework/urls_slash.py b/tests/check_framework/urls_slash.py new file mode 100644 index 0000000000..474f6293d5 --- /dev/null +++ b/tests/check_framework/urls_slash.py @@ -0,0 +1,13 @@ +from django.conf.urls import include, url +from django.http import HttpResponse + + +def view(request): + return HttpResponse('') + + +urlpatterns = [ + url('^', include([ + url(r'/starting-with-slash/$', view), + ])), +]