Problem/Motivation

While working toward #2982680: Add composer-ready project templates to Drupal core, we discovered that the Vendor Hardening Plugin sometimes fails to work, throwing an error:

Fatal error: Uncaught Error: Class 'Drupal\Component\FileSecurity\FileSecurity' not found in phar:///Users/ganderson/bin/composer/src/Composer/Plugin/PluginManager.php(196) : eval()'d code:245

The problem is that Composer plugins do not include the autoload.php file; instead, they dynamically create their autoload information on the fly at runtime. This allows plugin hooks to work before the autoloader is dumped. The limitation here, though, is that the plugin manager does not allow plugins to hook the dynamic autoload generation, because this would be circular. The upshot is that a plugin can only autoload classes that are located in their default location inside vendor, because the Composer Installers plugin is not given a chance to fix up any paths. Because of this, the Vendor Hardening plugin cannot find the Core File Security component, because the later is relocated to the /core directory.

Steps to reproduce:

Proposed resolution

In #3077455: Move Drupal Components out of 'core' directory, we considered fixing this by relocating the Core File Security component back to the vendor directory; however, that solution seemed too invasive to do at this time, so we are exploring potential workarounds instead.

There are two options:

  • Preferred: Put a complete copy of the File Security class in a new namespace inside the Vendor Hardening Plugin.
  • Alternate: Manually find the /core directory and require_once the File Security class.

Remaining tasks

None.

Follow-on Tasks

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

To work around a Composer class loader limitation, the Vendor Hardening plugin now has its own copy of the file security class from drupal/core.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

greg.1.anderson created an issue. See original summary.

greg.1.anderson’s picture

Status: Active » Needs review
FileSize
2.93 KB

Bad patch; please ignore.

greg.1.anderson’s picture

Bad patch; please ignore.

greg.1.anderson’s picture

Bad patch; please ignore.

greg.1.anderson’s picture

Re-rolled the miss-made patch from #3 (duplicated File Security class). No interdiff, as the intended contents are the same.

This patch demonstrates duplicating the File Security class inside the Vendor Hardening plugin.

Advantages:

- Completely self-contained; does not depend on drupal/core, and will keep on working if the File Security component is relocated or otherwise altered in core.

Disadvantages:

- Duplicate code: if a change to the File Security component is made in core, the same change must also be applied to the Vendor Hardening plugin.
- At some point we will want to relocate the File Security component to the vendor directory and alter the Vendor Hardening plugin to use the File Security class as a dependency.

greg.1.anderson’s picture

Fixed typo in #4 (forgot to change project org back). No interdiff; please just ignore patches prior to #5 / #6.

This patch demonstrates installing a class loader to autoload the File Security class.

Advantages:

- Not necessary to duplicate the File Security class.

Disadvantages:

- Has a hidden dependency on drupal/core.
- Writing .htaccess and web.config stops working if the File Security class is relocated, etc. (requires matching changes in Vendor Hardening plugin)

Mixologic’s picture

Status: Needs review » Needs work

Nothing really to review in Patch #5 since its a C/P from core.

For Patch #6:
These

+++ b/composer/Plugin/VendorHardening/VendorHardeningPlugin.php
@@ -248,4 +255,32 @@ public function writeAccessRestrictionFiles($vendor_dir) {
+    if (!is_dir("$corePath/$fileSecurityPath")) {
+      $this->io->writeError('<warning>Could not harden vendor directory with .htaccess and web.config files; drupal/core does contain the File Security component at $fileSecurityPath.</warning>');
+      return;
+    }

Should be "drupal/core does not contain the File Security component"

Also, maybe just 'file security', or FileSecurity or core-file-security.

greg.1.anderson’s picture

Adjusted error message as suggested - inserted missing word "not", and changed name of the file security component.

greg.1.anderson’s picture

Status: Needs work » Needs review

Back to 'needs review'

Mixologic’s picture

Status: Needs review » Reviewed & tested by the community

This lgtm. Lets get a committers opinion.

larowlan’s picture

Personally, I favour the autoloading fix rather than the duplication one. But I will seek another committer opinion

  1. +++ b/composer/Plugin/VendorHardening/VendorHardeningPlugin.php
    @@ -240,6 +241,12 @@ protected function cleanPathsForPackage($vendor_dir, $package_name, $paths_for_p
    +      return;
    
    @@ -248,4 +255,32 @@ public function writeAccessRestrictionFiles($vendor_dir) {
    +      return;
    ...
    +      return;
    

    should we return with an error code here, so CI systems will fail in this scenario

  2. +++ b/composer/Plugin/VendorHardening/VendorHardeningPlugin.php
    @@ -248,4 +255,32 @@ public function writeAccessRestrictionFiles($vendor_dir) {
    +    $classLoader = new ClassLoader();
    +    $classLoader->addPsr4('Drupal\\Component\\FileSecurity\\', "$corePath/$fileSecurityPath");
    +    $classLoader->register();
    

    do we need a whole new class loader for one class? is there any harm in just using require_once here instead? If the class doesn't exist we bail, so we're always going to need it.

  3. +++ b/composer/Plugin/VendorHardening/composer.json
    @@ -15,7 +15,6 @@
    -    "drupal/core-file-security": "^8.8"
    

    If someone uses this as a stand alone component, they would require this - so shouldn't we leave it? Or put it in suggests or something?

greg.1.anderson’s picture

1. We do not want to cause someone's composer install or composer dumpautoload command to fail if the vendor directory cannot be protected. We are operating on a best-effort basis.

2. Good idea. I'll get rid of the autoloader and just require the class, since we know we are going to use it.

3. If we require the drupal/core-file-security component, then Composer prints half a dozen lines of warnings, because it tries to require drupal/core (which replaces drupal/core-file-security), and then it cannot find some autoload classes, because /core has been relocated, but Composer expects to find it in vendor.

greg.1.anderson’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.97 KB
1.35 KB

Replace class loader with a simple `require_once`.

greg.1.anderson’s picture

Fixed a couple minor errors with #13; need to use 'is_file' instead of 'is_dir' now that we are testing the class file. Also, fixed quoting problem with error message.

Mixologic’s picture

Status: Needs review » Reviewed & tested by the community

All changes lgtm

greg.1.anderson’s picture

Issue summary: View changes

Updated issue summary.

catch’s picture

+1 to doing the require_once over duplication here.

greg.1.anderson’s picture

@catch #14 uses require_once and is RTBC. Do you think you could commit? Or do we need more review or other changes first?

Jaesin’s picture

I can confirm this is working. I was able to reproduce the error message when drupal core is not found and when I moved FileSecurity back to `lib/Drupal/Component/FileSecurity/FileSecurity.php` from vendor (in my setup), FileSecurity was loaded via `\Drupal\Composer\Plugin\VendorHardening\VendorHardeningPlugin::autoloadFileSecurity()` and `.htaccess` as well as `web.config` were successfully written to the vendor folder.

Mile23’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs followup
+++ b/composer/Plugin/VendorHardening/VendorHardeningPlugin.php
@@ -248,4 +254,30 @@ public function writeAccessRestrictionFiles($vendor_dir) {
+  /**
+   * Register a class loader for the FileSecurity class if it is not available.
+   */
+  protected function autoloadFileSecurity() {

+++ b/composer/Plugin/VendorHardening/composer.json
@@ -15,7 +15,6 @@
-    "drupal/core-file-security": "^8.8"

Needs a @todo and a follow-up for, uh... 8.8.x or 9.0.x? dunno. But a follow-up to change this when drupal/core doesn't replace drupal/core-file-security. At that point, the plugin can just require drupal/core-file-security.

RTBC +1 other than that.

greg.1.anderson’s picture

I can make a follow-up issue to put drupal/core-file-security dependency in the vendor hardening plugin (probably for Drupal 9).

How should we TODO this, though? We can't comment composer.json. Should we put it in a different file, or add a "_readme" or perhaps a "_todo" in the extra section?

Mile23’s picture

We can add it to the docblock for autoloadFileSecurity().

greg.1.anderson’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
3.27 KB
862 bytes

Added follow-on task #3079890: Re-add drupal/core-file-security in Vendor Hardening plugin composer.json and added @todo to autoloadFileSecurity docblock.

Ghost of Drupal Past’s picture

This patch , while without a doubt fixes the problem (thanks!) makes me deeply uneasy.

Is it time to step back and look at the wider context? It's not going to be news to anyone that we have a gigantic legacy codebase with relatively little room for changes trying to cooperate with other codebases which are opinionated in their own ways. It's a feeling and I can't offer a specific solution but I think we need to consider moving one of the codebases (in case, Drupal or composer) closer to each other. Multisite is another bag of hurt where the two doesn't want to cooperate well as they are now. And it seems to me that special casing a single file is the kind of solution which just feels wrong and cries for a more generic solution. And I am sure people more familiar with composer have considered some but ... well, I am just asking for considering more :) Perhaps even as a followup -- this, as I said, surely works and solves the problem at hand.

Maybe the current composer plugins are not enough and we need to submit a PR upstream to make composer even more extensible, maybe we just need a "Drupal plugin" ... I do not know. I am just saying, special casing a single file makes me feel uneasy.

greg.1.anderson’s picture

@chx: I totally get where you are coming from. To answer your question, I would like to first direct your attention to #3077455: Move Drupal Components out of 'core' directory. If that patch were committed, this one would not be necessary. All of the Drupal Components (lib/Drupal/Component) would be in the `vendor` directory, where they belong, and the Vendor Hardening plugin would be able to use the File Security component without any Tom-hackery. This would be my preferred solution to this problem.

However, there was concern from multiple quarters that relocating all of the Components would be too invasive an operation to do in 8.8.x. Therefore, we decided to postpone the correct fix until Drupal 9, and work around the problem in one way or another for 8.8. This issue presented two options for that: either special casing the single file, or duplicating the single file in the Vendor Hardening plugin. Between those two choices, the former was preferred by most reviewers.

It is our intention to follow up with #3077455 and #3079890 in the Drupal 9 timeframe to provide a more generic solution. In the meantime, though, we have only a few short weeks before the 8.8.0 alpha code freeze. This issue will unblock #2982680: Add composer-ready project templates to Drupal core, which will greatly improve the Composer experience on Drupal 8.8.x. Sometimes we need to make small sacrifices to take small steps in order to allow us to be successful with our large goals. Hopefully this will in the end prove out to be the best thing we can do right now. If you're still uncomfortable, maybe you could lobby for #3077455 in Drupal 8.8.x; however, I don't think that's going to go.

Ghost of Drupal Past’s picture

By no means would I want to hold this patch and I saw the corresponding issue yes. I ... mostly just don't know, I mean, there's drupal-composer/drupal-project which seems to work and as far as I can see the main (only?) drawback of that is it installs a docroot below the project dir but really, is that big of a deal in 2019. All I have are doubts. Anyways, carry on, I will try to gather more understanding in slack.

Mile23’s picture

Status: Needs review » Reviewed & tested by the community

+1 on #23, thanks.

FTR: I'm in Camp Duplicate Drupal/Core-File-Security In The Plugin, because it's the least weird of two not-so-great solutions. But either way there's technical debt, and we'll de-Drupalism it later, probably early in D9 dev time but maybe before.

We have consensus from the Composer Initiative people and some core maintainers, so let's do it.

larowlan’s picture

Status: Reviewed & tested by the community » Needs review

In #3077455: Move Drupal Components out of 'core' directory, we considered fixing this by relocating the Core File Security component back to the vendor directory; however, that solution seemed too invasive to do at this time, so we are exploring potential workarounds instead.

The core file security component is brand new, as in not in a tagged release. Is there an option to move just that component?

greg.1.anderson’s picture

@lawrolan: Yes, it would be possible to move just one component, and in fact there is a patch in #3077455 that does that. However, the problem with that solution is that there are then Components in two different locations, which did not seem desirable. It does technically work, though, and we can continue to investigate variations on that patch if you think it preferable.

greg.1.anderson’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC to either be committed as-is, or postponed / closed in favor of a different technique.

larowlan’s picture

Ok thanks, I'll discuss with some others and get back to you

alexpott’s picture

@larowlan asked for my thoughts on this issue - whether we should move move the file security component (just that one) to somewhere outside core or go for the approach in #23. My thoughts are that doing the require_once() here is not that bad and once we move all components outside of /core and use composer to get them then this code becomes dead.

Some thoughts on the patch...

  1. +++ b/composer/Plugin/VendorHardening/VendorHardeningPlugin.php
    @@ -248,4 +254,35 @@ public function writeAccessRestrictionFiles($vendor_dir) {
    +  /**
    +   * Register a class loader for the FileSecurity class if it is not available.
    +   *
    +   * @todo: Once the File Security component has been moved to the vendor
    +   * directory, then we can remove this method and instead require
    +   * "drupal/core-file-security": "^8.8" in our composer.json file.
    +   * See follow-on task: https://www.drupal.org/project/drupal/issues/3079890
    +   */
    +  protected function autoloadFileSecurity() {
    

    This is not registering a class loader or doing autoloading. It's including the FileSecurity class. So first line of the doc block is incorrect. Given we're planning to remove this method how about making it private. Also the name is odd because as before we're not autoloading.

  2. +++ b/composer/Plugin/VendorHardening/VendorHardeningPlugin.php
    @@ -248,4 +254,35 @@ public function writeAccessRestrictionFiles($vendor_dir) {
    +    $fileSecurityPath = 'lib/Drupal/Component/FileSecurity/FileSecurity.php';
    

    We should add a note to this file that until components are managed by composer we can't add new dependencies or classes to this component. Since if we do that this will break again. At the moment the FileSecurity class is self sufficient. This is a big advantage of copying the class as is ATM and then removing it once we can autoload properly.

  3. Just wondering but is the autoloader dumped multiple times when composer installers is working? It's odd that the onPostAutoloadDump is never fired at a time when all the code included by composer can be autoloaded. I think I've seen multiple autoloader dumps in certain situations with composer but I'm not sure. It'd be great if the issue summary had steps to reproduce the issue.
greg.1.anderson’s picture

Status: Reviewed & tested by the community » Needs work

1. This file used to install an autoloader, but was switched to a single require_once on request. I'll update the method name and comments.

2. If we did use an autoloader in this class, then the File Security component could at least be split into multiple classes in the future. Would you prefer that we go back to using an autoloader for that reason? Either way, I'll comment the File Security Component appropriately -- unless you'd prefer we copy the class here, as in #5.

3. Sometimes the autoload file is dumped after some other part of the Composer code path has set up autoloader. In that instance, the operation works. If you run composer dumpautoload, you can see the failure. You need a project that has been set up to use these components, e.g. https://github.com/greg-1-anderson/drupal-drupal-composer. That project isn't set up with a failure case at the moment. I'll update the issue summary.

Mixologic’s picture

Given the slack discussions, lets have the pendulum swing back to duplication.

The only thing missing then is

Alexpott: It would still be worth adding a note to both bits of code that this duplication exists in case changes are made
greg.1.anderson’s picture

Status: Needs work » Needs review
FileSize
6.66 KB
1.33 KB

Here's a new patch that goes back to the duplication of the file security component in the vendor hardening plugin.

I will update the issue summary, rewrite the change notice, and update the old follow-on issue #3079890: Re-add drupal/core-file-security in Vendor Hardening plugin composer.json

greg.1.anderson’s picture

Issue summary: View changes

Update issue summary to reflect latest strategy and document steps to reproduce.

Mixologic’s picture

Status: Needs review » Reviewed & tested by the community

The duplication and changes lgtm. Back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed c74a193 and pushed to 8.8.x. Thanks!

diff --git a/composer/Plugin/VendorHardening/FileSecurity.php b/composer/Plugin/VendorHardening/FileSecurity.php
index 5a87d10775..c8bcd6d328 100644
--- a/composer/Plugin/VendorHardening/FileSecurity.php
+++ b/composer/Plugin/VendorHardening/FileSecurity.php
@@ -1,6 +1,5 @@
 <?php
 
-// Re-namespaced duplicated class from Drupal\Component\FileSecurity.
 namespace Drupal\Composer\Plugin\VendorHardening;
 
 /**

We're not allowed comments here cause coding standards. As there is a comment already about duplication so I think this is okay.

  • alexpott committed c74a193 on 8.8.x
    Issue #3079481 by greg.1.anderson, Mixologic, larowlan, Mile23, Charlie...

Status: Fixed » Closed (fixed)

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

alberto56’s picture

A security update just came out for Drupal 8.8.1 and I'm getting this error when trying to install Drupal using composer.

Writing lock file
Generating autoload files
Hardening vendor directory with .htaccess and web.config files.

Fatal error: Uncaught Error: Class 'Drupal\Composer\Plugin\VendorHardening\FileSecurity' not found in /var/www/html/vendor/drupal/core-vendor-hardening/VendorHardeningPlugin.php:355
Stack trace:
#0 /var/www/html/vendor/drupal/core-vendor-hardening/VendorHardeningPlugin.php(88): Drupal\Composer\Plugin\VendorHardening\VendorHardeningPlugin->writeAccessRestrictionFiles('/var/www/html/v...')
#1 [internal function]: Drupal\Composer\Plugin\VendorHardening\VendorHardeningPlugin->onPostAutoloadDump(Object(Composer\Script\Event))
#2 phar:///usr/local/bin/composer/src/Composer/EventDispatcher/EventDispatcher.php(176): call_user_func(Array, Object(Composer\Script\Event))
#3 phar:///usr/local/bin/composer/src/Composer/EventDispatcher/EventDispatcher.php(96): Composer\EventDispatcher\EventDispatcher->doDispatch(Object(Composer\Script\Event))
#4 phar:///usr/local/bin/composer/src/Composer/Autoload/AutoloadGenerator.php(313): Composer\EventDispatcher\EventDispatcher->dispatchScript('post-autoload-d...', true, Array, Array)
#5 phar:///us in /var/www/html/vendor/drupal/core-vendor-hardening/VendorHardeningPlugin.php on line 355
The command '/bin/sh -c rm composer.lock &&   composer install &&   cat /var/www/html/core/lib/Drupal.php|grep VERS &&   ln -s /var/www/html/vendor/bin/drush /bin/drush &&   drush -v' returned a non-zero code: 255
alberto56’s picture

tjtj’s picture

my /core/composer/Plugin/VendorHardening directory is empty

Peter Buchanan’s picture

I'm not able to upgrade to Drupal 8.8.1 or 8.8.2. My terminal log shows:

Fatal error: Uncaught Error: Class 'Drupal\Composer\Plugin\VendorHardening\FileSecurity' not found in /Users/ThinkGov/Documents/Websites/Teija/Drupal_8_e_commerce/Teija2/vendor/drupal/core-vendor-hardening/VendorHardeningPlugin.php:355
Stack trace:

Is there a work around, I don't like not implementing Security updates to Drupal.

alberto56’s picture

@Peter Buchanan

Adding this as a dependency to my composer file helped:

"drupal/core-vendor-hardening": "^8.8"

Good luck!

Peter Buchanan’s picture

Thanks Alberto56, adding the dependency ("drupal/core-vendor-hardening": "^8.8") moves me on a bit.

Like https://www.drupal.org/project/drupal/issues/3101956 I am now getting the following error:

Fatal error: Uncaught Error: Class 'Drupal\Composer\Plugin\Scaffold\ScaffoldOptions' not found in /Users/ThinkGov/Documents/Websites/Teija/Drupal_8_e_commerce/Teija2/vendor/drupal/core-composer-scaffold/ManageOptions.php:55

Any thoughts anyone?

Mile23’s picture

@Peter Buchanan: It might be more useful to ask on the #composer Slack channel. https://www.drupal.org/slack

Peter Buchanan’s picture

Thanks Mile23, I've looked at the slack groups and can't see one for Composer. I'd have thought issues and work arounds would be more usefully documented in issues. maybe I'm being an old reactionary, I don't use slack:-)

Peter Buchanan’s picture

Thanks to the suggestion 46 from Alberto56 I've completed an update to Drupal 8.8.2 on a site that's using Composer. In doing so I discovered that having the right version of PHP is important (I'm still on 7.0.33). It's also important that all modules other than Token, Redirect and Mail System are updated, particularly Pathauto, before the Drupal update.

I used MAMP pro to trial the update and will continue to use it for testing, the PHP log was particularly helpful for troubleshooting.

The update has been a real trial, starting with Composer crashing system limits on my host account. I'll think carefully before recommending Drupal.

s_leu’s picture

The hint in #45 worked for me on Drupal 9.0.2 as well.

quietone’s picture

publish the CR