Skip to content

Avoid Django's ALLOWED_HOSTS check #3651

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## Unreleased

- `opentelemetry-instrumentation-django`: Fix `_DjangoMiddleware` to avoid hitting a host check ([#3651](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3651))

### Fixed

- `opentelemetry-instrumentation`: Fix dependency conflict detection when instrumented packages are not installed by moving check back to before instrumentors are loaded. Add "instruments-any" feature for instrumentations that target multiple packages.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ def process_request(self, request):
# Read more about request.META here:
# https://docs.djangoproject.com/en/3.0/ref/request-response/#django.http.HttpRequest.META

if self._excluded_urls.url_disabled(request.build_absolute_uri("?")):
if self._url_is_disabled(request):
return

is_asgi_request = _is_asgi_request(request)
Expand Down Expand Up @@ -305,7 +305,7 @@ def process_request(self, request):
def process_view(self, request, view_func, *args, **kwargs):
# Process view is executed before the view function, here we get the
# route template from request.resolver_match. It is not set yet in process_request
if self._excluded_urls.url_disabled(request.build_absolute_uri("?")):
if self._url_is_disabled(request):
return

if (
Expand All @@ -330,7 +330,7 @@ def process_view(self, request, view_func, *args, **kwargs):
duration_attrs[HTTP_ROUTE] = route

def process_exception(self, request, exception):
if self._excluded_urls.url_disabled(request.build_absolute_uri("?")):
if self._url_is_disabled(request):
return

if self._environ_activation_key in request.META.keys():
Expand All @@ -340,7 +340,7 @@ def process_exception(self, request, exception):
# pylint: disable=too-many-locals
# pylint: disable=too-many-statements
def process_response(self, request, response):
if self._excluded_urls.url_disabled(request.build_absolute_uri("?")):
if self._url_is_disabled(request):
return response

is_asgi_request = _is_asgi_request(request)
Expand Down Expand Up @@ -453,6 +453,15 @@ def process_response(self, request, response):

return response

def _url_is_disabled(self, request):
"""
Avoid `request.get_host` to bypass Django's `ALLOWED_HOSTS` check
"""
url = "{}://{}{}".format(
request.scheme, request._get_raw_host(), request.path
)
Comment on lines +460 to +462
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this break anyone who was relying on the previous behavior of request.build_absolute_uri("?")

Copy link
Author

@jourdanrodrigues jourdanrodrigues Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my experience (not proof of anything, more as an example), host checks are intentional, not an accidental feature of any particular lib (reason why I'm proposing this).

The consequence of these changes is that the host won't be checked anymore, everything else works normally.

I can put these behind an environment variable, Django settings or just the current code under that method abstraction so that it can more easily be overridden, without having to copy over the whole code.

Let me know what you think!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I defer to the python maintainers on how they would want this configurable, if they do. Thanks for the explanation!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me why the previous implementation appended a ? to the URL being checked, but I'm wondering if we should keep it to minimize changes in behavior. What do you think @jourdanrodrigues?

return self._excluded_urls.url_disabled(url)


def _parse_duration_attrs(
req_attrs, sem_conv_opt_in_mode=_StabilityMode.DEFAULT
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,10 @@ def setUpClass(cls):
def setUp(self):
super().setUp()
setup_test_environment()
conf.settings.ALLOWED_HOSTS = [
# Django adds "testserver" within "setup_test_environment" so this check doesn't break during tests.
"unknown"
]
test_name = ""
if hasattr(self, "_testMethodName"):
test_name = self._testMethodName
Expand Down Expand Up @@ -230,6 +234,20 @@ def test_templated_route_get(self):
self.assertEqual(span.attributes["http.scheme"], "http")
self.assertEqual(span.attributes["http.status_code"], 200)

def test_when_middleware_triggers_host_check_then_returns_bad_request(self):
def return_host(request):
# "HttpRequest.get_host" checks for allowed hosts and raises
# `django.core.DisallowedHost` if it isn't allowed.
return request.get_host()

with patch.object(
_DjangoMiddleware, "process_request", side_effect=return_host
) as process_mock:
response = Client().get("/route/2020/template/")

assert response.status_code == 400
process_mock.assert_called_once()

def test_traced_get(self):
Client().get("/traced/")

Expand Down
Loading