Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#27506 closed Cleanup/optimization (invalid)

HttpRequest.build_absolute_uri throws DisallowedHost

Reported by: Daniel Hahler Owned by: nobody
Component: HTTP handling Version: 1.10
Severity: Normal Keywords:
Cc: Carl Meyer, Florian Apolloner Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Tim Graham)

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?

Change History (4)

comment:1 by Claude Paroz, 7 years ago

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.

comment:2 by Tim Graham, 7 years ago

Cc: Carl Meyer Florian Apolloner added
Description: modified (diff)

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…)

comment:3 by Florian Apolloner, 7 years ago

Resolution: invalid
Status: newclosed

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.

Last edited 7 years ago by Florian Apolloner (previous) (diff)

comment:4 by Daniel Hahler, 7 years ago

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.

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