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.

ryanhiebert · 2024-03-25T15:53:21Z

django/middleware/csrf.py

-REASON_BAD_ORIGIN = "Origin checking failed - %s does not match any trusted origins."
+REASON_BAD_ORIGIN = (
+    "Origin checking failed - %s does not match any trusted origins to server %s."
+)


This ended up requiring a number of test changes. I think that this doesn't run afoul of our backward compatibility policy because I don't think that this is a documented property, but this seems like a likely place for a reviewer to have concerns.

Yeah, it’s fine to change human-readable error messages because user code shouldn’t generally rely on them. Django’s tests for this code naturally need to check the messages though.

ryanhiebert · 2024-03-25T15:55:24Z

django/middleware/csrf.py

+                    REASON_BAD_ORIGIN
+                    % (
+                        request.META["HTTP_ORIGIN"],
+                        self._host_origin(request) or "<disallowed host>",


It would probably be even better to handle this with a dedicated reason, but I didn't want to delay submitting since even with the messaging like this since even as it is it seems like an improvement over the status quo.

I think I agree this should have it's own reason

ryanhiebert · 2024-03-25T15:55:58Z

django/views/templates/csrf_403.html

@@ -53,7 +79,7 @@ <h2>Help</h2>
    <li>Your browser is accepting cookies.</li>

    <li>The view function passes a <code>request</code> to the template’s <a
-    href="https://docs.djangoproject.com/en/dev/topics/templates/#django.template.backends.base.Template.render"><code>render</code></a>
+    href="https://docs.djangoproject.com/en/{{ docs_version }}/topics/templates/#django.template.backends.base.Template.render"><code>render</code></a>


This is unrelated to my changes, except that I'm in here and noticed that it wasn't using the same version format for these doc urls.

This can be a separate commit, merged independently 👍
Let's pull this out and keep the changeset really tight to the problem being solved

sarahboyce

Thank you Ryan ⭐ I have initial comments

sarahboyce · 2024-04-25T12:58:21Z

django/views/csrf.py

@@ -61,6 +65,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.split(" - ")[0] == REASON_BAD_ORIGIN.split(" - ")[0],


Suggested change

"bad_origin": reason.split(" - ")[0] == REASON_BAD_ORIGIN.split(" - ")[0],

"bad_origin": reason.startswith("Origin checking failed"),

Or maybe

REASON_BAD_ORIGIN_PREFIX = "Origin checking failed" REASON_BAD_ORIGIN = f"{REASON_BAD_ORIGIN_PREFIX} - %s does not match any trusted origins to server %s."

and

"bad_origin": reason.startswith(REASON_BAD_ORIGIN_PREFIX),

not sure 🤔

Just hardcode the string:

Suggested change

"bad_origin": reason.split(" - ")[0] == REASON_BAD_ORIGIN.split(" - ")[0],

"bad_origin": reason.startswith("Origin checking failed"),

It’s easier to read and we have tests that will ensure it’s accurate.

sarahboyce · 2024-04-25T13:32:24Z

django/middleware/csrf.py

+                    REASON_BAD_ORIGIN
+                    % (
+                        request.META["HTTP_ORIGIN"],
+                        self._host_origin(request) or "<disallowed host>",


I think I agree this should have it's own reason

adamchainz

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

adamchainz · 2024-04-26T10:48:50Z

django/middleware/csrf.py

-REASON_BAD_ORIGIN = "Origin checking failed - %s does not match any trusted origins."
+REASON_BAD_ORIGIN = (
+    "Origin checking failed - %s does not match any trusted origins to server %s."
+)


Yeah, it’s fine to change human-readable error messages because user code shouldn’t generally rely on them. Django’s tests for this code naturally need to check the messages though.

adamchainz · 2024-04-26T10:52:51Z

django/middleware/csrf.py

+REASON_BAD_ORIGIN = (
+    "Origin checking failed - %s does not match any trusted origins to server %s."
+)


The new to server https://example.com suffix is a bit inaccurate, since https://example.com is technically an origin, not a server. How about instead:

Suggested change

REASON_BAD_ORIGIN = (

"Origin checking failed - %s does not match any trusted origins to server %s."

)

REASON_BAD_ORIGIN = (

"Origin checking failed - %r does not match %r or any other trusted origins."

)

Also switching to %r to quote the strings.

adamchainz · 2024-04-26T10:55:18Z

django/views/csrf.py

@@ -61,6 +65,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.split(" - ")[0] == REASON_BAD_ORIGIN.split(" - ")[0],


Just hardcode the string:

Suggested change

"bad_origin": reason.split(" - ")[0] == REASON_BAD_ORIGIN.split(" - ")[0],

"bad_origin": reason.startswith("Origin checking failed"),

It’s easier to read and we have tests that will ensure it’s accurate.

adamchainz · 2024-04-26T10:56:58Z

tests/csrf_tests/tests.py

+        msg = REASON_BAD_ORIGIN % (
+            req.META["HTTP_ORIGIN"],
+            "http://" + req.META["HTTP_HOST"],
+        )


IMO these tests would be better with hardcoded strings - easier to read and debug, and avoiding the chance of incorrect logic.

https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests

adamchainz · 2024-04-26T10:57:45Z

django/views/templates/csrf_403.html

+      <code>{{x_forwarded_proto}}</code></li>
+    <li><strong><code>X-Forwarded-Host</code></strong>:
+      <code>{{x_forwarded_host}}</code></li>


Suggested change

<code>{{x_forwarded_proto}}</code></li>

<li><strong><code>X-Forwarded-Host</code></strong>:

<code>{{x_forwarded_host}}</code></li>

<code>{{ x_forwarded_proto }}</code></li>

<li><strong><code>X-Forwarded-Host</code></strong>:

<code>{{ x_forwarded_host }}</code></li>

https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/coding-style/#template-style

adamchainz · 2024-04-26T10:58:21Z

django/views/templates/csrf_403.html

+  </pre></p>
+  {% else %}
+  If the expected server origin looks correct,
+  you may wish to add the origin to the CSRF_TRUSTED_ORIGINS setting.


Suggested change

you may wish to add the origin to the CSRF_TRUSTED_ORIGINS setting.

you may wish to add the origin to the <code>CSRF_TRUSTED_ORIGINS</code>

setting.

adamchainz · 2024-04-26T11:02:18Z

django/middleware/csrf.py

@@ -436,7 +444,12 @@ def process_view(self, request, callback, callback_args, callback_kwargs):
        if "HTTP_ORIGIN" in request.META:
            if not self._origin_verified(request):


Could we restructure the code to avoid calling _host_origin twice (within _origin_verified() and then on line 451). So here could look like:

Suggested change

if not self._origin_verified(request):

host_origin = self._host_origin(request)

if not self._origin_verified(request, host_origin):

And the error message could use host_origin or "<disallowed host>".

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

ryanhiebert force-pushed the proxies branch 2 times, most recently from a8dadcf to dbbf194 Compare March 25, 2024 16:02

Fixed #35328 - Improved debug messaging behind proxies.

d7f6022

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

sarahboyce reviewed Apr 25, 2024

View reviewed changes

adamchainz reviewed Apr 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ryanhiebert commented Mar 25, 2024

ryanhiebert Mar 25, 2024

adamchainz Apr 26, 2024

ryanhiebert Mar 25, 2024

sarahboyce Apr 25, 2024

ryanhiebert Mar 25, 2024

sarahboyce Apr 25, 2024

sarahboyce left a comment

sarahboyce Apr 25, 2024

adamchainz Apr 26, 2024

sarahboyce Apr 25, 2024

adamchainz left a comment

adamchainz Apr 26, 2024

adamchainz Apr 26, 2024

adamchainz Apr 26, 2024

adamchainz Apr 26, 2024

adamchainz Apr 26, 2024

adamchainz Apr 26, 2024

adamchainz Apr 26, 2024

	"bad_origin": reason.split(" - ")[0] == REASON_BAD_ORIGIN.split(" - ")[0],
	"bad_origin": reason.startswith("Origin checking failed"),

	you may wish to add the origin to the CSRF_TRUSTED_ORIGINS setting.
	you may wish to add the origin to the <code>CSRF_TRUSTED_ORIGINS</code>
	setting.

		@@ -436,7 +444,12 @@ def process_view(self, request, callback, callback_args, callback_kwargs):
		if "HTTP_ORIGIN" in request.META:
		if not self._origin_verified(request):

	if not self._origin_verified(request):
	host_origin = self._host_origin(request)
	if not self._origin_verified(request, host_origin):

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarahboyce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamchainz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment