Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dependency patch resolution: Only allow patches from trusted dependencies #551

Open
4 tasks done
RobinHoutevelts opened this issue Feb 16, 2024 · 14 comments
Open
4 tasks done
Labels
enhancement New features, options, or other additions.

Comments

@RobinHoutevelts
Copy link

Verification

  • I have updated Composer to the most recent stable release (composer self-update)
  • I have updated Composer Patches to the most recent stable release (composer update cweagans/composer-patches)
  • I am using one of the supported PHP versions (8.0+)
  • I have searched existing issues and discussions for my idea.

Is your feature request related to a problem?

I only allow patches from sources I trust.
By default cweagans/composer-patches seems to apply patches from any of my dependencies.

Describe your proposed solution(s)

{
   // don't allow patches from anyone else
   "ignore-dependency-patches": ["*"],
   // list of repositories you trust to apply patches from
   "dependency-patches": ["my-org/drupal-patches"]
}

Describe alternatives

No response

Additional context

You will have less bug reports related to Dependency patch resolution if users are forced to know where they want to apply patches from.

However, I don't mind it -not my project- but I would need a way to opt-out and instead define the sources I trust myself.

@RobinHoutevelts RobinHoutevelts added the enhancement New features, options, or other additions. label Feb 16, 2024
@RobinHoutevelts RobinHoutevelts changed the title Dependency patch resolution: Only allow patches from dependencies you trust Dependency patch resolution: Only allow patches from trusted dependencies Feb 16, 2024
Copy link

👋 Thanks for the idea! Please remember that this is an open source project - feature requests may or may not be implemented, and if they are, the timeline is unknown. If you need a guaranteed implementation or timeline, sponsorships are welcome!

@cweagans
Copy link
Owner

I'm on board. Maybe we can add another config value 'allow-dependency-patches' and have that act as an allow list if set. I don't want to have to set both. Having both set should probably be an error.

@RobinHoutevelts
Copy link
Author

Thanks!

Having both set should probably be an error.

Yes, that would also make this much better to maintain 👍

@RobinHoutevelts
Copy link
Author

RobinHoutevelts commented Feb 16, 2024

If I want to disable dependency patch resolution altogether:
Would setting allow-dependency-patches: [] (empty list) achieve that?

Edit: I guess I could cheat by doing allow-dependency-patches: ["drupal/core"] or something since it doesn't define patches 😛

@cweagans
Copy link
Owner

You can just disable the resolver - no need to try to make an allow list work for that :)

@RobinHoutevelts
Copy link
Author

@cweagans I'd be happy to take a stab at adding allow-dependency-patches.
But I'm currently busy for the next two weeks.

@cweagans
Copy link
Owner

No worries. Same thing here - I'll update here if I beat you to it!

@markfullmer
Copy link

Can we clarify the intention of the allow-dependency-patches definition in regards to the current behavior? With the introduction of this option, is it understood that if allow-dependency-patches is NOT defined, there will be no change in the processing of dependency patches, i.e., all will be allowed? In other words, this will be implemented in a way that allows implementers to opt out of a default "allow all dependency patches" behavior?

@cweagans
Copy link
Owner

Correct. If the option is NOT defined, all dependencies will be scanned for patches. If it IS defined, only the listed packages will be scanned.

@Natshah
Copy link

Natshah commented May 14, 2024

I'm so interested to having this option, something like allow-plugins but it could be called
allowed-patches
or
allowed-patchers

as we do have allowed-packages for drupal-scaffold

      "allowed-packages": [
        "drupal/core", // Only listing to demo
        "vardot/varbase"
      ],

as

      "allowed-dependency-patches": [
        "drupal/core", // Only listing to demo
        "drupal/starshot-patches", // If the Drupal CMS (starshot) team selected a list of important patches.
        "vardot/varbase-patches"
      ]

Maybe in projects the support team could chose to add more patches.
Not to keep coping them every time to 200 projects or more

      "allowed-dependency-patches": [
        "vardot/varbase-patches",
        "my-trusted-private-vendor/private-list-of-patches-by-the-support-team"
      ]

@cweagans
Copy link
Owner

cweagans commented Jun 4, 2024

@Natshah All composer patches config is nested inside of a extra.composer-patches to avoid naming conflicts like that.

As an aside, you might be interested in https://github.com/cweagans/composer-configurable-plugin/ as a replacement for this file: https://github.com/drupal/core-composer-scaffold/blob/11.x/ScaffoldOptions.php

It handles getting config from composer.json + the environment, merging it all down into one location, and ensures that it's the right data type. It probably wouldn't be backwards compatible, but it might be an interesting thing to explore!

@bradjones1
Copy link

Another use case here, again from Drupal-land. Now that Drupal CI is run on GitLab the template allows modules to express patches in their composer.json which will be applied during CI runs, e.g. if they require a patch to core. I'm in a situation where the patches in question conflict with others I have on my specific project, so when they are applied from the dependency they fail. Would be nice to be able to disable the patches to avoid this conflict.

metalbote pushed a commit to metalbote/composer-patches that referenced this issue Aug 2, 2024
metalbote pushed a commit to metalbote/composer-patches that referenced this issue Aug 2, 2024
@metalbote
Copy link

I think this is a very important feature, especially when i think about security, implemented this and PR

@regilero
Copy link

regilero commented Sep 3, 2024

While thinking about securely supporting patches, I'd like for example to only allow patches added in a local folder, and present in the first level of my composer.json.
I'm not sure what should be done of untrusted patches referenced by packages, is an error message enough? Should we make the build fail?
But allowing any package of patching the other ones seems strange.

Edit: it seems on version2 no dependencies patches are running. So it fixes the problem for packages updating other packages. But still being able to fix the trusted sources of patches seems good.
Edit again: I noticed the patch-import command on v2, and here the problems are back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New features, options, or other additions.
Projects
None yet
Development

No branches or pull requests

7 participants