Code

Opened 2 years ago

Closed 17 months ago

Last modified 17 months ago

#18210 closed Bug (fixed)

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

Reported by: toofishes 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 toofishes 2 years ago.
Patch with three new failing tests

Download all attachments as: .zip

Change History (12)

Changed 2 years ago by toofishes

Patch with three new failing tests

comment:1 Changed 2 years ago by toofishes

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

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

comment:3 Changed 22 months ago by aaugustin

  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Accepted

Marking as a release blocker because this is a regression.

comment:4 Changed 18 months ago by gabrielhurley

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 Changed 18 months ago by mrj

  • Has patch set

comment:6 Changed 17 months ago by aaugustin

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

comment:7 Changed 17 months ago by aaugustin

  • Triage Stage changed from Accepted to Ready for checkin

comment:8 Changed 17 months ago by Gabriel Hurley <gabriel@…>

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

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 Changed 17 months ago by Jannis Leidel <jannis@…>

In 233f86443c30580de24d4f302fa40ce9db5c0b9e:

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

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

comment:10 Changed 17 months ago by Jannis Leidel <jannis@…>

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 Changed 17 months ago by Preston Holmes <preston@…>

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.

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.