transferring this from security.drupal.org per team decision for a public issue.

reported by greggles:

Admin menu provides a "flush all caches" feature at example.com/admin_menu/flush-cache this doesn't use a token or confirmation form.

I thought of this because a new module - http://drupal.org/node/1097916 - is aiming to provide the same feature.

I feel like flushing all caches can be disastrous for sites that are utilizing most of their hardware capacity - do others think we should look to get a token or form on this page?

The same issue affects the link that may enable/disable modules.

Moshe notes that devel has a similar flush cache link itself.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

Status: Active » Needs review
FileSize
1.74 KB
pwolanin’s picture

Status: Needs review » Fixed

fixed

sun’s picture

Version: 6.x-1.x-dev » 7.x-3.x-dev
Priority: Major » Normal
Status: Fixed » Patch (to be ported)
sun’s picture

Sorry for the noise; now for real.

iancu35’s picture

Status: Patch (to be ported) » Needs review

#1: 1189672-1.patch queued for re-testing.

Dave Reid’s picture

Patch attached for 7.x-3.x.

sun’s picture

For 3.x, it's a bit more complicated, as the links are cached on the client-side.

I think the following is all that can be done:

  1. Generate a single token
  2. Supply that token via Drupal.settings
  3. Make the JS append the token to selected links
  4. Validate the token in the callbacks (as in the patch)

Hm... This also means that one is no longer able to recover from disaster situations by entering and hitting /admin_menu/flush-caches manually. Saved my day a couple of times already, especially in D7... but well, need to find a different solution in those cases then.

Dave Reid’s picture

Well - I tested this out though and the cache is per-user so this should actually be fine.

sun’s picture

Did you log out and back in? The session ID is part of the token, whereas the cached menu stays identical across sessions (until menu cache is cleared).

Of course, there'd be two ways out... we could as well add the session ID to admin_menu's cache hash. But I'm not sure how often Drupal actually regenerates a session though.

Dave Reid’s picture

Yeah, caching by session ID probably is better. Revised patch.

Dave Reid’s picture

Assigned: Unassigned » sun

Assigning to sun for review since this changes admin_menu to be cached by session ID rather than user ID.

sun’s picture

Thanks, looks good.

Merely this:

This also means that one is no longer able to recover from disaster situations by entering and hitting /admin_menu/flush-caches manually.

still concerns me. At least I need to recover from totally bogus data very very frequently on local dev sites.

I wonder whether we could add a 'admin_menu_security' variable, defaulting to TRUE, which would allow developers to disable the token validation by setting it to FALSE in settings.php?

Other ideas welcome.

Dave Reid’s picture

I would think that we'd recommend update.php for that now that it flushes caches regardless of if there are updates to run or not?

sun’s picture

Status: Needs review » Fixed

Ah, yeah, forgot about that issue.

Well, that applies to D7+. As I'm personally not really developing on D6 anymore, I'd be totally fine with the "regression" in D6.

So let's see how this goes.

Thanks for reporting, reviewing, and testing! Committed to all 3.x branches.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

sun’s picture

Committed the attached follow-up fixes to 6.x-3.x.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

sun’s picture

The change form $uid to session_id() seems to have caused a critical bug when switching users:
#1468482: Menu is cached by session ID, but session ID does not change when switching users via Devel