Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Having the PHP module in Drupal Core encourages new users of Drupal to enable and use it. When people use the PHP module, it can result in:
- Security holes when used improperly
- Instability when upgrading their site because they may be using some PHP code that might change between upgrades
- Performance issues as PHP code in nodes cannot be cached properly
- Shared systems, like Drupal Gardens, must hide it from their users in order to ensure safe usage of their service
Proposed resolution
- Remove the PHP module from Drupal core
- Move it to a contributed module at drupal.org/project/php
- People who do want to use it at least have both the access and knowledge to install new modules. If they have this knowledge, it is assumed that they know what they're getting themselves into by using it.
- Ensure security updates and improvements are maintained on the contributed module
Remaining tasks
- Patch needs to be reviewed
- The contributed PHP module needs additional maintainers
User interface changes
- The PHP module is no longer in the modules list
- The contributed module needs to be installed in order to have it back in there
Related
Comment | File | Size | Author |
---|---|---|---|
#164 | changelog.patch | 313 bytes | RobLoach |
#161 | 1203886-161-do-not-test.patch | 44.07 KB | RobLoach |
#160 | 1203886-156.patch | 44.07 KB | alexpott |
#157 | 1203886-157.patch | 20.05 KB | alexpott |
#156 | 1203886-156.patch | 44.07 KB | alexpott |
Comments
Comment #1
droplet CreditAttribution: droplet commentedWhy?
Comment #2
pillarsdotnet CreditAttribution: pillarsdotnet commentedShould be in contrib. The namespace is available.
Comment #3
Garrett Albright CreditAttribution: Garrett Albright commenteddroplet: Because with PHP module in core, people actually use it. And make their sites slow and/or insecure in the process.
There's nothing the PHP module does that can't be done by writing a proper module. All it does is let people cheat and shortcut around doing things the correct - fast and secure - way.
Agreed with pillarsdotnet that it should be a contrib module if anything - with a whole lot of warnings attached to its use.
Comment #4
pillarsdotnet CreditAttribution: pillarsdotnet commentedHere's a good example.
Comment #5
pillarsdotnet CreditAttribution: pillarsdotnet commentedAnother one.
Comment #7
catchPossible route towards an upgrade path for those who would not like to install the PHP contrib module: http://drupal.org/project/modulate
Tests fail because we're using PHP module as 'a random module' in lots of tests apparently.
Comment #8
pillarsdotnet CreditAttribution: pillarsdotnet commentedAnother alternative would be to patch the PHP filter to require that the node author/editor be
uid==1
or be a member of the 'administrator' role.Comment #9
pillarsdotnet CreditAttribution: pillarsdotnet commentedSee #1208988: Deny use of the PHP evaluator filter to Anonymous or Authenticated roles.
Comment #10
webchickHm. I'm not sure about this. One really nice thing about PHP module being in core is that as of D7, all modules can use it to hinge their "Use PHP to control X aspect of this module" ... Block visibility, dynamic field values, etc. If it's shut off (and forced off through Paranoia module or similar), all of those other potential holes vanish with it.
Very few modules are going to want to add an external php module dependency, however, so we'll be right back to losing the battle with "Administer FOO PHP PERMISSION" again...
Comment #11
catchI don't see how a move to contrib affects existing modules all that much, if the PHP stuff is optional, they're using module_exists('php'). If it's not optional, the whole point of the module is putting PHP in textareas and they're specifying dependencies[] = php already - so they don't need to do anything?
Having said that, I'd be fine with a two-step process. Remove the filter from core first - since that's the part that pillarsdotnet demonstrated so amusingly above is not doing anyone any good. Then discuss the general "use PHP for" stuff separately.
Comment #12
coderintherye CreditAttribution: coderintherye commented+1.
We removed PHP from core when creating a D6 distribution for our university. The reason was because we wanted to deploy thousands of sites, and give site administrators full control of the site, but without allowing them to execute arbitrary PHP code on the server. How much of a real security benefit this was would be hard to quantify, but it was definitely a big step forward in getting Drupal accepted for wide-scale deployment. It also had the added benefit of making it easier to convince people to code actual modules to solve use cases (since they couldn't just throw some random PHP in a block), and thus we could run coder over their code to help them follow best practices.
Comment #13
catchAlright I'm re-purposing this a bit.
PHP module currently does two things:
1. Provides drupal_eval() and an associated "Use PHP for settings" permission. This is the part that contrib modules are able to re-use.
2. Provides an text filter that lets you embed PHP in nodes, comments, custom blocks etc. this is the part that allows admins to easily give anonymous users permission to execute PHP on their sites, as so beautifully illustrated above.
While I don't think the former has to live in core, neither is it doing a massive amount of harm there.
However the text filter is actively harmful, and if moved to a contrib module would probably only be used by sites that were upgrading and had lots of legacy PHP content already (and couldn't be bothered to properly convert that into custom blocks). So removing that should be a lot less controversial, and fix most of the issues.
That leaves a PHP module that doesn't actually do anything useful by itself, but sometimes it's better to do nothing :)
Comment #14
sunWe need to find a maintainer that cares for issues like #486094: Allow to pass local variables to php_eval() -- without resolving issues like this, I somewhat fail to see the actual general purpose usefulness of php_eval() in the first place.
However, we can definitely drop the input filter. Which also solves #826550: PHP module should not create a text format in one shot.
Comment #15
catchTwo weeks later maybe there's a chance.
Comment #16
chx CreditAttribution: chx commentedIf the bot agrees, i agree as well.
Comment #17
sunFor now, I'd limit it to the patch in #14 though - for the reasons @catch outlined in #13.
Comment #19
catchI will 'maintain' this module in contrib if it is removed, if at least one person offers to co-maintain it with me.
In maintaining I will do at least the following:
- review patches for 8.x ports
- pointing people to http://drupal.org/project/modulate so they can transition away from it.
- probably add something to shout at people if they enable it.
Comment #20
msonnabaum CreditAttribution: msonnabaum commentedSounds like a good idea to me.
I would be willing to co-'maintain' said module with catch.
Comment #21
catchNote current proposal is to remove the PHP filter, not drupal_eval() and the permission.
Comment #22
Akaoni CreditAttribution: Akaoni commentedSubscribing.
Comment #23
droplet CreditAttribution: droplet commentedI'm still don't understand....
Security
why #4 ,#5 are security issue. If someone config Drupal into insecure, does it a problem of Drupal? Even writing code in .modules, it can be insecure code too ??
PHP module move to contrib. does it harm own site ?? One good thing of drupal is some guys monitor modules security issue, why we allow a harmful module on d.org ?
Because I'm one using PHP on block on existing sites, really want to know it.
Who can broken your site ?? users with permission or everyone ??
Performance.
It's not an issue of Drupal. every modules & everything can get it slow down, none of your business. (Only enable the modules won't slow down the site ?)
Best practices ..etc
Why you limit users own style?
Can't it make something to improve the PHP filter ??
"pointing people to http://drupal.org/project/modulate"
If this is the good solution & replacement. Can't it get into Core & combine with PHP filter to do some automatically method to get them done.
Comment #24
Garrett Albright CreditAttribution: Garrett Albright commenteddroplet, removing the filter from core will slow down people who would use it, and maybe give them time to find a better solution in the form of a contrib module which will do what they're looking for, or creating a custom module properly. Those like you who are in the bad habit of using it will still be able to do so with a contrib module if they so choose to do so, but hopefully this will slow down those new to Drupal.
The question is, when you have a problem to solve that requires writing PHP code, what's the proper way to solve it? Is it to use the PHP filter, or to write a proper module? The answer is always to write a module; no exceptions. However, when we have the filter in core, people see it and think that using it isn't that bad; encouraged, even. Removing the filter will avoid that misconception.
Comment #25
catchRe-uploading #14 so we can start again from there. Several people in irc did not want drupal_eval or the permission reviewed (including Earl for Views), so one step at a time.
The test failures look dodgy to me.
Comment #27
sunComment #28
coderintherye CreditAttribution: coderintherye commentedThe module dependency test fail seems like it could be legit, although the test itself looks bunk or out of date, since forum doesn't really rely on PHP does it or perhaps it does in a round about way.
That said, I'd say we should rewrite the module dependency test to enable some other module which has a dependency, but not a dependency on the PHP module. There are plenty of others that we could use in that test.
Feel free to ignore this reasoning if it seems incorrect. Here's the relevant code for that test
Module dependencies (ModuleDependencyTestCase) [Module]
Dependency chain created. Other system.test 467 ModuleDependencyTestCase->testModuleEnableOrder()
Comment #29
quicksketchI'll help maintain it also. I did the same thing by separating out the Webform PHP module from the Webform project. I maintained it for 1.5 years and then marked it abandoned, but immediately someone else came along to continue supporting it. Removing a module from core is of course a much bigger deal, but something as simple as PHP Filter would be a breeze to maintain in contrib. I'm also behind catch's suggestions to only remove the filter. I'm fine with drupal_eval() and the permission staying in core.
Comment #30
AaronMcHaleWill a contributing PHP module just add a filter to nodes, comments, etc, and core would still contain the ability to run PHP code in blocks etc?
Comment #31
Garrett Albright CreditAttribution: Garrett Albright commentedAaron, no. If this is done, it will not be done halfway. But there's no reason the contrib module version won't be able to continue to allow PHP code in blocks.
Comment #32
AaronMcHaleGarret - thanks for that.
Comment #33
David_Rothstein CreditAttribution: David_Rothstein commentedWell, just to clarify, the current consensus in this issue (and implemented by the patch in #25) would remove the ability to have PHP code in the body of a block, but it does not remove the feature where you can use PHP to set block visibility; that would still remain in core.
Here's a reroll to apply to the latest HEAD, which also:
The one remaining reference I found which I did not remove in this patch (and which may need a little discussion) is the reference to the PHP filter in the text_summary() function. For example, this code:
It would be great to remove it and just leave it up to the new PHP filter module (in contrib) to alter that behavior back in, but offhand I'm not sure how that would be possible...
Comment #34
pillarsdotnet CreditAttribution: pillarsdotnet commented@#33 by David_Rothstein on November 20, 2011 at 12:05am :
See #221257-169: text_summary() should output valid HTML and Unicode text, and not count markup characters as part of the text length
Comment #35
David_Rothstein CreditAttribution: David_Rothstein commentedHm, so I actually think it's even simpler than that. I looked into this issue a bit, and it seems to me like the check for the PHP filter here serves no purpose whatsoever. That is because check_markup() has already been called (and therefore, any PHP code has already been evaluated) before the text that's being trimmed is ever passed to text_summary().
That means for our purposes we should just be able to go ahead and remove it. (I've also created an issue at #1347920: text_summary() checks for specific filters, but it should be agnostic to filters for the more far-reaching fix, since there are more things we can remove also.)
The attached reroll therefore removes that last reference to $filters['php_code'] from the codebase. It also removes the PHP module from the list of dependencies for one of the tests I fixed above (since the test is no longer using the PHP filter, it no longer requires the module either).
Comment #37
David_Rothstein CreditAttribution: David_Rothstein commentedOops, I got carried away and deleted a little too much from text_summary(). This should work better.
Comment #38
AaronMcHaleHave I got this right, or am I reading this from somewhere else, because I remember seeing this somewhere, that this new contributing module would remove the separate PHP Filter created by default when enabling this module?
Thanks
AaronMcHale
Comment #39
sun#37: remove-php-filter-1203886-37.patch queued for re-testing.
Comment #40
wizonesolutionsSince this is actually a core issue now, I thought I'd name-drop PHP filter. It was a probably-good idea that I ran out of time (and arguably interest) to develop further.
Comment #41
David_Rothstein CreditAttribution: David_Rothstein commented#37: remove-php-filter-1203886-37.patch queued for re-testing.
Comment #42
David_Rothstein CreditAttribution: David_Rothstein commentedI think this issue actually counts as a security improvement, because if the PHP module really were to become nothing more than a wrapper around the php_eval() function and the "use PHP for settings" permission (as proposed here), we can consider making it a really hard requirement that no Drupal module is allowed to execute user-provided PHP code without checking for this module being enabled and this permission being granted.
This will make it easier to isolate PHP execution to one part of the codebase, and for people who don't want to allow it on their servers to more easily block it.
I think this is sort of recommended behavior already, but because turning on the PHP module has these other side effects (creation of the text format and filter, etc) some module maintainers don't really want to tell their users to turn it on in order to unlock "import" functionality or other functionality that requires PHP code to be executed.
Comment #43
coderintherye CreditAttribution: coderintherye commentedThank you David for summing up one of the reasons why this is an important and needed change. Looking forward to what the test results say here.
Comment #44
RobLoachAttached is a re-roll. Was getting a couple patch warnings. Although I'd opt to removing the module in its entirety and moving it to http://drupal.org/project/php , I'd say this is a good first step. Where in the configuration interface to we ask people to write PHP code? Seems like a major security flaw to me. If Drupal's or PHP's API changes, it could bring down sites.
Comment #45
cosmicdreams CreditAttribution: cosmicdreams commentedlike how a server's lack of php 5.3 brings down sites? These are deployment issues mostly. I'm all for removing the php filter into a module but I rather like David's take of built in protections against user supplied php in the config.
As for php's use in config, I've mainly seen in present when deciding where a block should be placed or some kind of logic that the module is looking for. A site builder typically gets the option to specify a white list, a black list, or custom php logic.
Comment #46
timmillwoodPeople new to drupal seem to find the PHP Filter and use it instead of creating custom modules.
It needs to be in contrib with a big warning saying "Are you really sure you want to use this module? why not just write a little custom module, here's the documentation on that."
Comment #47
corbacho CreditAttribution: corbacho commentedA typical use case of a CMS newbie: "I want to add the current year in the footer". He searches in Google and found many answers telling about, you just need this Copyright ©
echo date('Y');
* In wordpress: Just copy/paste that in footer.php of your theme (inside Wordpress admin panel)
* In Drupal, it requires 3 or 4 steps now (enable module, add block, select input filter, etc). If we remove PHP filter from core, add couple of steps more and 5 minutes?
This was just a silly example. I understand the huge security improvement as a Drupal developer and I'm totally in. But we have to be aware that we are making Drupal even more frustrating for not drupal-savy users
EDIT
@pillarsdonet. (you can't edit page.tpl.php from the admin UI, that's what I meant)
Comment #48
pillarsdotnet CreditAttribution: pillarsdotnet commentedIn Drupal, edit your page template and add the same text.
Still not convinced we need to store PHP code in the database.
Comment #49
pillarsdotnet CreditAttribution: pillarsdotnet commentedSo? Use elFinder then.
Comment #50
timmillwoodYes this module has it's uses, but does it need be in core? No.
Comment #51
webchickI think people grossly underestimate how difficult it is for people new to Drupal when we punt on things with, "Oh, there's a contributed module for that." There are sixteen thousand contributed modules. The chances of finding the one that does what you want when you don't even know what modules are yet are astronomically low. And "Oh, you should really just make a module for your footer text?" Please. :)
I tend to agree that this is basic functionality that I would expect to be included out of the box, and I don't understand why kicking it to contrib is viewed as making anything more secure. It merely makes Drupal core more limited and less useful to various "real world" scenarios that inevitably pop up.
Comment #52
catch@webchick, have you seen the attachments in post #4 and #5 of this thread? Putting this functionality in contrib is designed to make it harder to do that.
Footer text is entirely customizable in core at the moment, what we'd be dropping support for is putting arbitrary PHP in there which is considerably more obscure.
And if things like putting the current date in the footer are so important it'd be better handled by something like token support.
Comment #53
webchick(Summary of ensuing IRC argument :D)
It only makes it harder to do that because it makes this incredibly useful for newbies functionality impossible to find. Seriously, my first few Drupal sites I couldn't have built without the PHP input format, and I know of several people who eventually found their way to module development through using snippets from the handbook in PHP input format in blocks/nodes. Tokens are nice for things like date, but don't cover numerous other things people might want to put in there.
#4 and #5 simply say to me that we probably need some kind of a security threshold mechanism in core for text formats, similar to our janky permission descriptions but more forceful, that warns the crap out of people if they enable dangerous filters for non-privileged roles. You can pwn a site with Full HTML, too. (Granted, not quite as easily, but enabling Full HTML for anon is way more common than PHP.) Punting this capability to contrib doesn't in any way make that problem go away.
I really strongly feel that if this capability is removed, many new site builders will get completely stuck their first time or two out in building their sites. I'm totally fine with updating the module description to warn people that this is bad juju, but removing it from core altogether I feel would be a huge mistake. We don't babysit people as a general rule; we give them tools they need to solve their problems.
Comment #54
timmillwoodI agree that 'php filter' is a great tool for noobs. It's the way I learnt Drupal, it's the way I still sometimes test code snippets before putting them into a module, but in my day job I see so many huge enterprise level sites that are using it for large sections of the site. It was only a few days ago I saw a site with nearly 1000 nodes with php in. I understand that the education needs to be there to stop people doing this, but as I am often dealing with legacy sites who are just getting around to resolving these sort of issues, it's a little late for them. I am trying to think about new sites and how we can prevent them from making the same mistake.
Comment #55
corbacho CreditAttribution: corbacho commentedMoving the PHP filter to contrib doesn't solve the problem of #4 or #5 (only makes it a little harder). After placing it as a contrib module, I'm sure it will be anyway one of the most popular modules.
People will still install the "PHP filter" contrib module and produce anyway all kind of awful sites from Engineers point of view.. that you can't stop it. People, non-coders, want to put custom code everywhere specially 3rd part plugins and stuff like that. Unstoppable.
What can be wiser is to modify the input filter, with something like this in core: #1228748: New security check: PHP filter available to untrusted roles that checks / forbids dangerous configurations (PHP input in comments) and educate Drupal admins of the dangers of PHP as input filter.
------------
I took a little time to research about other CMS:
Joomla: If you order by "popular extensions" the 5th one is "Jumi", that lets you put PHP, Javascript, etc into your content.
Wordpress: core lets you to put php in the templates files via admin interface, but not in widgets and posts. So you need a plugin. One of the most popular ones is http://wordpress.org/extend/plugins/php-code-widget/
Conclusion: Adding custom code is a really popular thing to do, you have it in core or not
PHP filter module is really useful as block filter, damn bad idea as comments input filter (or node input filter)
Maybe is a Drupal chance to rethink the PHP input filter with extra permissions/ checkings, or "pass the potato" to a contribute module as it is. Both solutions have trade offs.
Comment #56
Jody LynnI second timmillwood in #54. We're making it too easy for people to destroy their sites, and practically putting a seal of approval on it by having it in core. I also am perpetually dealing with enterprise sites completely trashed by php in the nodes.
Using PHP in blocks is not quite as bad to me, though, and the use cases for it from noobs are more understandable. Because nodes are meant to be content though, using it there tends to be a part of much worse hacks. Perhaps one compromise would be to change it from a filter to an option for block content only.
Comment #57
RobLoachWe could move PHP Text Filter to contrib, and have it consumed by Spark. Distributions allow us to re-think the way Drupal is packaged, and Spark is a perfect opportunity to use it.
If people use the PHP text filter, it means that when they upgrade to the next version of Drupal, their site will go down. If their host upgrades their version of PHP and they're using an obsolete function, their site will give a fatal error and break. It's a huge security flaw, and ends up being extremely detrimental to new users rather than helping them.
Comment #58
mitchell CreditAttribution: mitchell commented> Seriously, my first few Drupal sites I couldn't have built without the PHP input format
Module Builder is worth mentioning here. webchick, do you think you would have been able to get started integrating new code almost as easily if that helper module was recommended in place of php.module? Both methods aim to provide a shortcut in the module development process, but MB strikes me as a solution that reinforces more sustainable use of Drupal.
Comment #59
webchickHey, I wrote that module. ;)
And, no, absolutely not.
PHP input format is a matter of: 1) search for problem you're having + "php", 2) copy/paste snippet, 3) save.
Building a module, even with a wizard, is a matter of understanding what a hook is, understanding what hook(s) you need, understanding what files need to go where in your directory tree, and a whole pile of other Drupal-specific things.
And no, moving this to Spark is not an answer, either. That distribution is focused on content authoring experience, so that doesn't make any sense.
Comment #60
wizonesolutionsSo can we just say in big red letters something like (I haven't checked this for pinpoint accuracy):
WARNING: YOU ARE SUBJECTING YOURSELF TO THE FOLLOWING:
At least let them know there may be some foot-shooting going on.
Maaaaaaaan I (almost) wish I had the time to work on PHP handler. The project page brings back fond memories. Co-maintainers welcome :D *ahem*
Comment #61
RobLoachThought I'd tally up the benefits and cons of having PHP module in Drupal Core:
Benefits of shipping PHP module in Drupal Core
Cons of shipping PHP module in Drupal Core
Comment #62
RobLoach.
Comment #63
droplet CreditAttribution: droplet commented@#62 is a big problem but it doesn't work properly ?
http://www.armchairarcade.com/neo/node/1618#comment-26184
EDIT:
im being bad guy and test PHP there, but it prints
<?php echo 'abc'; ?>
directly without executedComment #64
corbacho CreditAttribution: corbacho commented#61 It seems webchick and me are playing the devil's advocate role
Drupal 5 -> Drupal 6 we moved PHP Filter to its own module and to be disabled by default.
If people enables it, it's because they need it, nobody is forcing them.
Placing the module in contrib, only it will rise frustration, but still people will do it.
So...
Could be that the key point here I think is to separate "PHP filter" from to "use PHP for settings" (called "use PHP for block visibility" in D6)
That setting is important, for block visibility and for other contrib modules. For example #870938: Add new permission for controlling imports. merlin wouldn't like to force Views users to enable PHP Input filter for that functionality. That dilemma wouldn't be solve after moving the module "as it is" to contrib space.
Is it an option to split both functionalities and move PHP filter to contrib ?. Also, if we could make that PHP Filter is *always* disabled to use for anonymous role, wouldn't solve #62 ?
Comment #65
RobLoachIt works on some sites, but I'm going to tell you which ones. The scary thing about PHP access is that once you have it, you can manipulate
global $user
accordingly (not to mention many other scary things).This is the result of the PHP module being in core. If something is in Core, then people are encouraged to use it. If the module was shipped via a contributed module, then the people installing it at least already have file access to the system. And I assume that means they'd kind of know what they're getting themselves into.
I think potentially having more than 7,130,000 insecure Drupal sites is somewhat more frustrating :-) . You bring up a good point about where do the two places PHP is used go. I'd consider running any PHP code through the browser a blatantly large security hole. We should move it all out to /project/php (to ease the upgrade process). I'm okay with splitting up the functionality too though.
With work going on in #1591686: Add Twig itself and #1499460: [meta] New theme system, we could potentially have "use Twig for settings". Maybe even a Twig Filter too, which would be infinitely more secure than letting users write PHP code in the browser. That'll have to be discussed once that work progresses though.
Comment #66
catchThat's not relevant to this issue at all, we're only talking about removing the filter here, not drupal_eval() or the permission, so it can be struck off the benefits list IMO.
Comment #67
David_Rothstein CreditAttribution: David_Rothstein commentedExactly, and what we're saying here also is that it's even the other way around (a benefit of the change), since as described above maintainers will be much more likely to require this permission if turning on the core PHP module doesn't have other strange side effects such as creation of a new text format.
But if we can't all agree on actually removing the filter from core, maybe we can at least agree on splitting the module in two (and leaving both halves in core as separate modules) and repurpose this issue to do that for starters? That would still have some of the major security benefits we've discussed here. And also, if we ever do agree to remove it from core, having it already split into a separate module would obviously make that process extremely simple as a followup...
Comment #68
catchI'd have no objection to a patch that splits the module in two for now.
Comment #69
wizonesolutions+1 on splitting in two if nothing else.
Comment #70
RobLoachhttp://drupal.org/project/php ... The "use PHP for settings" permission is moved to system module. We probably need a note in CHANGELOG.txt too.
Comment #70.0
RobLoachUpdated issue summary.
Comment #71
RobLoachhttp://drupal.org/project/php ... Updated the issue summary.
Comment #73
RobLoachLooks like the Paranoia module disables PHP module. This still needs a re-roll.
Comment #74
RobLoachComment #76
RobLoach#74: 1203886.patch queued for re-testing.
Comment #78
timmillwoodI heavily support this, every day I have calls with customer pleading them to disable the PHP filter module listing security and performance impacts.
Comment #78.0
timmillwoodUpdated issue summary.
Comment #78.1
RobLoachdsffds
Comment #79
mgiffordI hadn't heard of the performance problems with PHP filter.
I definitely agree with it being yanked from Core though. Just too easy to slip into bad practices and have more PHP in the database rather than in a repository.
This patch applies nicely locally.
Comment #80
timmillwoodIt'd still be great to get this into D8.
On a daily basis I am advising people to rework their sites not to use PHP Filter, and it seems to be assumed it's ok because it's in core.
Comment #81
mgiffordSomeone has to re-roll the patch with updated tests in it unfortunately. Agreed that it would be important to put in to D8 if we can!
Comment #82
RobLoachRequires a re-roll... On the plus side, new users can post PHP. On the negative side, new users can post PHP.
Comment #83
worldlinemine CreditAttribution: worldlinemine commentedIn general, I believe that the PHP filter supports bad practices and encourages new users to adopt these poor behaviors. This leads to a high frequency of unstable and insecure sites each one of which can lead to a negative perception of the greater project. Easing a new users approach to implementing custom PHP should be achieved via alternative paths and is important, but the use of the PHP Filter to support the ease of introduction to the Drupal project is "penny wise and pound foolish".
Common related issue:
Discussed in greater detail and a mention of poorly written PHP code as breaking cron:
Comment #84
tim.plunkettAttempted reroll.
Comment #85
RobLoachUpdated to the latest in the PHP module.
Comment #86
webchickThis is another one of these.
Comment #87
David_Rothstein CreditAttribution: David_Rothstein commentedHaving that in system.module looks wrong on so many levels.
Comment #88
David_Rothstein CreditAttribution: David_Rothstein commentedAlso not clear where this issue is going:
The latest patches, however, remove the whole module... Why did the issue change gears again?
Comment #89
dawehnerLet's open an issue for that: #1889718: Drop php argument validation and default argument
Comment #90
Dries CreditAttribution: Dries commentedI'm not actually convinced we want to remove this module from core. I'd prefer to keep it in core.
Comment #91
David_Rothstein CreditAttribution: David_Rothstein commentedSee #88.......
Comment #92
David_Rothstein CreditAttribution: David_Rothstein commentedRewording the title to be even more explicit...
Comment #93
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedI say it dies.
Comment #94
cweagans+1. Seriously, code does not belong in the database. If you want to write code, write it in a module. IMO, the optimal solution is this:
git rm -rf core/modules/php
but I'd settle for just removing the php text filter.
Comment #95
mgiffordIt's such a powerful tool. We've inherited so many sites where someone has just threw in PHP willy-nilly into a node. It's like a spoke wrench. In the right hands it will make your wheel true, but for the vast majority your more likely to break a spoke or more.
It would just be nice to make it not so accessible. If folks had to install a module before being able to access the filter they might think twice and just writing the PHP properly in code rather than taking the obvious short cut and adding it to the node/view.
Comment #96
cweagansNo. The first problem is that people keep calling this a tool. Code in the database is a bug. Period.
This is easily solved by a bit of documentation:
Rather than putting code in your node body, copy this function into a new module (then provide an example hook_node_view implementation). Rather than putting code in your block body, copy this function into a new module (then provide example hook_block_view implementation). This documentation page would take 30 minutes to write, and if that's all that's needed to remove php module from core, I'll volunteer to write it. This problem is easily solved with documentation, and we should not make it easy for code to be put back into the database.
Comment #97
DamienMcKennaI'd also recommend adding a warning to the description of the PHP module to indicate it is a security hazard.
Comment #98
DamienMcKennaComment #99
mitchell CreditAttribution: mitchell commented#96: Perhaps a new tool could build on the possibility to write PHP files to appropriately solve the use case of coding within the site builder UI.
Comment #100
cweagansHere's how to "to appropriately solve the use case of coding within the site builder UI": don't do it. I will never support including that kind of tool in core, and I will never work on one in contrib. If you want to write code, do it the right way: in a code editor.
Comment #101
cweagansMitchell, also, chx already wrote that: https://drupal.org/project/modulate
Comment #102
RobLoachLikely need an update!
Ran into the PHP module last week. Tried to update a Drupal site, and it resulted in a #whitescreenofdeath. The reason was because a node was using the PHP filter, and calling part of the Twitter module to show recent Tweets. Unfortunately, the Twitter module's API has recently changed, so updating the Twitter module made the PHP filter code fail, and caused a white screen of death. You could blame the Twitter module, but no, it's people thinking using the PHP module is a good idea because it's part of Drupal core.
Let me reiterate... This module sucks and should be avoided. Please don't let it live in Drupal core. We should promote good practices, not terrible ones that cause your site to die.
Comment #103
timmillwood+1 RobLoach
Comment #104
cweagans+1 RobLoach
Comment #105
Garrett Albright CreditAttribution: Garrett Albright commentedThe Ubuntu forum attack and downtime post mortem.
Get PHP filter TF out of core, please.
Comment #106
catchBack to Dries. #90 was more than six months ago and doesn't have any justification for marking won't fix.
Comment #107
greg.1.anderson CreditAttribution: greg.1.anderson commentedWithout #94 (or paranoia or modulate modules), anyone with sufficient access to a Drupal site can enable the php module and execute arbitrary php code. Of course, it is very bad if someone happens to obtain or guess an admin password on a site, but arbitrary code execution makes this so much worse.
Not only should php module move to contrib, it should also require Bad judgement, as shown by #102, #5, #4, etc. Let's get this out of core.
Comment #108
Crell CreditAttribution: Crell commentedJust throwing my belated voice in here: Yes, we do want to remove PHP module from core. I want to remove it from contrib, too, but one battle at a time. :-) I'm with Rob and cweagans. PHP code in the database is a bug, not a feature.
php.module--
Comment #109
catch@greg.1.anderson that's already implemented to an extent: #786208: Support PHP module.
Comment #110
greg.1.anderson CreditAttribution: greg.1.anderson commented@catch well done. Now we only need to decide whether we move php module out of core, or bad judgement in. :)
Comment #111
timmillwoodGuess it's now up to Dries...
Comment #112
gregglesI also read the Ubuntuforums post-mortem and thought of this issue.
I also would like to see the module removed from core by leaving the permission.
To say that it needs to be in core to ease the onramp ignores the fact that Drupal's users are comfortable downloading modules.
Attached is a patch that I think gets part way there, though tests need work.
Comment #113
mgiffordAnother option would be to bring in something like the Paranoia module in Core. It took a while for that module to get upgraded to D7.
Comment #114
gregglesI assume at least this tag removal was unintentional.
I don't see paranoia module in core as a good answer. It actually removes the "use php for settings" permission which I think needs to be in core (so it can be used by contrib, so that paranoia can block it). It's also a much more extreme solution than the proposal from comment #13 on this issue so if #13 is controversial then paranoia is more controversial.
Comment #115
mgiffordYes. No idea why it was gone again when I started editing this reply. Added back in the Platform Initiative too which I had removed (without doing anything intentionally or randomly hitting the keyboard).
Mind you I'm in Rangoon at the moment. The Internet isn't so reliable here.
Comment #116
Dries CreditAttribution: Dries commentedThought about it more and I think it makes sense to delete this module in its entirely, and offer it as a contributed module (either in full or partially).
Comment #117
gregglesI'm hopeful you mean "remove everything except the permission" and not "remove it entirely". We're slowly coalescing around contrib modules using this permission to control access to executing php and it would be a shame to lose momentum on that front.
Comment #118
webchickThe permission could probably just move into system module.
Comment #119
catchI have a vague memory of the permission being in the module so that it was easier for paranoia etc. to prevent people ever getting the permission, but I'm not sure if there's any use cases that couldn't be solved if it's just in system module too.
Comment #120
DamienMcKennaDevils advocate: what's the purpose of retaining a permission for which core has no use? Is there precedent for this?
Comment #121
gregglesIt doesn't matter much to me whether the permission is in php.module or system.module.
Paranoia blocks enabling php.module and it blocks granting the "use PHP for settings" permission to any roles.
By keeping the permission in php.module, someone could get some of the protections of paranoia just be removing the php.module from their site. If we move it to system that idea goes away. So, I guess I have a slight preference for leaving it in php.module out of respect to people who like deleting php.module from their site.
Regardless of where it is, uid 1 can still bypass it (unless #540008: Add a container parameter that can remove the special behavior of UID#1 gets in).
Comment #121.0
gregglesfdsa
Comment #122
greggles@DamienMcKenna - I've updated the issue summary to give some ideas. If you need more basis search the issue comments for the word "permission."
Comment #123
webchickThe purpose of the permission being added in D7 is to provide a single "killswitch" for removing PHP access from all users (except UID1 per #121). If this permission isn't provided by core, then every contrib module providing this functionality starts doing separate "Use PHP for Block Visibility", "Use PHP for Views", "Use PHP for Ponies" permissions (as it was in D6) which are much harder to audit.
Comment #124
webchickOops. Cross-posted with an issue summary update. :D Thanks, greggles!
Comment #125
effulgentsia CreditAttribution: effulgentsia commentedI have no objection to retaining the permission in core, either in system.module, or in php.module. However:
Is that the only option? Why can't there be a canonical php.module in contrib that offers the permission and the eval wrapper, and all other contrib modules use that?
Comment #126
webchickTrue enough, I guess. But since one of the arguments against this feature was "if it's in core, people will use it," it seems advantageous to keep such a killswitch in core so that people will use it. :)
Comment #127
DamienMcKennaJust to seek clarification again, what is the value of considering something (a permission) a global killswitch when a) user 1 is unaffected, b) most of the dangerous, security-threatening, worst-case-used-of-a-technology-ever happens through user 1 anyway?
Comment #128
ezra-g CreditAttribution: ezra-g commentedThis was acknowledged in comment #121 with the reference to #540008: Add a container parameter that can remove the special behavior of UID#1.
Comment #129
gregglesElement b of the hypothetical in #127 hypothetical is false.
Edited to make it clear what hypothetical I was referring to
Comment #130
DamienMcKennaJust to mention it, the only times I've seen the PHP module used it was for user 1, hence my reservations.
Resolving #540008: Add a container parameter that can remove the special behavior of UID#1 would relieve most of my concerns, and I'm sorry for overlooking its gravity when it was first mentioned.
Back to lurking :)
Comment #131
RobLoachComment #133
tim.plunkettYou're not removing the actual filter (
\Drupal\php\Plugin\Filter\Php
)Comment #134
RobLoachThanks! http://github.com/RobLoach/drupal/commit/a3a03c39720a18cd1586759e64a6ea6...
Comment #136
RobLoachUsed a seperate remote as the mirror wasn't quite up to date:
https://github.com/RobLoach/drupalmirror/compare/1203886
Comment #138
RobLoachComment #139
RobLoach#138: 1203886-2.diff queued for re-testing.
Comment #139.0
RobLoachupdated to focus on the current proposal as put forth and endorsed by catch, David_Rothstein, etc.
Comment #140
RobLoachUpdated the issue summary, and ensured the PHP module is functional.
Comment #140.0
RobLoachfff
Comment #140.1
RobLoachUpdated issue summary.
Comment #140.2
RobLoachUpdated issue summary.
Comment #141
mparker17You updated the issue summary, so we can remove that tag. :)
Comment #142
mparker17I don't see the PHP module in the list of available modules anymore. I don't see options to write custom PHP code in the block visibility settings. I don't see a PHP input filter option when editing text formats.
The tests pass!
The code looks good!
I'd say this is RTBC!
Comment #143
David_Rothstein CreditAttribution: David_Rothstein commentedI'm not changing the status from RTBC because I think the module could be removed first and these other issues dealt with later, but are the concerns raised by @greggles, @webchick, and others answered? And I think we still need to make sure the module is split up at some point; simply dumping it in contrib as is won't provide the security benefits.
To clarify that, the only "safe" way for a module to execute user-provided PHP code (in such a way that even user 1 cannot directly bypass it) is to:
module_exists('php') && user_access('use PHP for settings')
before allowing the code to be entered.module_exists('php')
before allowing the code to be executed.This is best practice currently, but I think we should aim to enforce it (at the security team level) in Drupal 8. The path to that is:
module_exists('php')
part of the above, because they don't want to force their users to endure the unwanted side effects (the extra text format, etc) of turning that module on (see #870938: Add new permission for controlling imports for the Ctools issue around this). In Drupal 8, last time I checked it's even worse because some Views code had moved into the module too; the module has gotten bigger when it should be getting smaller. To force module maintainers who want to execute PHP to rely on the PHP module, we have to strip it down and split all the side effects and features out of the main module.Does that sound like a path forward to address the concerns above?
Comment #144
catchI'd be OK with adding two helpers like that to core. Assigning this to Dries, I opened this issue in 2011 so it's clear what my view is.
Comment #145
Dries CreditAttribution: Dries commentedPer #143, I think we can deal with these issues later and commit this now. Patch needs to be re-rolled though.
Comment #146
webchickTagging for a follow-up issue in #143.
Comment #147
xjmSmall suggestion: the patch will reroll more easily and be smaller to review with
git config --global diff.renames copies
.Comment #148
RobLoach-D
however, which will only print the header of the files rather than the diff to nothing. I did that in this patch for ya.Comment #150
RobLoachIt appears that results in a diff that applying seems to not enjoy. This is it without
-D
. For those following at home:https://github.com/RobLoach/drupalmirror/pull/1
https://github.com/RobLoach/drupalmirror/pull/1.diff
Comment #152
RobLoachInterdiff
Comment #153
ZenDoodles CreditAttribution: ZenDoodles commentedJust changing the title back. Looks like it got reset accidentally.
(Also... zomg +1)
Comment #154
David_Rothstein CreditAttribution: David_Rothstein commentedThe title change was on purpose, but it seems like we've agreed to deal with that in followups.
Hopefully the maintainer of https://drupal.org/project/php will agree with a "Remove most of the PHP module from the PHP module" issue, though :)
Comment #155
webchickSince that looks like it was a straight-up re-roll, moving back to RTBC.
Comment #156
alexpottPatch no longer applied. :( core/modules/php/lib/Drupal/php/Tests/PhpFilterTest.php had changed in #2083811: Remove langcode nesting for fields in entity $form structures and this patch deletes it
Comment #157
alexpottActually let me upload the nice to review patch generated with
git diff -D 8.x HEAD
Comment #160
alexpottWell who know -D is only for reviewing should have read more of the comments :D uploading the patch from #156 again
Comment #161
RobLoachRebased and it resulted in the same was what Alex posted. +1
Comment #162
Dries CreditAttribution: Dries commentedCommitted to 8.x. Live from NYC. Finally. Let their be rejoicing. :)
Comment #163
jbrown CreditAttribution: jbrown commentedHooray!
Comment #164
RobLoachThree things:
Comment #165
ZenDoodles CreditAttribution: ZenDoodles commentedPatch only removes the php filter from CHANGELOG.txt.
Opened follow up issue #2088851: Handle remaining security/permissions issues related to removing the php module from core.
Comment #166
ZenDoodles CreditAttribution: ZenDoodles commentedOops
Comment #167
webchickCommitted and pushed to 8.x. Thanks!
Comment #168
ami7878 CreditAttribution: ami7878 commentedThis is nonsense.
We cannot have this in the core because "Having the PHP module in Drupal Core encourages new users of Drupal to enable and use it ..."
This is like saying lets stop making cars because they encourage new and inexperienced drivers to use it and cause accidents.
Come on, please ...
Comment #169
DamienMcKenna@ami7878: I don't know how many Drupal websites you've worked on, but the experiences of probably everyone else in this issue is that the PHP module, specifically the PHP text filter, is used by people who shouldn't have used it in the first place, who leave massive security holes in their sites through their inexperience. IMHO I'd go so far as to say that Drupal never should have had the PHP module in core in the first place, but I'm happy it's finally gone.
Lets be clear about something: solving a problem by using PHP code in a custom block or a node form field is always the wrong way of doing it.
Lets move on.
Comment #170
Garrett Albright CreditAttribution: Garrett Albright commentedNew drivers must go through training and licensing programs to make sure that they know how to operate a car properly.
This is more like, instead of shipping cars with a stereo system that, when turned on, disable the airbags and anti-lock brakes and turn the car's body into a high-powered magnet, let's stop shipping cars with a stereo system and instead make the user install their own stereo system with those effects if they wish.
Me too. I've felt that way pretty much ever since I discovered this module existed.
Comment #171
danillonunes CreditAttribution: danillonunes commentedPHP module is more like selling a car with a small fridge in front of the driver's seat filled with 12 cans of beer.
Comment #172.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #173
cilefen CreditAttribution: cilefen commentedHere are a few words on finding and eliminating content that was using the PHP module.
Comment #174
Garrett Albright CreditAttribution: Garrett Albright commentedcliefen's post shows how to find nodes and blocks, but there's still Views fields, Rules or Context selectors, etc… lots of other dark dirty corners where eval() may be lurking.
Comment #175
cilefen CreditAttribution: cilefen commented@Garrett Albright: Good point - I updated and linked back to your comment.
Comment #176
MakeOnlineShop CreditAttribution: MakeOnlineShop commentedI find it very funny that you could think that a newbie will be able to use Drupal...
I am trying Drupal 8 now and find its usability exactly as before if you know what I mean...
It really seem that coders are doing it for themselves, not for users...
Don't worry, nobody but experts will use Drupal 8 !
Comment #177
wizonesolutions@MakeOnlineShop This is probably not the best thread to get feedback on your comment. A newbie wouldn't be migrating D7 -> D8 and trying to remove
eval()
'd PHP.If you have encountered problems and solved them, update the documentation. If you have problems you have not been able to solve, ask on (not an exhaustive list) Drupal Answers, Twitter #Drupal, or the forums.
Drupal is a community project, so if you want to improve its UX, contribute something back. It doesn't have to be code. It could just be more information about the problems you faced (but not here, not the right thread).
Your comment is too vague to be of any use to anyone who might want to improve Drupal's usability.
Comment #178
liquidcms CreditAttribution: liquidcms commentedYes, i guess security issues being solved here.. but i have a quick issue with a client needing to replicate a term field over to a text field so we can finish content import. Sure would be easy being able to create a simple php action and run through VBO.. sad this feature has been dropped.
Comment #179
gregglesThis feature is not dropped. It's still available for Drupal 8 or 9 at https://www.drupal.org/project/php
Comment #180
liquidcms CreditAttribution: liquidcms commenteda faint glimmer of what was there in D7.. i think PHP module only adds the PHP input filter? No PHP actions, no PHP in custom tokens, etc. but maybe these were all added individually by the contrib modules.