node_delete_confirm uses the deprecated $_POST to get its variables. This patch changes the menu array to pass the node through as an argument.

Note that this patch removes the access control check from the function, as there is already access control in the menu. Could this increase the chance of malicious code calling the function directly and deleting content?

CommentFileSizeAuthor
#4 node_delete_1.patch1.86 KBedmund.kwok
node_delete_0.patch1.83 KBhickory

Comments

pwolanin’s picture

pwolanin commented
Status: Needs review » Needs work

Why not just pass in the nid rather than the whole node?

Also, I think your patch may break other code, since you no longer look to arg(1). I think in most cases the nid is coming from there, rather than from $_POST.

hickory’s picture

hickory commented

Quite the opposite actually - it's only called from the 'node/nid' URL, and takes $nid from arg(1), not from the form.

pwolanin’s picture

pwolanin commented
Status: Needs work » Needs review

Ahhh, sorry I just looked at the full node_menu() function. Now I see that the node is loaded and available.

Actually I'm now wondering why all the calls to arg(1) in this function aren't replaced with $node->nid?

Also, in terms of the access control your approach may be fine, since it's similar to this: http://drupal.org/node/83288

edmund.kwok’s picture

edmund.kwok commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new node_delete_1.patch1.86 KB

+1 for removal of $_POST.

Node delete still works fine after patch. Rerolled for HEAD; changed slight coding style to match most of other modules in core. RTBC for me.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Good work guys! Committed to CVS HEAD. Keep the patches coming. :)

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)