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

Allow replace to work as originally intended when replace is from a trusted repo #6843

Closed
wants to merge 3 commits into from

Conversation

mbaynton
Copy link
Contributor

Problem

replace was rendered basically inert by #2690 because one particular
repository composer can use is open
to unvetted submissions, and this could allow any malicious publisher to make Composer select their replacement of a popular dependency.

However, if the repository a replace originates from is not open, but rather is maintained entirely by an organization the composer user trusts, then it is still safe to honor those replace relationships, and when working with such repositories it would be incredibly useful for these to work like they were originally intended. So, this PR replaces hardcoded logic that was tailored to packagist with a decision based on a repository-specific property.

Motivation

I'm working on a project that aims to utilize Composer more like the package manager of a Linux distribution than as a developer tool. That is, it would primarily install things not from packagist, but from a more stable repository of selected packages that are forked from upstream sources and maintained for security and major bugfixes only. (Drupal 8 is the impetus, you can read more about it here.)

If I name my vendors and packages to match those in packagist and list my repository in the root composer.json, my packages are correctly preferred due to repository priority and packagist always being lowest. Awesome! But I think there are some real downsides to using the names for vendors and packages found in packagist, such as

  1. Using packagist vendor/package names makes my packages indistinguishable from upstream packages in the FriendsOfPHP security advisory database, and this would prevent writing a generic security status checker for Drupal 8 that works whether you choose to source packages from iglue or direct from upstream authors.
  2. En-masse plagarism of vendor and package names in another repository just seems like not the best idea. Humans could get confused, upstream authors could get annoyed.

So instead of using the same vendor names, I've been experimenting with suffixing "@iglue" to vendors, e.g. symfony@iglue/routing and using replace to allow them to satisfy dependencies. But that requires this PR for replace to have its originally intended effect.

What's changed?

  • A new trust-replace property is added to all repositories. By default it is false, which retains existing solver behavior.
  • trust-replace can be set by the user in the root composer.json by adding it to a repository's options, or in the case of composer repositories, it can be set by the repository through the repository's root packages.json. Values in composer.json take precedence.
  • When a repository's replace data is trusted, any packages from that repository claiming to replace a provider are whitelisted, not just those that also match by name (see Pool::computeWhatProvides()).
  • Lots of new tests.

Benefits

  • Makes replace work again!
  • Doesn't really create any BC breaks in the process.
  • Probably a useful feature for private packagist customers.
  • Makes Composer a better tool for managing dependencies within a more curated / long-term support environment than packagist.

Considerations

  • I believe it is reasonable for composer repositories to assert themselves as trusted in their root packages.json because any such repository must be added by the user to a project's composer.json, and this implies the user trusts that repository. Any repository in a project's composer.json already has the means to override which code gets installed by publishing alternative packages under the same vendor/package name as is found on packagist.
  • The same implied trust applies to e.g. vcs repositories, however I've left their trust-replace defaulting to false to prevent surprises with developers publishing to packagist and getting different solver behavior.
  • For composer repositories with hasProviders() == true, the repository .json dumps also need to be modified from what packagist does to include candidate replacement package data within the provider files of the packages they replace. So for example /p/symfony/routing%hash%.json needs to contain also packages in that repository of different names that replace symfony/routing.
  • This introduces ambiguity in solver output when all of the following are true:
    • A required package no longer exists in any repository;
    • More than one replacement for the required package is offered in the same repository; and
    • that repository is trusted.

I think all of of those conditions being met in reality would be rare, but this is an area of possible future work. Currently, the first replacement found among the repository's packages is selected, so it's only somewhat stable.
I'd preliminarily suggest a heuristic of preferring packages that claim to replace fewer things. This would produce the "right" behavior at least in cases like not selecting the whole of symfony/symfony as the best replacement for just one of its components. The below test code could then be added to
solverInstallReplacedPackageWithRepoTrustProvider(); currently it exposes the solver ambiguity and results in test failure.

 $data['lts distribution of symfony test'] = array(
     'repo1Data' => array(
         'trust-replace' => true,
         'packages' => array(
             array(
                 'name' => 'ltsvendor/routing',
                 'version' => '3.2.13',
                 'replace' => array(new Link('ltsvendor/routing', 'symfony/routing', $this->getVersionConstraint('==', '3.2.13'))),
                 'expectInstalled' => true,
             ),
             array(
                 'name' => 'ltsvendor/config',
                 'version' => '3.2.13',
                 'replace' => array(new Link('ltsvendor/config', 'symfony/config', $this->getVersionConstraint('==', '3.2.13'))),
             ),
             array(
                 'name' => 'ltsvendor/symfony',
                 'version' => '3.2.13',
                 'replace' => array(
                     new Link('ltsvendor/symfony', 'symfony/routing', $this->getVersionConstraint('==', '3.2.13')),
                     new Link('ltsvendor/symfony', 'symfony/config', $this->getVersionConstraint('==', '3.2.13')),
                 ),
             ),
             array(
                 'name' => 'app',
                 'version' => '1',
                 'require' => array('a' => new Link('app', 'symfony/routing', $this->getVersionConstraint('>=', '3.2'), 'requires')),
                 'expectInstalled' => true,
             )
         )
     ),
     'repo2Data' => array(
         'trust-replace' => false,
         'packages' => array(
             array(
                 'name' => 'symfony/symfony',
                 'version' => '3.2.14',
                 'replace' => array(
                     new Link('symfony/symfony', 'symfony/routing', $this->getVersionConstraint('==', '3.2.14')),
                     new Link('symfony/symfony', 'symfony/config', $this->getVersionConstraint('==', '3.2.14')),
                 ),
             ),
             array(
                 'name' => 'symfony/routing',
                 'version' => '3.2.14',
             ),
             array(
                 'name' => 'symfony/config',
                 'version' => '3.2.14',
             ),
         )
     ),
     'installRequests' => array('app'),
 );
 $r = $data['lts distribution of symfony test'];
 $r['repo1Data']['packages'] = array_reverse($r['repo1Data']['packages']);
 $data['lts distribution of symfony test.2'] = $r;

I'd be happy to update the doc/ folder before any merging happened.

mbaynton added a commit to iglue-repo/repository-server that referenced this pull request Nov 26, 2017
@naderman
Copy link
Member

In principle I like the idea and I had something like this in mind before. But I think it would have been great if you had gotten in touch with us to discuss this before pouring this amount of work into it.

Since this is a rather unusual edge case that hardly anyone seems to have had a use for so far, I'd rather not touch these pieces of code in Composer v1 anymore. We do plan on working on Composer v2 which will change the protocol to talk to Composer repositories and generally make a lot of changes to separate the retrieval of package info from the solving of dependencies. This should make changes like these much easier to review / test.

So I'll close this for now, and I'd be happy to revisit this once those refactorings are done for v2. Most notably #4163

@naderman naderman closed this Nov 29, 2017
@mbaynton
Copy link
Contributor Author

Okay, well that's too bad. I didn't discuss it in advance because I thought it was pretty solid and adhered to reasonable expectations for a change (adds a feature without breaking anything.) I'm unsure of the statement about hardly anyone needing it -- nobody's using replace because replace doesn't work, not because they wouldn't use it if it did. I think fork github repo -> change vendor appropriately in composer.json and mark as replacement -> add vcs repository to another composer project would be a really common workflow if you could do it.

So I'll need to figure out how to wedge this into a plugin I guess, if I'm able. In the interests of asking ahead of time, would you be amenable to adding extensibility points (new events or such) to make that more achievable? I'm not seeing a feasible way to achieve the same result through plugins at the moment, though I've only looked briefly.

@naderman
Copy link
Member

@mbaynton I don't see a problem with adding extension points that don't impact composer.

Replace is actually used a lot (as we can see on packagist.org) and it works fine for most people's use cases. Hardly anyone is trying to replace an entire framework at once though, which is why I'm saying this is a very uncommon use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants