Opened 4 years ago

Closed 3 years ago

#16384 closed Bug (fixed)

Documentation should warn against accessing request.POST in middleware

Reported by: tomchristie Owned by: tomchristie
Component: Documentation Version: 1.3
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

This page https://docs.djangoproject.com/en/dev/topics/http/middleware/ ought to include a note warning against accessing request.POST in middleware.

As per Jacob's comment in #613, middleware that hits request.POST should (usually) be considered a bug. It means that the view will be unable to set any custom upload handlers, perform custom parsing of the request body, or enforce permission checks prior to file uploads being accepted.

I'll provide a patch for this when I get a moment. I'd expect the text to be something like:

"Accessing request.POST or request.REQUEST inside middleware from process_request or process_view is bad practice, and should be avoided. (*)
Doing so will prevent any view running after the middleware from being able to modify the upload handlers for the request (link), or being able to access the request content using request.read() or request.raw_post_data.

(*) The CSRFMiddleware can be considered an exception, as it can be disabled by using the @csrf_exempt decorator."

Any suggestions for tweaks to the text or opinions on if/where this should be added on the page?...

Attachments (2)

16384.patch (1.7 KB) - added by tomchristie 4 years ago.
authors.diff (474 bytes) - added by tomchristie 4 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 4 years ago by aaugustin

  • Easy pickings unset
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Django encourages using CsrfViewMiddleware, which does load request.POST, making this advice a bit pointless (and even counter-productive in some cases).

I'm feeling uneasy about the (implied) suggestion to use @csrf_exempt, because of the security implications.

I agree that we should mention this pitfall in the documentation, but I can't come up with a really good way to explain it.

Maybe we should just to state the facts, i.e. say that middleware shouldn't access request.POST, but that Django's implementation of CSRF protection and custom upload handlers are incompatible.

comment:2 Changed 4 years ago by tomchristie

I'm feeling uneasy about the (implied) suggestion to use @csrf_exempt, because of the security implications.

That's a good point, I think that part is badly written. Django's docs do already adequately explain how to modify upload handlers on the fly using @csrf_exempt/@csrf_protect, so it should mention both (and link to that part of the documentation) rather than imply that you should just turn off CSRF validation.

How about the following:

"Note

Accessing request.POST or request.REQUEST inside middleware from process_request or process_view will prevent any view running after the middleware from being able to modify the upload handlers for the request, and should normally be avoided.

Note that the CSRF middleware can be considered an exception, as it provides the @csrf_exempt and @csrf_protect decorators which allow views to explicitly control at what point the CSRF validation should occur."

comment:3 Changed 4 years ago by aaugustin

OK, that's much better. I hadn't realized the proper solution for upload handlers was already described in the docs. Thanks for creating #16430.

Changed 4 years ago by tomchristie

comment:4 Changed 4 years ago by tomchristie

  • Has patch set
  • Status changed from new to assigned

comment:5 Changed 4 years ago by aaugustin

  • Triage Stage changed from Accepted to Ready for checkin

Excellent. With the link to the section about upload handlers, where there is a complete example of CSRF protection, it's perfectly clear.

Changed 4 years ago by tomchristie

comment:6 Changed 4 years ago by tomchristie

Added self to authors, re: #15785, #16384, #16430

comment:7 Changed 3 years ago by jacob

  • Resolution set to fixed
  • Status changed from assigned to closed

In [16734]:

Fixed #16384: warn against accessing request.POST/REQUEST in middleware.

Thanks, Tom Christie.

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