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

Packages using `replace` are in need of moderating #362

Closed
peterjmit opened this Issue Oct 30, 2013 · 5 comments

Comments

Projects
None yet
3 participants
@peterjmit

peterjmit commented Oct 30, 2013

TLDR; Requiring a package from a trusted source can result in receiving a package from an un-trusted source.

We already have #329 however this only addresses the issue from the point of a package maintainer/submitter, and not the end user


I had a situation this evening where by composer was pulling in an unexpected package:

My app had two dependencies relating to this issue (peterjmit/ecommerce-bundle is a private "VCS" package)

{
  "peterjmit/ecommerce-bundle": "~0.9",
  "stripe/stripe-php": "~1.8"
}

I was carrying out some upgrades, and switched stripe over to "stripe/stripe-php": "~1.9" however peterjmit/ecommerce-bundle had its own dependency which remained at "stripe/stripe-php": "1.8.3"

This mismatch in requirements resulted in composer downloading an unexpected package in its place

{
  "easybib/stripe-php": "dev-master"
}

I only discovered this because my test suite found that the class "Stripe" was not found.

Since this has happened I have been advised that this is due to the replace flag https://github.com/easybib/stripe-php/blob/master/composer.json#L35 for which there has now been a PR submitted to remove the replace entry.

It is not impossible for a situation to arise (especially when taking into account #335) where malicious code could end up being incorporated into an application as a result of this behaviour.

Therefore I think that packages containing replace either have to be heavily moderated, or perhaps better any repositories containing replace should require the original package owner to allow the flag.

Other options could include adding an option to composer to turn off the behaviour of replace however replace as described in the composer docs is pretty useful. Or there could be a user prompt confirming the replacement.

As it stands I think that the fact that an end user can specify a package from a trusted entity (in this case Stripe) but receive a package from an unknown entity is dangerous. And while care & attention should be given to the requiring/updating of dependencies - under the current mechanism, this issue is easily missed.

@Seldaek

This comment has been minimized.

Show comment
Hide comment
@Seldaek

Seldaek Oct 31, 2013

Member

Just so you know we already did take some measures to prevent abuse. The only way to actually abuse something is if you require a package that does not exist, and a replacer replaces it. Then it's assumed to be valid because it is in most cases a valid use of replace. Be it for renaming packages or whatever. If two replacers exist we also prefer the one coming from the same vendor as the replaced package again to prevent misuse/abuse.

If we do introduce a moderation process, then we should at least make sure it's possible to add packages replacing other packages owned by the same maintainer or replacing packages in the same vendor name. But then it becomes really complex because what do you do when the package changes? If we only check at submission time this prevents confused users to create problems, but it won't prevent targetted abuse. If we do check every new version then it might get problematic for renames because if you have package foo/bar and submit a new one baz/qux (replacing foo/bar), then delete foo/bar we can't check anymore that you owned foo/bar, so new versions of baz/qux saying they replace it would get in the moderation queue. Then we need to track which packages can replace which.

TLDR: I agree but as always it's harder than it sounds at first, so it's unlikely moderation can be implemented in a timely manner, and we should at least work on educating newcomers to prevent those mistakes (i.e. #329).

Member

Seldaek commented Oct 31, 2013

Just so you know we already did take some measures to prevent abuse. The only way to actually abuse something is if you require a package that does not exist, and a replacer replaces it. Then it's assumed to be valid because it is in most cases a valid use of replace. Be it for renaming packages or whatever. If two replacers exist we also prefer the one coming from the same vendor as the replaced package again to prevent misuse/abuse.

If we do introduce a moderation process, then we should at least make sure it's possible to add packages replacing other packages owned by the same maintainer or replacing packages in the same vendor name. But then it becomes really complex because what do you do when the package changes? If we only check at submission time this prevents confused users to create problems, but it won't prevent targetted abuse. If we do check every new version then it might get problematic for renames because if you have package foo/bar and submit a new one baz/qux (replacing foo/bar), then delete foo/bar we can't check anymore that you owned foo/bar, so new versions of baz/qux saying they replace it would get in the moderation queue. Then we need to track which packages can replace which.

TLDR: I agree but as always it's harder than it sounds at first, so it's unlikely moderation can be implemented in a timely manner, and we should at least work on educating newcomers to prevent those mistakes (i.e. #329).

@peterjmit

This comment has been minimized.

Show comment
Hide comment
@peterjmit

peterjmit Oct 31, 2013

@Seldaek That all makes sense r.e. moderation and what is already in place, however one thing you said seems at odds with behaviour I witnessed.

I requested a package "stripe/stripe-php": "~1.9" which does exist. However because one of my other dependencies requests "stripe/stripe-php": "1.8.3" I ended up receiving different package from a different vendor.

This sounds like it may perhaps be a composer bug? (that being said I don't know how composer resolves these replacements)

peterjmit commented Oct 31, 2013

@Seldaek That all makes sense r.e. moderation and what is already in place, however one thing you said seems at odds with behaviour I witnessed.

I requested a package "stripe/stripe-php": "~1.9" which does exist. However because one of my other dependencies requests "stripe/stripe-php": "1.8.3" I ended up receiving different package from a different vendor.

This sounds like it may perhaps be a composer bug? (that being said I don't know how composer resolves these replacements)

@Seldaek

This comment has been minimized.

Show comment
Hide comment
@Seldaek

Seldaek Oct 31, 2013

Member

Thing is, if you require ~1.9 (i.e. >=1.9 AND <2.0) and somethng else requires 1.8.3, those two requirements are conflicting since they don't overlap, and no single version of a package can satisfy both. The replacer package says it replaces * so it satisfies both requirements and gets installed instead of you seeing an error for conflicting requirements. It all makes sense, but it can be confusing for users unfortunately in those edge cases.

Member

Seldaek commented Oct 31, 2013

Thing is, if you require ~1.9 (i.e. >=1.9 AND <2.0) and somethng else requires 1.8.3, those two requirements are conflicting since they don't overlap, and no single version of a package can satisfy both. The replacer package says it replaces * so it satisfies both requirements and gets installed instead of you seeing an error for conflicting requirements. It all makes sense, but it can be confusing for users unfortunately in those edge cases.

@peterjmit

This comment has been minimized.

Show comment
Hide comment
@peterjmit

peterjmit Feb 20, 2014

This is being further discussed here composer/composer#2690

peterjmit commented Feb 20, 2014

This is being further discussed here composer/composer#2690

@naderman

This comment has been minimized.

Show comment
Hide comment
@naderman

naderman Feb 21, 2014

Member

We won't need a moderation process after #2690 at all, so closing this

Member

naderman commented Feb 21, 2014

We won't need a moderation process after #2690 at all, so closing this

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