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

[#551] Dependency patch resolution: Only allow patches from trusted d… #588

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions docs/usage/configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,27 @@ Technically, this value can be a path to a file that is nested in a deeper direc

---

### `allow-dependency-patches`

```json
{
[...],
"extra": {
"composer-patches": {
"allow-dependency-patches": [
"some/package",
]
}
}
}
```

**Default value**: null

`allow-dependency-patches` allows you to explicitly apply patches defined by only the listed dependencies. For instance, if your project requires `some/packageA` and `some/packageB`, and both packages define patches, listing `some/packageA` in `allow-dependency-patches` would cause that only `some/packageA` will be evaluated while `some/packageB` will be ignored. If set `allow-dependency-patches: []`, no package will be evaluated. This does _not_ affect the _target_ of those patches. For instance, listing `drupal/core` here would not cause patches _to_ `drupal/core` to be ignored.

---

### `ignore-dependency-patches`

```json
Expand Down
4 changes: 4 additions & 0 deletions src/Plugin/Patches.php
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,10 @@ public function activate(Composer $composer, IOInterface $io): void
'type' => 'string',
'default' => 'patches.json',
],
"allow-dependency-patches" => [
'type' => 'list',
'default' => null,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably default to []

],
"ignore-dependency-patches" => [
'type' => 'list',
'default' => [],
Expand Down
42 changes: 33 additions & 9 deletions src/Resolver/Dependencies.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,21 +24,42 @@ public function resolve(PatchCollection $collection): void
return;
}

$this->io->write(' - <info>Resolving patches from dependencies.</info>');

// Using both config keys, if $allowed_dependencies is set, it takes precedence
$allowed_dependencies = $this->plugin->getConfig('allow-dependency-patches');
$ignored_dependencies = $this->plugin->getConfig('ignore-dependency-patches');

// First check, if we do allow dependency patches at all.
if ($allowed_dependencies === []) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe empty($allowed_dependencies) in light of defaulting the value to []?

$this->io->write(' - <info>No patches from dependencies are allowed.</info>');
return;
}

$this->io->write(' - <info>Resolving patches from dependencies.</info>');

$lockdata = $locker->getLockData();
foreach ($lockdata['packages'] as $p) {
// If we're supposed to skip gathering patches from a dependency, do that.
if (in_array($p['name'], $ignored_dependencies)) {
continue;
$allowed = in_array($p['name'], $allowed_dependencies ?? []);
$ignored = in_array($p['name'], $ignored_dependencies);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should simplify this a bit and disallow setting both allowed/ignored dependencies. You can either say "I only want patches from these listed deps" or "I want patches from all deps except these specific ones"

It doesn't make sense to say "I want patches from all deps except these specific ones, but only from these listed deps"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have 4 possibilities, if we remain the ignored dependencies possibility:
a) Current behavior: i have no allow-dependency-patches, and no ignored-dependency-patches, i wanted to preserve that all dependent patches will be used. (Case null)
b) Current behavior: I have no allow-dependency-patches, and some ignored-dependency-patches, all dependent patches should be installed except those listed in ignored. (Case null)
c) New behavior: I do not want any dependency patches, so allow-dependency should be a empty list. (Case [] )
d) New behavior: I want do allow some packages, but because of composer-merge i forgot, i have forbitten in another file package x/y to dependent patches. So this will not break existing installations but is really an enhancement without breaking.

If its okay to break current behavior, we could do differently. But i wanted to keep existing behavior, and wanted to keep expected behavior, like an empty list of allowed dependency implicit none is allowed, and if i ignore something, it will be ignored.


// Allowed dependencies is not set in composer.json, and we're not
// supposed to skip gathering patches from this dependency
if (is_null($allowed_dependencies) && !$ignored) {
$this->lookForPatches($p, $collection);
}

// Find patches in the composer.json for dependencies.
if (!isset($p['extra']) || !isset($p['extra']['patches'])) {
continue;
// Allowed dependencies are set, act only on allowed, if they're not
// ignored also.
if ($allowed && !$ignored) {
$this->lookForPatches($p, $collection);
}
}
}

private function lookForPatches(array $p, PatchCollection $collection): void
{

// Find patches in the composer.json for dependencies.
if (isset($p['extra']['patches'])) {
foreach ($this->findPatchesInJson($p['extra']['patches']) as $package => $patches) {
foreach ($patches as $patch) {
$patch->extra['provenance'] = "dependency:" . $package;
Expand All @@ -47,8 +68,11 @@ public function resolve(PatchCollection $collection): void
$collection->addPatch($patch);
}
}
}

// TODO: Also find patches in a configured patches.json for the dependency.
// Find patches in a patch-file for dependencies.
if (isset($p['extra']['composer-patches']['patches-file'])) {
// @todo Handle patch files.
}
}
}