reverse() does not properly escape namespaced views

[teolicy]

Easier to provide an example than to explain.

Given a freshly created django 1.3 project with urls.py as follows:

from django.conf.urls.defaults import patterns, include, url
from django.contrib import admin
admin.autodiscover()
urlpatterns = patterns('',
    url(r'^\.admin/', include(admin.site.urls)),
)

Trying to:

from django.core.urlresolvers import reverse
reverse('admin:index')

returns: '/%5C.admin/'
(the \ is escaped to be a literal \ rather than be interpreted as escaping the dot)

Changing the urlpattern line to:

url(r'^[.]admin/', include(admin.site.urls)),

Does not help.

As far as I could determine, this only happens for namespaced URLs.

[Chris Beaven]

(that was me)

[dmclain]

I've uploaded a patch that changes the behavior of reverse and RegexURLResolver.reverse so that namespaced urls are properly normalized. The issue was that when reverse traversed the URLResolver tree, it would build up a prefix consisting of all the regex's, but it would just concatenate them rather than normalizing them. Since most people don't have regex special characters in the prefix to namespaced urls, it wasn't a problem. I also tried to make it so that if a brave soul tries to capture a view function argument, it will work.

The patch also changes a couple of the unit tests to use regex's that ought to be the same as they were before after being normalized with django.utils.regex_helper.normalize.

[Tim Valenta]

When I have a variable capture in the URL prefix responsible for declaring a namespace, I get a KeyError on the patched version of line 321 of the urlresolver.py file, which claims it's missing "username" (in my case).

[Julien Phalip]

I've had a quick look at this and I'm not sure if this patch addresses the core issue. dmclain, you mention that the resolver did not normalise prefixes, yet I see there already are passing tests for the following pattern at source:django/trunk/tests/regressiontests/urlpatterns_reverse/extra_urls.py#L12:

url(r'^prefix/(?P<prefix>\w+)/', include('regressiontests.urlpatterns_reverse.included_urls2')),

As mentioned by the reporter, the bug only occurs when a namespace is used, yet I fail to see how your patch specifically addresses that. Could you elaborate on your approach?

Also, looking at your tests, the following passes:

(r'^other[246]/', include(otherobj2.urls)),

... yet the tests unexpectedly fail if I swap the digits:

(r'^other[426]/', include(otherobj2.urls)),

Finally, for completeness, it'd be good to test what is supposed to happen, for example, with "/other9/inner/".

[dmclain]

As mentioned by the reporter, the bug only occurs when a namespace is used, yet I fail to see how your patch specifically addresses that. Could you elaborate on your approach?

When there is a namespaced url being resolved, the reverse() function builds a prefix composed of all the url pieces that correspond to the nested namespaces. These pieces are standard regular expressions, but rather than being normalized, they are blindly prepended onto the path. This results in errors if the regex does anything regex-y like capturing variables or having a character class in them.

I've had a quick look at this and I'm not sure if this patch addresses the core issue. dmclain, you mention that the resolver did not normalise prefixes, yet I see there already are passing tests for the following pattern at source:django/trunk/tests/regressiontests/urlpatterns_reverse/extra_urls.py#L12:

That test doesn't trigger the behavior this bug is talking about because it's not namespaced and so doesn't go down the prefix building code path.

Also, looking at your tests, the following passes:

(r'^other[246]/', include(otherobj2.urls)),

... yet the tests unexpectedly fail if I swap the digits:

(r'^other[426]/', include(otherobj2.urls)),

That is the expected behavior. The method django uses to normalize a regular expression just takes the first option out of a character class. Similarly it uses a period when it's reversing the any character class which is represented by a period in regexs. See source:django/trunk/django/utils/regex_helper.py#L12

Finally, for completeness, it'd be good to test what is supposed to happen, for example, with "/other9/inner/".

I assume you mean doing the url resolving forward instead of just testing reverse? I'll add a test for that.

[dmclain]

Adding a resolve test for variable capturing namespaced paths

[Jannis Leidel]

Yeah, __DJANGO_REVERSE_PREFIX is not going to fly as a parameter name, why not just prefix?

[Jacob]

I think the idea behind the silly parameter name is to avoid clashes with a URL parameter named prefix. But __DJANGO_REVERSE_PREFIX is really overdoing it; I'd say go with _prefix on the assumption that URL parameters marked "privateish" would be silly.

[Jannis Leidel]

Ah, got it, that makes sense. Not using kwargs would be even nicer.

[dmclain]

The new plan is to separate RegexUrlResolver.reverse() into a one line reverse() that calls reverse_with_prefix() which will be the function that the bare reverse() calls.

[dmclain]

getting rid of the abuse of kwargs

[patchhammer]

Testing adding a comment to a ticket

[dmclain]

New patch applies cleanly against trunk after r16177

[anonymous]

I still get KeyErrors representative of my view's named pattern variables, tracing back to line 323 in patched file:

candidate = (prefix_norm + result) % dict(zip(params, unicode_args))

I'm of the opinion this should be taking into account the prefix_args dict:

candidate = (prefix_norm + result) % dict(zip(prefix_args + params, unicode_args))

[dmclain]

I made the change suggested by anonymous/tiliv, and added some tests to verify that using args to reverse works. I'm feeling pretty good about this patch, but am happy to go another round or 4 if it needs more of anything.

[Gabriel Hurley]

I just ran headlong into this bug in my own work, and this patch worked like a charm. The code looks acceptable, I'm not seeing any adverse effects and it resolves the issue (no pun intended). Bumping to RFC for a final review.

Thanks for the work on this!

[Gabriel Hurley]

In [17251]:

Fixed #15900 -- Calls to reverse with nested namespaced urls are escaped properly and capture parameters as expected.

Thanks to teolicy for the report, and dmclain for the patch.