This pull request converts a few outstanding routes that perform destructive actions to use POST requests only, requiring a valid CSRF token in the posted data. This closes a few unlikely and low-severity CSRF vulnerabilities.
This PR is spun off from #366, but incorporates the feedback to fix the display of the buttons on the admin page, and to improve the wording of the delete-announcement button.
One additional change I made that is not in #366 is to place the delete-announcement button below the details of the current announcement, for consistency with the other buttons on the page. (This also makes that line less cramped.)
One question I have but did not raise previously: the delete-announcement link was styled with the btn-red class, which is no longer in the CSS, and so it appeared as a plain link (and now a plain button). Should the delete-announcement button be red, similar to the disable-captchas button?
CSRF vulnerabilities
The routes in question are:
/do/admin/enable_posting
/do/admin/enable_registration
/do/admin/require_captchas
/do/admin/deleteannouncement
/do/create_invite
Currently, should a malicious party convince an authenticated user to click a link to any of these, the corresponding action will be performed. For the first four routes, this user has to be an administrator, so this would have to be highly targeted.
While these are destructive in that they cause changes to the state of the application, for the first three routes which act as toggles, the user is redirected to the admin index page and a message is displayed describing the action taken, allowing the user to quickly reverse the action.
The announcement-deleting route also redirects the user to the admin index page, though it may not be so obvious that there is now no announcement set.
And while the final route will create a new invite code from the user’s pool, this is really just an annoyance.
This pull request converts a few outstanding routes that perform destructive actions to use POST requests only, requiring a valid CSRF token in the posted data. This closes a few unlikely and low-severity CSRF vulnerabilities.
This PR is spun off from #366, but incorporates the feedback to fix the display of the buttons on the admin page, and to improve the wording of the delete-announcement button.
One additional change I made that is not in #366 is to place the delete-announcement button below the details of the current announcement, for consistency with the other buttons on the page. (This also makes that line less cramped.)
One question I have but did not raise previously: the delete-announcement link was styled with the
btn-redclass, which is no longer in the CSS, and so it appeared as a plain link (and now a plain button). Should the delete-announcement button be red, similar to the disable-captchas button?CSRF vulnerabilities
The routes in question are:
/do/admin/enable_posting/do/admin/enable_registration/do/admin/require_captchas/do/admin/deleteannouncement/do/create_inviteCurrently, should a malicious party convince an authenticated user to click a link to any of these, the corresponding action will be performed. For the first four routes, this user has to be an administrator, so this would have to be highly targeted.
While these are destructive in that they cause changes to the state of the application, for the first three routes which act as toggles, the user is redirected to the admin index page and a message is displayed describing the action taken, allowing the user to quickly reverse the action.
The announcement-deleting route also redirects the user to the admin index page, though it may not be so obvious that there is now no announcement set.
And while the final route will create a new invite code from the user’s pool, this is really just an annoyance.