Opened 12 years ago

Closed 9 years ago

#17092 closed Cleanup/optimization (fixed)

Internal Redirects with Apache

Reported by: anonymous Owned by: nobody
Component: HTTP handling Version: 1.6
Severity: Normal Keywords:
Cc: leanmeandonothingmachine Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

The CGI specification allows for a CGI script to return a 'Location' header
which refers to a location within the local web server. In Apache this is honoured when the Status returned by the CGI script is also 200. mod_wsgi also supports this. See http://code.google.com/p/modwsgi/issues/detail?id=14.

[6164] added the host to all redirects. While this is good for 99.9% of use cases, this makes these sorts of internal redirects impossible with django. Would it be reasonable to change fix_location_header to only add the host if the response code is not 200? Or we could add a variable such as Response.add_host_to_location that defaults to True that could be used to control this behavior on a per response basis. I'd be happy to work on the path for this once I hear back which implementation would be more likely to be accepted.

Attachments (4)

add_host_to_location.diff (4.1 KB ) - added by anonymous 12 years ago.
add_host_to_location.2.diff (4.1 KB ) - added by anonymous 12 years ago.
add_host_to_location.3.diff (4.1 KB ) - added by anonymous 12 years ago.
add_host_to_location.4.diff (3.6 KB ) - added by anonymous 10 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 by leanmeandonothingmachine, 12 years ago

Cc: leanmeandonothingmachine added

comment:2 by Preston Holmes, 12 years ago

Resolution: wontfix
Status: newclosed
Triage Stage: UnreviewedAccepted

One can not assume that all responses Django will serve will be going through a CGI spec compliant interface. So it does not make sense to make this change in core.

There are a number of other limitations with the feature, one being that the request_method is converted to GET and any request content is lost. If the redirect is just to static content, there is likely something better that can be done with mod_rewrite, or in a real edge case, one could put together django middleware that would strip out the host portion before returning.

comment:3 by anonymous, 12 years ago

This cannot be done with middleware. apply_response_fixes gets called after middleware has already had it's shot at the response. Mod_rewrite doesn't help when you want django to do some checks and only conditionally pass the internal redirect response. Leaving the only possible option an ugly hack.

request.get_host = lambda: ''

While we shouldn't assume that all responses will support the CGI spec, it also doesn't seem right to me to break a widely used standard such as CGI in core with no clean way to get the standard behavior.

I'm leaving as closed but in case any core devs reconsider I would still be happy to contribute a patch that would allow a clean way to get this behavior.

comment:4 by Carl Meyer, 12 years ago

Resolution: wontfix
Status: closedreopened

Given that both CGI and mod_wsgi support this, I think that Django should provide a clean way to return a host-free Location header (not by default, but by request). Not entirely sure what that ought to look like, but a flag on HttpResponse sounds as reasonable as anything.

by anonymous, 12 years ago

Attachment: add_host_to_location.diff added

comment:5 by anonymous, 12 years ago

Has patch: set

by anonymous, 12 years ago

Attachment: add_host_to_location.2.diff added

by anonymous, 12 years ago

Attachment: add_host_to_location.3.diff added

comment:6 by anonymous, 12 years ago

Anyone have any feedback on add_host_to_location.3.diff.

comment:7 by Aymeric Augustin, 11 years ago

Status: reopenednew

comment:8 by Tomek Paczkowski, 10 years ago

Patch needs improvement: set

I could not apply patch. I'm getting this:

$ curl https://code.djangoproject.com/raw-attachment/ticket/17092/add_host_to_location.3.diff | git apply
fatal: corrupt patch at line 97

by anonymous, 10 years ago

Attachment: add_host_to_location.4.diff added

comment:9 by anonymous, 10 years ago

Patch needs improvement: unset
Version: 1.31.6

After 3 years a patch goes stale, updated it. Lets see if it takes another three years for someone to look at it.

comment:10 by Aymeric Augustin, 10 years ago

I think this asks the broader question of Django's unconditional fixes. We recently removed some fixes for IE 4-6 and Netscape 4.

Rather than tainting the HttpResponse API with a kwarg for each unconditional "fix" (or "break"), shouldn't we move them to the CommonMiddleware?

comment:11 by anonymous, 10 years ago

I would agree with that, but before putting the time for that sort of re-factor, i'd like to make sure it's going to get accepted.

Time for django-developers i guess.

comment:12 by Aymeric Augustin, 10 years ago

Yes, django-developers it is.

comment:13 by Tim Graham, 10 years ago

Patch needs improvement: set

Reading the django-developers thread it seems like a different solution is desired.

comment:14 by Collin Anderson, 9 years ago

This should in theory get fixed by #23960

comment:15 by Claude Paroz, 9 years ago

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top