[Django] #24013: reverse() escapes unreserved characters supplied by prefix
Django
-----------------------------+--------------------
Reporter: tgerdes | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: 1.6
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+--------------------
This is very similar to ticket #22223. The escaping of unreserved
characters still occurs if the unreserved characters are supplied by the
prefix.
For example:
{{{
In [3]: set_script_prefix("http://www.example.com/")
In [4]: reverse('test', args=['foo:bar'])
Out[4]: u'http%3A//www.example.com/foo:bar'
}}}
I believe the same changes to the urlquote call for normalizing the prefix
applies the same as the fix in
https://github.com/django/django/commit/e167e96cfea670422ca75d0b35fe7c4195f25b63
This is also a regression in 1.6.
--
Ticket URL: <https://code.djangoproject.com/ticket/24013>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: tgerdes | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: 1.6
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
I haven't seen the need to set script prefix as an absolute URI (or
documentation that says doing so is valid). Could you confirm in what
cases this happens? Thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/24013#comment:1>
Django
Reporter: tgerdes | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: 1.6
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by tgerdes):
Actually that use case is probably an abuse of the [undocumented]
set_script_prefix method, fostered by years of community advice (ex,
https://djangosnippets.org/snippets/1713/ which you should really use
[https://docs.djangoproject.com/en/dev/ref/request-
response/#django.http.HttpRequest.build_absolute_uri
request.build_absolute_uri()] for instead).
To attempt to justify myself, the context where we use set_script_prefix()
is outside of the wsgi process, for example in celery tasks or in
management commands where we still want to generate absolute URLs.
Let me amend it to a use case that does matter and requires a fix here:
if your script prefix has this class of character in it. For example a
apache/nginx proxying the location "/script:name/" to the django project.
And to refer to some django docs: you can force this with
[https://docs.djangoproject.com/en/dev/ref/settings/#force-script-name
FORCE_SCRIPT_NAME].
So if I do:
{{{
# settings.py
FORCE_SCRIPT_NAME="/script:name/"
# views.py
def test_view(request, arg):
# This view is registered with
#url(r'^(.*)$', 'test_app.views.test_view', name='test')
return HttpResponse(reverse("test", args=["foo:bar"]))
}}}
The output is
{{{ /script%3Aname/foo:bar }}}
expected is
{{{ /script:name/foo:bar }}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24013#comment:2>
Django
Reporter: tgerdes | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Unreviewed => Accepted
Comment:
Makes sense. A regression test that currently fails is attached. I
couldn't confirm it's a regression in 1.6 since the test fails with this
error on 1.5:
{{{
======================================================================
ERROR: test_script_name_escaping
(regressiontests.urlpatterns_reverse.tests.ReverseShortcutTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File
"/home/tim/code/django/tests/regressiontests/urlpatterns_reverse/tests.py",
line 277, in test_script_name_escaping
reverse('optional', args=['foo:bar']),
File "/home/tim/code/django/django/core/urlresolvers.py", line 522, in
reverse
return iri_to_uri(resolver._reverse_with_prefix(view, prefix, *args,
**kwargs))
File "/home/tim/code/django/django/core/urlresolvers.py", line 415, in
_reverse_with_prefix
candidate = (prefix_norm + result) % dict(zip(prefix_args + params,
unicode_args))
ValueError: unsupported format character 'A' (0x41) at index 9
}}}
But maybe that's just an effect of the particular test. We can likely
backport to 1.7 if we have a patch to fix the issue (1.6 is only receiving
security fixes at this time).
--
Ticket URL: <https://code.djangoproject.com/ticket/24013#comment:3>
Django
Reporter: tgerdes | Owner: bpeschier
Type: Bug | Status: assigned
Component: Core (URLs) | Version: 1.6
Keywords: ams2015 | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by bpeschier):
* keywords: => ams2015
* owner: nobody => bpeschier
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/24013#comment:4>
Django
Reporter: tgerdes | Owner: bpeschier
Type: Bug | Status: assigned
Keywords: ams2015 | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by bpeschier):
So the problem is very similar, as mentioned, to #22223 and the solution
is also very similar: treat the prefix as another path segment and use the
same safe-argument when calling {{{urlquote}}} 1]. This makes the test for
#20022 fail, but that test is incorrect and can be easily fixed (it should
test escaping of {{{%}}}, not {{{~}}}, which is an unreserved char)
There is another problem however: all url patterns should be valid regex,
but the script prefix does not have to be (or does it?). When
{{{reverse}}} tries to use the prefix, it assumes it needs to normalize it
and thus that it is a valid regex. This breaks {{{normalize}}} when you
add a singular {{{)}}} which is a valid path part, but not valid regex.
The prefix-argument is undocumented and defaults to the script prefix. So
we can either assume all prefixes can be treated as plain strings, or we
can regex-escape {{{get_script_prefix}}} when {{{reverse}}} populates the
prefix 2].
1]:
https://github.com/django/django/blob/master/django/core/urlresolvers.py#L448
2]:
https://github.com/django/django/blob/master/django/core/urlresolvers.py#L532
--
Ticket URL: <https://code.djangoproject.com/ticket/24013#comment:5>
Django
Reporter: tgerdes | Owner: bpeschier
Type: Bug | Status: assigned
Keywords: ams2015 | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* has_patch: 0 => 1
* stage: Accepted => Ready for checkin
Comment:
[https://github.com/django/django/pull/4272 PR] from bpeschier looks good.
--
Ticket URL: <https://code.djangoproject.com/ticket/24013#comment:6>
Django
Reporter: tgerdes | Owner: bpeschier
Component: Core (URLs) | Version: 1.6
Keywords: ams2015 | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"c9f1a12925ef18d82b5f9683d589bb2c1e067650" c9f1a129]:
{{{
#!CommitTicketReference repository=""
revision="c9f1a12925ef18d82b5f9683d589bb2c1e067650"
Fixed #24013 -- Fixed escaping of reverse() prefix.
Prefix was treated as a part of the url pattern, which it is not.
Improved tests to conform with RFC 3986 which allows certain
characters in path segments without being escaped.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24013#comment:7>