The permissions doesn't make it clear that it's using php. Instead of "use drupal executor" something like "execute php in Drupal executor" might be better.

drupal_executor_ajax is vulnerable to CSRF. HTTP referrers can be spoofed. I suggest using a token system - see http://drupalscout.com/tags/csrf for details.

You should add a dependency on php.module in core, that's a typical way to provide this feature.

Instead of doing your own output buffering and using eval, you should rely on http://api.drupal.org/api/drupal/includes--common.inc/function/drupal_ev...

All that said, you could also consider abandoning this module because devel module already has this functionality as this issue suggests: #1316052: Explain the difference from Devel's "Execute PHP Code".

Comments

grisendo’s picture

grisendo CreditAttribution: grisendo commented

Thank you very much for the suggestions.

I will try to apply all of them, I have some doubts:

  • Changing the permission name: OK, I'll change it.
  • Use token system for avoiding CSRF: Of course, I'll change it, I just saw it few minutes ago into "Admin menu" module with "Flush all caches" and "Enable/Disable developer modules". Can I ask you something? It shouldn't be also safe to check HTTP_REFERER as I do right now? I am not an expert into security as you can see... However, anyway I'll apply the token approach.
  • Dependency on php.module: Is it really necessary? I'll add it if it's necessary, but why is it necessary since I don't use any function of that module?
  • Use of drupal_eval: OK, I'll change it. I have to check if everything in the output is equal, since I made a little look into "drupal_evel" and I saw it does something changing themes.
  • Abandoning the module: I was considering it after seeing the response, but I saw some differences between them, as I explained in #1316052: Explain the difference from Devel's "Execute PHP Code". Anyway, even if I have to abandon the module, as our company will keep using it during development (disabling it in production sites), at least I have learned a couple of things about security :)

Thank you!

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented

The benefit of making a dependency on php.module is that things like http://drupal.org/project/paranoia will block php.module. So if your module relies on and uses that code then people who block php.module will know that your module cannot run on their systems.

If the "execute php" feature of devel is not good enough then the solution should be to work to improve it rather than duplicating it.

grisendo’s picture

grisendo CreditAttribution: grisendo commented
Status: Active » Closed (won't fix)

Obsolete/Abandoned project