CSRF invalidation improvements

[CodenameTim]

The exception flow for CSRF issues is a bit difficult. I think we can make it easier for developers to determine the root cause of a CSRF invalidation by doing the following:

1. Log a detailed explanation of the error. For example when a CSRF token is missing from the headers, the error message presented to the user is “CSRF token missing.” This doesn’t explain where it’s missing or how it could potentially be added in. For new Django devs this is a very cryptic message.
2. (less sure) Use a unique constant for each RejectRequest usage in django.middleware.csrf. This makes it easier to identify where exactly the error is coming from. It also creates consistency in how we create these errors.
3. Add a system check that confirms that all values in CSRF_TRUSTED_ORIGINS have a scheme specified. Not specifying a scheme doesn’t break things until a request is made and it fails to validate a request. So despite having a reasonable looking configuration, it doesn’t work until the user gets to a remote environment. If the setting requires a scheme to be specified (whereas ALLOWED_HOSTS doesn’t), Django should be validating those values. This would raise the issue sooner for the developer, allow for a more specific error message and be easier to solve.

Edit:
I’m opening this for discussion to get community buy-in before implementing any changes.

[carltongibson]

Hey @CodenameTim

So, yes, this can be hard, and comes up a lot.

Some thoughts:

1. Better logging yes! Imagine a good set of messages that told you want you needed to do — You didn’t set SECURE_PROXY_SSL_HEADER, for example, which hit me recently (for the 800th time) — imagine being pointed directly there, rather than just an origins didn’t match. +1.
   • Logging already goes via _reject(), which uses the django.security.csrf logger — we’d need to think about levels there — maybe show for INFO, but not for WARNING — Letting folks know how to turn it up to see messages, and then down again once it’s working, because as you say, often you don’t see this until it’s in production, but you don’t want an email for every CSRF failure…
2. Not sure about constants… There already ones defined, and failures look like: raise RejectRequest(REASON_NO_CSRF_COOKIE) +0
   • A map of those constants to newer, better messages might be enough.
   • But likely I haven’t quite followed you.
3. This sounds good. Details, but initial +1.

I think the way forward is to demonstrate with an example of the kind of message we can give for one case, as a kind of tracer bullet, and get something open.

Folks are always raving about the error messages in things like Elm and Rust. A bit of that featuring Django wouldn’t hurt at all!

[CodenameTim]

@carltongibson

Logging already goes via _reject(), which uses the django.security.csrf logger — we’d need to think about levels there — maybe show for INFO, but not for WARNING — Letting folks know how to turn it up to see messages, and then down again once it’s working, because as you say, often you don’t see this until it’s in production, but you don’t want an email for every CSRF failure…

Hmm. I think the reasonable default for this logging is to go to log file rather than email. It might be worth having some how-to on making them an email, but that seems like an easy way to annoy a lot of folks like you said.

@carltongibson

Not sure about constants… There already ones defined, and failures look like: raise RejectRequest(REASON_NO_CSRF_COOKIE) +0

• A map of those constants to newer, better messages might be enough.
• But likely I haven’t quite followed you.

This was definitely the weakest of my points. When I get around to implementing this, I’ll see what a change would look like. I suspect it may get rejected since it would result in code churn with no benefit other than “this should be easier to read now”.

---

I appreciate the feedback Carlton!

[adamghill]

@carltongibson

Better logging yes! Imagine a good set of messages that told you want you needed to do — You didn’t set SECURE_PROXY_SSL_HEADER

Just chiming in that this bit me on prod (again) and my initial reaction was to update CSRF_TRUSTED_ORIGINS. Mostly I think because the error is “403 forbidden CSRF origin didn’t match” and CSRF_TRUSTED_ORIGINS seems like the setting to fix that. adamghill: "Is there a reason that `CSRF_TRUSTED_ORIGINS` (ht…" - Indieweb.Social has the thread where lots of people chime in to help me.

tl;dr: Using Cloudflare SSL proxy means I need to set SECURE_PROXY_SSL_HEADER = ("HTTP_X_FORWARDED_PROTO", "https”) and now CSRF is happy.

@CodenameTim Let me know if there is anything I can help here – documentation, code, testing, whatever – better error messages could really be useful for devs in the future.

[vanschelven]

1. I can’t speak to “new Django devs” but I’d say “everything is cryptic until you look it up” and “CSRF token is missing Django” gives the right answer when you put it in my favorite search engine. I’m -1
2. Not so much “constants”, but at $dayjob I just went ahead and implemented it outside of Django: GitHub - bugsink/verbose_csrf_middleware: Django CSRF middleware, but verbose We could integrate something like that into Django; though there are always security-implications (information exposure) and although I can oversee those for my own use-case pretty well, it’s a bit harder to do so in the general case
3. This seems like a no-brainer +1 to me.