Opened 13 years ago

Closed 12 years ago

#15900 closed Bug (fixed)

reverse() does not properly escape namespaced views

Reported by: teolicy Owned by: dmclain
Component: Core (Other) Version: 1.3
Severity: Normal Keywords:
Cc: dmclain, tonightslastsong@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Attachments (8)

15900_normalize_namespace_prefixes_on_reverse.patch (3.7 KB ) - added by dmclain 13 years ago.
15900_tests_for_namespaced_urls_capturing_variables.patch (1.8 KB ) - added by dmclain 13 years ago.
15900_tests_for_namespaced_urls_capturing_variables.diff (1.8 KB ) - added by dmclain 13 years ago.
15900_normalize_namespace_prefixes_on_reverse.diff (4.9 KB ) - added by dmclain 13 years ago.
15900_normalize_namespace_prefixes_on_reverse.2.diff (5.6 KB ) - added by dmclain 13 years ago.
Adding a resolve test for variable capturing namespaced paths
15900_normalize_namespace_prefixes_on_reverse.3.diff (5.6 KB ) - added by dmclain 13 years ago.
getting rid of the abuse of kwargs
15900_normalize_namespace_prefixes_on_reverse.4.diff (4.9 KB ) - added by dmclain 13 years ago.
15900_normalize_namespace_prefixes_on_reverse.5.diff (6.1 KB ) - added by dmclain 13 years ago.

Download all attachments as: .zip

Change History (28)

comment:1 by anonymous, 13 years ago

Component: UncategorizedCore (Other)
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

comment:2 by Chris Beaven, 13 years ago

(that was me)

comment:3 by dmclain, 13 years ago

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.

comment:4 by dmclain, 13 years ago

Cc: dmclain added
Has patch: set
Owner: changed from nobody to dmclain
Status: newassigned

comment:5 by Tim Valenta, 13 years ago

Cc: tonightslastsong@… added

comment:6 by Tim Valenta, 13 years ago

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).

comment:7 by Julien Phalip, 13 years ago

Patch needs improvement: set

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/".

in reply to:  7 comment:8 by dmclain, 13 years ago

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.

by dmclain, 13 years ago

Adding a resolve test for variable capturing namespaced paths

comment:9 by dmclain, 13 years ago

Patch needs improvement: unset

comment:10 by Jannis Leidel, 13 years ago

Patch needs improvement: set

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

comment:11 by Jacob, 13 years ago

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.

comment:12 by Jannis Leidel, 13 years ago

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

comment:13 by dmclain, 13 years ago

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.

by dmclain, 13 years ago

getting rid of the abuse of kwargs

comment:14 by dmclain, 13 years ago

Patch needs improvement: unset

comment:15 by patchhammer, 13 years ago

Testing adding a comment to a ticket

comment:16 by dmclain, 13 years ago

New patch applies cleanly against trunk after r16177

comment:17 by anonymous, 13 years ago

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))

comment:18 by dmclain, 13 years ago

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.

comment:19 by Gabriel Hurley, 12 years ago

Triage Stage: AcceptedReady for checkin
UI/UX: unset

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!

comment:20 by Gabriel Hurley, 12 years ago

Resolution: fixed
Status: assignedclosed

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.

Note: See TracTickets for help on using tickets.
Back to Top