The vendor hardening plugin should support changing the permissions of certain paths

Created on 5 July 2025, 23 days ago

Problem/Motivation

In ✨ Package Manager should use a copy of Composer that is local to the current project, if available Active , I propose that we allow Package Manager to use a copy of Composer that is local to the project. This would greatly reduce friction when using Automatic Updates or Project Browser, or really, anything that uses Package Manager. It would particularly benefit people who use Drupal on shared hosting, who cannot always control which version of Composer they have (some hosts use outdated versions that Package Manager doesn't support, and are unwilling to update it).

However, the idea of keeping Composer's binaries in vendor runs into the wall that is drupal/core-vendor-hardening, which deletes Composer's binaries entirely and that's that.

Proposed resolution

Give the vendor hardening plugin the ability to change the permissions of certain files, rather than deleting them outright.

I propose the following backwards-compatible configuration syntax:

"extra": {
  "drupal-core-vendor-hardening": {
    "composer/composer": [
      // New capability: change the files in this directory to have 644 permission.
      {"path": "bin", "set-permissions": true}
    ],
    // Old syntax -- delete this path
    "symfony/filesystem": ["Tests"]
  }
}

The default configuration would remain as it is now (delete Composer's binaries), but Drupal CMS would be able to override that behavior to make setting up Package Manager less painful.

The plugin would NOT allow setting arbitrary permissions. The set-permissions flag would mean "set these files to 644", without any possibility of overriding that.

Remaining tasks

Decide if this is okay (the security team probably needs to sign off on this), and then implement it if so.

✨ Feature request
Status

Active

Version

11.0 πŸ”₯

Component

composer

Created by

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @phenaproxima
  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

    After pondering the vendor hardening plugin, I think what might make the most sense is...to special-case composer/composer. We should still remove its binaries, so that vendor/bin/composer doesn't exist...but vendor/composer/composer/bin/composer should keep existing, and have its execute bit removed.

    In other words, Composer's linked binaries won't exist anymore, but its original binaries will, without their execute bits.

    I think that's the right balance of making Package Manager easier to use and not having to do an extensive refactor of the entire vendor hardening plugin and its configuration.

  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

    Discussed the above with @catch and Slack and the feeling seems to be that I should go ahead and create that MR, then let the security team and release managers consider it.

  • Merge request !12719Resolve #3534278 "The vendor hardening" β†’ (Closed) created by phenaproxima
  • Pipeline finished with Failed
    13 days ago
    Total: 170s
    #547388
  • Pipeline finished with Failed
    13 days ago
    Total: 136s
    #547390
  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

    Tagging for security review to confirm that this solution doesn't introduce any security flaws, and release manager review to confirm that this change is non-disruptive and, hopefully, eligible for backport to 11.2.3.

    Also tagging to point out that Drupal CMS needs this.

  • Pipeline finished with Canceled
    13 days ago
    Total: 379s
    #547396
  • Pipeline finished with Failed
    13 days ago
    Total: 930s
    #547399
  • Pipeline finished with Failed
    13 days ago
    Total: 182s
    #547404
  • The Needs Review Queue Bot β†’ tested this issue. It fails the Drupal core commit checks. 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.

  • Pipeline finished with Failed
    13 days ago
    Total: 494s
    #547406
  • Pipeline finished with Canceled
    13 days ago
    Total: 335s
    #547414
  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

    Get lost, bot.

  • Pipeline finished with Success
    13 days ago
    Total: 385s
    #547419
  • The Needs Review Queue Bot β†’ tested this issue. It fails the Drupal core commit checks. 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.

  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

    The bot is wrong and stupid.

  • The Needs Review Queue Bot β†’ tested this issue. It fails the Drupal core commit checks. 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.

  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

    What a nuisance!

  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    Is there a case for making this configurable, at least with an on/off flag, for sites that will never use automatic updates and want to retain the existing behaviour?

  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    Also the bot doing this has a point:

    Checking core/tests/Drupal/Tests/Composer/fixtures/fake-composer/bin/composer
    
    check failed: file core/tests/Drupal/Tests/Composer/fixtures/fake-composer/bin/composer should not be executable
    

    The core commit check script ensures all files are not executable, so we need an exception or some other way to set this up.

  • Pipeline finished with Canceled
    12 days ago
    Total: 582s
    #547951
  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

    The core commit check script ensures all files are not executable, so we need an exception or some other way to set this up.

    Okay, refactored the test a bit in the hopes that it will work on CI now.

  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

    Is there a case for making this configurable, at least with an on/off flag, for sites that will never use automatic updates and want to retain the existing behaviour?

    I think this is already configurable (by putting composer/composer: ['bin'] into the extra.drupal-vendor-hardening section of composer.json), but I should add a test to prove it.

  • Pipeline finished with Canceled
    12 days ago
    #547960
  • Pipeline finished with Failed
    12 days ago
    #547973
  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts
  • Pipeline finished with Canceled
    12 days ago
    #548008
  • Pipeline finished with Success
    12 days ago
    #548012
  • πŸ‡ΊπŸ‡ΈUnited States xjm

    I confirmed with @phenaproxima that in HEAD, the binaries are removed entirely, so the change proposed by this issue is adding an internal configuration to make the composer binaries retained and read-only at the given path.

    Composer is only a dev dependency for core, not a production one, so this mainly becomes important for sites that have Package Manager enabled (i.e. Composer is a production dependency). If this passes security review, then it's essentially a feature addition of a single internal constant and the binaries.

    However, other sites could be affected depending on the sophistication of their deployment workflows. Ideally sites' production builds will be deployed with --no-dev, but we can't be sure this is the case everywhere, so it's a bit dangerous to suddenly have these binaries popping up everywhere in unpredictable situations.

    I asked @phenaproxima to consider instead making this an optional configuration that is off by default, but that Drupal CMS (and perhaps later Package Manager itself) enables. That way, it could be backported safely with very little API surface or risk to existing sites, but Drupal CMS could also get access to the configuration immediately. This would also be a security hardening: sites or applications would need to opt into making the binaries available, and they would remain unavailable in the default case for deployments where they are not needed.

    This will also need a CR and a release note.

    Thanks!

  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

    Submitted for your consideration: a way that keeps the status quo very much intact, but gives Drupal CMS an out.

    Right now, there is no way to stop a package from being cleaned. The way the plugin is written utterly precludes that. This MR provides such a way: set a package's cleanup paths to false:

    "extra": {
      "drupal-core-vendor-hardening": {
        "composer/composer": false
      }
    }
    

    If a project sets this up, then composer/composer will not be touched. This leaves the default configuration unchanged -- the package will be cleaned as it already is. The test confirms both scenarios.

    In other words, existing sites will continue working exactly as they do now with no configuration change required. Drupal CMS, however, will be able to keep Composer's binary and will be responsible for taking whatever additional steps are necessary to make it read-only.

    This is the most minimal change I could come up with that should (I hope) be eligible for backport, without changing what will happen for new or existing sites.

  • Pipeline finished with Failed
    12 days ago
    Total: 306s
    #548367
  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts
  • Pipeline finished with Failed
    12 days ago
    Total: 149s
    #548374
  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts
  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts
  • Pipeline finished with Success
    12 days ago
    Total: 1321s
    #548389
  • Pipeline finished with Failed
    10 days ago
    Total: 210s
    #550603
  • Pipeline finished with Success
    10 days ago
    Total: 453s
    #550617
  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    From a security point of view I think this is fine, given it's new behaviour that is explicitly opt-in, so existing sites don't have to change anything. Site owners can do slightly more dangerous things with this feature but that's up to them to configure it and it's not up to us to prevent that.

    I don't see any release management issues either given there is little disruption here. The only possible minor disruption is the addition of the File constraint which there is a chance is already used in the wild somewhere, though given this just uses the Symfony constraint it is likely they have made the same implementation anyway. We should add a change record for this just in case.

    We should also document this new capability in composer/Plugin/VendorHardening/README.txt, marking NW for that and the CR.

    Also removing the bot tag as I think the bot was correct earlier when we had that executable file in the patch, that is gone so the bot should be happy.

  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

    Documentation and change record added.

  • Pipeline finished with Canceled
    5 days ago
    Total: 369s
    #554132
  • Pipeline finished with Success
    5 days ago
    Total: 443s
    #554137
  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    No further comments, this is as small a change as can be to get this feature in. Marking RTBC for another committer to double check everything.

  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

    Committers, please backport this to 11.2.x. πŸ™

    Doing so will allow this security improvement to come to existing Drupal CMS projects, and it will greatly help new Drupal CMS users get set up with Package Manager (without needing to wait until 11.3.0). The change set is as small as possible to facilitate a clean backport, and I have been explicit about this request since I started talking about it with the release managers. :)

  • πŸ‡ΊπŸ‡ΈUnited States xjm

    When this is committed, remember to tag the issue for the release notes of the correct 11.2.x patch release (which is 11.2.3 as of now).

    • catch β†’ committed ce807bf9 on 11.2.x
      Issue #3534278 by phenaproxima, longwave, xjm: The vendor hardening...
    • catch β†’ committed e96610e2 on 11.x
      Issue #3534278 by phenaproxima, longwave, xjm: The vendor hardening...
  • πŸ‡¬πŸ‡§United Kingdom catch

    I think this looks good and it will help Drupal CMS site a lot.

    Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

Production build 0.71.5 2024