Limit Replace / Provides to packages required by name in root package or any dep #2690

Closed
naderman opened this Issue Feb 11, 2014 · 51 comments

Projects

None yet
@naderman
Member

Problem

Replace

Original Goal

Allow a fork to state that it is compatible with another package's API so that packages depending on the original are satisfied by the fork as well. Users may intentionally pick the fork, or if the original is poorly maintained the fork may be picked automatically.

Issue

People publish potentially malicious forks that replace commonly used libraries/frameworks. Users construct dependencies which make it impossible to install the original correctly. The solver decides to install the potentially malicious fork instead - users do not expect nor understand this and may not check the results of the update process because they trust composer too much.

Provides

Original Goal

Allow packages depend on a virtual package - a name which is not an actual package but merely represents an interface. There may then be multiple packages which are all treated equally which provide implementations of the same interface. The user may either pick one of the implementations directly with a require, or he may imply which one is to be used through another dependency.

Issue

If no provider is directly specified or specified through dependencies, it is randomly selected. Users may be ok with the initial choice, but they do not expect a change when running composer update to a potentially malicious package which also provides the same virtual package.

Proposed Solution

Only packages which match a dependency _by name_ (ignoring replaces/providers) or a dependency of any potential dependency identified recursively _by name_ will be considered for installation. The error reporting should further query for alternative replacers/providers which the user can require in the root package to satisfy the dependency. This is related to the improved error reporting utility necessary as a consequence of #2661

Consequences

Positive

  • We should see fewer users confused by the packages composer picks
  • Less potential for malicious packages to be installed if users run update automatically on CI or production (_HINT: DO NOT RUN COMPOSER UPDATE IN ANY AUTOMATED FASHION_)
  • We may be able to reduce the composer memory footprint and complexity on packagist as we no longer need a list of all providers/replacers for installation but only for error reporting

Negative

  • There may be situations in which Composer will now not be able to update anymore without explicit specification of a dependency, but this should be easy to understand for users. And explicity may even be helpful.
@naderman naderman added this to the Urgent milestone Feb 11, 2014
@naderman naderman self-assigned this Feb 11, 2014
@Ocramius

👍

Ping @WalterTamboer: this is the issue that was affecting your package.

@lstrojny

We use replace with a Magento project to make sure, no dependency installs another version of Zend Framework 1 (which is already bundled with Magento). What would be the alternative for that behavior?

@Ocramius

@lstrojny it would work the same, but the top level composer.json must include a reference by name to the provided fork in order for replace or provides to work

@naderman
Member

@Ocramius @lstrojny Meaning in his case, since they use replace on the package itself if I understand that correctly, nothing would change for them at all, since the root package is installed anyway.

@lstrojny

@ocramius, @naderman Magento itself is not handled via Magento. Download the tarball, add a composer.json and add a replace-directive. Still works in the future?

@rk3rn3r
rk3rn3r commented Feb 11, 2014

what we are using atm:

we have a patched symfony 2.X[.Y] in use, because on some points we needed things that were not able to be injected.

we maintain patched 2.X.Y tags for every symfony 2.X.Y that we have in use, this could be 2.0.9, 2.0.15, 2.1.13 etc. and we also provide 2.X.* branches.

in our composer.json we have

"name": "trivago/symfony",
...
"replace": {
    "symfony/symfony",
    ... // components listing ommited
}

our applications for example have:

"require": 
    "trivago/symfony": "2.0.15"

this makes sure:

  • we only install our internal patched symfony 2
  • we never load symfony 2 updated dependencies, as long we wanted to update them
  • tools that depend on symfony 2 will use our components instead of official / unpatched ones
    • for example a library lib1 from github has "require": "symfony/console" than trivago/symfony will satisfy this dependency

This is what we really need to make sure everything works quite well and I don't know if I get your suggestion right about changing this behaviour. Do you think it will work?

@Ocramius

@rk3rn3r yes, that will work.

@Seldaek
Member
Seldaek commented Feb 17, 2014

@naderman I agree the plan looks great on paper. I'm not entirely sure what it means in the code though. Or rather not sure if we'll really save much memory if we still have to load all deps versions recursively to check for things.

Also one issue right now is that when loading symfony/symfony in the solver we also load all the symfony components because they're replaced and the exclusive-or (whatever they're called) rules must be added to the solver for each package replaced. I'm not sure if this model improves on that or not, but it'd be great if so.

@naderman
Member

I do think this will require quite a few large changes to how the RuleSetGenerator works. I would probably implement it as two passes. One in which we only recursively determine which packages are even explicitly named in any dep as a require - these would currently be loaded too. But then we no longer need to load any potential replacers in the second pass unless they were explicitly mentioned in the first pass. So in a lot of cases that should be avoidable.

@lmammino

Does the replace feature exists in other package manager such as gem or npm? If yes how do they handle similar situations?

@naderman
Member

It exists in pretty much every linux distribution's package manager. The dependency management in Composer stems from Suse's RPM installer.

@naderman naderman referenced this issue in composer/packagist Feb 21, 2014
Closed

Detect and warn on "replace" at submission time #329

@lmammino

How they managed to avoid such situation (if they did)? I don't think we should reinvent the wheel if a good solution exists...

@naderman
Member

They don't have open repositories like packagist.

@naderman naderman added a commit to naderman/composer that referenced this issue Feb 21, 2014
@naderman naderman Whitelist packages with names matching those specified before generat…
…ing rules

Addresses #2690 doesn't do any performance optimisations yet which we
could do now
3148ffd
@naderman naderman added a commit to naderman/composer that referenced this issue Feb 21, 2014
@naderman naderman Add tests for the changes in #2690 bdd7937
@naderman naderman added a commit to naderman/composer that referenced this issue Feb 21, 2014
@naderman naderman Add tests for the changes in #2690 ec12b8a
@naderman
Member

This should be fixed now, let me know if any issues arise.

@naderman naderman closed this Feb 21, 2014
@lsmith77

@naderman I assume you will write a follow up blog post .. or maybe better yet update the original blog post?

@lsmith77

oh .. and thanks :)

@Ocramius

Thanks indeed! :-)

Maybe a silly question: when is the next phar deployment going to happen?

@Ocramius

Nvm - just got the updated version right now.

@stof
stof commented Feb 21, 2014

@Ocramius the phar is build by a webhook on each push

@Ocramius

@stof yes, it was just lagging behind

@gquemener

Wow this security hole amazes me! I guess it'd have been not that hard to introduce malicious code into all symfony projects for example.
Good catch @naderman, and Thank you!! 🍻

@fprochazka

@naderman great work!

@AmyStephen

Good work!

@tompedals

👍

@donquixote

Will "suggest" add a package to the whitelist?
We have a use case where this would be quite useful..
https://drupal.org/comment/8528371#comment-8528371 (especially comments 16 and 25-28)

We don't want to add it under "require" unless we really know it is going to be needed. But we could add it under "suggest". I hope that "suggest" will be considered for "allowed" packages.

@naderman
Member

I have no idea why you think that is related to suggest. Suggest is solely used to output information to the user and has no implications on dependency resolution at all.

@KingCrunch

@donquixote My 2 cents, but actually that sounds more like an issue in the way Guzzle provides their packages (--> issue in Guzzle). For example it would be solved automatically, if guzzle/guzzle would be a meta-package.

@donquixote

It is currently the only way to mention a package without requiring it. And we could assume that a user trusts a package that is listed under "suggest".
Better would be a new key, "allow" or "whitelist".

The goal is to whitelist a package from which we don't know yet whether it will be (indirectly) required.

@donquixote

@KingCrunch:
https://drupal.org/comment/8527875#comment-8527875

  • docrtine/doctrine
  • guzzle/guzzle
  • symfony/symfony
  • symfony-cmf/symfony-cmf
  • zendframework/zendframework

Do the others all have the "metapackage" setting, and this problem will not occur for them?

@stof
stof commented Feb 28, 2014

@donquixote there is no doctrine/doctrine.

@Seldaek
Member
Seldaek commented Feb 28, 2014

As I understand it you have drupal core and drupal modules running as two different composer projects, or rather core shipping with dependencies inlined and then a proper composer project including modules and their dependencies. Then you have two autoloaders: 1) drupal core & core deps, 2) drupal modules & deps.

So if a dep requires guzzle, guzzle will be installed in the module deps but a part of it is already present in drupal core, so it conflicts.

One way I see that you can maybe solve this (assuming the module composer stuff is ran with composer as library i.e. you have programmatic control over it) is by adding a repository with higher priority than packagist, and that repo would contain a guzzle/guzzle package that would be a metapackage, replaces guzzle/whatever-drupal-core-uses, and require all the other bits of guzzle.

That said, this sounds extremely error prone to me. You'll need to maintain this list to match what you use in drupal core, you'll have to maintain the list of requires with what guzzle/guzzle (& symfony etc) replace, and possibly other problems I don't think of right now.

To sum it up, I think the right way would be having one project with everything (drupal core + modules + deps of everyone). It would simplify all that greatly and make drupal behave like all other projects. You could still ship a zip archive to people which includes all core dependencies so they can get started easy, but then the update process would run a composer update then run migration scripts, and the module installer would modify the composer.json and run an update.

@donquixote

(this is going to be a cross-post with Jordi but I'm posting it anyway)

@stof: indeed, doctrine has no place in this list.
What I could find is these composer.json files with a huge "replace" section but no mention of "metapackage".

So the scenario would be if your root package has a "replace" for e.g. "symfony/yaml", but some indirectly required package of which you don't know yet requires "symfony/symfony".

The solution is to require a meta package which resovles this conflict - like a "symfony-require-all". But I only want to require this if really needed by a vendor package - which I don't know yet. So basically I want to put the 3rd package on the whitelist and allow Composer to use it if needed to resolve the conflicts.

An alternative could be to process the "require" list somewhere in the middle of the process, and conditionally add the metapackage, or simply add all sub-packages explicitly and remove the "symfony/symfony" uber-package.

@stof
stof commented Feb 28, 2014

@donquixote a metapackage is a package with no code at all in it. symfony/symfony is not a metapackage. It has code in it.

@Seldaek The case of having 2 levels of composer resolution (once for the CMS and once for the project based on it) has been resolved by @simensen for Sculpin by creating https://github.com/dflydev/dflydev-embedded-composer

@Seldaek
Member
Seldaek commented Feb 28, 2014

@stof yup but I'm not quite sure how this embedded composer model fares with this situation.

Anyway @donquixote another option might be to just add conflict rules for all versions of guzzle/guzzle and symfony/symfony and whatever else you need. That way it should prevent the modules from depending on any of those packages, and they'll have to depend on the subpackages they need. It's not great but it might be the cheapest to implement for you at this point. Unless @simensen says his thing works perfectly fine with this edge case then it may be a better option.

@donquixote

@stof (point 1): Ok. This was refering to KingCrunch who suggested that the approach of guzzle of not using metapackage is flawed. My point was that "others are doing it too", so then symfony/symfony and others are also doing it wrong.

@stof (point 2): I am going to look at dflydev asap, after this post.

@Seldaek:
I would agree, but

  • It is always hard to estimate for me if a suggested change in Drupal 8 core is realistic or not. This one would be quite a big and consequential change. It might be worth it, but it is not my decision to make, and it is surely going to be a long discussion. Atm I am just exploring the possibilities.
  • In the current work flow, it is desirable to have some Composer packages available in Drupal core, before we know which multisite directory we are in, and which modules are enabled. This is a nasty chicken-and-egg problem, and having two separate Composer instances is a tempting way out.

You can have conflicting "require" statements in a non-Drupal or non-CMS scenario. The main difference, I think, will be that in a typical Composer scenario you know your packages, so you don't need any "conditional" stuff. You simply add "symfony-require-all" in the "require" section, and don't need any conditional whitelist.

In this case, a "whitelist" could still make sense if you follow a philosophy that is more agnostic of your indirect packages requirements.

@donquixote

That way it should prevent the modules from depending on any of those packages, and they'll have to depend on the subpackages they need.

This can work for modules' direct requirements, but not for modules that require composer packages they have no control about, and this package requires guzzle/guzzle.

@KingCrunch

@donquixote

Ok. This was refering to KingCrunch who suggested that the approach of guzzle
of not using metapackage is flawed. My point was that "others are doing it too", so
then symfony/symfony and others are also doing it wrong.

"Wrong" is such a harsh word. But yeah, thats my opinion. 😄

@stof
stof commented Feb 28, 2014

@Seldaek EmbeddedComposer registers the packages installed in Sculpin core itself as a platform repository during the installation of the project.
However, Sculpin itself can also be installed as a normal composer dependency for projects fully managed through composer. the embedded composer stuff is used when using the sculpin phar (which includes Sculpin and its deps, and these deps cannot be updated during the installation of plugins because the phar cannot be changed).
I'm not sure it will fit with the case of Drupal where the core deps are copied to the core repo itself.

@holtkamp
holtkamp commented Mar 2, 2014

Even with this functionality implemented, it seems Composer resolves a combination of dependencies to the official Zend Framework 1 package to an officious 'dummy' package as registered at Packagist:
https://packagist.org/packages/joshribakoff/zf1-empty, which uses version '*': the most general version.

I tried to include a reference to a patched version of this dummy project as a workaround, but that resulted in an unresolvable set of dependencies):
joshribakoff/zf1-empty#1 by @igorw

It seems the only way to prevent the zf1-empty project to be installed is to use patches of all packages that depend on the Zend Framework to let them require a non-specific version:

"zendframework/zendframework1": "*",

For example: https://github.com/holtkamp/zendframework1-doctrine2/blob/patch-composer-prevent-zf1-empty/composer.json#L4
How can I enable the suggested 'strict name matching' for packages in Packagist, is this even possible?

BTW, this started in composer/packagist#362 and I already suggested @joshribakoff to unregister the package from Packagist, joshribakoff/zf1-empty#2

@Seldaek
Member
Seldaek commented Mar 3, 2014

@holtkamp are you sure you're using the latest composer version (run self-update)? If so, can you post a composer.json that would allow us to reproduce this?

@holtkamp
holtkamp commented Mar 3, 2014

@Seldaek, yeah, using the latest version fff913d.

When using https://gist.github.com/holtkamp/9337191 and then running

php composer.phar update --no-dev --verbose --profile

results in https://gist.github.com/holtkamp/9337203

What I think is that Composer deduces the most suitable version of the Zend Framework to be '*', which is provided 'only' by that dummy zf1-empty project at https://packagist.org/packages/joshribakoff/zf1-empty

The (annoying way) to circumvent it is by patching some packages to decrease the required version, also to '*'. For example: https://github.com/holtkamp/zendframework1-doctrine2/tree/patch-composer-prevent-zf1-empty

And then using https://gist.github.com/holtkamp/9337260 works. The more packages involved, the more annoying it gets to keep up with all those patched versions.

I hope this clarifies the issue...

@joshribakoff

@holtkamp
You're referencing a fork of a repo you made yourself instead of the official repo, by overriding the repository source. Also the branch/tag you're referencing does not exist. I removed the "repositories" section of your composer.json and changed the version constraint to "*" and it installs the official ZF1 package.

My "dummy" package is for the purposes listed in the 2nd paragraph here, https://getcomposer.org/doc/04-schema.md#replace If composer does the wrong thing, that doesn't make my package officious... it would mean there's a security flaw in composer.

In my opinion composer is working fine here, and my package is not maliciously exploiting anything, and is used in important projects, so it cannot be changed. The error is in you referencing a tag that does not exist.

@joshribakoff

Also there's no tag in your fork matching the version contraint, only a branch. Composer's version contraints do not match against branches, they match tags.

Edit: looks like to match a branch you must prefix the branch name with "dev-". Just like how you have to put "dev-master" to specify the "master" branch.

@Seldaek
Member
Seldaek commented Mar 4, 2014

@holtkamp so the issue is you require guilhermeblanco/zendframework1-doctrine2 which requires zendframework/zendframework1: ~1.9 but you also require your zf1 fork in version dev-patch-two-level-cache-updates. Since that dev version does not match ~1.19, it gives you an error and refuses to install (since the replace issue has been fixed). You can fix it by requiring this instead:

        "zendframework/zendframework1": "dev-patch-two-level-cache-updates as 1.12.0"

The as 1.12.0 at the end makes an alias to make your dev version pass the ~1.9 requirement and make it look more like an official version. Read more about this at https://getcomposer.org/doc/articles/aliases.md#require-inline-alias

@holtkamp
holtkamp commented Mar 4, 2014

@Seldaek, ok, thanks, I guess I missed that section about inline aliasing.
@joshribakoff, thanks for your input, not sure I understand it though. I WANT to override the official package with my own changes (this introduced the 'error'...). Composer DOES consider branches, and I DID prefix them with 'dev-', right? With the suggestion of @Seldaek it now works.

The whole approach is also described here I found out: http://mnapoli.fr/overriding-dependencies-with-composer/
Thanks guys!

@stof
stof commented Mar 4, 2014

@holtkamp sure composer recognize branch. But dev-my_branch does not match the ~1.9 constraint. This is why you would need to alias it to something else

@C-Duv
C-Duv commented Apr 3, 2014

Can we also limit replace/provide on Satis side? In my case (composer/satis#129) requiring a package fetches every package that can replace it. Taking time and using too much disk space.

@rk3rn3r
rk3rn3r commented Jul 9, 2014

@naderman @Seldaek I already tweeted about an issue related to this change.

we have a package .../amqp that has

            "require": {
                "ext-amqp": "*",
                "ext-json": "*",
                "php": ">=5.4.24",
                ...
            },
            "provide": {
                "ext-amqp": "*"
            }

because we wanted to show php extension amqp is necessary, but you don't need it until you make use of one application part, where it is used and so it is not necessary when you run composer install/update.

recently it worked fine, package amqp got always installed, no matter if ext-amqp is available or not.

now on composer install it is not installed (no message, fails silently) if ext-amqp is not availale, on composer update you get an exception:

[Composer\DependencyResolver\SolverProblemsException]                        

    Problem 1                                                                  
      - The requested package ../amqp could not be found in any version, there may be a typo in the package name.

I know why you made these changes, but I am confused why it fails silently on "composer install".

do you have a solution for this, I suggest using "suggest"?

remove ext-amqp from require, change "provide" to "suggest" the ext-amqp extension?

            "require": {
                "ext-json": "*",
                "php": ">=5.4.24",
                ...
            },
            "suggest": {
                "ext-amqp": "*"
            }
@stof
stof commented Jul 9, 2014

well, if it is not a strict requirement, you should put it in suggest, not in require with a provide hack (whcih would produce weird thing for packages which really require the extension)

@rk3rn3r
rk3rn3r commented Jul 9, 2014

@stof yeah, this was my idea. but the recent idea was, that ext-amqp is required, but the .../amqp package is used in a big application and only 1-2 webpages make use of amqp, but the AmqpBundle is needed, because it's loaded in Smyfony Kernel. Ok, makes sense to put in registerBundles that AmqpBundle and bundles that references AmqpBundle in a extension_loaded('amqp') if clause...
but this is...... ugly.... a bit to me.

we will go the way now with suggest.

the idea of this change was:

  • you have a dependeny to an interface (for example FirstLevelCacheInterface my-app/my-cache)
  • and some packages that provide the my-app/my-cache
  • one requires ext-apc
  • another ext-xcache and so on
  • composer would pick the one that matches

I think the issue we have is:
when NO package could be installed for .../amqp (in our case because ext-amqp is not available and there is no other implementation for my-app/amqp on a dev server of a dev that is not working on the part of the application that uses amqp) NO package is installed, but there should be the exception that NO package for requirement ../amqp could be installed because ext-amqp dependency do not met, like we had before this change.
furthermore provide is ignored, what also is an issue, but I understand why it is needed to make the dependency resolving a bit easier

@dzuelke dzuelke added a commit to dzuelke/mongo-php-adapter that referenced this issue Feb 23, 2016
@dzuelke dzuelke Back to "replace" for ext-mongo
To use this package, either composer.json or any dependency needs to require "ext-mongo", see composer/composer#2690 and alcaeus#67 and heroku/heroku-buildpack-php#146
818a3b3
@dzuelke dzuelke added a commit to dzuelke/mongo-php-adapter that referenced this issue Feb 23, 2016
@dzuelke dzuelke Fix require instructions in README
To use this package, either composer.json or any dependency needs to require "ext-mongo", see composer/composer#2690 and alcaeus#67 and heroku/heroku-buildpack-php#146
a1a9d18
@dzuelke dzuelke referenced this issue in alcaeus/mongo-php-adapter Feb 23, 2016
Merged

Back to "replace" for ext-mongo #70

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment