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

ryanhiebert · 2024-03-25T02:52:55Z

Trac ticket number

Branch description

When in debug mode, we can make some better error messaging when the Origin header checking fails. In common scenarios, we can notice that they are likely missing headers from proxies. In any case, we can remove the verbose messaging around CSRF Tokens, which are not the way they have failed and are unhelpful to solving the error they are viewing.

Checklist

This PR targets the main branch.
The commit message is written in past tense, mentions the ticket number, and ends with a period.
I have checked the "Has patch" ticket flag in the Trac system.
I have added or updated relevant tests.
I have added or updated relevant docs, including release notes if applicable.

django/middleware/csrf.py

django/views/templates/csrf_403.html

sarahboyce

Thank you Ryan ⭐ I have initial comments

django/views/csrf.py

django/middleware/csrf.py

adamchainz

Thank you for your work here @ryanhiebert ! I have some comments with small improvements.

django/middleware/csrf.py

django/views/csrf.py

tests/csrf_tests/tests.py

django/views/templates/csrf_403.html

django/middleware/csrf.py

adamchainz · 2024-05-20T13:14:00Z

Lint error:

Error: ./django/views/csrf.py:26:5: F401 'django.middleware.csrf.REASON_BAD_ORIGIN' imported but unused

Set up pre-commit for yourself: https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/coding-style/#pre-commit-checks

django/middleware/csrf.py

tests/csrf_tests/tests.py

django/middleware/csrf.py

django/views/csrf.py

sarahboyce · 2024-05-24T14:11:59Z

django/views/templates/csrf_403.html

Suggested change

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.

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.

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

sarahboyce · 2024-05-24T14:41:42Z

django/views/templates/csrf_403.html

Maybe?

Suggested change

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.

<pre>

SECURE_PROXY_SSL_HEADER = ("HTTP_X_FORWARDED_PROTO", "https")

USE_X_FORWARDED_HOST = True

</pre>

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.

<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.

sarahboyce · 2024-05-24T14:45:45Z

django/views/templates/csrf_403.html

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)

sarahboyce · 2024-05-24T14:52:39Z

django/views/csrf.py

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?

sarahboyce · 2024-05-24T15:05:17Z

docs/releases/5.1.txt

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.

ryanhiebert force-pushed the proxies branch 2 times, most recently from 0df060c to 95759af Compare March 25, 2024 02:59

ryanhiebert commented Mar 25, 2024

View reviewed changes

django/middleware/csrf.py Outdated Show resolved Hide resolved

ryanhiebert commented Mar 25, 2024

View reviewed changes

django/middleware/csrf.py Outdated Show resolved Hide resolved

ryanhiebert commented Mar 25, 2024

View reviewed changes

django/views/templates/csrf_403.html Outdated Show resolved Hide resolved

ryanhiebert force-pushed the proxies branch 3 times, most recently from d857f02 to d7f6022 Compare March 30, 2024 01:51

sarahboyce reviewed Apr 25, 2024

View reviewed changes

django/views/csrf.py Outdated Show resolved Hide resolved

django/middleware/csrf.py Outdated Show resolved Hide resolved

adamchainz reviewed Apr 26, 2024

View reviewed changes

ryanhiebert force-pushed the proxies branch from dd8a97c to 1e99404 Compare May 18, 2024 09:38

adamchainz suggested changes May 20, 2024

View reviewed changes

django/middleware/csrf.py Outdated Show resolved Hide resolved

django/middleware/csrf.py Outdated Show resolved Hide resolved

tests/csrf_tests/tests.py Outdated Show resolved Hide resolved

ryanhiebert force-pushed the proxies branch 3 times, most recently from c6805e2 to 6e3a65e Compare May 20, 2024 15:03

sarahboyce reviewed May 22, 2024

View reviewed changes

django/middleware/csrf.py Outdated Show resolved Hide resolved

django/views/csrf.py Outdated Show resolved Hide resolved

ryanhiebert force-pushed the proxies branch 3 times, most recently from 755cbb3 to dc5ce91 Compare May 22, 2024 13:27

sarahboyce reviewed May 24, 2024

View reviewed changes

Fixed #35328 - Improved debug messaging behind proxies.

1ad20e8

ryanhiebert force-pushed the proxies branch from dc5ce91 to 1ad20e8 Compare December 16, 2024 11:10

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

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

-  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.

-        "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,

-* 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.

Uh oh!

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

Are you sure you want to change the base?

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

Conversation

ryanhiebert commented Mar 25, 2024

Trac ticket number

Branch description

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sarahboyce left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

adamchainz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adamchainz commented May 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sarahboyce May 24, 2024

Choose a reason for hiding this comment

Uh oh!

sarahboyce May 24, 2024

Choose a reason for hiding this comment

Uh oh!

sarahboyce May 24, 2024

Choose a reason for hiding this comment

Uh oh!

sarahboyce May 24, 2024

Choose a reason for hiding this comment

Uh oh!

sarahboyce May 24, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

adamchainz commented May 20, 2024 •

edited

Loading