Opened 6 years ago

Closed 4 years ago

#9559 closed Uncategorized (invalid)

CSRFMiddleware should strip POST dat instead of showing the user an error message if a forgery is detected

Reported by: zain Owned by: nobody
Component: CSRF Version: 1.0
Severity: Normal Keywords: csrf, csrfmiddleware
Cc: glennfmaynard@… Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX:

Description

If a page receives a POST that doesn't contain the 'csrfmiddlwaretoken' variable, it shows the following message: "Cross Site Request Forgery detected. Request aborted."

Instead of showing the user this message, I propose just stripping out the POST data. That could help improve user experience in the case of when a site outside your control is redirecting to you.

For example; if a user is paying you via Paypal web payments, they get redirected back to your website at the end. During this step, Paypal POSTs some (non-critical) information. At this point, the CSRF middleware shows the user an error. As a result, it is impossible to use the CSRF Middleware on a website that accepts paypal web payments.

The patch I have attached merely sets request.POST = [] instead of giving the user an HttpResponseForbidden message.

Attachments (1)

csrf.patch (1.2 KB) - added by zain 6 years ago.

Download all attachments as: .zip

Change History (5)

Changed 6 years ago by zain

comment:1 Changed 6 years ago by Daniel Pope <dan@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Design decision needed

Assigning an empty list to request.POST breaks API compatibility. You should assign an empty django.http.QueryDict. If the view is to be executed anyway it also might be useful to move the real POST data to request.UNTRUSTED_POST so that it would be possible to process PayPal data.

However, I think the error message would more useful to users than silently suppressing POST data. A minor refactoring of CsrfMiddleware would let users easily subclass it to provide whatever behaviour they prefer when the POST data doesn't validate.

comment:2 Changed 6 years ago by Glenn

  • Cc glennfmaynard@… added

If you're expecting an external website to POST data at a specific page on yours, shouldn't you @csrf_view_exempt the view it's returning to?

I guess it would help for contrib.csrf to expose a way to check a request manually, so you can @csrf_view_exempt a view and then handle a failed CSRF check some other way. This would make it easy to implement things like @csrf_view_clear_post, too.

comment:3 Changed 4 years ago by gabrielhurley

  • Component changed from Contrib apps to contrib.csrf

comment:4 Changed 4 years ago by lukeplant

  • Easy pickings unset
  • Resolution set to invalid
  • Severity set to Normal
  • Status changed from new to closed
  • Type set to Uncategorized

This suggestion has problems. It is perfectly possible for a view to respond to an empty POST request with some action - if the URL and session data have all the necessary information needed, for example, or if all other data retrieved from POST is optional, or could be valid if empty (so that view code does things like request.POST.get('somefield', '') and proceeds without requiring the data to be non-empty).

With either of the proposed change (empty list or empty QueryDict), the request would make it through, which is a vulnerability. In other words, the suggestion assumes that empty POST requests are not dangerous, when that is not necessarily the case.

In addition, the correct way to handle the scenario given has been described above (use csrf_exempt), and fuller examples are now in the docs.

Therefore closing INVALID.

Note: See TracTickets for help on using tickets.
Back to Top