Code

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#18495 closed Bug (invalid)

404 with non-/ WSGI, script prefix not removed in core/urlresolvers.py: resolve()

Reported by: django@… Owned by: nobody
Component: Core (URLs) Version: 1.4
Severity: Normal Keywords: 404, WSGIScriptAlias, wsgi, resolve, url, prefix
Cc: StalkR Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Hello,

I've just had a nasty WSGI bug with Django 1.4 while using a non-/ WSGIScriptAlias with apache2. I tracked down the bug and found a cause so here is a patch for review, if you like.

Sorry in advance if this is a known and fixed bug, but then please consider patching 1.4 stable release?

The bug

Let:

urlpatterns = patterns('', r'^bug/$', 'app.views.Bug')

Django runserver => everything OK

apache2 + WSGIScriptAlias / => everything OK

apache2 + WSGIScriptAlias /test => breaks

Details: request URL '/test/bug/', tried '^bug/$' did not match current URL 'bug/' - silly right? :) obviously this is not reporting the actual URL it was checked against, maybe something to fix to have a more meaningful error message.

A quick debug of WSGI reveals apache is properly giving all the info to Django: SCRIPT_NAME='/test', PATH_INFO='/bug/'.

Proposed fix

After a quick search it seems to be a known bug on the interwebs but solutions I've seen were ugly (write the prefix statically in the app).

Diving into Django I found core/urlresolvers.py with:

class RegexURLResolver(LocaleRegexProvider):
[...]
    def resolve(self, path):
[...]
            new_path = path[match.end():] 
            for pattern in self.url_patterns:

With request path '/test/bug/', new_path becomes 'test/bug/', and this obviously does not match against '^bug/$'.

Proposed fix: if present, remove get_script_prefix() from new_path (after applying the same regexp).

             new_path = path[match.end():]
+            # Get script prefix, apply regexp and remove from path if present.
+            prefix = get_script_prefix()
+            if prefix:
+                match = self.regex.search(prefix)
+                if match:
+                    prefix = prefix[match.end():]
+                    if new_path.startswith(prefix):
+                        new_path = new_path[len(prefix):]
             for pattern in self.url_patterns:

(patch attached)

Now everything works as expected, I hope I did not miss anything.

Let me know if you have any questions and thanks for providing this great framework :)

Cheers,

StalkR

Attachments (1)

urlresolvers_remove_prefix_from_path.diff (853 bytes) - added by django@… 2 years ago.
fix proposal #1 to strip script prefix in core/urlresolvers.py:resolve()

Download all attachments as: .zip

Change History (6)

Changed 2 years ago by django@…

fix proposal #1 to strip script prefix in core/urlresolvers.py:resolve()

comment:1 Changed 2 years ago by StalkR

  • Cc StalkR added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Just checked, this also affects git HEAD (1.5.dev20120619153728) as the code is the same.

The proposed patch applies on HEAD and fixes the issue there too.

comment:2 Changed 2 years ago by lukeplant

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

Having dug into the code, I'm finding it difficult to believe this bug report. The only thing passed to the URL resolving mechanism is 'request.path_info':

https://github.com/django/django/blob/4a103086d5c67fa4fcc53c106c9fdf644c742dd8/django/core/handlers/base.py#L102

This, in turn, comes straight from environ['PATH_INFO']:

https://github.com/django/django/blob/4a103086d5c67fa4fcc53c106c9fdf644c742dd8/django/core/handlers/wsgi.py#L127

And you are saying that PATH_INFO is sent correctly by Apache/modwsgi, so I can't see what could be going wrong. It's difficult to see how new_path could ever be 'test/bug/' as you claim.

The proposed fix is certainly wrong - we don't need the script name at all at this point in the URL resolving process, it shouldn't be a part of the path that is being matched at all.

The only possible bug I can see so far is that the debugging page is not reporting the 'actual URL' it was testing. However, this is a difficult thing to express concisely, since you've got different definitions of the 'actual URL' - the PATH_INFO that Django is matching against (/bug/), the full URL from the client (/test/bug/), and the URL minus the initial / (bug/) which is used in the error message, due to the fact that the RegexURLResolver matches this initial slash automatically. The message is as helpful as it can be in the normal case.

However, if you could supply a test that uses the test client (which itself uses the WSGIRequest class) that would allow us to verify what you are talking about, or some other analysis so that we can see what is going on, I will certainly believe you. Please do re-open if you can provide this information, I'm closing INVALID for now.

comment:3 Changed 2 years ago by StalkR

Thanks for the quick response. TL;DR: good news, it was my setup, sorry for the noise.

I tried to build a test client without success so I went back to my setup and code and eventually found the culprit: the 404 was triggered by a custom template tag I use to resolve the current URL to a view in order to know if a view is active, from a template.

Template tag:

{% active request 'app.views.Home' %}

Code:

@register.simple_tag
def active(request, view):
  if resolve(request.path).url_name == view:
    return 'active'
  else:
    return ''

The fix is obviously:

-  if resolve(request.path).url_name == view:
+  if resolve(request.path_info).url_name == view:

Also I'm not using reverse() because unfortunately some of my URLs regexp are not reversible because using conditional parameters.

So again, thanks and sorry for the noise.

How could we improve the debugging message? I really like the main exception handler giving you the entire call stack with locals etc. If this was also on 404, it would have helped me locate the bug in the template tags and not think it was in the main request handler. What do you think?

comment:4 Changed 2 years ago by lukeplant

I think this is probably a fairly obscure corner case, and it wouldn't be worth the complication to get 404 pages to include stack traces. Sorry!

comment:5 Changed 2 years ago by StalkR

Ok, I understand. Thanks for your time!

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.