Code

Opened 6 years ago

Closed 18 months ago

#9064 closed Bug (wontfix)

Redirect is broken when HTTP_X_FORWARDED_HOST contains multiple hosts

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

Description

HttpRequest.getHost uses HTTP_X_FORWARDED_HOST as a primary method of acquiring the host name of the server but does not take into account that it can be a list of multiple hosts separated by comma. Only the first host should be used if the header contains several.

Attached a patch for separating at the first comma if multiple hosts are listed.

Attachments (1)

multiple_proxies.patch (547 bytes) - added by Artur 6 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 6 years ago by tommycli@…

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement set

The current patch seems to work. It's meant to patch against trunk/django/http/__init__.py .

However, the patch doesn't specify which file it's applied to. Please resubmit with the proper directory hierarchy information.

comment:2 Changed 6 years ago by kratorius

  • Component changed from Uncategorized to HTTP handling
  • Has patch set

Changed 6 years ago by Artur

comment:3 Changed 6 years ago by Artur

Fixed directory hierarchy information in patch

comment:4 Changed 5 years ago by ericholscher

  • Triage Stage changed from Unreviewed to Accepted

I remember being bitten by this as well. Marking it as accepted because if some agents are forwarding these headers and it's breaking, we should probably take it into consideration.

When searching for similar tickets, I found a ruby on rails ticket dealing with the same issues: http://dev.rubyonrails.org/ticket/3397

It appears they are honoring the last host. It also seems to be happening when there are multiple proxies in front of Django.

comment:5 Changed 5 years ago by mtredinnick

We really shouldn't be using this header to compute redirects at all. if the hostname has been changed as part of the transmission process, it's up to the servers doing the changing to do rewrites on the way out as well. The best solution here is to ditch it altogether. Systems relying on this are arguably broken. Not holding my breath, though, since some people seem to like.

Note, also, there was a huge thread about the complete lack of standardisation and variance in behaviour for the X-FORWARDED-HOST header two or three years ago on django-dev. Here's one thread, although I have a memory there was another as well at some point. Unsurprising that it's confusing, since it's an "X-*" header (totally non-standard).

comment:6 Changed 4 years ago by louis

Ran into problem myself. Using django (1.1.1 final), mod_python, apache2 and nginx (as proxy).
The previous patch didn't work in my case but below did:

LINE 45:
    def get_host(self):
        """Returns the HTTP host using the environment or request headers."""
        # We try three options, in order of decreasing preference.
        if 'HTTP_X_FORWARDED_HOST' in self.META:
            host = self.META['HTTP_X_FORWARDED_HOST']
            host = host.split(',')[-1].strip()
        elif 'HTTP_HOST' in self.META:
            host = self.META['HTTP_HOST']
            host = host.split(',')[-1].strip()
        else:
            # Reconstruct the host using the algorithm from PEP 333.
            host = self.META['SERVER_NAME']
            host = host.split(',')[-1].strip()
            server_port = str(self.META['SERVER_PORT'])
            if server_port != (self.is_secure() and '443' or '80'):
                host = '%s:%s' % (host, server_port)
        return host

comment:7 Changed 3 years ago by aaugustin

The question here is "how to use X-FORWARDED-HOST?", but #6880 asks "should we use X-FORWARDED-HOST at all?" and contains a patch to not use it anymore. If this patch is applied, the problem discussed here vanishes.

get_host is the only place in Django that uses the non-standard X-FORWARDED-HOST header. I think removing it is the correct way to go:

  • Like mtredinnick said, reverse proxies are supposed to do the rewrites both on the request and the response.
  • Developers with a weird proxy setup may write their own middleware to do tweak headers.
  • People using the remote address for geolocation are facing very difficult problem, for which Django can not provide a generic solution. Consider the setup below: you would probably want the address of the Internet-facing proxy; how is Django supposed to find it in the list of 5 IPs in X-FORWARDED-HOST?
    user on private network -> internal proxy -> Internet-facing proxy -----> load-balancer -> reverse proxy -> Django
    

Finally, even if a decent solution was found for this problem, there would still be a parallel issue with is_secure (see also #6548) as proxies can convert from HTTP to HTTPS and vice-versa. I do not know how that one could be resolved.

comment:8 Changed 3 years ago by lukeplant

  • Severity set to Normal
  • Type set to Bug

comment:9 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:10 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:11 Changed 18 months ago by aaugustin

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

Django no longer interprets X-Forwarded-Host headers by default, for security reasons. See also #6880.

Generally speaking this header isn't necessary if proxies under the owner's control are properly configured.

So I'm going to close this ticket, essentially for the reasons given by Malcolm in comment 5.

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.