Issue #3340022 by Wim Leers, phenaproxima: Tighten ComposerPluginsValidator:...

Issue #3340022 by Wim Leers, phenaproxima: Tighten ComposerPluginsValidator: support only specified version constraint

package_manager/src/InstalledPackagesList.php

@@ -50,8 +50,12 @@ final class InstalledPackagesList extends \ArrayObject {
    * {@inheritdoc}
    */
   public function offsetGet(mixed $key): ?InstalledPackage {
-    // Overridden to provide a clearer return type hint.
-    return parent::offsetGet($key);
+    // Overridden to provide a clearer return type hint and compatibility with
+    // the null-safe operator.
+    if ($this->offsetExists($key)) {
+      return parent::offsetGet($key);
+    }
+    return NULL;
   }
 
   /**

package_manager/src/Validator/ComposerPluginsValidator.php

@@ -4,7 +4,7 @@ declare(strict_types = 1);
 
 namespace Drupal\package_manager\Validator;
 
-use Drupal\Component\Render\FormattableMarkup;
+use Composer\Semver\Semver;
 use Drupal\Component\Serialization\Json;
 use Drupal\Core\Config\ConfigFactoryInterface;
 use Drupal\Core\StringTranslation\StringTranslationTrait;
@@ -13,29 +13,28 @@ use Drupal\package_manager\Event\PreApplyEvent;
 use Drupal\package_manager\Event\PreCreateEvent;
 use Drupal\package_manager\Event\PreOperationStageEvent;
 use Drupal\package_manager\Event\StatusCheckEvent;
-use Drupal\package_manager\PathExcluder\VendorHardeningExcluder;
 use Drupal\package_manager\PathLocator;
 use PhpTuf\ComposerStager\Domain\Exception\RuntimeException;
 use Symfony\Component\EventDispatcher\EventSubscriberInterface;
 
 /**
- * Validates the allowed composer plugins, both in active and stage.
+ * Validates the allowed Composer plugins, both in active and stage.
  *
  * Composer plugins can make far-reaching changes on the filesystem. That is why
  * they can cause Package Manager (more specifically the infrastructure it uses:
  * php-tuf/composer-stager) to not work reliably; potentially even break a site!
  *
- * This validator restricts the use of composer plugins:
- * - using arbitrary composer plugins is discouraged by composer, but disallowed
- *   by this module (it is too risky):
+ * This validator restricts the use of Composer plugins:
+ * - Allowing all plugins to run indiscriminately is discouraged by Composer,
+ *   but disallowed by this module (it is too risky):
  *   @code config.allowed-plugins = true @endcode is forbidden.
- * - installed composer plugins that are not allowed (in composer.json's
- *   @code config.allowed-plugins @endcode) are not executed by composer, so
- *   these are safe
- * - installed composer plugins that are allowed need to be either explicitly
- *   supported by this validator (and they may need their own validation to
+ * - Installed Composer plugins that are not allowed (in composer.json's
+ *   @code config.allowed-plugins @endcode) are not executed by Composer, so
+ *   these are safe.
+ * - Installed Composer plugins that are allowed need to be either explicitly
+ *   supported by this validator (they may still need their own validation to
  *   ensure their configuration is safe, for example Drupal core's vendor
- *   hardening plugin), or explicitly trusted, by adding it to the
+ *   hardening plugin), or explicitly trusted by adding it to the
  *   @code package_manager.settings @endcode configuration's
  *   @code additional_trusted_composer_plugins @endcode list.
  *
@@ -55,48 +54,49 @@ final class ComposerPluginsValidator implements EventSubscriberInterface {
   use StringTranslationTrait;
 
   /**
-   * Composer plugins known to modify other packages, but that are validated.
+   * Composer plugins known to modify other packages, but are validated.
    *
    * (The validation guarantees they are safe to use.)
    *
-   * Keys are composer plugin package names, values are associated validators or
-   * excluders necessary to make those composer plugins work reliably with the
-   * Package Manager module.
-   *
-   * @var string[][]
+   * @var string[]
+   *   Keys are Composer plugin package names, values are version constraints
+   *   for those plugins that this validator explicitly supports.
    */
   private const SUPPORTED_PLUGINS_THAT_DO_MODIFY = [
     // cSpell:disable
-    'cweagans/composer-patches' => ComposerPatchesValidator::class,
-    'drupal/core-vendor-hardening' => VendorHardeningExcluder::class,
+    // @see \Drupal\package_manager\Validator\ComposerPatchesValidator
+    'cweagans/composer-patches' => '^1.7.3',
+    // @see \Drupal\package_manager\PathExcluder\VendorHardeningExcluder
+    'drupal/core-vendor-hardening' => '*',
     // cSpell:enable
   ];
 
   /**
-   * The composer plugins are known not to modify other packages.
+   * Composer plugins known to NOT modify other packages.
    *
    * @var string[]
+   *   Keys are Composer plugin package names, values are version constraints
+   *   for those plugins that this validator explicitly supports.
    */
   private const SUPPORTED_PLUGINS_THAT_DO_NOT_MODIFY = [
     // cSpell:disable
-    'composer/installers',
-    'dealerdirect/phpcodesniffer-composer-installer',
-    'drupal/core-composer-scaffold',
-    'drupal/core-project-message',
-    'phpstan/extension-installer',
+    'composer/installers' => '^2.0',
+    'dealerdirect/phpcodesniffer-composer-installer' => '^0.7.1 || ^1.0.0',
+    'drupal/core-composer-scaffold' => '*',
+    'drupal/core-project-message' => '*',
+    'phpstan/extension-installer' => '^1.1',
     // cSpell:enable
   ];
 
   /**
-   * The additional trusted composer plugin package names.
+   * The additional trusted Composer plugin package names.
    *
    * Note: these are normalized package names.
    *
    * @var string[]
-   * @see \Composer\Package\PackageInterface::getName()
-   * @see \Composer\Package\PackageInterface::getPrettyName()
+   *   Keys are package names, values are version constraints.
    */
-  protected array $additionalTrustedComposerPlugins;
+  private array $additionalTrustedComposerPlugins;
 
   /**
    * Constructs a new ComposerPluginsValidator.
@@ -114,9 +114,14 @@ final class ComposerPluginsValidator implements EventSubscriberInterface {
     protected PathLocator $pathLocator,
   ) {
     $settings = $config_factory->get('package_manager.settings');
-    $this->additionalTrustedComposerPlugins = array_map(
-      [__CLASS__, 'normalizePackageName'],
-      $settings->get('additional_trusted_composer_plugins')
+    $this->additionalTrustedComposerPlugins = array_fill_keys(
+      array_map(
+        [__CLASS__, 'normalizePackageName'],
+        $settings->get('additional_trusted_composer_plugins')
+      ),
+      // For now, additional_trusted_composer_plugins cannot specify a version
+      // constraint.
+      '*'
     );
   }
 
@@ -133,17 +138,6 @@ final class ComposerPluginsValidator implements EventSubscriberInterface {
     return strtolower($package_name);
   }
 
-  /**
-   * @return string[]
-   */
-  private function getSupportedPlugins(): array {
-    return array_merge(
-      array_keys(self::SUPPORTED_PLUGINS_THAT_DO_MODIFY),
-      self::SUPPORTED_PLUGINS_THAT_DO_NOT_MODIFY,
-      $this->additionalTrustedComposerPlugins,
-    );
-  }
-
   /**
    * Validates the allowed Composer plugins, both in active and stage.
    */
@@ -160,44 +154,76 @@ final class ComposerPluginsValidator implements EventSubscriberInterface {
       : $this->pathLocator->getProjectRoot();
     try {
       // @see https://getcomposer.org/doc/06-config.md#allow-plugins
-      $value = Json::decode($this->inspector->getConfig('allow-plugins', $dir));
+      $allowed_plugins = Json::decode($this->inspector->getConfig('allow-plugins', $dir));
     }
     catch (RuntimeException $exception) {
       $event->addErrorFromThrowable($exception, $this->t('Unable to determine Composer <code>allow-plugins</code> setting.'));
       return;
     }
 
-    if ($value === 1) {
+    if ($allowed_plugins === 1) {
       $event->addError([$this->t('All composer plugins are allowed because <code>config.allow-plugins</code> is configured to <code>true</code>. This is an unacceptable security risk.')]);
       return;
     }
 
-    // Only packages with `true` as a value are actually executed by composer.
-    assert(is_array($value));
-    $allowed_plugins = array_keys(array_filter($value));
-    // Normalized allowed plugins: keys are normalized package names, values are
-    // the original package names.
-    $normalized_allowed_plugins = array_combine(
+    // TRICKY: additional trusted Composer plugins is listed first, to allow
+    // site owners who know what they're doing to use unsupported versions of
+    // supported Composer plugins.
+    $trusted_plugins = $this->additionalTrustedComposerPlugins
+      + self::SUPPORTED_PLUGINS_THAT_DO_MODIFY
+      + self::SUPPORTED_PLUGINS_THAT_DO_NOT_MODIFY;
+
+    assert(is_array($allowed_plugins));
+    // Only packages with `true` as a value are actually executed by Composer.
+    $allowed_plugins = array_keys(array_filter($allowed_plugins));
+    // The keys are normalized package names, and the values are the original,
+    // non-normalized package names.
+    $allowed_plugins = array_combine(
       array_map([__CLASS__, 'normalizePackageName'], $allowed_plugins),
       $allowed_plugins
     );
-    $unsupported_plugins = array_diff_key($normalized_allowed_plugins, array_flip($this->getSupportedPlugins()));
-    if ($unsupported_plugins) {
-      $unsupported_plugins_messages = array_map(
-        fn (string $raw_allowed_plugin_name) => new FormattableMarkup(
-          "<code>@package_name</code>",
-          [
-            '@package_name' => $raw_allowed_plugin_name,
-          ]
-        ),
-        $unsupported_plugins
-      );
+
+    $installed_packages = $this->inspector->getInstalledPackagesList($dir);
+    // Determine which plugins are both trusted by us, AND allowed by Composer's
+    // configuration.
+    $supported_plugins = array_intersect_key($allowed_plugins, $trusted_plugins);
+    // Create an array whose keys are the names of those plugins, and the values
+    // are their installed versions.
+    $supported_plugins_installed_versions = array_combine(
+      $supported_plugins,
+      array_map(
+        fn (string $name): ?string => $installed_packages[$name]?->version,
+        $supported_plugins
+      )
+    );
+    // Find the plugins whose installed versions aren't in the supported range.
+    $unsupported_installed_versions = array_filter(
+      $supported_plugins_installed_versions,
+      fn (?string $version, string $name): bool => $version && !Semver::satisfies($version, $trusted_plugins[$name]),
+      ARRAY_FILTER_USE_BOTH
+    );
+
+    $untrusted_plugins = array_diff_key($allowed_plugins, $trusted_plugins);
+
+    $messages = array_map(
+      fn (string $raw_name) => $this->t('<code>@name</code>', ['@name' => $raw_name]),
+      $untrusted_plugins
+    );
+    foreach ($unsupported_installed_versions as $name => $installed_version) {
+      $messages[] = $this->t("<code>@name</code> is supported, but only version <code>@supported_version</code>, found <code>@installed_version</code>.", [
+        '@name' => $name,
+        '@supported_version' => $trusted_plugins[$name],
+        '@installed_version' => $installed_version,
+      ]);
+    }
+
+    if ($messages) {
       $summary = $this->formatPlural(
-        count($unsupported_plugins),
+        count($messages),
         'An unsupported Composer plugin was detected.',
         'Unsupported Composer plugins were detected.',
       );
-      $event->addError($unsupported_plugins_messages, $summary);
+      $event->addError($messages, $summary);
     }
   }
 

package_manager/tests/fixtures/fake_site/composer.json

@@ -50,7 +50,7 @@
     },
     "cweagans/composer-patches": {
       "type": "path",
-      "version": "24.12.1999",
+      "version": "1.7.333",
       "url": "../path_repos/cweagans--composer-patches",
       "options": {
         "symlink": false

package_manager/tests/fixtures/fake_site/composer.lock

@@ -4,7 +4,7 @@
         "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies",
         "This file is @generated automatically"
     ],
-    "content-hash": "681e4c106deb0665ca393b0fc797b685",
+    "content-hash": "672a2db4be3ba5e4f025ddea6b7dacd7",
     "packages": [
         {
             "name": "drupal/core",

package_manager/tests/fixtures/path_repos/cweagans--composer-patches/composer.json

@@ -2,7 +2,7 @@
     "name": "cweagans/composer-patches",
     "description": "A fake version of cweagans/composer-patches",
     "type": "composer-plugin",
-    "version": "24.12.1999",
+    "version": "1.7.333",
     "extra": {
         "class": "\\cweagans\\Fake\\ComposerPatches"
     },

package_manager/tests/src/Kernel/ComposerPatchesValidatorTest.php

@@ -219,7 +219,7 @@ class ComposerPatchesValidatorTest extends PackageManagerKernelTestBase {
       ]);
     }
     if ($in_stage & static::REQUIRE_PACKAGE_FROM_ROOT && !($in_active & static::REQUIRE_PACKAGE_FROM_ROOT)) {
-      $stage_manipulator->requirePackage('cweagans/composer-patches', '24.12.1999');
+      $stage_manipulator->requirePackage('cweagans/composer-patches', '1.7.333');
     }
     if (!($in_stage & static::REQUIRE_PACKAGE_FROM_ROOT) && $in_active & static::REQUIRE_PACKAGE_FROM_ROOT) {
       $stage_manipulator

package_manager/tests/src/Kernel/ComposerPluginsValidatorTest.php

@@ -204,6 +204,22 @@ class ComposerPluginsValidatorTest extends PackageManagerKernelTestBase {
       [],
     ];
 
+    yield 'a supported composer plugin for which any version is supported: party like it is Drupal 99!' => [
+      [
+        'allow-plugins.drupal/core-composer-scaffold' => TRUE,
+      ],
+      [
+        [
+          'name' => 'drupal/core-composer-scaffold',
+          'version' => '99.0.0',
+          'type' => 'composer-plugin',
+          'require' => ['composer-plugin-api' => '*'],
+          'extra' => ['class' => 'AnyClass'],
+        ],
+      ],
+      [],
+    ];
+
     yield 'one UNsupported but disallowed plugin — pretty package name' => [
       [
         'allow-plugins.composer/plugin-a' => FALSE,
@@ -308,6 +324,52 @@ class ComposerPluginsValidatorTest extends PackageManagerKernelTestBase {
         ),
       ],
     ];
+
+    yield 'one supported composer plugin but incompatible version — newer version' => [
+      [
+        'allow-plugins.phpstan/extension-installer' => TRUE,
+      ],
+      [
+        [
+          'name' => 'phpstan/extension-installer',
+          'version' => '20.1',
+          'type' => 'composer-plugin',
+          'require' => ['composer-plugin-api' => '*'],
+          'extra' => ['class' => 'AnyClass'],
+        ],
+      ],
+      [
+        ValidationResult::createError(
+          [
+            new TranslatableMarkup('<code>phpstan/extension-installer</code> is supported, but only version <code>^1.1</code>, found <code>20.1</code>.'),
+          ],
+          new TranslatableMarkup('An unsupported Composer plugin was detected.'),
+        ),
+      ],
+    ];
+
+    yield 'one supported composer plugin but incompatible version — older version' => [
+      [
+        'allow-plugins.dealerdirect/phpcodesniffer-composer-installer' => TRUE,
+      ],
+      [
+        [
+          'name' => 'dealerdirect/phpcodesniffer-composer-installer',
+          'version' => '0.6.1',
+          'type' => 'composer-plugin',
+          'require' => ['composer-plugin-api' => '*'],
+          'extra' => ['class' => 'AnyClass'],
+        ],
+      ],
+      [
+        ValidationResult::createError(
+          [
+            new TranslatableMarkup('<code>dealerdirect/phpcodesniffer-composer-installer</code> is supported, but only version <code>^0.7.1 || ^1.0.0</code>, found <code>0.6.1</code>.'),
+          ],
+          new TranslatableMarkup('An unsupported Composer plugin was detected.'),
+        ),
+      ],
+    ];
   }
 
   /**
@@ -335,6 +397,8 @@ class ComposerPluginsValidatorTest extends PackageManagerKernelTestBase {
           [
             new TranslatableMarkup('<code>not-cweagans/not-composer-patches</code>'),
             new TranslatableMarkup('<code>also-not-cweagans/also-not-composer-patches</code>'),
+            new TranslatableMarkup('<code>phpstan/extension-installer</code> is supported, but only version <code>^1.1</code>, found <code>20.1</code>.'),
+            new TranslatableMarkup('<code>dealerdirect/phpcodesniffer-composer-installer</code> is supported, but only version <code>^0.7.1 || ^1.0.0</code>, found <code>0.6.1</code>.'),
           ],
           new TranslatableMarkup('Unsupported Composer plugins were detected.'),
         ),

package_manager/tests/src/Traits/ComposerInstallersTrait.php

@@ -30,6 +30,13 @@ trait ComposerInstallersTrait {
       'url' => $package_path,
       'options' => [
         'symlink' => FALSE,
+        'versions' => [
+          // Explicitly state the version contained by this path repository,
+          // otherwise Composer will infer the version based on the git clone or
+          // fall back to `dev-master`.
+          // @see https://getcomposer.org/doc/05-repositories.md#path
+          'composer/installers' => $package_list['composer/installers']->version,
+        ],
       ],
     ], JSON_UNESCAPED_SLASHES);
     $working_dir_option = "--working-dir=$dir";