Opened 8 years ago

Last modified 8 years ago

#27506 closed Cleanup/optimization

HttpRequest.build_absolute_uri throws DisallowedHost — at Version 2

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 (2)

comment:1 by Claude Paroz, 8 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, 8 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…)

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