Opened 12 years ago

Closed 12 years ago

Last modified 10 years ago

#19517 closed Bug (wontfix)

Add support for X-Forwarded-Port to HttpRequest.get_host when USE_X_FORWARDED_HOST is in use

Reported by: david_k_hess@… Owned by: nobody
Component: HTTP handling Version: 1.4
Severity: Normal Keywords: httprequest, get_host, X-Forwarded-Host, USE_X_FORWARDED_HOST
Cc: hirokiky@…, matt@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

The use case is an app served on ports 80, 443 (SSL) and 444 (SSL) served by gunicorn behind nginx. I'm sending X-Forwarded-Host and X-Forwarded-Port from nginx to gunicorn. Port 80 is open solely to redirect to 443 and both are open to the public internet. 444 is firewalled and reserved for the admin interface which is protected by this middleware component:

class ProtectAdminMiddleware(object):
    def process_request(self, request):
        if request.path.startswith("/admin") and request.META["HTTP_X_FORWARDED_PORT"] != "444":
            raise Http404

When turning on USE_X_FORWARDED_HOST, the code in HttpRequest.get_host does not take port into account:

    [...]
    if settings.USE_X_FORWARDED_HOST and (
        'HTTP_X_FORWARDED_HOST' in self.META):
        host = self.META['HTTP_X_FORWARDED_HOST']
    elif 'HTTP_HOST' in self.META:
        host = self.META['HTTP_HOST']
    [...]

Because of this, at least two things in the admin interface on port 444 don't function correctly; the CSRF token breaks saying there is a problem between referrer on 444 and token on 443 and you can't login. Also, redirects from say "/admin" on port 444 redirect to "/admin/" on 443 instead of staying on 444. That's just what I was able to reach with logins broken.

I monkey-patched HttpRequest.get_host to add support for X-Forwarded-Port when USE_X_FORWARDED_HOST is specified and it appears to have corrected these issues:

    [...]
    if settings.USE_X_FORWARDED_HOST and (
        'HTTP_X_FORWARDED_HOST' in self.META):
        host = self.META['HTTP_X_FORWARDED_HOST']
        if 'HTTP_X_FORWARDED_PORT' in self.META:
            server_port = str(self.META['HTTP_X_FORWARDED_PORT'])
            if server_port != (self.is_secure() and '443' or '80'):
                host = '%s:%s' % (host, server_port)
    elif 'HTTP_HOST' in self.META:
        host = self.META['HTTP_HOST']
    [...]

Change History (9)

comment:1 by Sharif Naas, 12 years ago

Easy pickings: set
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

comment:2 by Hiroki Kiyohara, 12 years ago

I found pull-request for this ticket here https://github.com/django/django/pull/619 written by mattrobenolt.

comment:3 by Hiroki Kiyohara, 12 years ago

Has patch: set

comment:4 by Hiroki Kiyohara, 12 years ago

Cc: hirokiky@… added

comment:5 by Jannis Leidel, 12 years ago

Resolution: wontfix
Status: newclosed

I'm rejecting this after a discussion during the 2013 sprint:

  • The use case of handing a SSL request with a load balancer is handled already by the SECURE_PROXY_SSL_HEADER setting.
  • HTTP_X_FORWARDED_PORT is not a standard header and this ticket hasn't produced compelling evidence that Django is breaking any spec.
  • Adding a new setting pair of HTTP_X_FORWARDED_PORT and USE_X_FORWARDED_PORT just for this edge case is a no-go, too.

In other words, the issue is an edge case that can be solved by a custom middleware in your application (e.g. by setting HTTP_HOST).

comment:6 by Matt Long, 10 years ago

To be fair, X-Forwarded-Host is also not in any spec or standard that I can find. It's added automatically by Apache's mod_proxy, but that seems to be the only place. Having special handling for one custom header without the other seems incorrect as there is currently no way (besides middleware to change HTTP_HOST as jezdez suggests) to accurately get the original request's host/port combo as the OP describes.

It seems reasonable to me to expect that HTTP_X_FORWARDED_PORT would also be respected by HttpRequest.get_host if USE_X_FORWARDED_HOST is enabled. Currently, I will also be opting to monkey patch get_host rather than add the (admittedly small) overhead of an additional middleware on all requests.

comment:7 by Matt Long, 10 years ago

Cc: matt@… added

comment:8 by Collin Anderson, 10 years ago

There's a new Forwarded: header that standardizes for, proto, host, and by, but not port. http://tools.ietf.org/html/rfc7239

If you want to discuss it further, please use the https://groups.google.com/d/forum/django-developers mailing list, as this is a closed ticket.

comment:9 by Horacio G. de Oro, 10 years ago

I have the same problem with bad redirects on the testing environment (where the non-standar 8443 port is used). I found a workaround for Nginx. Just add:

error_page 497 https://$host:8443$request_uri;

With that configuration, instead of the error message The plain HTTP request was sent to HTTPS port, your browser receives a redirect to the real URL. This is NOT a solution, this introduces an extra redirect, but worked for me (and it's better than modifying the aplication, since this is a problem I have in the testing environment only).

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