Menu disable / enable on GET request; vulnerable to CSRF

since all enable/disable does is show/hide items, this seems more like a potential annoyance than any kind of security threat.

ok, so if we want to avoid another annoying confirm form, what would a URL token solution look like?

here's a patch that adds tokens to those links as you suggest.

for preventing both GET and POST CSFR, it's not clear to me that it can be prevented if the "bad" site or script can send an initial GET request to get the page source containing the token/form token? Obviously this requires a more thorough knowledge of the target and more code, but why is this not possible?

pwolanin - the bad site would have to have your session id in order to get a valid token on the url. otherwise they have to send your browser to the drupal site twoce (once to get token and once to send to destination url). if they can do that they really own you.

ok, thanks for the clarification. So, is this token solution sufficient, or would it still be better to go for a confirm form? Or something else (e.g. we could turn this page into a form and enable/disable could be buttons rather than links)?

I think it would be better consistency wise too to make this page a form. It looks quite pointlessly repetitive to output dozens of tokenized links.

This page is becoming a form due to the drag and drop work: http://drupal.org/node/181126. An enable checkbox seems like a good option here.

I took quicksketch's patch and simplified the form and added a submit handler. I am not 100% that quicksketch's D&D will get in D6 but if it does, it's quite trivial to augment my patch to become his. The submit function shows the sheer power of FAPI3 :) If you want to test do not forget to rebuild your menu.

With this patch applied, if I add a node and use the "Menu settings" option, the link shows up on the actual navigation but not under admin/build/menu settings anywhere.

I did a fresh install and I am unable to reproduce this.

It was an issue w/ pager not showing up.

Added pager. Got tons of notices. Fixed notices at its core -- menu_tree_data was no prepared that it might not start at root. Removed a helper variable there as there was little point. Now works well.

Installed this. Created menu items from both the node/edit node/add and admin/build menu forms. All seems to be fine.

Generally loooks good, but:

- I don't understand why we complicate our code with an empty output first:

+  $output = '';
+  $output .= theme('table', $header, $rows, array('id' => 'menu-overview'));

- This patch gives menu_overview_form() a multiline function comment, which does not fit coding standards.

These are not big issues though.

If these are not big issues then why not commit and reopen as minor? Grr.

OK tested the patch:

- I would put the enabled checkboxes to the left of the table, as it is in the module listing (or the language listing, or...).
- It looks quite strange to see the "expanded" information, but not being able to modify it (need to go to edit). Why don't make a checkbox out of it?
- Empty menus show an empty table and a Submit button, instead of a friendly note that there are no menu items yet.
- We removed "Submit" buttons and used "Save" "Save settings" etc. labels instead to be more to the point. Here "Save configuration" seems to be fit for the purpose.

Changes as requested. I would like to draw attention to this submit function which utilizes quite some form API 3 goodies to achieve simplicity.

Testers welcome.

Tested and looks good.

Although chx did not mention, he implemented my suggestion to move the Enabled checkbox column before the Expanded column and add align=center to the checkbox cells for consistency with other such forms throughout Drupal. Tested the new patch again, and everything seems to work fine, so committed. (I did remove "sortable drag and drop" from the comment "Theme the menu overview form into a sortable drag and drop table." though, as it is not what's happening here :)

However, there is a UI problem here still, which should be solved in a follow up patch. See the attached images. The UI looks as good as it can be when the window is not wide. But when the window is wider, big gaps appear. Actually, the enabled and expanded columns should not expand in size, but the menu item column should take up the space (or the table should not expand at all). Let's get this solved, and we can close this issue.

Rock! Thanks chx! This'll make the Drag and Drop patch for menus (http://drupal.org/node/181126) much smaller and easier. Note that I'll probably switch the column order if that's acceptable to use the title first, then checkboxes. That way the drag handle will be on the left side next to the title, rather than a bunch of checkboxes.

OK, we will discuss checkbox placement there then.

Goba, if you think the other issue will get in, then please set this to fixed.

Sure, will do as soon as this turns out.

Looked into http://drupal.org/node/181126, so I am closing this in the hopes that the UI issues will be fixed with the menu DND. These UI issues are not critical anyway.