[Django] #18210: Regression and crash with any "special" prefix values passed to reverse()
Django
-----------------------------+---------------------
Reporter: toofishes | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: 1.4
Severity: Normal | Keywords: reverse
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+---------------------
After updating to Django 1.4, I get no fewer than 5 messages a day where
the Django 404 page generation gets totally fouled up and ends up
resulting in a 500 server error. The common thread here was these URLs
arrived via the Apache ErrorDocument route. I'm running under mod_wsgi,
and I narrowed it down the attached test cases to show the broken
behavior. Applying this to the 1.3.X branch results in all 3 new tests
passing, but on 1.4.X and trunk all three tests fail in related but
different ways.
The high level reason has to to with some of the crazy PATH_INFO,
SCRIPT_NAME, and SCRIPT_URL usage Django is doing, from what I can tell.
In the ErrorDocument situation, the SCRIPT_URL envvar is not set to be the
WSGI script; instead, it remains set to the original missing URI
(something such as '/static/magazine/2010/ALM-2010-Feb/bump%20map.png').
This causes all sorts of issues because PATH_INFO is much shorter (in my
case, it gets rewritten to '/404').
I'm not sure how critical this bug is, but it is extremely trivial to
cause Django to 500 under any ErrorDocument setup at the moment- if one
includes a '{', ')', or '%' character in the URL they are requesting that
ends up getting handled via ErrorDocument, the application will error 100%
of the time as stands, from what I can tell.
All of the normalize(prefix) stuff in reverse() appears to be new in 1.4,
and that is where all three of these failures can be traced back to.
--
Ticket URL: <https://code.djangoproject.com/ticket/18210>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Type: Bug | Status: new
Component: Core (URLs) | Version: 1.4
Needs tests: 0 | Patch needs improvement: 0
Changes (by toofishes):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
Changeset [17251] introduced the normalize() call in the reverse()
function, which was related to ticket #15900.
--
Ticket URL: <https://code.djangoproject.com/ticket/18210#comment:1>
Django
Type: Bug | Status: new
Component: Core (URLs) | Version: 1.4
Comment (by toofishes):
And a very telling comment from #15900:
> Since most people don't have regex special characters in the prefix to
namespaced urls, it wasn't a problem.
--
Ticket URL: <https://code.djangoproject.com/ticket/18210#comment:2>
Django
Type: Bug | Status: new
Component: Core (URLs) | Version: 1.4
Keywords: reverse | Triage Stage: Accepted
Changes (by aaugustin):
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
Comment:
Marking as a release blocker because this is a regression.
--
Ticket URL: <https://code.djangoproject.com/ticket/18210#comment:3>
Django
Reporter: toofishes | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: 1.4
Keywords: reverse | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by gabrielhurley):
A few things:
1. Pull request to fix this here:
https://github.com/django/django/pull/490
2. While I agree this was a regression and that the exceptions have to
be fixed, I believe the 1.3-era output (which your attached tests reflect)
is also wrong for cases 2 and 3. The first case with the curly braces
escaped is the correct behavior, and the other two cases should reflect
the URLs with the special characters escaped as well. The tests in the
pull request reflect that.
3. Normally the `prefix` argument to reverse is used internally to
handle namespaces `include`d within one another wherein it'd be very hard
to get special regex characters into that prefix. That doesn't mean this
isn't an issue, only that it's an uncommon (and in fact undocumented)
thing to do. In the internal case the value of prefix is always processed
by `normalize` before being passed in and has to have originated from a
valid regex anyhow.
3. The intention of the quote "Since most people don't have regex
special characters in the prefix to namespaced urls, it wasn't a problem"
in the original context was to say that the bug reported in #15900 hadn't
been discovered because nesting captured groups via multiple namespace-
included URLconfs was uncommon. Those captured groups were the "regex
special characters" in question and they weren't being treated as part of
the regex URL pattern at all. This issue is effectively the reverse of
that one where unintended special characters are being treated as part of
the regex (caveat see point #2).
--
Ticket URL: <https://code.djangoproject.com/ticket/18210#comment:4>
Django
Reporter: toofishes | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: 1.4
Keywords: reverse | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by mrj):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/18210#comment:5>
Django
Reporter: toofishes | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: 1.4
Keywords: reverse | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
toofishes: can you confirm that Gabriel's patch resolves your problem,
given your ErrorDocument setup?
--
Ticket URL: <https://code.djangoproject.com/ticket/18210#comment:6>
Django
Reporter: toofishes | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: 1.4
Keywords: reverse | Triage Stage: Ready for
Has patch: 1 | checkin
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by aaugustin):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/18210#comment:7>
Django
Reporter: toofishes | Owner: nobody
Type: Bug | Status: closed
Component: Core (URLs) | Version: 1.4
Keywords: reverse | Triage Stage: Ready for
Has patch: 1 | checkin
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"90e530978d590a5bdcf75525aa03f844766018b8"]:
{{{
#!CommitTicketReference repository=""
revision="90e530978d590a5bdcf75525aa03f844766018b8"
Fixed #18210 -- Escaped special characters in reverse prefixes.
Ensured that special characters passed in to reverse via the
prefix argument are properly escaped so that calls to
django.utils.regex_helpers.normalize and/or string formatting
operations don't result in exceptions.
Thanks to toofishes for the error report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/18210#comment:8>
Django
Reporter: toofishes | Owner: nobody
Type: Bug | Status: closed
Component: Core (URLs) | Version: 1.4
Keywords: reverse | Triage Stage: Ready for
Has patch: 1 | checkin
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jannis Leidel <jannis@…>):
In [changeset:"233f86443c30580de24d4f302fa40ce9db5c0b9e"]:
{{{
#!CommitTicketReference repository=""
revision="233f86443c30580de24d4f302fa40ce9db5c0b9e"
Merge pull request #490 from gabrielhurley/reverse-prefix-special-chars
Fixed #18210 -- Escaped special characters in reverse prefixes.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/18210#comment:9>
Django
Reporter: toofishes | Owner: nobody
Type: Bug | Status: closed
Component: Core (URLs) | Version: 1.4
Keywords: reverse | Triage Stage: Ready for
Has patch: 1 | checkin
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jannis Leidel <jannis@…>):
In [changeset:"dd740e2b2e2780cc5b4357f1cd9b62f830945ec4"]:
{{{
#!CommitTicketReference repository=""
revision="dd740e2b2e2780cc5b4357f1cd9b62f830945ec4"
[1.5.x] Fixed #18210 -- Escaped special characters in reverse prefixes.
Ensured that special characters passed in to reverse via the
prefix argument are properly escaped so that calls to
django.utils.regex_helpers.normalize and/or string formatting
operations don't result in exceptions.
Thanks to toofishes for the error report.
Backport of 90e530978d590a5bdcf75525aa03f844766018b8 from master.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/18210#comment:10>
Django
Reporter: toofishes | Owner: nobody
Type: Bug | Status: closed
Component: Core (URLs) | Version: 1.4
Keywords: reverse | Triage Stage: Ready for
Has patch: 1 | checkin
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Preston Holmes <preston@…>):
In [changeset:"72ebdd3507c4304a845bc840fcfe6fa89c11d7a5"]:
{{{
#!CommitTicketReference repository=""
revision="72ebdd3507c4304a845bc840fcfe6fa89c11d7a5"
Fixed #18210 -- Escaped special characters in reverse prefixes.
Ensured that special characters passed in to reverse via the
prefix argument are properly escaped so that calls to
django.utils.regex_helpers.normalize and/or string formatting
operations don't result in exceptions.
Thanks to toofishes for the error report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/18210#comment:11>