Do not allow drupal/core-composer-scaffold to be used by packages other than core

👏 Great find, and thanks for the detailed write-up! 🙏🤩

Issue summary update: improved markup and issue links; no material changes.

This definitely improves reliability, arguably also security. Added to roadmap at 🌱 Drupal 10 Core Roadmap for Automatic Updates Active because you tagged it core-mvp.

Composer and the
composer/installers plugin have a limitation that one project cannot be moved
inside of another project.

Woah, this very much sounds like the "inter-package symlinks should indeed not be allowed, but intra-package symlinks should be!" problem that is described in 📌 Add symlink support to Composer Stager 2.0, require that version, and simplify UX & tests accordingly Fixed and being solved in https://github.com/php-tuf/composer-stager/issues/58. i.e. composer/installers makes sure that each installed package remains one atomic unit. That is exactly what the symlink support is aiming to do going forward!

(I bet they did that for composer/installers to allow parallellization in the future: if each package is an atomic unit, operations can be run in parallel. If that is not the case, they must be run serially. Clearly, the drupal/core-composer-scaffold plugin must execute after all other packages have been installed/updated!)

For instance what if a plugin moves a scaffold file into a directory that we are excluding from the apply phase?

🤔I don't understand yet how this can happen; isn't that exactly what the code in 📌 Exclude unknown paths in project base: only allow vendor + web root + whatever drupal/core-composer-scaffold allows Fixed is proposing to do? (Take into account the targets of the scaffold plugin, and not exclude those?)

We also might want to consider at some supporting any package if drupal/core-composer-scaffold provided a UI like getAllScaffoldfiles() that would return all files across all packages that are allowed. Without this I don't think we can support it.

Very interesting! Tagging for that: that plugin is part of Drupal core, and so we can just file a Drupal core issue that blocks general Automatic Updates/Package Manager support for this plugin 👍

re @Wim Leers question about

For instance what if a plugin moves a scaffold file into a directory that we are excluding from the apply phase?

What I mean is a scaffold plugin could more files into any of the directories that excluded by any of the classes in package_manager/src/PathExcluder. for instance SiteFilesExcluder excludes our files directories but for all we know a scaffold plugin could move file in there and then that file would be excluded when we apply the update to the site

Couldn't that be prevented by ensuring that a new validator + excluder run last? 🤔

1. re #5 yes for sure we could figure out a way to solve it. but I think the solution would be complex and it would benefit very few sites.

   so that is why I think we should not support it for core-mvp. then we can decide later if there is actually a need to support it.

   I think if it turns out that there very few site that are affecting by this restriction then I think it might be reasonable that those sites have to custom or contrib solution

2. I think any solution that we come up will always have the possibility of detecting a scaffold file has moved a file into a location we can't support and those preventing a critical security update from applying

   For instance we exclude directories like the file directory because 1, it could be very large, 2. we expect files could be adding there during the update process.

   If a scaffold move a file into the sites directory we would have to stop an update if we detected that.

1. 👍 — but we should document explicitly that this could be supported in the future

Doesn't make sense to do this until 📌 ScaffoldFilePermissionsValidator should use ComposerInspector instead of ComposerUtility Fixed is done, otherwise we're doing double work.

📌 ScaffoldFilePermissionsValidator should use ComposerInspector instead of ComposerUtility Fixed landed!

Discussed this with @omkar.podey, who'd already started looking into this! 👍 He has a pretty clear idea of how to approach this, very curious to see what he comes up with! 😄

Using composer/Plugin/Scaffold/AllowedPackages.php makes it easier to manage which packages are allowed to scaffold files.

sample code -

"extra": {
    "drupal-scaffold": {
      "allowed-packages": [
        "fixtures/drupal-core-fixture",
        "fixtures/scaffold-override-fixture"
      ],
      "locations": {
        "web-root": "./docroot"
      },
      "symlink": __SYMLINK__,
      "file-mapping": {
        "[web-root]/.htaccess": false,
        "[web-root]/robots.txt": "assets/robots-default.txt"
      }
    }

@Wim.leers i talked with @phenaproxima yesterday and he confirmed that any modules not in the allowed packages list except "drupal/legacy-scaffold-assets", "drupal/core", will not be allowed to scaffold files by default, He also helped writing the documentation for drupal/core-composer-scaffold and has vouched for it's implementation. So I was able to simplify the validator as well as the tests.

Looking good! And thanks for catching "strict types" just in time before my review! 😁👍

Please just mark going forward — I will have reduced capacity for reviews. @tedbow or @phenaproxima can also review! 😊

This is looking very good. There is one choice here that I strongly object to, though.

This looks really good. Thanks for addressing my feedback! A few more things and then IMHO this is good to go.

Looks great to me! This is still tagged for a follow-up, originating in #3. Once that's filed, this is RTBC!

Created 📌 [PP-1] Support any package to scaffold files Postponed in Automatic Updates + blocked that issue on a core feature request ✨ Create something like getAllScaffoldfiles() in core-composer-scaffold. Active

Beautiful end result! 👏😄

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