EmbarkStudios / cargo-deny

❌ Cargo plugin for linting your dependencies 🦀
http://embark.rs
Apache License 2.0
1.79k stars 86 forks source link

Bug: Yanked crate advisory gives false negative result if crate is in local cache #724

Open sergiimk opened 1 week ago

sergiimk commented 1 week ago

Describe the bug

Recently a url 2.5.3 crate was yanked and our CI rightfully started failing with:

error[yanked]: detected yanked crate (try `cargo update -p url`)

However running cargo deny check advisories locally was producing ok for all of our team members.

Only after rm -rf ~/.cargo/registry the problem became reproducible locally.

I suspect that code that check for crate presence finds it in the registry cache without checking the actual status of the crate in the source registry.

To reproduce

You could publish and yank a test crate in crates.io

cargo-deny version

0.16.2

What OS were you running cargo-deny on?

Linux

Additional context

No response

Jake-Shadle commented 1 week ago

cargo-deny doesn't update the local sparse index cache (even though it could), a deliberate decision to rely solely on cargo itself to mutate the cache so that any potential bug in cargo-deny won't negatively impact users in any way. It's up to cargo when it wants to update the cache, in this case I assume every user had a local lockfile and all versions for every crate were in the crate and src caches, so cargo didn't feel the need to update the index.

I suppose I could add an option to force fetch index updates (but not write them to disk for the aforementioned reason) but to be honest that's not a great idea since that's an individual HTTP request per unique crate name when 99.9% of the time it's not going to result in a change to the yanked status for any of the versions in your crate graph.

Personally, this kind of seems to me a good thing that your CI catches it, as it's clear exactly what crate and version was yanked, so it's a trivial command to fix and push that everyone will get once the changed is merged and pulled.

But if you think you want a force-index-fetch option so this never happens that's fine, it just won't be high priority for me since I have other stuff to do.

sergiimk commented 5 days ago

Thanks a lot for the explanation @Jake-Shadle. I think maximally relying on cargo makes total sense.

My fear was that if we add a more caching to our CI (which we plan on doing) it will also stop detecting yanked crates and dependency linting will give false negatives there too.

I wonder if adding cargo update --dry-run before cargo-deny would help, or the yanked crate will stay in the local cache.