Commit e167e96c authored by Claude Paroz's avatar Claude Paroz
Browse files

Fixed #22223 -- Prevented over-escaping URLs in reverse() 

And follow more closely the class of characters defined in the
RFC 3986.
Thanks Erik van Zijst for the report and the initial patch, and
Tim Graham for the review.
parent 8780849d

django/contrib/admin/utils.py

+1 −1
Original line number Diff line number Diff line
@@ -57,7 +57,7 @@ def quote(s): 
    res = list(s)

    for i in range(len(res)):

        c = res[i]

        if c in """:/_#?;@&=+$,"<>%\\""":

        if c in """:/_#?;@&=+$,"[]<>%\\""":

            res[i] = '_%02X' % ord(c)

    return ''.join(res)

django/core/urlresolvers.py

+4 −2
Original line number Diff line number Diff line
@@ -20,7 +20,7 @@ from django.utils.datastructures import MultiValueDict 
from django.utils.deprecation import RemovedInDjango20Warning

from django.utils.encoding import force_str, force_text, iri_to_uri

from django.utils.functional import lazy

from django.utils.http import urlquote

from django.utils.http import RFC3986_SUBDELIMS, urlquote

from django.utils.module_loading import module_has_submodule

from django.utils.regex_helper import normalize

from django.utils import six, lru_cache
@@ -453,7 +453,9 @@ class RegexURLResolver(LocaleRegexProvider): 
                # arguments in order to return a properly encoded URL.

                candidate_pat = prefix_norm.replace('%', '%%') + result

                if re.search('^%s%s' % (prefix_norm, pattern), candidate_pat % candidate_subs, re.UNICODE):

                    candidate_subs = dict((k, urlquote(v)) for (k, v) in candidate_subs.items())

                    # safe characters from `pchar` definition of RFC 3986

                    candidate_subs = dict((k, urlquote(v, safe=RFC3986_SUBDELIMS + str('/~:@')))

                                          for (k, v) in candidate_subs.items())

                    return candidate_pat % candidate_subs

        # lookup_view can be URL label, or dotted path, or callable, Any of

        # these can be passed in at the top, but callables are not friendly in

django/utils/html.py

+2 −1
Original line number Diff line number Diff line
@@ -7,6 +7,7 @@ import sys 


from django.utils.encoding import force_text, force_str

from django.utils.functional import allow_lazy

from django.utils.http import RFC3986_GENDELIMS, RFC3986_SUBDELIMS

from django.utils.safestring import SafeData, mark_safe

from django.utils import six

from django.utils.six.moves.urllib.parse import quote, unquote, urlsplit, urlunsplit
@@ -215,7 +216,7 @@ def smart_urlquote(url): 


    url = unquote(force_str(url))

    # See http://bugs.python.org/issue2637

    url = quote(url, safe=b'!*\'();:@&=+$,/?#[]~')

    url = quote(url, safe=RFC3986_SUBDELIMS + RFC3986_GENDELIMS + str('~'))



    return force_text(url)

django/utils/http.py

+3 −0
Original line number Diff line number Diff line
@@ -30,6 +30,9 @@ RFC1123_DATE = re.compile(r'^\w{3}, %s %s %s %s GMT$' % (__D, __M, __Y, __T)) 
RFC850_DATE = re.compile(r'^\w{6,9}, %s-%s-%s %s GMT$' % (__D, __M, __Y2, __T))

ASCTIME_DATE = re.compile(r'^\w{3} %s %s %s %s$' % (__M, __D2, __T, __Y))



RFC3986_GENDELIMS = str(":/?#[]@")

RFC3986_SUBDELIMS = str("!$&'()*+,;=")





def urlquote(url, safe='/'):

    """

tests/admin_views/tests.py

+6 −6
Original line number Diff line number Diff line
@@ -36,7 +36,7 @@ from django.utils import translation 
from django.utils.cache import get_max_age

from django.utils.encoding import iri_to_uri, force_bytes, force_text

from django.utils.html import escape

from django.utils.http import urlencode, urlquote

from django.utils.http import urlencode

from django.utils.six.moves.urllib.parse import parse_qsl, urljoin, urlparse

from django.utils._os import upath

from django.utils import six
@@ -1748,7 +1748,7 @@ class AdminViewStringPrimaryKeyTest(TestCase): 
        prefix = '/test_admin/admin/admin_views/modelwithstringprimarykey/'

        response = self.client.get(prefix)

        # this URL now comes through reverse(), thus url quoting and iri_to_uri encoding

        pk_final_url = escape(iri_to_uri(urlquote(quote(self.pk))))

        pk_final_url = escape(iri_to_uri(quote(self.pk)))

        should_contain = """<th class="field-__str__"><a href="%s%s/">%s</a></th>""" % (prefix, pk_final_url, escape(self.pk))

        self.assertContains(response, should_contain)
@@ -1756,14 +1756,14 @@ class AdminViewStringPrimaryKeyTest(TestCase): 
        "The link from the recent actions list referring to the changeform of the object should be quoted"

        response = self.client.get('/test_admin/admin/')

        link = reverse('admin:admin_views_modelwithstringprimarykey_change', args=(quote(self.pk),))

        should_contain = """<a href="%s">%s</a>""" % (link, escape(self.pk))

        should_contain = """<a href="%s">%s</a>""" % (escape(link), escape(self.pk))

        self.assertContains(response, should_contain)



    def test_recentactions_without_content_type(self):

        "If a LogEntry is missing content_type it will not display it in span tag under the hyperlink."

        response = self.client.get('/test_admin/admin/')

        link = reverse('admin:admin_views_modelwithstringprimarykey_change', args=(quote(self.pk),))

        should_contain = """<a href="%s">%s</a>""" % (link, escape(self.pk))

        should_contain = """<a href="%s">%s</a>""" % (escape(link), escape(self.pk))

        self.assertContains(response, should_contain)

        should_contain = "Model with string primary key"  # capitalized in Recent Actions

        self.assertContains(response, should_contain)
@@ -1785,7 +1785,7 @@ class AdminViewStringPrimaryKeyTest(TestCase): 
        log_entry_name = "Model with string primary key"  # capitalized in Recent Actions

        logentry = LogEntry.objects.get(content_type__name__iexact=log_entry_name)

        model = "modelwithstringprimarykey"

        desired_admin_url = "/test_admin/admin/admin_views/%s/%s/" % (model, escape(iri_to_uri(urlquote(quote(self.pk)))))

        desired_admin_url = "/test_admin/admin/admin_views/%s/%s/" % (model, iri_to_uri(quote(self.pk)))

        self.assertEqual(logentry.get_admin_url(), desired_admin_url)



        logentry.content_type.model = "non-existent"
@@ -1795,7 +1795,7 @@ class AdminViewStringPrimaryKeyTest(TestCase): 
        "The link from the delete confirmation page referring back to the changeform of the object should be quoted"

        response = self.client.get('/test_admin/admin/admin_views/modelwithstringprimarykey/%s/delete/' % quote(self.pk))

        # this URL now comes through reverse(), thus url quoting and iri_to_uri encoding

        should_contain = """/%s/">%s</a>""" % (escape(iri_to_uri(urlquote(quote(self.pk)))), escape(self.pk))

        should_contain = """/%s/">%s</a>""" % (escape(iri_to_uri(quote(self.pk))), escape(self.pk))

        self.assertContains(response, should_contain)



    def test_url_conflicts_with_add(self):