Remove php filters from filter module, into a dedicated module,

If you cant sleep due to PHP filters you should get out of the hosting business. ;->

SCNR

See http://drupal.org/node/46857 also.

I prefer this solution over the one proposed by chx.

+1 on this approach.

For hosting providers, this is fairly important.

the php filter is like handing people a loaded gun.

I thought this as the second step after my patch. But even if only this one gets in, I am fine.

And this is not post 4.7 but pre 4.7. Releasing another version with PHP filter... dangerous.

I prefer this approach over chx's too. Some notes:

* I'd rather it be called filter_php.module, to sort properly and to be more indicative of it's function.
* I'd all for it being post 4.7, and not pre 4.7. This is, IMO, not as big a problem as chx makes it out to be.
* "run PHP (be carefull)" is, IMO, too .. .. . .. "condescending". I'd much rather a neutral "allow input to be PHP".

I'll see about working on a revision sometime next week.

I think this needs to be done as an option and not an absolute (i.e. we are removing this from core and making it a contributed module). This has the potential to cause problems for a lot of sites that might be using this if they all of a sudden upgrade and can't author PHP anymore. PHP is frequently used for small dynamic blocks and pages, see: http://drupal.org/node/39282

If we are talking about a hosted situation wouldn't someone be able to download this module and install it anyway? If they don't have FTP access then it could be something in the settings.php file.

This approach seems to drastic to correct a problem that doesn't exist for many people. It would be good as an optional feature though.

Ber's patch contains the php_filter.module , we could keep it in core.

+1, I think this is an excellent idea.

+1 for this concept.

+1 for me. php code evaualtion should be on an separate module.
this patch also adds the permissions "run PHP (be carefull)" which is more
straightforward than going to administer » input formats » PHP code » edit .

A little problem:

if ($delta == 0) {
   return drupal_eval($text);
else {
  return $text;
}

should be:

if ($delta == 0) {
  return drupal_eval($text);
}
else {
  return $text;
}

fax8: please recheck your comment. I am unable to detect the difference between those two snippets.

Morbus, I think it's in the indentation... :-)

looks good to me. we need to update existing input formats, right? also need to enable this module for sites currently using the filter. the update path needs work.

+1 for this.

Keep it in core, as a separate module, disabled by default.

Let people enable it if they so want. Staying in core means that it will be constantly scrutinized and not neglected/overlooked.

Morbus Iff, the if block doesn't have a closing }.

Ah! Thankee.

• Update path definitely needed (the delta for the linebreak filter has changed as well!)
• The permission 'run php' is not used in the php_input.module, and is not tied to the PHP filter. So it is misleading and unnecessary.
• The $delta == 0 stuff in php_input is unnecessary, it is only needed when you have multiple filters in a single hook_filter().

Also, by requiring the user to set up the php_input module, there is a risk that they will plunk it into the default Filtered HTML format. How do we avoid this?

• I know that the upgrade path is being worked on. Should be pretty easy, one query against system (flip the new module on) and one against the filters table.
• Yes, the permission is not needed.
• I advice to avoid the filter clash by assigning a negative delta to those filters which can not be combined with any other. Checked database tables, there is no "unsigned" on filters.delta. filter_admin_filters_save is the place to check this. BTW. there is nothing to stop the idiot currently from adding PHP filter to filtered HTML. But I can see your problems so I am willing to cooperate with Ber on this.

ok folks. anyone willing to refresh this patch and write the update?

I've wrtiten paranoia.module which disables the creation of new php filters. You still need to delete the existing one manually (although I could move that into the install script).

Is this a useful solution to this thread?

Lets consider this for the next version.

Reviving:

Rerolled Bèr's patch + changed php_input to php_filter + incorporated Unconed's points.

Update patch + language fixes to be addressed once primary changes are given the green light.

Please feel free to update patch rather than wait for me to do so :)

Thanks,
-K

Missed a chunk.

Re. Steven's question, how we avoid putting PHP filter into Filtered HTML -- what about a mechanism that would make it possible for a filter to define that it is a standalone filter and it can not be mated with anything? Or if it is decided that it is only PHP filter that needs this mechanism, then simply make filter module recogn it and refuse to put it into any format which is not empty and refuse to add anything to a format which contains the PHP filter.

I personally think that a dire warning will suffice. Let's not over-engineer here.

Thanks,
-K

First of all, i did not test the patch (yet)..
But wouldn't it be better, to not display the option at all if the sufficient permissions aren't there for the user?

Once again, I did not tested the patch yet...

Looks good, but please rename the module to php.module. Thanks.

Blanket rename of php_filter to just php as per Dries' request. No other changes. Setting to RTBC.

I'm working on the updater patch as we speak.

-K

Updater patch attached. While #35 is RTBC, this patch is _not_. Please review and test.

Thanks,
-K

This change is indeed needed.

At the same time, we should be developing a broader solution to the problem of conditionally limiting PHP input. I've posted a complimentary patch to add a variable we can set in settings.php, 'php_input', which would be the means of preventing all user-input PHP code on a site, http://drupal.org/node/134779.

I'm not convinced this is the best approach... if you are afraid of PHP input, you may want to lock down other things on a Drupal site too. A paranoia.module seems a much better solution than removing useful features from Drupal core. Unless someone can bring up some solid arguments why that approach is not viable, I'd prefer not committing this patch.

Pasteable PHP snippets are a very useful way of customizing your site for example. With this change, we now require novice users to 1) Enable an additional module 2) Add a new input format, with the right permissions 3) Add the filter to the newly created format. If we do go down this road, at the very least, there should be a module_enable hook to create the format with the filter in it, otherwise we will have clueless people dropping the php filter into the Filtered HTML format.

As for a global php_input variable... this is very un-Drupal-like. It doesn't appear like the variable would cleanly disable PHP from the UI everywhere, but merely lock it down for use. That could lead to a frustrating experience for users for that installation.

I love having PHP around to do quick stuff on my site (running queries from node/add/page -- lovely!) . As for security well, it can be dangerous, but to the hell with it, we have now much better defaults and the choice checker than we had a year and a half ago. I would like to hear what Heine thinks over this one.

.install file patch attached to add a PHP code input format.

There are 3 patches now:
#35 - reviewed.
#36 - To be reviewed & tested.
#41 - To be reviewed & tested.

1. I think it makes sense to move this to an additional module that can be removed from disk.

2. At the same time, I agree with Steven that it is safer/better if _we_ can pre-configure this, then when the user has to configure it him selves. It's less prone to errors.

I think we can do (1) and pre-configure the PHP input format so (2) isn't a concern. Heck, we could even enable the PHP input filter by default but still make it easy to disable.

----

Another solution, but not necessarily better, would be to rely on PHP's "disable_functions" setting. You can globally disable function calls by listing them in the disable_functions variable in your php.ini file. I haven't tried it yet, but if a hosting company globally disables eval(), maybe we can wrap the PHP filter code in a function_exist() call? So, when eval() is disabled, we degrade gracefully (assuming that function_exist() would work for disabled functions). Not sure if it would work, but I figured, I'd float the idea.

The disable_functions approach might be better because it is a technique that hosting companies are probably already familiar with -- it fits their workflow. However, it might be problematic because other parts of Drupal might rely on eval(). The locale module, for example, uses something like '@eval("return intval($locale_formula);");' ...

I'm just wondering if paranoia.module isn't better because there are other things to consider. A somewhat insecure contrib module could allow a privileged admin to upload .php files for example. And generally, you may want to lock down some other settings in the administration, like allowed file extensions. Remember that .pdf XSS vulnerability which is still out there?

It just seems to me that turning the php filter off is just one of many things that need to be done, and that there is very little reason to make core less useful out of the box. The PHP code filter is way more than just a development tool. It helps people build sites and gives those who would otherwise shy away from PHP an easy way of customizing simple snippets.

As long as it as simple as turning on the relevant module, I'd be okay with it though.

This would be easier to evaluate as a single patch rather than three. The first one at any rate needs more work (see below).

As long as it as simple as turning on the relevant module, I'd be okay with it though.

I think that's the idea. In fact, most people won't have to turn it on--that will be done in the relevant install profile.

Besides the PHP filter, the other part of core we need a solution for is the PHP block configuration. That's what's in my patch at http://drupal.org/node/134779 (and, yes, it deals with the UI as well as the data handling). Should I rewrite that to be part of this patch?

I'm thinking we should move drupal_eval() into php.module, perhaps renaming it to php_eval(). That way we know it can't be called unless the module is present.

When this patch is applied, contrib modules wanting to conditionally add user PHP input could test for the php module:

if (module_exists('php')) {
  // PHP input here.
}

(replacing the need for the configuration setting I'd previously suggested).

Steven -- what else would a paranoia module do, and how? Could you try and provide an itemized list of some ideas?

I see two problems with a paranoia module:

1. The user can disable/enable it. We want to ISP owner to have control here, not the user. So when the ISP owner, removes php.module, he/she knows that the site owner can't re-enable it.

2. ISP have their own workflow and audit processes. When you look at this through the eyes of a very conservative ISP, there is only one solution ... and that's to disable certain functions at the PHP-level. It's a simple switch that is guaranteed to work, and that doesn't put additional burden on their shoulders. They don't want to audit our code, or inspect whether the paranoia module really works. All they want to know is whether Drupal degrades gracefully when eval() is disabled. If I were an ISP, I would not want to rely on or trust paranoia.module, I just want to use disable_function. Is that a viewpoint you can relate to?

However, if you look at this from a user's point of view, a paranoia module might be the better choice.

Update number changed from 2007 to 2008.

#35 - reviewed.
#46 - To be reviewed & tested.
#41 - To be reviewed & tested.

I think everybody is in general agreement that the filter should live in a separate module #35: User Node Favorites/Bookmarks. There is no other way to disable it from everybody including UID #1. It is still going to remain in core.

New installations will (based on the installation profile) have it disabled by default; it is safe to assume that by the time an admin decides that he needs a PHP filter, he will have understood how to enable modules. Enabling the module #41: User option to make email address visible. installs the format and sets everything up.

Upgraded installations will see no difference [#46] other than having the ability to turn it off via the modules page. A warning has been provided for this case.

ISPs aside, without the PHP filter disabled, one cannot have a safe multi-site installation without custom frippery. Multi-site A's admin will be able to access the settings.php etc. of multi-site B using the PHP filter. The point of a multi-site is lost unless all the sites have the same admin or something similarly restrictive.

-K

Dries: http://drupal.org/project/paranoia

You can't disable the module if you can't access the database.

Zen -- looks good to me. When I enable the PHP filter module, will it properly configure itself or is that left for the user?

Updated the update patch to sync with HEAD.

@Dries: The .install and the system update both create a PHP code input format and add the PHP evaluator filter to it.

#35 - reviewed.
#49 - To be reviewed & tested.
#41 - To be reviewed & tested.

Thanks,
-K

And the patch :/

#35 - reviewed.
#41 - To be reviewed & tested.
#50 - To be reviewed & tested.

-K

Zen, could you roll everything into one big patch. I'd like to test and commit this. :)

All in one patch.

Thanks,
-K

I've tested this, and committed it to CVS HEAD. We can refine this as necessary but it marks an important first step. Thanks Zen and Ber.

http://drupal.org/node/124158 follows up on this patch by moving PHP page matching and drupal_eval() to php.module.

Unfortunately the upgrade path of this patch is less than satisfactory. See http://drupal.org/node/194304