Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
Right now our default file copier in the Php File Syncer from Composer Stager. Composer Stager also has a rsync based file syncer.
We would like to do #3355105: Warn strongly if the rsync file syncer is not in use but as of now we don't actually test the rsync copier because we thought we could not get it on drupalci.
Proposed resolution
Step #1 Install rsync on drupalci
Done! 🎉
Asked in #drupal-infrastructure.
Relevant links:
- https://www.drupal.org/drupalorg/docs/drupal-ci/customizing-drupalci-tes...
- https://www.drupal.org/drupalorg/docs/drupal-ci/custom-docker-images-in-...
- https://www.drupal.org/drupalorg/docs/drupal-ci/running-drupalci-locally
Step #2: Make it our default file copier
Given the problems discovered by @phenaproxima in #35 we should just switch to using the rsync file syncer as the default.
We may want to change #3355105: Warn strongly if the rsync file syncer is not in use after this is done to not allow the php file copier.
Step #3 Fix our tests/prod code
At least our build tests fail if we use the rsync file copier.
This is because the rsync file copier correctly preserves the directory permissions.
This introduces the problem of what to do with sites/default/default.* files. In normal drupal site sites/default will be write protected and system_requirements will try to enforce this.
Therefore if either of the sites/default/default.* gets updated in a core update the update will fail because the Composer Scaffold plugin will try to delete the file, see #16,
There are 2 ways we could solve this
- Require project's composer.json to set
extra->drupal-scaffold->file-mappingto excludedefault.*scaffold fills which are not necessary for the site to functionIf it is choice between reliably providing security updates and having these files updated then security updates is probably more important.
We could write a validator that checks this and if they are not set we can link to a
hook_helpentry that gives them the command linecomposercommand to update these(TBD)In the future we could even provide a "prepare my site" form that would set this automatically through package manager itself.
The cons of this approach is that if you reinstall a site off the codebase then your setting.php could have been created by out of date version of a
default.settings.php. This should not affect an already installed siteWe could also make a suggestion in the core issue #3091285: Composer scaffolding fails when permissions on default.settings.yml or default.settings.php is not writable. that the installer should not need to use
sites/default/default.settings.phpat all as we should always have the up-to-date version incore/assets/scaffold/files/default.settings.php -
Before staging and committing an update we could set permission of
sites/defaultto writable automatically and then re-harden them before and after composer operations.The feels dangerous to make the directory writable if for some reason we get hard failure before the file sync does its operations and we don't get chance to harden them again
I, tedbow, am strongly in favor of 1)
Remaining tasks
| Comment | File | Size | Author |
|---|
Issue fork automatic_updates-3362143
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:
changes
Comments
Comment #2
wim leersTrying
sudo apt-get install rsync.Comment #3
wim leersComment #4
wim leersIf this were in, we could do more in #3355105: Warn strongly if the rsync file syncer is not in use.
Comment #5
wim leersNow the patch will apply.
Comment #8
wim leers#5:
Per http://grep.xnddx.ru/search?text=apt-get&filename=drupalci.yml, I'd expect this to work fine 🤓
Comment #9
wim leers#8:
Progress!
Borrowing from my past experience with Docker, I've used exactly this line … let's find out if it works.
Comment #10
wim leersOr rather this — I'm no Linux/Debian expert at all. (A quick web search appears to confirm this: https://serverfault.com/questions/227190/how-do-i-ask-apt-get-to-skip-an....)
Comment #12
wim leersThat worked! Only failed due to #3362110: Since #3361983 was committed to Drupal core, psr/http-message needed to be explicitly required for build tests, which was just fixed.
Re-testing…
Comment #13
wim leersComment #14
tedbowComment #15
tedbowOk the problem now is that
\Drupal\Tests\automatic_updates\Build\CoreUpdateTest::testApifails but only if it is using the rsync file syncer not the php version. The build test have not changed otherwise.From the error https://www.drupal.org/pift-ci-job/2673917
This looks like errors doing the composer update in the stage directory. From the message output it looks like it has already done the composer updates and then it fails on trying to delete
default.services.ymlMy guess is this has something to do with this being a scaffold file that is specified in
core/composer.jsonunder drupal-scaffold > file-mapping. Maybe a permission issue trying to delete the old file before the scaffold moves the new one in place. If you look in\Drupal\Tests\automatic_updates\Build\CoreUpdateTest::setUpstreamCoreVersionwe specifically alter the core/assets/scaffold/files/* version file withThis is part of Drupal $versionso we can check later in\Drupal\Tests\automatic_updates\Build\CoreUpdateTest::assertUpdateSuccessfulto make sure it was copied over. If I comment these parts out the the test passes.\Drupal\Composer\Plugin\Scaffold\Operations\ReplaceOp::processdoes try to remove the destination file first so this might be where the error is coming from.Not sure why this would happen with the rsync file copier and not the php file copier. But since this is failing on running the composer update in the staged copy and the file copier is not involved in that step then my guess is it is because the "create" stage where the files are copied from active to stage by the file copier happens differently with the php copier vs the rsync copier.
One guess is the permissions are different in stage after being copied with rsync copier vs the php file copier. I would the permission would be the same but it might be the one leaves the permissions wrong.
I think there are 2 possibilities if this really is where the problem is coming from
My guess is it is number 1 but sure
Unassigning myself in case someone wants to debug this more before I look at this again tomorrow.
Comment #16
omkar.podey commentedYep you were right i just confirmed that it does fail in
\Drupal\Composer\Plugin\Scaffold\Operations\ReplaceOp::processon$fs->remove($destination_path), now comparing file permissions.I tried to get the file permissions by but the fileperms in
ReplaceOp::processalways fails , also checked if the file exists in the first place and it does.Comment #17
wim leersWoah! 😱
Comment #18
tedbow@omkar.podey thanks for researching that.
I think one way to test this
\Drupal\automatic_updates_test_api\ApiControllerto stop after copying the code base, don't run the composer update in the stage, don't apply, don't destroy\Drupal\Tests\automatic_updates\Build\CoreUpdateTest::testApito use@testWithfor the file copier.Though you could something like add this make easier
sites/default/default.services.ymland directories above it. Are they different between the 2 copiers?If the permissions are different this might be the problem
Comment #19
tedbowAs far as I can tell the permissions and ownership of the files seem the same
Php copier
see the
copier-php.txtmake sure I didn't use the wrong directoryrsync copier
see the
copier-rsync.txtto see I got the correct directory🤔so I am not sure what is going on here. Someone should look at listing and see if I am missing something.
Also could be good if someone else checked
3362143-use-rsync_debugand see if they get the same resultsdiff /private/tmp/build_workspace_74b987b4b4c6ac8e7d589ab6ddcace65dZmtDj/project/web/sites/default/default.services.yml /private/tmp/build_workspace_0ddf880575312372786ce46a74c83133vPQa0y/project/web/sites/default/default.services.ymlTo check if there was any differences between the 2 files. Maybe 1 was corrupted which made the deletion fail.
I didn't see any difference
This seem very unlikely to be the cause but wanted to rule it out
Comment #20
tedbowOk did some testing without automatic updates involved.
I wanted to manually test the composer operation without Automatic Updates or Composer stager involved
I don't think
default.services.ymlhas changed between 10.0.0 and 10.0.9 so I couldn't test that. But default.settings.yml has changed between 10.0.6 and 10.0.9 so I could test this and the same permissions issues applied.Here are commands I ran
This failed with a very similar error.
Even same line number
Filesystem.php line 284:Turns out you could argue the scaffold plugin doesn't handle permission correctly because you are have to do manual steps.
see https://www.drupal.org/forum/support/upgrading-drupal/2022-06-25/compose...
https://drupal.stackexchange.com/questions/290296/composer-require-fails...
I will look to see if there is core issue. Or maybe we have to do some permissions changing ourselves
Comment #21
wim leersIs it really the scaffold plugin, or is it just file system permissions?
Those files have these permissions (this is from a
git cloneof Drupal core):which is the same as any other file in Drupal core:
… so whichever user is the owner is the only one able to modify these. (
-rw-r--r--akasudo chmod 644, which also exists in core'scommit-code-check.sh.)So I'm now wondering if we're executing
composeras a different user than the owner. Could you check that? 🙏I think that is what arguably
\Drupal\Composer\Plugin\Scaffoldneeds to check, because I find zero matches for:… in that namespace 😬
Comment #22
wim leersFound the relevant core issue: #3091285: Composer scaffolding fails when permissions on default.settings.yml or default.settings.php is not writable..
Comment #23
tedbowTurns out there is core issue for this #3091285: Composer scaffolding fails when permissions on default.settings.yml or default.settings.php is not writable.
that has a suggested work around
As one work around that issue points the scaffold docs https://www.drupal.org/docs/develop/using-composer/using-drupals-compose...
That would allow excluding certain scaffold files that you no longer need updates for by updating the project's composer.json
I think could think of 2 solutions
extra->drupal-scaffold->file-mappingto excludedefault.*scaffold fills which are not necessary for the site to functionIf it is choice between reliably providing security updates and having these files updated then security updates is probably more important.
In the future we could even provide a "prepare my site" form that would set this automatically through package manager itself.
I am in favor of #1 because it seems less error prone
Comment #24
wim leersBut the core issue does not seem to mention the last paragraph of #21 as the root cause. But if that is a correct extrapolation from me … then we would be able to write a validator for it! 😊🥳
So fingers crossed 😄🤞
Comment #25
tedbowNote that if do all the steps in #20 except
php ./core/scripts/drupal quick-start standardI don't get the error. I think this because installing drupal changes these file permissions.
Comment #26
tedbowre #24
We do have
\Drupal\automatic_updates\Validator\ScaffoldFilePermissionsValidatorbut maybe we have bug in thereMaybe this belongs in package_manager. On the other hand if you are using Project Browser only you probably would not run into this because it cannot update core and I think the core scaffold will only move these files if core has been updated
Comment #27
wim leers#23: I offered a third solution in #24, but I think I need to clarify it:
composercommands is not the same user as PM (Package Manager)/AU (Automatic Updates) will executecomposeras, this problem will occur.In other words: this is "just" a manifestation of what #3351895: Add Drush command to allow running cron updates via console and by a separate user, for defense-in-depth was all about: properly secured sites have their executable code owned by a user that is not the site owner.
So the most secure solution is to detect when this is the case (i.e. run
whoamifrom a validator in PM and check if it does NOT match the owner ofsites/default/default.settings.php), inform the user that a cron command MUST be set up that invokes the Symony Console (currently still Drush — see #3360485: Add Symfony Console command to allow running cron updates via console and by a separate user, for defense-in-depth) command.This also means that PB (Project Browser) will not be able to install any project, only browsing projects. (Unless we add a queuing system that will be executed by that command whenever it is invoked via
crontab.)When this is NOT the case, PM/AU (and PB) de facto run in "reduced security" mode (which is how pretty much every WordPress site is run). When this is the case, all functionality is available through the UI.
If my understanding is correct, then I think the above is the only approach that will be accepted by Drupal's security team, and hence it'd block getting PM + AU to stable.
Comment #28
wim leersIts logic:
👆 I think that
is_writable()may just not be doing what we think it does. At least some comments on the official PHP docs say that too:— https://www.php.net/manual/en/function.is-writable.php#61331,
So maybe it just checks that it's writable by a user?
OTOH it works fine on my system:
🤔
Comment #29
wim leersInvestigating the failure.
Comment #30
wim leersIt's very peculiar that
\Drupal\Tests\automatic_updates\Build\CoreUpdateTest::testApi()fails tests but::testUidoesn't. So we end up with this matrix:testUi()testApi()phprsyncIn principle, they should run exactly the same
composercommands in the exact same way.This makes no sense. If the entire
rsyncrow was ❌, it would make sense.There must be a subtle bug (behavior difference) somewhere.
Comment #31
wim leers#20:
I thought this was referring to
Drupal'sFilesystem. But it'scomposer's!See https://github.com/composer/composer/blob/2.5.5/src/Composer/Util/Filesy....
Also rang alarm bells at #3091285-92: Composer scaffolding fails when permissions on default.settings.yml or default.settings.php is not writable..
@tedbow's conclusion in #23 where he wrote is the only viable path forward I see right now.
Comment #35
phenaproximaI traced into this, and what I found was straightforward: when the stage directory is created,
sites/default(the directory) has different permissions depending on whether the file syncer is rsync, or the PHP syncer.In the active directory, the permissions are 644. That's because Drupal actively tries to write-protect the directory (it does this at install time, and in system_requirements()) for security hardening. Makes sense.
Now, if you're using the rsync file syncer, those permissions are preserved when the stage directory is created. That's because Composer Stager calls rsync with the --archive option, which implies the --perms option, which preserves permissions. Because of that, the scaffold files cannot be updated in sites/default, and the update fails due to ScaffoldFilePermissionsValidator. Makes sense.
The PHP file syncer, as it turns out, is less reliable. It delegates to Symfony's Filesystem component (specifically the
\Symfony\Component\Filesystem\Filesystem::copy()method), which does try to preserve permissions on copied files...but it does not preserve permissions on directories. When creating directories, it does this:$this->mkdir(\dirname($targetFile));\Symfony\Component\Filesystem\Filesystem::mkdir()takes a second argument -- the permissions, which default to 777. But copy() doesn't pass that argument, so the directory is created with 777 permissions.This causes our build tests, which use the PHP file syncer by default, to pass by coincidence. As it turns out, we do try to ensure that we're handling permissions of sites/default properly -- if you look at
\Drupal\Tests\automatic_updates\Build\CoreUpdateTest::createTestProject(), this is the last bit of the method:What we didn't realize, though, is that the PHP file syncer is disregarding those permissions, and creating the staged copy of sites/default with world-writable permissions. So this test is wrongfully, but understandably, assuming that the permissions from the active sites/default directory are reflected in the staged sites/default.
So, as part of fixing this problem, we will want to be certain that the staged sites/default directory has exactly the same permissions as the active one, in all cases.
Comment #36
tedbow@phenaproxima thanks for the research on #35
I think that given this we have to switch the default
file_syncer.We already have this #3355105: Warn strongly if the rsync file syncer is not in use but maybe we should change that be an error if you are using the php file copier. We found this problem with
sites/defaultbut is it safe to use the php file copier if we know will reset directory permissionsComment #37
tedbowComment #38
tedbowComment #39
phenaproximaComment #42
phenaproximaComment #43
tedbowComment #44
phenaproximaComment #45
tedbowLooks good!!!!
Comment #46
phenaproximaComment #48
phenaproximaComment #49
wim leersQuoting https://git.drupalcode.org/project/automatic_updates/-/merge_requests/84...
I very strongly disagree with:
This makes no sense. We're assuming
rsyncis available on every system. We're not even warning the user. It will fail hard, without a warning.IF we were to do this, we'd at minimum need a
hook_requirements()check. And that's precisely what #3355105: Warn strongly if the rsync file syncer is not in use is doing.But really, if we're going to do this, why even have the
phpfile syncer?!The proper solution is to:
rsync) and automatically switch to it — that's present in the MR for #3355105 but was removed by @phenaproxima at https://git.drupalcode.org/project/automatic_updates/-/merge_requests/84...This was hastily done and is incomplete. We should do better. This is very confusing 😞
Comment #50
tedbow#49
That is currently not possible. PHP file copier does not retain correct directory permissions as explained in #35. We found this issue because the scaffold had an error when we switched to rsync which does retain correct file permissions. The fact that folder permissions get changed from a protected status to 777 is not safe or at least we should assume it is not till when investigate more.
Hopefully we can get these permissions problems solved with the PHP file syncer or look into compensating on side by fixing permissions. I don't think it is worth totally removing it right now.
We should confirm this and make sure Composer stager itself does not have a pre-condition for this situations
Right now
\PhpTuf\ComposerStager\Infrastructure\Service\Finder\ExecutableFinder::findwould throw an exception in this situation probably which would error on ourbeginphaseNot sure about others, I personally don't find comments like this helpful and find them demotivating
I created this follow-up #3364135: Determine if the PHP syncer can and should be the default file copier
Comment #51
tedbowSince #3355105: Warn strongly if the rsync file syncer is not in use we will add an error in pre-create in `
RsyncValidator`Comment #52
wim leers👍