Opened 4 years ago

Closed 4 months 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 4 years ago.
add_host_to_location.2.diff (4.1 KB) - added by anonymous 4 years ago.
add_host_to_location.3.diff (4.1 KB) - added by anonymous 4 years ago.
add_host_to_location.4.diff (3.6 KB) - added by anonymous 12 months ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 4 years ago by leanmeandonothingmachine

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

comment:2 Changed 4 years ago by ptone

  • Resolution set to wontfix
  • Status changed from new to closed
  • Triage Stage changed from Unreviewed to Accepted

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 Changed 4 years ago by anonymous

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 Changed 4 years ago by carljm

  • Resolution wontfix deleted
  • Status changed from closed to reopened

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.

Changed 4 years ago by anonymous

comment:5 Changed 4 years ago by anonymous

  • Has patch set

Changed 4 years ago by anonymous

Changed 4 years ago by anonymous

comment:6 Changed 4 years ago by anonymous

Anyone have any feedback on add_host_to_location.3.diff.

comment:7 Changed 2 years ago by aaugustin

  • Status changed from reopened to new

comment:8 Changed 14 months ago by oinopion

  • 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

Changed 12 months ago by anonymous

comment:9 Changed 12 months ago by anonymous

  • Patch needs improvement unset
  • Version changed from 1.3 to 1.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 Changed 12 months ago by aaugustin

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 Changed 12 months ago by anonymous

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 Changed 12 months ago by aaugustin

Yes, django-developers it is.

comment:13 Changed 11 months ago by timgraham

  • Patch needs improvement set

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

comment:14 Changed 4 months ago by collinanderson

This should in theory get fixed by #23960

comment:15 Changed 4 months ago by claudep

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.
Back to Top