[Django] #26037: HttpRequest._get_raw_host() uses either HTTP_X_FORWARDED_HOST or HTTP_X_FORWARDED_PORT => should use both
Django
HTTP_X_FORWARDED_PORT => should use both
-------------------------------+--------------------
Reporter: benoitbryon | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 1.9
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
Situation is Django running behind a reverse proxy such as:
* Django settings declare `USE_X_FORWARDED_HOST = True` and
`USE_X_FORWARDED_PORT = True`
* reverse proxy passes headers `X-Forwarded-Host` and `X-Forwarded-Port`.
Say host "example.com" and port "8080" for example.
I was expecting `request.get_absolute_uri()` to use both forwarded host
and port.
Or more precisely, I was expecting `request.get_host()` to return
"example.com:8080" with the example above.
But I get "example.com" only, without mention of the forwarded port.
As of Django version 1.9, it seems that, given
`settings.USE_X_FORWARDED_HOST` is True, then `request.get_host()` takes
only `X-Forwarded-Host` into account and ignores `X-Forwarded-Port`.
I guess issue comes from `HttpRequest._raw_host()` which doesn't use
`HttpRequest.get_port()` in the case `settings.USE_X_FORWARDED_HOST` is
True.
References:
* `HttpRequest.get_host()`:
https://github.com/django/django/blob/b0c56b895fd2694d7f5d4595bdbbc41916607f45/django/http/request.py#L72-L89
* `HttpRequest.get_port()`:
https://github.com/django/django/blob/b0c56b895fd2694d7f5d4595bdbbc41916607f45/django/http/request.py#L110-L116
* `settings.USE_X_FORWARDED_PORT` was introduced by
https://code.djangoproject.com/ticket/25211
--
Ticket URL: <https://code.djangoproject.com/ticket/26037>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
HTTP_X_FORWARDED_PORT => should use both
Reporter: benoitbryon | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 1.9
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by benoitbryon):
* needs_docs: => 0
* needs_better_patch: => 0
* needs_tests: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/26037#comment:1>
Django
HTTP_X_FORWARDED_PORT => should use both
Reporter: benoitbryon | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 1.9
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by benoitbryon):
In the case `X_FORWARDED_PORT` is considered a edge-case and not a
standard (see discussion at https://code.djangoproject.com/ticket/19517),
an alternative to resolve the use case would be mentioning both domain and
port in X_FORWARDED_HOST header, i.e. don't use X_FORWARDED_PORT at all.
Example:
* Django settings declare `USE_X_FORWARDED_HOST = True`
* Reverse proxy sets header `X_FORWARDED_HOST = "example.com:8080"`
* `request.get_host()` returns `example.com:8080`
If this approach is preferred, it would be nice to mention it in the
documentation, so that users know when to use
settings.USE_X_FORWARDED_HOST and settings.USE_X_FORWARDED_PORT. At the
moment, it is not obvious that we cannot use HOST and PORT at the same
time.
--
Ticket URL: <https://code.djangoproject.com/ticket/26037#comment:2>
Django
HTTP_X_FORWARDED_PORT => should use both
Reporter: benoitbryon | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* cc: mattrobenolt (added)
* component: Uncategorized => HTTP handling
Comment:
Matt, any feedback on this?
--
Ticket URL: <https://code.djangoproject.com/ticket/26037#comment:3>
Django
HTTP_X_FORWARDED_PORT => should use both
Reporter: benoitbryon | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by mattrobenolt):
Hm, this is a bit tricky.
The HTTP_HOST header typically includes the port number as well, so I'd
expect X-Forwarded-Host to include this information.
So in my opinion, X-Forwarded-Host would supersede the use of X-Forwarded-
Port.
But in the real world, I've never actually seen this.
If I were doing this in nginx, I'd have a rule like:
{{{
proxy_set_header X-Forwarded-Host $host;
}}}
And this would give you the value of the HTTP_HOST header from the client,
which would already include the port number since this is what's sent
along from clients.
I guess the argument could be made that the value of X-Forwarded-Port, in
theory, could be used to override this? For the case of multiple proxies
or something.
But again, I've never seen this case in practice.
I'd vote to just document that X-Forwarded-Host takes priority over X
-Forwarded-Port imo. If someone needs some other crazy logic, it's not
hard to implement your own. I see this most often done in middlewares
anyways to mutate the META dict to coerce everything to behave as they
want.
Now technically, in theory, you can construct an HTTP request that doesn't
include the port number in the HTTP_HOST, but this is going against the
spec: http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.23
Given that the spec requires this, I think it's fair to assume that X
-Forwarded-Host should as well contain the port number.
Overall, this is only a problem if the port specified in X-Forwarded-Host
differed from what was passed through to X-Forwarded-Port, and in which
case, I can't think of when that would happen.
So I'm open to hearing a scenario where this may happen.
--
Ticket URL: <https://code.djangoproject.com/ticket/26037#comment:4>
Django
HTTP_X_FORWARDED_PORT => should use both
Reporter: benoitbryon | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by benoitbryon):
+1 for documentation tells "X-Forwarded-Host takes priority over X
-Forwarded-Port". This would have saved me from trying to setup both
USE_X_FORWARDED_PORT and USE_X_FORWARDED_HOST.
+1 for documentation tells "X-Forwarded-Host may include port too", with a
mention to http://tools.ietf.org/html/rfc7239#page-7 or
http://tools.ietf.org/html/rfc7230#section-5.4.
I updated my setup...
* nginx with `proxy_set_header X-Forwarded-Host $host:$server_port;`
(removed `x-forwarded-port`)
* `settings.USE_X_FORWARDED_HOST = True` (removed `USE_X_FORWARDED_PORT`
=> False by default)
... and it just works as expected.
--
Ticket URL: <https://code.djangoproject.com/ticket/26037#comment:5>
Django
HTTP_X_FORWARDED_PORT => should use both
Reporter: benoitbryon | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.9
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* type: Bug => Cleanup/optimization
* component: HTTP handling => Documentation
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/26037#comment:6>
Django
HTTP_X_FORWARDED_PORT => should use both
Reporter: benoitbryon | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.9
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
Would there be any value in adding a system check for certain invalid
settings configurations?
--
Ticket URL: <https://code.djangoproject.com/ticket/26037#comment:7>
Django
HTTP_X_FORWARDED_PORT => should use both
Reporter: benoitbryon | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.9
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* has_patch: 0 => 1
Comment:
Meanwhile, a [https://github.com/django/django/pull/6373 documentation PR]
has also been proposed. I'm not sure if the explanation is sufficient.
Perhaps Benoit and Matt have some input.
--
Ticket URL: <https://code.djangoproject.com/ticket/26037#comment:8>
Django
settings
-------------------------------------+-------------------------------------
Reporter: benoitbryon | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Documentation | Version: 1.9
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/26037#comment:9>
Django
settings
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Documentation | Version: 1.9
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"5cda4677b3df1be971000ef27470d3efc308d3be" 5cda4677]:
{{{
#!CommitTicketReference repository=""
revision="5cda4677b3df1be971000ef27470d3efc308d3be"
Fixed #26037 -- Documented precedence of USE_X_FORWARDED_HOST/PORT
settings.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26037#comment:10>
Django
settings
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Documentation | Version: 1.9
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"1920d4d5a8d69a488b59c7c4cd4f80a514fe2df8" 1920d4d]:
{{{
#!CommitTicketReference repository=""
revision="1920d4d5a8d69a488b59c7c4cd4f80a514fe2df8"
[1.9.x] Fixed #26037 -- Documented precedence of USE_X_FORWARDED_HOST/PORT
settings.
Backport of 5cda4677b3df1be971000ef27470d3efc308d3be from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26037#comment:11>