Use a form for Block Deletion
quicksketch - May 19, 2008 - 00:00
| Project: | MultiBlock |
| Version: | 5.x-1.0 |
| Component: | Code |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Description
Right now the delete block instance form is simply a menu callback. This means that there isn't any warning at all to prevent a user from accidentally deleting a block instance. It also makes the block instances vulnerable to Cross-site Request Forgery attacks (aka CSRF attacks). Meaning that a malicious site that an administrator visits while logged into the Drupal site could delete block instances simply by requesting the delete block URLs.
Adding a confirmation form prevents this problem by adding the security of form_tokens.
This patch adds such a form, and provides a central API function for getting all the blocks defined by multiblock (multiblock_get_block).
| Attachment | Size |
|---|---|
| multiblock-delete.patch | 5.68 KB |
#1
This updated patch adds the $reset flag to the multiblock_get_block_list() function and call it from the hook_block() implementation. This makes it so that if _block_rehash() is called a second time in one page load, a new list of blocks is returned instead of the cached version.
#2
quicksketch,
These are good changes. I made a couple modifications, let me know if this is acceptable to you and I'll commit it.
1. I removed multiblock_get_block_list() because it is only a few lines and I'm not sure it would be used anywhere else
2. I set $reset of multiblock_get_block() to FALSE in hook_block because this is where we would gain any performance from saving in a static variable
3. I set $reset of multiblock_get_block() to TRUE in multiblock_general because we need the most up to date information for the admin to see and the admin for will rarely be used
I think that's all I changed in the patch, let me know.
#3
This all looks great to me. That's fine that the reset flag has been switched around from my original patch, I'm probably in a bit of a strange situation where I'm manually calling _block_rehash() in my module that depends on Multiblock. I can easily call multiblock_get_block(NULL, TRUE) to reset the list manually before calling my own _block_rehash. Tested this all out and it looks just fine.
#4
committed, thanks so much for the patch.
#5
Automatically closed -- issue fixed for two weeks with no activity.