This is currently insecure by default. PHP code shouldn't really be hiding in the UI of any Drupal site, but if we're going to maintain support for it, let's at least make sure it requires the presence of the generally disapproved PHP Filter module to use.

Comments

rszrama created an issue. See original summary.

nicholas.alipaz’s picture

nicholas.alipaz commented

I am personally against the need for PHP filter to enable the option for PHP based conditions. The reason being is like you said, PHP in the database is insecure by default. Making the users enable it on the site opens up the rest of the site to the possibility of it being enabled elsewhere (can of worms). Better to leave it a alone in my opinion, not to mention the difficulty in making users enable and configure the PHP filter to continue with upgrade to the next iteration of this module. Sounds like a major change with possibility of breaking existing functionality for current users.

rszrama’s picture

rszrama commented

Ok, I'll remove the capability entirely in a 2.x branch.

rszrama’s picture

rszrama commented

Note: when I do so, I'll also add a hook into the page visibility settings check so custom PHP can still be used to enable the JavaScript.

rszrama’s picture

rszrama commented

lol, in fact, the module currently only evaluates the PHP if the PHP module is enabled, meaning the settings form's PHP option wouldn't work anyways. I think that might mean no one's using this feature. ; )

      elseif (module_exists('php')) {
        $page_match = php_eval($pages);
      }
rszrama’s picture

rszrama commented
Status: Active » Fixed

Given the current behavior of the module is to only evaluate PHP if the PHP Filter module is enabled, I'm going to add the same check to the settings form. That seems to be the safest way to resolve the conflict (as opposed to simply removing the module_exists() check in the visibility code, which would turn on who knows what code in sites that were trying to use it before). I'll spawn a follow-up issue for removing this in 2.x.

rszrama’s picture

rszrama commented
Status: Fixed » Active

Derp, forgot patch.

rszrama’s picture

rszrama commented
Assigned: Unassigned » rszrama
Status: Active » Fixed

Ok, committed.

  • rszrama committed 570eab4 on 7.x-1.x 
    Issue #2683647 by rszrama: restrict the PHP based visibility option on...

  • rszrama committed 570eab4 on 7.x-2.x 
    Issue #2683647 by rszrama: restrict the PHP based visibility option on...
nicholas.alipaz’s picture

nicholas.alipaz commented

Good sleuthing, I agree whole-heartedly with your plans.

Status: Fixed » Closed (fixed)

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