search

shacker / django-todo

A multi-user, multi-group todo/ticketing system for Django projects. Includes CSV import and integrated mail tracking.
http://django-todo.org
BSD 3-Clause "New" or "Revised" License
819 stars 285 forks source link

CSRF on task deletion / mark done #48

Closed multun closed 5 years ago

multun commented 5 years ago

Tasks are deleted / added with a bare GET request, without CSRF protection. Redirecting a privileged user to this link http://django-todo.org/todo/delete/42/ results in task 42 being deleted. Same holds for marking tasks as done.

multun commented 5 years ago

@shacker what do you think ?

shacker commented 5 years ago

GET requests never need CSRF protection, only POST. But one should never alter stored state through a GET request and this should be a POST request. I know better. Will fix, thanks for the catch.

multun commented 5 years ago

GET requests never need CSRF protection, only POST

well they may need a protection, but it can't be implemented, that's why people use POST

Will fix, thanks for the catch.

o/ I wish I had time to fix it myself, sorry about that

shacker commented 5 years ago

well they may need a protection, but it can't be implemented

GET requests still need permissions protection, but not CSRF protection. If a GET request is always idempotent per standards (i.e. they don't ever change state on the server), then it's not needed.

I've just rewritten the task_delete and task_toggle_done views to use POST rather than get, and released version 2.2.1. Ironically the trickiest aspect of this is not the Python, but the fact that each previously simple link now has to become a form, and forms are block-level elements and that caused all kinds of hassles trying to get Bootstrap to behave. For some reason the provided form-inline class was not working and I had to resort to style=display:inline; in two places on each form, which felt hacky. If anyone can fix that without style attributes, contributions appreciated!

Thanks for the report.