Problem/Motivation

#3323461: Hosting environment (e.g. cPanel) may add additional files (including symlinks) to the project, which breaks AU showed the problem that there may be unrelated files and folders in the base directory of the composer project that we don't want to stage because of 2 reasons

  1. The may contain symlinks
  2. The may contain important hosting config which would not want to overwrite because the could be managed a unknown Composer plugin

#3331168: Limit trusted Composer plugins to a known list, allow user to add more should take care of 2). After that it should be ok to exclude this paths automatically

Proposed resolution

We need a subscriber that listens to CollectIgnoredPathsEvent and excludes any paths in the base directory of the project that are not 1 of the following

  1. The vendor directory
  2. The webroot
  3. In the list of scaffold files as determined by the drupal/core-composer-scaffold plugin

For number 3) \Drupal\Composer\Plugin\Scaffold\Handler::scaffold() seems to have rather complex logic to determine the scaffold files as it seems any package can provide scaffold files, not just Drupal core. We could copy this logic for now but as a follow-up we could create a Drupal core issue to propose refactoring this logic into 1 public method like getScaffoldFiles()

We should probably not commit this until #3331168: Limit trusted Composer plugins to a known list, allow user to add more is committed by we can work on getting this to RTBC in the meantime.

Remaining tasks

Create core follow-up if this issue is successful

Issue fork automatic_updates-3331310

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

tedbow created an issue. See original summary.

kunal.sachdev’s picture

kunal.sachdev commented
Assigned: Unassigned » kunal.sachdev

kunal.sachdev opened merge request !648

kunal.sachdev’s picture

kunal.sachdev commented
Title: Exclude unknown paths in project base » [PP-3] Exclude unknown paths in project base
Status: Active » Postponed

Postponed on #3328985: Allow Validators/Excluders to store info on an individual stage use..

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Component: Code » Package Manager
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: [PP-3] Exclude unknown paths in project base » [PP-2] Exclude unknown paths in project base

#3331168: Limit trusted Composer plugins to a known list, allow user to add more is no longer postponed, so decreasing postponedness.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Postponed » Needs work

@kunal.sachdev or @Wim Leers re #4 I am not sure why this issue is postponed on #3328985: Allow Validators/Excluders to store info on an individual stage use..

Does this validator need to store metadata in the stage? If so what?

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Related issues: +#3338346: Do not allow drupal/core-composer-scaffold to be used by packages other than core

Relating #3338346: Do not allow drupal/core-composer-scaffold to be used by packages other than core because if we didn't allow other uses besides drupal core then this issue would only have to worry about what files drupal core puts in the base folder, probably my inspecting a composer.json directly rather than worry what any package might use the scaffold for.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: [PP-2] Exclude unknown paths in project base » [PP-2] Exclude unknown paths in project base: only allow paths that already exist in active + whatever drupal/core-composer-scaffold allows

#7: Yes, @kunal.sachdev should've documented the reason when he postponed it in #4… Almost a month later, it's no longer clear to me either.

I suspect it's due to some need for first gathering information during PreCreateEvent (i.e. "active") and passing that on to a later event just before applying during PreApplyEvent (i.e. "stage"), to ensure differences in scaffolded files between "active" and "stage" are taken into account, and the superset of the two is allowed — to ensure scaffolded files can be added or deleted?

(Otherwise a file scaffolded pre-update could not be deleted post-update, because it'd be excluded.)

@kunal.sachdev: Can you please confirm/deny my theory? 🙏🤓

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: [PP-2] Exclude unknown paths in project base: only allow paths that already exist in active + whatever drupal/core-composer-scaffold allows » [PP-2] Exclude unknown paths in project base: only allow vendor + web root + whatever drupal/core-composer-scaffold allows

I think the more explicit title makes the connection with #3338346: Do not allow drupal/core-composer-scaffold to be used by packages other than core clearer and sets clearer expectations.

Also: AFAICT this can happen independently of #3338346: Do not allow drupal/core-composer-scaffold to be used by packages other than core.

kunal.sachdev’s picture

kunal.sachdev commented
Title: [PP-2] Exclude unknown paths in project base: only allow vendor + web root + whatever drupal/core-composer-scaffold allows » Exclude unknown paths in project base: only allow vendor + web root + whatever drupal/core-composer-scaffold allows

I definitely didn’t postponed it because of the reason in #9, I had postponed this issue on #3328985: Allow Validators/Excluders to store info on an individual stage use.. because it was needed in #3315834: GitExcluder should not ignore .git directories that belong to packages installed by Composer so I thought we might need it in our issue as this is similar to GitExcluder. But now I think I can continue to work on this issue and will postpone if needed.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

👍 Great! 😊

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Title: Exclude unknown paths in project base: only allow vendor + web root + whatever drupal/core-composer-scaffold allows » [PP-1] Exclude unknown paths in project base: only allow vendor + web root + whatever drupal/core-composer-scaffold allows
Status: Needs work » Postponed

Postponed on #3335908: The 'fake_site' fixture cannot be using with `composer show` because the packages are not installed because we need to test that case where project root and web root are different. We don't want to make another `fake_site` fixture until we fix the current one

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Title: [PP-1] Exclude unknown paths in project base: only allow vendor + web root + whatever drupal/core-composer-scaffold allows » Exclude unknown paths in project base: only allow vendor + web root + whatever drupal/core-composer-scaffold allows
Status: Postponed » Needs work
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Looks like some essential files are being excluded … like composer.lock 😳😁

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented

I have pushed up a start to how I think we can test a project with different web and project root. see the comments in \Drupal\Tests\package_manager\Kernel\PathExcluder\UnknownPathExcluderTest::createTestProjectForTemplate

At least 1 test should fail now until it is implemented

kunal.sachdev’s picture

kunal.sachdev commented
Assigned: kunal.sachdev » Unassigned
Status: Needs work » Needs review
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work

Looking great — this needs docs improvements and additional test cases, but it looks superb otherwise! 🤩

kunal.sachdev’s picture

kunal.sachdev commented
Assigned: Unassigned » kunal.sachdev
kunal.sachdev’s picture

kunal.sachdev commented
Assigned: kunal.sachdev » Unassigned
Status: Needs work » Needs review
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » phenaproxima
Status: Needs review » Reviewed & tested by the community

Disabling UnknownPathExcluder yields:

/opt/homebrew/bin/php /private/var/folders/x1/mlh998q15c70vs7_y8s1mgqm0000gp/T/ide-phpunit.php --configuration /Users/wim.leers/core/core/phpunit.xml --filter Drupal\\Tests\\package_manager\\Kernel\\PathExcluder\\UnknownPathExcluderTest --test-suffix UnknownPathExcluderTest.php /Users/wim.leers/core/modules/contrib/automatic_updates/package_manager/tests/src/Kernel/PathExcluder
Testing started at 4:22 PM ...
PHPUnit 9.5.26 by Sebastian Bergmann and contributors.

Testing /Users/wim.leers/core/modules/contrib/automatic_updates/package_manager/tests/src/Kernel/PathExcluder
.F.F                                                                4 / 4 (100%)

Time: 00:05.197, Memory: 10.00 MB

There were 2 failures:

1) Drupal\Tests\package_manager\Kernel\PathExcluder\UnknownPathExcluderTest::testUnknownPath with data set "unknown file where web and project root different" (true, null, array('unknown_file.txt'))
Failed asserting that file "/private/tmp/package_manager_testing_roottest88114527/stage/gpovbxs9gsHOEM5lDu9rZm4cy0wRpOUBcVv2Y8PDzyI/unknown_file.txt" does not exist.

/Users/wim.leers/core/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/Users/wim.leers/core/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/Users/wim.leers/core/modules/contrib/automatic_updates/package_manager/tests/src/Kernel/PathExcluder/UnknownPathExcluderTest.php:159
/Users/wim.leers/core/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

2) Drupal\Tests\package_manager\Kernel\PathExcluder\UnknownPathExcluderTest::testUnknownPath with data set "unknown directory where web and project root different" (true, 'unknown_dir', array('unknown_dir/unknown_dir.README.md', 'unknown_dir/unknown_file.txt'))
Failed asserting that file "/private/tmp/package_manager_testing_roottest68717519/stage/_rMZ7beLyeq8uyvirzOGCp0Nbn-xABMUPvwZzWEgYOU/unknown_dir/unknown_dir.README.md" does not exist.

/Users/wim.leers/core/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/Users/wim.leers/core/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/Users/wim.leers/core/modules/contrib/automatic_updates/package_manager/tests/src/Kernel/PathExcluder/UnknownPathExcluderTest.php:159
/Users/wim.leers/core/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

FAILURES!
Tests: 4, Assertions: 21, Failures: 2.
Process finished with exit code 1

→ test coverage works as expected! 👍

I left one documentation nit suggestion that I cannot apply because GitLab is once again buggy 🤷‍♂️ @tedbow or @phenaproxima can apply it prior to commit. I suggest @phenaproxima commits this one because @tedbow has many other things on his plate today.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Assigned: phenaproxima » Unassigned
Status: Reviewed & tested by the community » Needs work

I think this generally makes sense but there are a few things we could change, IMHO, to increase the clarity of the code and the comments. Nothing major, though.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs work » Reviewed & tested by the community

@phenaproxima thanks for the review. The feedback was pretty straight forward so I addressed. There were a couple parts we could just remove so we didn't need update those comments.

going to self RTBC because the feedback was straight forward

  • tedbow committed 8171813d on 8.x-2.x authored by kunal.sachdev 
    Issue #3331310 by kunal.sachdev, tedbow, Wim Leers: Exclude unknown...
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Reviewed & tested by the community » Fixed

@phenaproxima hopefully I addressed your feedback correctly. I pretty much copied your suggested improvements

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Yay, this unpostponed #3323461: #3323461-19: Hosting environment (e.g. cPanel) may add additional files (including symlinks) to the project, which breaks AU.

The commits after I RTBC'd all look like solid improvements 👍

Status: Fixed » Closed (fixed)

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