[Django] #27506: HttpRequest.build_absolute_uri throws DisallowedHost
Django
------------------------------------------------+------------------------
Reporter: Daniel Hahler | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: HTTP handling | Version: 1.10
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
I've noticed that using ``build_absolute_uri`` might throw
``DisallowedHost``, which looks like an unexpected side effect.
This is used for example by opbeat_python
(https://github.com/opbeat/opbeat_python/blob/1b1e299d84b902c362e5833d6990e61b80c4a29e/opbeat/contrib/django/client.py#L125,
called when capturing
(https://github.com/opbeat/opbeat_python/blob/1b1e299d84b902c362e5833d6990e61b80c4a29e/opbeat/contrib/django/client.py#L149))
to add additional information to a captured event.
This happens already during some exceptional state, where an extra
exception causes extra issues - another project being affected seems to be
django-slack (https://github.com/lamby/django-slack/issues/40).
I've not investigated much, but it looks like Django uses this as a
central place to handle/throw the `DisallowedHost` exception?!
Is there another API that should be used instead?
--
Ticket URL: <https://code.djangoproject.com/ticket/27506>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: Daniel Hahler | Owner: nobody
Cleanup/optimization |
Component: HTTP handling | Version: 1.10
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Claude Paroz):
It might make sense to add a `check_valid=True` default parameter to both
`get_host` and `build_absolute_uri`. Let's see what other developers
think.
--
Ticket URL: <https://code.djangoproject.com/ticket/27506#comment:1>
Django
-------------------------------------+-------------------------------------
Cleanup/optimization |
Component: HTTP handling | Version: 1.10
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 Tim Graham):
* cc: Carl Meyer, Florian Apolloner (added)
Old description:
> I've noticed that using ``build_absolute_uri`` might throw
> ``DisallowedHost``, which looks like an unexpected side effect.
>
> This is used for example by opbeat_python
> (https://github.com/opbeat/opbeat_python/blob/1b1e299d84b902c362e5833d6990e61b80c4a29e/opbeat/contrib/django/client.py#L125,
> called when capturing
> (https://github.com/opbeat/opbeat_python/blob/1b1e299d84b902c362e5833d6990e61b80c4a29e/opbeat/contrib/django/client.py#L149))
> to add additional information to a captured event.
>
> This happens already during some exceptional state, where an extra
> exception causes extra issues - another project being affected seems to
> be django-slack (https://github.com/lamby/django-slack/issues/40).
>
> I've not investigated much, but it looks like Django uses this as a
> central place to handle/throw the `DisallowedHost` exception?!
>
> Is there another API that should be used instead?
New description:
I've noticed that using `HttpRequest.build_absolute_uri()` might throw
`DisallowedHost`, which looks like an unexpected side effect.
This is used for example by opbeat_python
(https://github.com/opbeat/opbeat_python/blob/1b1e299d84b902c362e5833d6990e61b80c4a29e/opbeat/contrib/django/client.py#L125,
called when capturing
(https://github.com/opbeat/opbeat_python/blob/1b1e299d84b902c362e5833d6990e61b80c4a29e/opbeat/contrib/django/client.py#L149))
to add additional information to a captured event.
This happens already during some exceptional state, where an extra
exception causes extra issues - another project being affected seems to be
django-slack (https://github.com/lamby/django-slack/issues/40).
I've not investigated much, but it looks like Django uses this as a
central place to handle/throw the `DisallowedHost` exception.
Is there another API that should be used instead?
--
Comment:
I seem to remember having a similar thought that `HttpRequest` seemed like
an usual place to do that validation, but I never investigated if there
could be a better design.
Adding Carl to CC because he authored the addition of
`settings.ALLOWED_HOSTS` (d51fb74360b94f2a856573174f8aae3cd905dd35). Maybe
he remembers something about that. I found a comment from him in the
private security tracker for the patch that has this todo item:
Consider validating all requests instead of only when `get_host()` is
called, to make it less likely a misconfigured whitelist will be missed?
I'm now leaning away from this, because it seems cleaner to only do the
work when the value is requested, but I do think it's problematic that it
could be so easy to deploy and forget to configure `ALLOWED_HOSTS` until
someone tries to send a password reset email or something.
with Florian's reply:
> * `CommonMiddleware` checks the host every request.
> * CSRF middleware checks the host on every secure request.
> So essentially you are most likely checking it all the time already (So
I guess we could move that check up in the wsgi handling…)
--
Ticket URL: <https://code.djangoproject.com/ticket/27506#comment:2>
Django
-------------------------------------+-------------------------------------
Cleanup/optimization |
Component: HTTP handling | Version: 1.10
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 Florian Apolloner):
* status: new => closed
* resolution: => invalid
Comment:
> I've noticed that using HttpRequest.build_absolute_uri() might throw
DisallowedHost, which looks like an unexpected side effect.
I'd call it "works as designed" :D The main idea here is that you need
`get_host` and `build_absolute_uri` ''usually'' in cases where you are
sending emails or providing redirect urls for other sites/apis. And those
are generally the cases where you'd want to ensure that you do not allow
invalid hosts. So by default this is an important defense line and should
stay on.
To allow access to the unvetted URI we've added ''get_raw_uri'' which
should work in your case -- I am closing the ticket as invalid, please
reopen if you have any questions there.
--
Ticket URL: <https://code.djangoproject.com/ticket/27506#comment:3>
Django
-------------------------------------+-------------------------------------
Cleanup/optimization |
Component: HTTP handling | Version: 1.10
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Daniel Hahler):
Makes total sense now - thanks for the explanation.
I've found `get_raw_uri` also (just above!), and created a PR to fix it
for Opbeat: https://github.com/opbeat/opbeat_python/pull/143.
--
Ticket URL: <https://code.djangoproject.com/ticket/27506#comment:4>