Skip to content
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

Fixed #35328 - Improved debug messaging behind proxies. #18014

Open
wants to merge 1 commit 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS
Original file line number Diff line number Diff line change
Expand Up @@ -891,6 +891,7 @@ answer newbie questions, and generally made Django that much better:
Russ Webber
Ryan Hall <ryanhall989@gmail.com>
Ryan Heard <ryanwheard@gmail.com>
Ryan Hiebert <ryan@ryanhiebert.com>
ryankanno
Ryan Kelly <ryan@rfk.id.au>
Ryan Niemeyer <https://profiles.google.com/ryan.niemeyer/about>
Expand Down
32 changes: 17 additions & 15 deletions django/middleware/csrf.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,10 @@
# This matches if any character is not in CSRF_ALLOWED_CHARS.
invalid_token_chars_re = _lazy_re_compile("[^a-zA-Z0-9]")

REASON_BAD_ORIGIN = "Origin checking failed - %s does not match any trusted origins."
REASON_DISALLOWED_HOST = "Host is not allowed."
REASON_BAD_ORIGIN = (
"Origin checking failed - %r does not match %r or any other trusted origins."
)
ryanhiebert marked this conversation as resolved.
Show resolved Hide resolved
REASON_NO_REFERER = "Referer checking failed - no Referer."
REASON_BAD_REFERER = "Referer checking failed - %s does not match any trusted origins."
REASON_NO_CSRF_COOKIE = "CSRF cookie not set."
Expand Down Expand Up @@ -268,19 +271,10 @@ def _set_csrf_cookie(self, request, response):
# Set the Vary header since content varies with the CSRF cookie.
patch_vary_headers(response, ("Cookie",))

def _origin_verified(self, request):
def _origin_verified(self, request, host_origin):
request_origin = request.META["HTTP_ORIGIN"]
try:
good_host = request.get_host()
except DisallowedHost:
pass
else:
good_origin = "%s://%s" % (
"https" if request.is_secure() else "http",
good_host,
)
if request_origin == good_origin:
return True
if request_origin == host_origin:
return True
if request_origin in self.allowed_origins_exact:
return True
try:
Expand Down Expand Up @@ -434,9 +428,17 @@ def process_view(self, request, callback, callback_args, callback_kwargs):
# Reject the request if the Origin header doesn't match an allowed
# value.
if "HTTP_ORIGIN" in request.META:
if not self._origin_verified(request):
try:
host_origin = "%s://%s" % (
"https" if request.is_secure() else "http",
request.get_host(),
)
except DisallowedHost:
return self._reject(request, REASON_DISALLOWED_HOST)
if not self._origin_verified(request, host_origin):
return self._reject(
request, REASON_BAD_ORIGIN % request.META["HTTP_ORIGIN"]
request,
REASON_BAD_ORIGIN % (request.META["HTTP_ORIGIN"], host_origin),
)
elif request.is_secure():
# If the Origin header wasn't provided, reject HTTPS requests if
Expand Down
10 changes: 10 additions & 0 deletions django/views/csrf.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,16 @@ def csrf_failure(request, reason="", template_name=CSRF_FAILURE_TEMPLATE_NAME):
"re-enable them, at least for this site, or for “same-origin” "
"requests."
),
"bad_origin": reason.startswith("Origin checking failed"),
"forwarded_may_fix": (
request.headers.get("X-Forwarded-Proto", "") == "https"
and not request.is_secure()
or request.headers.get("Origin", "").endswith(
f'://{request.headers.get("X-Forwarded-Host", "")}'
)
),
ryanhiebert marked this conversation as resolved.
Show resolved Hide resolved
"x_forwarded_proto": request.headers.get("X-Forwarded-Proto"),
"x_forwarded_host": request.headers.get("X-Forwarded-Host"),
Comment on lines +64 to +73
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"bad_origin": reason.startswith("Origin checking failed"),
"forwarded_may_fix": (
request.headers.get("X-Forwarded-Proto", "") == "https"
and not request.is_secure()
or request.headers.get("Origin", "").endswith(
f'://{request.headers.get("X-Forwarded-Host", "")}'
)
),
"x_forwarded_proto": request.headers.get("X-Forwarded-Proto"),
"x_forwarded_host": request.headers.get("X-Forwarded-Host"),
"bad_origin": True,
"forwarded_may_fix": (
request.headers.get("X-Forwarded-Proto", "") == "https"
or request.headers.get("Origin", "").endswith(
f'://{request.headers.get("X-Forwarded-Host", "")}'
)
),
"x_forwarded_proto": None,
"x_forwarded_host": None,

Thank you for the updates @ryanhiebert
There are a couple of things I think we should still add tests/assert for. If I make the above changes, nothing fails. These changes are:

  • bad_origin always True
  • and not request.is_secure() is removed from "forwarded_may_fix"
  • can we also assert the values of x_forwarded_proto and x_forwarded_host rendering in the template?

"DEBUG": settings.DEBUG,
"docs_version": get_docs_version(),
"more": _("More information is available with DEBUG=True."),
Expand Down
29 changes: 29 additions & 0 deletions django/views/templates/csrf_403.html
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,34 @@ <h2>Help</h2>
</pre>
{% endif %}

{% if bad_origin %}
{% if forwarded_may_fix %}
<p>The <code>Origin</code> header does not match
the expected server origin,
but common proxy headers are present in the request
and may include parts of the <code>Origin</code> header.</p>
Comment on lines +48 to +51
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<p>The <code>Origin</code> header does not match
the expected server origin,
but common proxy headers are present in the request
and may include parts of the <code>Origin</code> header.</p>
<p>The <code>Origin</code> header does not match the expected server origin,
but common proxy headers are present in the request and may include parts of
the <code>Origin</code> header.</p>

This file is roughly wrapped at 80 characters so let's apply similar wrapping here

<ul>
<li><strong><code>X-Forwarded-Proto</code></strong>:
<code>{{ x_forwarded_proto }}</code></li>
<li><strong><code>X-Forwarded-Host</code></strong>:
<code>{{ x_forwarded_host }}</code></li>
</ul>
<p>If you’re sure that you are only running behind a secure proxy
that always set these headers to avoid spoofing as described in
<a href="https://docs.djangoproject.com/en/{{ docs_version }}/ref/settings/#secure-proxy-ssl-header">this warning in the docs</a>,
you may wish to add one or more of the following
settings to permit Django to trust these headers.</p>
<p><pre>
SECURE_PROXY_SSL_HEADER = ("HTTP_X_FORWARDED_PROTO", "https")
USE_X_FORWARDED_HOST = True
</pre></p>
Comment on lines +58 to +66
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe?

Suggested change
<p>If you’re sure that you are only running behind a secure proxy
that always set these headers to avoid spoofing as described in
<a href="https://docs.djangoproject.com/en/{{ docs_version }}/ref/settings/#secure-proxy-ssl-header">this warning in the docs</a>,
you may wish to add one or more of the following
settings to permit Django to trust these headers.</p>
<p><pre>
SECURE_PROXY_SSL_HEADER = ("HTTP_X_FORWARDED_PROTO", "https")
USE_X_FORWARDED_HOST = True
</pre></p>
<p>If you are running behind a secure proxy that sets these headers, you may
want to add one or more of the following settings to permit Django to trust
these headers.</p>
<p><pre>
SECURE_PROXY_SSL_HEADER = ("HTTP_X_FORWARDED_PROTO", "https")
USE_X_FORWARDED_HOST = True
</pre>
Modifying these setting can compromise your site’s security. Ensure you fully
understand your setup before changing it. See <a href="https://docs.djangoproject.com/en/{{ docs_version }}/ref/settings/#secure-proxy-ssl-header"><code>SECURE_PROXY_SSL_HEADER</code></a>
for more details.</p>

image

{% else %}
If the expected server origin looks correct,
you may wish to add the origin to the <code>CSRF_TRUSTED_ORIGINS</code>
setting.
Comment on lines +68 to +70
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
If the expected server origin looks correct,
you may wish to add the origin to the <code>CSRF_TRUSTED_ORIGINS</code>
setting.
If the expected server origin looks correct, you may wish to add the origin
to the <code>CSRF_TRUSTED_ORIGINS</code> setting.

(wrapping nit)

{% endif %}
{% else %}

<p>In general, this can occur when there is a genuine Cross Site Request Forgery, or when
<a
href="https://docs.djangoproject.com/en/{{ docs_version }}/ref/csrf/">Django’s
Expand All @@ -68,6 +96,7 @@ <h2>Help</h2>
tab or hitting the back button after a login, you may need to reload the
page with the form, because the token is rotated after a login.</li>
</ul>
{% endif %}

<p>You’re seeing the help section of this page because you have <code>DEBUG =
True</code> in your Django settings file. Change that to <code>False</code>,
Expand Down
3 changes: 2 additions & 1 deletion docs/releases/5.1.txt
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,8 @@ Cache
CSRF
~~~~

* ...
* The error messaging is improved when ``Origin`` checking fails,
including better hints when likely proxy headers are detected.
Comment on lines +193 to +194
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* The error messaging is improved when ``Origin`` checking fails,
including better hints when likely proxy headers are detected.
* The ``'403_csrf.html'`` template now has improved messaging when ``Origin``
checking fails, including better hints when likely proxy headers are detected.

Docs are wrapped at 80 characters and we will now need to rebase and move this to the 5.2 release notes.


Database backends
~~~~~~~~~~~~~~~~~
Expand Down
152 changes: 137 additions & 15 deletions tests/csrf_tests/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
CSRF_SECRET_LENGTH,
CSRF_SESSION_KEY,
CSRF_TOKEN_LENGTH,
REASON_BAD_ORIGIN,
REASON_CSRF_TOKEN_MISSING,
REASON_NO_CSRF_COOKIE,
CsrfViewMiddleware,
Expand Down Expand Up @@ -895,6 +894,20 @@ def test_reading_post_data_raises_os_error(self):
with self.assertRaises(OSError):
mw.process_view(req, post_form_view, (), {})

@override_settings(ALLOWED_HOSTS=["www.example.com"])
def test_good_origin_disallowed_host(self):
"""A request with a disallowed host is rejected."""
req = self._get_POST_request_with_token()
req.META["HTTP_HOST"] = "www.disallowed.com"
req.META["HTTP_ORIGIN"] = "https://www.disallowed.com"
mw = CsrfViewMiddleware(post_form_view)
self._check_referer_rejects(mw, req)
with self.assertLogs("django.security.csrf", "WARNING") as cm:
response = mw.process_view(req, post_form_view, (), {})
self.assertEqual(response.status_code, 403)
msg = "Host is not allowed."
self.assertEqual(cm.records[0].getMessage(), "Forbidden (%s): " % msg)

@override_settings(ALLOWED_HOSTS=["www.example.com"])
def test_bad_origin_bad_domain(self):
"""A request with a bad origin is rejected."""
Expand All @@ -903,11 +916,15 @@ def test_bad_origin_bad_domain(self):
req.META["HTTP_ORIGIN"] = "https://www.evil.org"
mw = CsrfViewMiddleware(post_form_view)
self._check_referer_rejects(mw, req)
self.assertIs(mw._origin_verified(req), False)
self.assertIs(mw._origin_verified(req, "http://www.example.com"), False)
with self.assertLogs("django.security.csrf", "WARNING") as cm:
response = mw.process_view(req, post_form_view, (), {})
self.assertEqual(response.status_code, 403)
msg = REASON_BAD_ORIGIN % req.META["HTTP_ORIGIN"]
msg = (
"Origin checking failed - 'https://www.evil.org' "
"does not match 'http://www.example.com' "
"or any other trusted origins."
)
self.assertEqual(cm.records[0].getMessage(), "Forbidden (%s): " % msg)

@override_settings(ALLOWED_HOSTS=["www.example.com"])
Expand All @@ -918,11 +935,15 @@ def test_bad_origin_null_origin(self):
req.META["HTTP_ORIGIN"] = "null"
mw = CsrfViewMiddleware(post_form_view)
self._check_referer_rejects(mw, req)
self.assertIs(mw._origin_verified(req), False)
self.assertIs(mw._origin_verified(req, "http://www.example.com"), False)
with self.assertLogs("django.security.csrf", "WARNING") as cm:
response = mw.process_view(req, post_form_view, (), {})
self.assertEqual(response.status_code, 403)
msg = REASON_BAD_ORIGIN % req.META["HTTP_ORIGIN"]
msg = (
"Origin checking failed - 'null' "
"does not match 'http://www.example.com' "
"or any other trusted origins."
)
self.assertEqual(cm.records[0].getMessage(), "Forbidden (%s): " % msg)

@override_settings(ALLOWED_HOSTS=["www.example.com"])
Expand All @@ -934,11 +955,15 @@ def test_bad_origin_bad_protocol(self):
req.META["HTTP_ORIGIN"] = "http://example.com"
mw = CsrfViewMiddleware(post_form_view)
self._check_referer_rejects(mw, req)
self.assertIs(mw._origin_verified(req), False)
self.assertIs(mw._origin_verified(req, "https://www.example.com"), False)
with self.assertLogs("django.security.csrf", "WARNING") as cm:
response = mw.process_view(req, post_form_view, (), {})
self.assertEqual(response.status_code, 403)
msg = REASON_BAD_ORIGIN % req.META["HTTP_ORIGIN"]
msg = (
"Origin checking failed - 'http://example.com' "
"does not match 'https://www.example.com' "
"or any other trusted origins."
)
self.assertEqual(cm.records[0].getMessage(), "Forbidden (%s): " % msg)

@override_settings(
Expand All @@ -961,11 +986,15 @@ def test_bad_origin_csrf_trusted_origin_bad_protocol(self):
req.META["HTTP_ORIGIN"] = "http://foo.example.com"
mw = CsrfViewMiddleware(post_form_view)
self._check_referer_rejects(mw, req)
self.assertIs(mw._origin_verified(req), False)
self.assertIs(mw._origin_verified(req, "https://www.example.com"), False)
with self.assertLogs("django.security.csrf", "WARNING") as cm:
response = mw.process_view(req, post_form_view, (), {})
self.assertEqual(response.status_code, 403)
msg = REASON_BAD_ORIGIN % req.META["HTTP_ORIGIN"]
msg = (
"Origin checking failed - 'http://foo.example.com' "
"does not match 'https://www.example.com' "
"or any other trusted origins."
)
self.assertEqual(cm.records[0].getMessage(), "Forbidden (%s): " % msg)
self.assertEqual(mw.allowed_origins_exact, {"http://no-match.com"})
self.assertEqual(
Expand All @@ -987,11 +1016,15 @@ def test_bad_origin_cannot_be_parsed(self):
req.META["HTTP_ORIGIN"] = "https://["
mw = CsrfViewMiddleware(post_form_view)
self._check_referer_rejects(mw, req)
self.assertIs(mw._origin_verified(req), False)
self.assertIs(mw._origin_verified(req, "http://www.example.com"), False)
with self.assertLogs("django.security.csrf", "WARNING") as cm:
response = mw.process_view(req, post_form_view, (), {})
self.assertEqual(response.status_code, 403)
msg = REASON_BAD_ORIGIN % req.META["HTTP_ORIGIN"]
msg = (
"Origin checking failed - 'https://[' "
"does not match 'http://www.example.com' "
"or any other trusted origins."
)
self.assertEqual(cm.records[0].getMessage(), "Forbidden (%s): " % msg)

@override_settings(ALLOWED_HOSTS=["www.example.com"])
Expand All @@ -1001,7 +1034,7 @@ def test_good_origin_insecure(self):
req.META["HTTP_HOST"] = "www.example.com"
req.META["HTTP_ORIGIN"] = "http://www.example.com"
mw = CsrfViewMiddleware(post_form_view)
self.assertIs(mw._origin_verified(req), True)
self.assertIs(mw._origin_verified(req, "http://www.example.com"), True)
response = mw.process_view(req, post_form_view, (), {})
self.assertIsNone(response)

Expand All @@ -1013,7 +1046,7 @@ def test_good_origin_secure(self):
req.META["HTTP_HOST"] = "www.example.com"
req.META["HTTP_ORIGIN"] = "https://www.example.com"
mw = CsrfViewMiddleware(post_form_view)
self.assertIs(mw._origin_verified(req), True)
self.assertIs(mw._origin_verified(req, "https://www.example.com"), True)
response = mw.process_view(req, post_form_view, (), {})
self.assertIsNone(response)

Expand All @@ -1031,7 +1064,7 @@ def test_good_origin_csrf_trusted_origin_allowed(self):
req.META["HTTP_HOST"] = "www.example.com"
req.META["HTTP_ORIGIN"] = "https://dashboard.example.com"
mw = CsrfViewMiddleware(post_form_view)
self.assertIs(mw._origin_verified(req), True)
self.assertIs(mw._origin_verified(req, "https://www.example.com"), True)
resp = mw.process_view(req, post_form_view, (), {})
self.assertIsNone(resp)
self.assertEqual(mw.allowed_origins_exact, {"https://dashboard.example.com"})
Expand All @@ -1051,12 +1084,101 @@ def test_good_origin_wildcard_csrf_trusted_origin_allowed(self):
req.META["HTTP_HOST"] = "www.example.com"
req.META["HTTP_ORIGIN"] = "https://foo.example.com"
mw = CsrfViewMiddleware(post_form_view)
self.assertIs(mw._origin_verified(req), True)
self.assertIs(mw._origin_verified(req, "https://www.example.com"), True)
response = mw.process_view(req, post_form_view, (), {})
self.assertIsNone(response)
self.assertEqual(mw.allowed_origins_exact, set())
self.assertEqual(mw.allowed_origin_subdomains, {"https": [".example.com"]})

@override_settings(ALLOWED_HOSTS=["www.example.com", "localhost"], DEBUG=True)
def test_bad_origin_x_forwarded_host_with_port(self):
"""Give a helpful message if we see a likely X-Forwarded-Host header."""
req = self._get_POST_request_with_token()
req.META["HTTP_HOST"] = "localhost:8000"
req.META["HTTP_ORIGIN"] = "http://www.example.com:8080"
req.META["HTTP_X_FORWARDED_HOST"] = "www.example.com:8080"
mw = CsrfViewMiddleware(post_form_view)
self._check_referer_rejects(mw, req)
self.assertIs(mw._origin_verified(req, "http://www.example.com:8000"), False)
with self.assertLogs("django.security.csrf", "WARNING") as cm:
response = mw.process_view(req, post_form_view, (), {})
self.assertEqual(response.status_code, 403)
msg = (
"Origin checking failed - 'http://www.example.com:8080' "
"does not match 'http://localhost:8000' "
"or any other trusted origins."
)
self.assertEqual(cm.records[0].getMessage(), "Forbidden (%s): " % msg)
self.assertContains(response, "X-Forwarded-Host", status_code=403)

@override_settings(ALLOWED_HOSTS=["www.example.com", "localhost"], DEBUG=True)
def test_bad_origin_x_forwarded_host_no_port(self):
"""Give a helpful message if we see a likely X-Forwarded-Host header."""
req = self._get_POST_request_with_token()
req.META["HTTP_HOST"] = "localhost"
req.META["HTTP_ORIGIN"] = "http://www.example.com"
req.META["HTTP_X_FORWARDED_HOST"] = "www.example.com"
mw = CsrfViewMiddleware(post_form_view)
self._check_referer_rejects(mw, req)
self.assertIs(mw._origin_verified(req, "http://localhost"), False)
with self.assertLogs("django.security.csrf", "WARNING") as cm:
response = mw.process_view(req, post_form_view, (), {})
self.assertEqual(response.status_code, 403)
msg = (
"Origin checking failed - 'http://www.example.com' "
"does not match 'http://localhost' "
"or any other trusted origins."
)
self.assertEqual(cm.records[0].getMessage(), "Forbidden (%s): " % msg)
self.assertContains(response, "X-Forwarded-Host", status_code=403)

@override_settings(ALLOWED_HOSTS=["www.example.com"], DEBUG=True)
def test_bad_origin_x_forwarded_proto(self):
"""Give a helpful message if we see a likely X-Forwarded-Proto header."""
req = self._get_POST_request_with_token()
req.META["HTTP_HOST"] = "www.example.com"
req.META["HTTP_ORIGIN"] = "https://www.example.com"
req.META["HTTP_X_FORWARDED_PROTO"] = "https"
mw = CsrfViewMiddleware(post_form_view)
self._check_referer_rejects(mw, req)
self.assertIs(mw._origin_verified(req, "http://www.example.com"), False)
with self.assertLogs("django.security.csrf", "WARNING") as cm:
response = mw.process_view(req, post_form_view, (), {})
self.assertEqual(response.status_code, 403)
msg = (
"Origin checking failed - 'https://www.example.com' "
"does not match 'http://www.example.com' "
"or any other trusted origins."
)
self.assertEqual(cm.records[0].getMessage(), "Forbidden (%s): " % msg)
self.assertContains(response, "X-Forwarded-Proto", status_code=403)

@override_settings(ALLOWED_HOSTS=["www.example.com"], DEBUG=True)
def test_no_likely_proxy_headers(self):
"""Show a message about CSRF_TRUSTED_ORIGINS if no proxy headers."""
req = self._get_POST_request_with_token()
req.META["HTTP_HOST"] = "www.example.com"
req.META["HTTP_ORIGIN"] = "https://www.example.com"
mw = CsrfViewMiddleware(post_form_view)
self._check_referer_rejects(mw, req)
self.assertIs(mw._origin_verified(req, "http://www.example.com"), False)
with self.assertLogs("django.security.csrf", "WARNING") as cm:
response = mw.process_view(req, post_form_view, (), {})
self.assertEqual(response.status_code, 403)
msg = (
"Origin checking failed - 'https://www.example.com' "
"does not match 'http://www.example.com' "
"or any other trusted origins."
)
self.assertEqual(cm.records[0].getMessage(), "Forbidden (%s): " % msg)
self.assertContains(
response, "If the expected server origin looks correct", status_code=403
)
self.assertContains(
response, "you may wish to add the origin to", status_code=403
)
self.assertContains(response, "CSRF_TRUSTED_ORIGINS", status_code=403)


class CsrfViewMiddlewareTests(CsrfViewMiddlewareTestMixin, SimpleTestCase):
def _set_csrf_cookie(self, req, cookie):
Expand Down
2 changes: 1 addition & 1 deletion tests/view_tests/tests/test_csrf.py
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ def test_template_encoding(self):
from django.views.csrf import Path

with mock.patch.object(Path, "open") as m:
csrf_failure(mock.MagicMock(), mock.Mock())
csrf_failure(mock.MagicMock(), "Example reason")
m.assert_called_once_with(encoding="utf-8")

@override_settings(DEBUG=True)
Expand Down