Opened 12 years ago

Closed 11 years ago

Last modified 11 years ago

#18210 closed Bug (fixed)

Regression and crash with any "special" prefix values passed to reverse()

Reported by: Dan McGee Owned by: nobody
Component: Core (URLs) Version: 1.4
Severity: Release blocker Keywords: reverse
Cc: 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

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.

Attachments (1)

reverse_prefix_failing_tests.patch (1016 bytes ) - added by Dan McGee 12 years ago.
Patch with three new failing tests

Download all attachments as: .zip

Change History (12)

by Dan McGee, 12 years ago

Patch with three new failing tests

comment:1 by Dan McGee, 12 years ago

Changeset [17251] introduced the normalize() call in the reverse() function, which was related to ticket #15900.

comment:2 by Dan McGee, 12 years ago

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.

comment:3 by Aymeric Augustin, 12 years ago

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

Marking as a release blocker because this is a regression.

comment:4 by Gabriel Hurley, 11 years ago

A few things:

  1. Pull request to fix this here: https://github.com/django/django/pull/490
  1. 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.
  1. Normally the prefix argument to reverse is used internally to handle namespaces included 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.
  1. 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).

comment:5 by mrj, 11 years ago

Has patch: set

comment:6 by Aymeric Augustin, 11 years ago

toofishes: can you confirm that Gabriel's patch resolves your problem, given your ErrorDocument setup?

comment:7 by Aymeric Augustin, 11 years ago

Triage Stage: AcceptedReady for checkin

comment:8 by Gabriel Hurley <gabriel@…>, 11 years ago

Resolution: fixed
Status: newclosed

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

comment:9 by Jannis Leidel <jannis@…>, 11 years ago

In 233f86443c30580de24d4f302fa40ce9db5c0b9e:

Merge pull request #490 from gabrielhurley/reverse-prefix-special-chars

Fixed #18210 -- Escaped special characters in reverse prefixes.

comment:10 by Jannis Leidel <jannis@…>, 11 years ago

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

comment:11 by Preston Holmes <preston@…>, 11 years ago

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

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