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

Warn when a dependency is available from multiple registries #11906

Closed

Conversation

LawnGnome
Copy link

This PR adds a warning when one or more dependencies — that aren't otherwise disambiguated by specifying a registry key, or being of the path or git types — are available in more than one registry source.

The underlying issue here is defined as a dependency confusion attack in #9162: a user with multiple registries defined may believe that they are using a package from one registry, but are in fact getting it from a different registry.

Given a package that uses rand via rand = "0.8.5", but which also sees a different rand package in a different registry, the output looks like this:

image

There is, at present, no way to disable this warning. I would be happy to explore adding a flag or environment variable check to do so if that would be useful.

Implementation wise, this takes the form of a new function alongside PackageSet::warn_no_lib_packages_and_artifact_libs_overlapping_deps, invoked when resolving packages. A small sub-module has been added with a minimal state machine to drive this check — originally, this was all inline in the new PackageSet function, but the inlined version just made me want spaghetti. 🍝

The Source trait has also gained a new contains_package_name function. Some implementations are simpler than others.

To be clear, I'm not very attached to the exact mechanics here, and since I'm essentially learning Cargo as I go here, it's likely that I've missed at least one nuance in terms of how to walk through the dependency graph and their associated sources.

This PR also updates the couple of affected tests that already existed, and adds a set of new test cases to exercise the various possibilities.

Fixes #9162.

@rustbot
Copy link
Collaborator

rustbot commented Mar 29, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @weihanglo (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added A-directory-source Area: directory sources (vendoring) A-documenting-cargo-itself Area: Cargo's documentation A-git Area: anything dealing with git A-overrides Area: general issues with overriding dependencies (patch, replace, paths) A-registries Area: registries A-source-replacement Area: [source] replacement S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 29, 2023
@epage
Copy link
Contributor

epage commented Mar 29, 2023

Could you post your proposal for how to solve this in #9162?

Considering that thread ended with a cargo team member saying

I like the registry = "crates.io" way to disambiguate. I will need to think about the rest.

it doesn't sound like we are ready to move on to reviewing a PR at this time.

@weihanglo
Copy link
Member

Also, I am not fully convinced that leads to a “dependency confusion attack” like other package managers have. The situation could definitely be improved, but I'd like to hear more about why you see it as a potential vulnerability. Let's discuss in #9162.

@weihanglo
Copy link
Member

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 17, 2023
@bors
Copy link
Contributor

bors commented Sep 7, 2023

☔ The latest upstream changes (presumably #12527) made this pull request unmergeable. Please resolve the merge conflicts.

@epage
Copy link
Contributor

epage commented Mar 6, 2024

Closing due to inactivity. Let's pick up the conversation in the issue.

@epage epage closed this Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-directory-source Area: directory sources (vendoring) A-documenting-cargo-itself Area: Cargo's documentation A-git Area: anything dealing with git A-overrides Area: general issues with overriding dependencies (patch, replace, paths) A-registries Area: registries A-source-replacement Area: [source] replacement S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warning and disambiguation of crates that are in both crates.io and alternative registries
5 participants