Code

Opened 3 years ago

Closed 2 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 3 years ago.
15900_tests_for_namespaced_urls_capturing_variables.patch (1.8 KB) - added by dmclain 3 years ago.
15900_tests_for_namespaced_urls_capturing_variables.diff (1.8 KB) - added by dmclain 3 years ago.
15900_normalize_namespace_prefixes_on_reverse.diff (4.9 KB) - added by dmclain 3 years ago.
15900_normalize_namespace_prefixes_on_reverse.2.diff (5.6 KB) - added by dmclain 3 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 3 years ago.
getting rid of the abuse of kwargs
15900_normalize_namespace_prefixes_on_reverse.4.diff (4.9 KB) - added by dmclain 3 years ago.
15900_normalize_namespace_prefixes_on_reverse.5.diff (6.1 KB) - added by dmclain 3 years ago.

Download all attachments as: .zip

Change History (28)

comment:1 Changed 3 years ago by anonymous

  • Component changed from Uncategorized to Core (Other)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Bug

comment:2 Changed 3 years ago by SmileyChris

(that was me)

comment:3 Changed 3 years ago by 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.

comment:4 Changed 3 years ago by dmclain

  • Cc dmclain added
  • Has patch set
  • Owner changed from nobody to dmclain
  • Status changed from new to assigned

comment:5 Changed 3 years ago by tiliv

  • Cc tonightslastsong@… added

comment:6 Changed 3 years ago by tiliv

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 follow-up: Changed 3 years ago by julien

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

comment:8 in reply to: ↑ 7 Changed 3 years ago by 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.

Changed 3 years ago by dmclain

Adding a resolve test for variable capturing namespaced paths

comment:9 Changed 3 years ago by dmclain

  • Patch needs improvement unset

comment:10 Changed 3 years ago by jezdez

  • Patch needs improvement set

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

comment:11 Changed 3 years ago by 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.

comment:12 Changed 3 years ago by jezdez

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

comment:13 Changed 3 years ago by 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.

Changed 3 years ago by dmclain

getting rid of the abuse of kwargs

comment:14 Changed 3 years ago by dmclain

  • Patch needs improvement unset

comment:15 Changed 3 years ago by patchhammer

Testing adding a comment to a ticket

comment:16 Changed 3 years ago by dmclain

New patch applies cleanly against trunk after r16177

comment:17 Changed 3 years ago by 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))

comment:18 Changed 3 years ago by 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.

comment:19 Changed 2 years ago by gabrielhurley

  • Triage Stage changed from Accepted to Ready 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 Changed 2 years ago by gabrielhurley

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.