Opened 19 years ago

Closed 16 years ago

Last modified 12 years ago

#285 closed defect (worksforme)

WSGIRequest should set request.path to full uri path

Reported by: rmunn@… Owned by: Malcolm Tredinnick
Component: HTTP handling Version: 1.0-beta
Severity: normal Keywords:
Cc: sam@…, remco@…, cwebber@…, djangotrac.285@…, richard.davies@…, Leo Soto M. Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Following the instructions at http://wiki.dreamhost.com/index.php/Django, I got Django running under FastCGI. But the login form points to the wrong URL: instead of pointing to http://django.example.com/django-admin.fcgi/admin/, it points to http://django.example.com/admin/.

The reason is found in django/core/handlers/wsgi.py, where the WSGIRequest class's __init__() sets self.path equal to environ['PATH_INFO']. The request.path value eventually makes its way to the login template, which uses "app_path" as the form submission URL. In a CGI environment (including FastCGI), PATH_INFO contains the path referenced *after* the script name (here, "/admin/"). SCRIPT_NAME contains the path to the script (here, "/django-admin.fcgi". To construct a proper self-referential URL, you need to concatenate SCRIPT_NAME and PATH_INFO to obtain, in this case, "/django-admin.fcgi/admin/".

Attachments (6)

285.patch (2.3 KB ) - added by Chris Beaven 17 years ago.
285.2.patch (1.9 KB ) - added by Chris Beaven 17 years ago.
script_name_path_info.diff (1.8 KB ) - added by jmelesky 17 years ago.
try and accomodate request.path appropriately in mod_python and under wsgi, add request.full_path for script_name plus path
script_name_path_info2.diff (15.4 KB ) - added by jmelesky 17 years ago.
corrected and far more comprehensive patch, against [6292]
script_name_path_info3.diff (19.6 KB ) - added by jmelesky 17 years ago.
another patch revision, against [6300], including docs and suggestions from Malcom
script_name_path_info4.diff (20.1 KB ) - added by jmelesky 17 years ago.
Another revision, against [6316]. This one actually passes the test suite.

Download all attachments as: .zip

Change History (46)

comment:1 by hugo <gb@…>, 19 years ago

I think that's more a bug in the FCGI description. It needs a rewrite rule that rewrites the URLs matching /admin/ to /django-admin.fcgi/admin/ so that the right PATH_INFO is passed in. You might have more luck with lighttpd+FCGI or Apache+FCGI, depending on the server you use.

comment:2 by rmunn@…, 19 years ago

I was looking through the code, and there are a lot of places that assume that request.path describes the entire URL. Changing that to a concatenation of SCRIPT_NAME and PATH_INFO would be quite tricky, because you'd need to distinguish between URL's destined for URL pattern matching (where you'd want to match against the PATH_INFO component only), and URL's destined for output in HTML (where you'd want to use the concatenation of SCRIPT_NAME and PATH_INFO). I'm starting to think that the complexity of the fix is not worth it when, as Hugo said, you can solve this one with a simple rewrite rule at the server level.

comment:3 by rmunn@…, 19 years ago

Resolution: wontfix
Status: newclosed

Closing ticket as "wontfix" since I've concluded that this should be solved with a rewrite rule at the server level.

comment:4 by Thomas Güttler, 17 years ago

According to http://www.ietf.org/rfc/rfc3875 (See 4.1.6.)

PATH_INFO of http://django.example.com/django-admin.fcgi/admin/ should bei /admin/

It is sad, that django does not handle his according to the RFC. If you cannot add a rewrite
rule, you can modify the PATH_INFO variable to include the path to the script. I did
this for django/core/servers/cgi.py (from Trac ticket). You need to modify your url.py, too.

comment:5 by Chris Beaven, 17 years ago

Component: Admin interfaceHTTP handling
Resolution: wontfix
Status: closedreopened
Summary: Admin view uses wrong login URL under FastCGIWSGIRequest should set request.path to full uri path
Triage Stage: UnreviewedAccepted

Well if the RFC says it, we should be doing it.

It's not difficult. Thomas' suggestion is close, but rather than modify PATH_INFO, it's better to just change request.path to concatenate SCRIPT_NAME and PATH_INFO. This way, the Django url resolver can still (with a small tweak) use PATH_INFO for it's resolving so nothing breaks.

Related tickets: #1516, #2407

by Chris Beaven, 17 years ago

Attachment: 285.patch added

comment:6 by Chris Beaven, 17 years ago

Has patch: set

Note: in debugging this, I noticed that ModPythonRequest wasn't actually setting PATH_INFO to the correct value!

It is currently using .path_info which according to the mod_python docs is "What follows after the file name, but is before query args, if anything".

It "sounds" like the right thing, but it isn't. Since we're not actually using SCRIPT_NAME in ModPythonRequest, it should just be set to .uri instead. This is a pretty big claim and I'm no expert, so maybe I'm totally wrong in which case someone please show me the flaw in my simplistic logic :)

by Chris Beaven, 17 years ago

Attachment: 285.2.patch added

comment:7 by anonymous, 17 years ago

Cc: sam@… added

in reply to:  3 comment:8 by jumo@…, 17 years ago

Replying to rmunn@pobox.com:

Closing ticket as "wontfix" since I've concluded that this should be solved with a rewrite rule at the server level.

this isn't a cool way to fix this bug, i seems indeed very stupid for django beginners wich had some hours of trouble caused by it.

comment:9 by Chris Beaven, 17 years ago

The ticket has been reopened, so obviously the "wontfix" doesn't apply now.

comment:10 by jmelesky, 17 years ago

Owner: changed from nobody to jmelesky
Status: reopenednew

by jmelesky, 17 years ago

Attachment: script_name_path_info.diff added

try and accomodate request.path appropriately in mod_python and under wsgi, add request.full_path for script_name plus path

comment:11 by jmelesky, 17 years ago

Status: newassigned

This patch was made against [6230] and has not been fully tested.

In order to accomodate mod_python, it has a new conf directive:

PythonOption django.root /base/django/uri

So, if, for example, you're trying to run django in location /blog, you'd have something like:

<Location /blog>

... blah blah ...
PythonOption django.root /blog

</Location>

I'll attach a doc patch after i do some more testing.

comment:12 by jmelesky, 17 years ago

Oh, and this is being worked on as part of the Sprint14Sep

by jmelesky, 17 years ago

Attachment: script_name_path_info2.diff added

corrected and far more comprehensive patch, against [6292]

comment:13 by jmelesky, 17 years ago

Needs documentation: set

The prior patch was both limited and wrong. This patch corrects the bug and expands the expected behavior to include request.get_full_path(), as well as updating a number of internal references from request.path to request.full_path. It has been tested against mod_python, mod_wsgi on apache, and against mod_[fs]cgi on lighty.

I don't, however, know if this is enough. We may also need to update the behavior of {% url %}.

Also, though this is a big change, for existing mod_python installs it is not actually backwards-incompatible. Leaving out the PythonOption django.root should retain the original behavior.

by jmelesky, 17 years ago

Attachment: script_name_path_info3.diff added

another patch revision, against [6300], including docs and suggestions from Malcom

by jmelesky, 17 years ago

Attachment: script_name_path_info4.diff added

Another revision, against [6316]. This one actually passes the test suite.

comment:14 by jmelesky, 17 years ago

Needs documentation: unset
Version: SVN

There is one problem of note, here. If you're running, say, FastCGI, the SCRIPT_NAME gets populated back as 'blahblah.fcgi'. This wasn't an issue before. Now, though, redirects will point back to '/blahblah.fcgi/blah/blah' instead of '/blah/blah'. This will likely still work (unless the server is rewriting everything), but produces ugly and unexpected URLs.

Which points out a fundamental issue: SCRIPT_NAME + PATH_INFO is not the same thing as the URI that the user sees. I think more and more that these are related, but separate problems.

We end up with three distinct pieces of information:

  1. The absolute URL prefix (Apache's <Location> directive, "/mysite")
  2. The name of the callable thing (SCRIPT_NAME, "/user/django.fcgi")
  3. The remainder of the path ("/blog/tags/Sprint14Sep")

The first two might be different due to server URL rewriting.

The question, then, is: Do we want to support all three pieces of information? Because if we do, my patch is insufficient. I'm happy to continue work on this if y'all want to move in that direction, but this needs more design decision.

comment:15 by jmelesky, 17 years ago

Triage Stage: AcceptedDesign decision needed

comment:16 by Brian Rosner, 16 years ago

#5576 marked as a duplicate.

comment:17 by anonymous, 16 years ago

Cc: remco@… added

comment:18 by Jacob, 16 years ago

Triage Stage: Design decision neededReady for checkin

Patch needs review (which Malcolm says he'll do), but otherwise this looks great.

comment:19 by Malcolm Tredinnick, 16 years ago

Owner: changed from jmelesky to Malcolm Tredinnick
Status: assignednew

comment:20 by Rob Hudson <treborhudson@…>, 16 years ago

I just got bit by this and wanted to verify what SmileyChris saw with WSGI vs modpython, that req.uri under mod_python is the path and not req.path_info. Looks like this patch will fix it in this small case. Thanks.

comment:21 by cwebber@…, 16 years ago

Cc: cwebber@… added

This patch is great, but it doesn't actually set the path in the wsgi request. Maybe this works with mod_python, but in our scgi environment at my work I'm not seeing it take any effect.

It appears to me that in wsgi.py after self.full_path is set up that this should actually become self.path, correct?

IE, afaict for this to take any effect, WSGIRequest's init_ should look like this:

class WSGIRequest(http.HttpRequest):
    def __init__(self, environ):
        self.environ = environ
        self.path = force_unicode(environ.get('PATH_INFO', '/'))
        self.full_path = (force_unicode(environ.get('SCRIPT_NAME', ''))
                          + force_unicode(environ.get('PATH_INFO', '/')))
        self.META = environ
        self.META['PATH_INFO'] = self.path
        self.META['SCRIPT_NAME'] = force_unicode(environ.get('SCRIPT_NAME', ''))
        self.method = environ['REQUEST_METHOD'].upper()
        self.path = self.full_path  ### <-- the added line

comment:22 by Thomas Güttler, 16 years ago

Cc: hv@… added

comment:23 by ruddick@…, 16 years ago

I applied these changes, and they mostly fixed the problem, but the links in the upper right corner of the admin page (Documentation, Change Password, Log out) still do not use the full path. I do not know enough to fix the problem, so I will leave it to your expertise.

in reply to:  18 comment:24 by Marc Fargas, 16 years ago

Triage Stage: Ready for checkinAccepted

Replying to jacob:

Patch needs review (which Malcolm says he'll do), but otherwise this looks great.

As Malcolm is currently "offline" I'm moving this back to Accepted to clean up the "RFC" list (Ready For Checkin")

comment:25 by Jacob, 16 years ago

milestone: 1.0 alpha

comment:26 by gsf@…, 16 years ago

Is #3414 a duplicate of this ticket?

comment:27 by Malcolm Tredinnick, 16 years ago

I'm reviewing this in detail today and I'm going to change something fundamental in the interests of maintaining as much backwards-compatibility as possible. Instead of full_path and path, we'll have path (which is SCRIPT_NAME and PATH_INFO) and path_info. That means references to request.path will still be the right thing. This will reduce the amount of porting required almost everywhere (you have to fix your root URLConf once -- and even that's optional with mod_python -- and things will Just Work(tm)).

I want to work out something around the fact that some FastCGI implementations might be broken, according to jmelesky's comment 14. SCRIPT_NAME + PATH_INFO + QUERY_STRING should be the submitted URL; if it isn't, part of the web server pipeline is setting environment variables incorrectly and we'll have to work around that. That and some testing of the mod_python comments people have brought up are the blockers for me right now and I'm working on that this weekend. So this is back on the front burner.

comment:28 by CHasenpflug, 16 years ago

Cc: djangotrac.285@… added

comment:29 by bronger@…, 16 years ago

Cc: bronger@… added

comment:30 by Richard Davies <richard.davies@…>, 16 years ago

Cc: richard.davies@… added

I want to make sure that this thread is aware of one use case, which is unusually behaved, but should be supported.

I run Django with Lighttpd, using the error-handler-404 mechanism borrowed from the standard Rails config for this web server (http://github.com/rails/rails/tree/master/railties/configs/lighttpd.conf)

When run in this manner, Lighttpd sets only REQUEST_URI and SCRIPT_NAME (to the handler!), but neither PATH_INFO nor QUERY_STRING (http://trac.lighttpd.net/trac/wiki/FrequentlyAskedQuestions#Whatkindofenvironmentdoesserver.error-handler-404setup)

I have just submitted a patch against #3414 which builds both PATH_INFO and QUERY_STRING out of REQUEST_URI if they are missing in WSGIHandler._init_ . It looks like similar code is probably needed in the eventual patch generated from this thread?

comment:31 by Leo Soto M., 16 years ago

Cc: Leo Soto M. added

comment:32 by Malcolm Tredinnick, 16 years ago

Resolution: fixed
Status: newclosed

(In [8015]) Changed/fixed the way Django handles SCRIPT_NAME and PATH_INFO (or
equivalents). Basically, URL resolving will only use the PATH_INFO and the
SCRIPT_NAME will be prepended by reverse() automatically. Allows for more
portable development and installation. Also exposes SCRIPT_NAME in the
HttpRequest instance.

There are a number of cases where things don't work completely transparently,
so mod_python and fastcgi users should read the relevant docs.

Fixed #285, #1516, #3414.

in reply to:  description comment:33 by anonymous, 16 years ago

maybe FORCE_SCRIPT_NAME = will help

comment:34 by allo, 16 years ago

Resolution: fixed
Status: closedreopened
Version: SVN1.0-beta-1

This is broken in 1.0 beta again: my admin-loginform now points to /mysite.fcgi/admin/, which is wrong, it should be /admin/.
My setup is a lighttpd with fastcgi and rewrite from /(.*) to /mysite.fcgi$1
it would be cool if this can be fixed soo.

in reply to:  34 comment:35 by Richard Davies <richard.davies@…>, 16 years ago

Resolution: worksforme
Status: reopenedclosed

Have you tried

FORCE_SCRIPT_NAME = ''

in your settings.py? That will almost certainly fix your issue - it fixed the same problem for me when updating to more recent Django.

I'm reclosing this as "worksforme". You can reopen if you still have the problem...

Replying to allo:

This is broken in 1.0 beta again: my admin-loginform now points to /mysite.fcgi/admin/, which is wrong, it should be /admin/.
My setup is a lighttpd with fastcgi and rewrite from /(.*) to /mysite.fcgi$1
it would be cool if this can be fixed soon.

comment:36 by allo, 16 years ago

i tried FORCE_SCRIPT_NAME='/mysite.fcgi' and without /, because i thought it should filter the prefix. now it works, thanks.

comment:37 by Malcolm Tredinnick, 16 years ago

Please don't reopen this ticket no matter what the problem is here. The root problem has been fixed. You are seeing something that is a consequence of the solution, but that's just needing a tweak to the solution (if it's not a config issue on your end). If that's the case, it's a new ticket, not this one.

comment:38 by anonymous, 16 years ago

Cc: hv@… removed

comment:39 by Torsten Bronger <bronger@…>, 16 years ago

Cc: bronger@… removed

comment:40 by Jacob, 12 years ago

milestone: 1.0 alpha

Milestone 1.0 alpha deleted

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