Move up/down endpoints should use POST requests

[jdavisp3]

Currently the move up/down endpoints allow GET requests to change state. They should be POST requests.

[lingfish]

Just a question -- why?

[jdavisp3]

Because GET requests should not, by definition, change state.

http://programmers.stackexchange.com/questions/188860/why-shouldnt-a-get-request-change-data-on-the-server

[zopieux]

The keyword to search for is CSRF.

[lingfish]

Fair enough -- so does anyone plan to give the author a pull req?

[mverleg]

There are a few practical problems:

• Method move_up_down_links doesn't have access to request directly, so it can't generate a csrf tokens.
• Forms cannot be nested, so the POST request has to be generated with javascript instead, which means it won't work without javascript.
• Requiring POST would break functionality for anyone who used a custom template, probably without visible error.

I still think it's important though, it's not secure now, one can easily manipulate order through a simple CSRF url posted somewhere.

[jdavisp3]

I think this was fixed?

[jdavisp3]

Oh, not yet.

[shuckc]

Might be possible to make the up/down links all submit the main form (rather than a nested form, or random get request) with different actions for each button. Would allow dropping all the custom url generation as it would all go via. the existing page change handler, with some magic in the Form generation to recover the action and emit an event or similar to update the model. I'm not nearly enough of a django hacker to get it working though.

[shuckc]

I've been looking at what django-mptt does (they have a drag-drop re-ordering form widget) and it is quite similar to what I suggested. They use the admin jQuery integration to fire a POST request back to the original page form handler, which they intercept in the changelist_view(self, request) hook.

https://github.com/django-mptt/django-mptt/blob/master/mptt/admin.py#L147

Client-side, the result from the Ajax call is ignored, and jQuery simply reloads the current page to show the new ordering:

https://github.com/django-mptt/django-mptt/blob/master/mptt/static/mptt/draggable-admin.js#L223

This seems a neat solution. We would need to add inheritance to any changelist page that includes an OrderedTabularInline (replacing the url extending code we have at the moment).

[ralokt]

@shuckc I take it you mean this? https://caniuse.com/#feat=form-submit-attributes

Because as far as I see it, there is no need to do away with the existing move_view or any jQuery shenanigans - it should be sufficient to change order_controls.html to use buttons with the formaction attribute for it to work.

Of course, it would be nice if the move_view were also be changed to validate that it is used with POST or PUT.

I'm prepared to do this if it's considered a worthwhile change.