The security team has determined that letting Package Manager's path to Composer be configurable is not a good idea. In the worst-case scenario, it is a potential RCE vector.
Completely remove the package_manager.settings:executables config structure, with an update path.
Refactor our ExecutableFinder to look for Composer in exactly one place: locally installed in the project. If it's not installed in the project, fall back to PATH detection.
The expectation here is that, if you want to use Package Manager, you need either:
PATH, in a supported version.composer require composer/composer), which you are expected to do at the command-line. Drupal CMS users won't have a problem with this, since Drupal CMS can add Composer to its runtime dependencies anyway. Plain core users are generally more technical and can be reasonably expected to run a simple command to get Composer into their runtime (or dev) dependencies.For rsync, we should continue to allow that to be overridden, only by a setting. We have received far fewer complaints (if any) about rsync being undetectable in PATH, so although it is prudent for this to be changeable by the site owner/developer, we don't need to make it easy.
A pair of configuration options will be removed.
Package Manager no longer supports storing the paths of Composer and rsync in configuration. Instead, Composer should be installed locally as a dependency of the Drupal project, and the path to rsync should be stored in a setting. See this change record for more information.
| Comment | File | Size | Author |
|---|
Show commandsStart 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, plain diff MR !12932
Comments
Comment #2
phenaproximaComment #4
phenaproximaComment #5
catchTwo questions about this.
1. The MR currently completely removes the configuration for composer, as an experimental alpha module that's fine as far as core's concerned.
However package_manager is already used in production by Drupal CMS sites (or other sites using project browser/automatic updates) and those sites may be relying on having configured the composer path. Also depending on the host and who installed the site, running composer require composer/composer from the command line may not be an easy option.
So I'm wondering if this needs a two step process.
PATHcomposer can't be found. When this is the case, we could have a hook_requirements() warning telling people they need to fix the problem, but it wouldn't immediately break.2. It might be possible for automatic updates or project browser to add an update that detects whether composer is found locally or in
PATH, and if it's not, runscomposer require composer/composervia package manager to install a local composer. Once the local composer is installed, the site would then no longer be using the configuration any more and it would be safe to remove. If automatic updates/project browser introduces that update path before Drupal 12 support is added, it would then more or less guarantee that sites continue to work once the configuration is removed.The above isn't necessary for core, but I think it probably needs discussion with the Drupal CMS team whether that's something that's worth attempting.
Comment #6
phenaproximaI was thinking along similar lines. It probably makes sense to remove the
executablesstructure from config schema, so that it can stay in existing sites but is no longer considered "valid" config, so as not to break sites (and also give us a clear way to tell if the site owner has fixed the problem).Meanwhile, the executable finder can use it as a final fallback, and trigger a deprecation warning. We can also do a separate requirements error (or warning, I guess) that having a configured path to Composer is deprecated and will not be supported in Drupal 12.
Comment #8
catchCrediting @benjifisher for slack discussion related to this.
Comment #9
dwwReading summary and MR, I wonder if a setting fallback (instead of having to install it via composer or adjust the PATH) for finding composer would also make sense. Seems easier to migrate from the config to a setting than the other choices. I don’t see how it’s worse from a security standpoint. We already have to trust settings isn’t writable by malicious actors.
Comment #10
phenaproximaI agree with @dww and have added the settings-based fallback.
Comment #11
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #12
phenaproximaComment #13
pameeela commentedComment #14
mglamanRead it over. Looks good to me! I like that we're getting the Composer binary path from within it's vendor directory versus vendor/bin/composer.
Comment #15
phenaproximaComment #16
dwwSorry, only on my phone. Hard to closely review. But found a few concerns to start.
Comment #17
poker10 commentedI have added one minor comment about the "read-only" vs "non-executable" wording.
Comment #18
phenaproximaFixed both suggestions, which are minor enough that I think we can go directly back to RTBC here.
Comment #19
catchThis removes the rsync and composer config from the schema without removing the config keys themselves - this is to support the hook_requirements() pointing to settings.php.
However it's not clear to me how someone is supposed to actually remove those config keys from a site?
We might need to add an upgrade path and skip the hook_requirements() - there should be one for when composer/rsync can't be found anyway.
Or if we otherwise want to keep that, how do people remove this from config?
Package manager isn't beta yet so manual steps are OK but a lot of sites have it installed via Drupal CMS now and we shouldn't cause them to have data integrity issues without an obvious way out.
Comment #20
phenaproximaThe idea -- and I probably should have documented this in the issue summary, or open a postponed follow-up -- is that in Drupal 12, we'll have an update path to remove those keys.
So we commit this now, and give people a nice, long runway to make the requisite changes. The change record documents how to do it. But if people don't remove the keys, they will still only get requirements warnings, not errors, and if they're okay with that, then the keys will automatically go away when they update to Drupal 12.
Does that seem reasonable? Tentatively restoring RTBC here since I think that's what we'd kinda-sorta agreed on in Slack discussions, but by all means kick this back if I'm jumping the gun here. (Or, if you want me to open a follow-up for the upgrade path that removes the keys.)
Comment #21
catchI think a follow-up is good, but yeah let's open it - 12.x will be open soon.
Comment #22
phenaproximaOpened #3545555: [PP-1] Remove the `package_manager.settings:executables` config structure.
Comment #26
catchCommitted/pushed to 11.x and cherry-pick (with a small amount of manual merge conflict resolution) to 11.2.x, thanks!
Comment #28
mondrakeFiled #3545592: Use #[IgnoreDeprecations] instead of #[Group('legacy')] - again for follow up