In the new menu system, user_load and node_load are called without checking that the path parameter is numeric.

Try it yourself to see the errors- go to node/zzz

Part of the problem is outlined here: http://drupal.org/node/146667 which is that PHP may try to treat a string as an array.

This is problematic for node_load and user_load, since they allw the first parameter to be either numeric or else treat it as an array. Attached patch adds a simple is_array() validation.

CommentFileSizeAuthor
#12 user_node_load.patch1.87 KBchx
#7 user_node_load_3.patch3.57 KBpwolanin
#1 user_node_load_2.patch2.89 KBpwolanin
user_node_load_1.patch1.45 KBpwolanin

Comments

pwolanin’s picture

pwolanin commented
StatusFileSize
new user_node_load_2.patch2.89 KB

Addresses a similar problem in comment module's comment_render(), and changes the callback form node/%/% to node/%/comment/% to minimize false matches to this callback.

chx’s picture

chx commented
Status: Needs review » Reviewed & tested by the community

Yes, this is needed.

Steven’s picture

Steven commented

If we change the path for the comment callback, shouldn't links pointing to it be changed too? Unless this callback is not actually used anywhere... in which case it should be removed.

flk’s picture

flk commented

this patch also solves the problem regarding editing the item 'My Account' (admin/build/menu/item/edit/23).
when you try to change anything about that item atm you get the foreach error caused by user_load which doesnt check whether the value being sent is numeric.

flk’s picture

flk commented

further more access denied error occurs when one tries to edit 'My Account' item after using this patch due to the fact when you submit the form it passes it menu_edit_item_validate which in turn does menu_get_item($path).
What the current patch does it solves the problem with user_load by making sure the value passed is numeric...once it has passed user_load, menu_get_item still returns false due to teh fact that in the menu_router table the path use/% doesnt exist.

(well thats how i see it anyways lol)

not sure if this should be another bug or not

pwolanin’s picture

pwolanin commented
Status: Reviewed & tested by the community » Needs work

good point - I found it now:

function comment_node_url() {
  return arg(0) .'/'. arg(1);
}
pwolanin’s picture

pwolanin commented
Status: Needs work » Needs review
StatusFileSize
new user_node_load_3.patch3.57 KB
chx’s picture

chx commented

Why are you changing the comment url...?

pwolanin’s picture

pwolanin commented

@chx changing the callback form node/%/% to node/%/comment/% to minimize false matches to this callback. For example, if I want to pass parameters via the URL to a PHP-format node.

webernet’s picture

webernet commented

Tested the patch, it fixes the notices/warnings with "node/asdf" and, as expected, returns a "Page not found" instead of "Access denied".

Looks good to me.

chx’s picture

chx commented
Status: Needs review » Needs work

You are breaking many existing links for no good reason, and it does not belong here. Please remove the path changes.

chx’s picture

chx commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new user_node_load.patch1.87 KB

Rerolled against head and removed the path changes and per webernet's tests it's ready.

Steven’s picture

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

Tested and works as expected. Committed to HEAD.

Anonymous’s picture

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