search

mozilla / cargo-vet

supply-chain security for Rust
Apache License 2.0
649 stars 43 forks source link

Violations should not appear in imports.lock if the crate is not used #486

Closed bholley closed 1 year ago

bholley commented 1 year ago

Right now on m-c, cargo-vet wants to pull in a violation entry from embark for a crate (epaint) we don't use. I'm guessing that when we modified imports.lock to be more selective we missed violations somehow. We should fix that.

bholley commented 1 year ago

Over to @mystor or @afranchuk to fix when one of them is back.

afranchuk commented 1 year ago

It took some searching to find where they are filtered out (having not written the code), but I did find it. @mystor what was the reasoning behind this? If I read the code correctly, if someone prunes the store (e.g. removing a violation for foo) and then uses foo later, the live imports will have it so the imports.lock will be updated and an appropriate error will occur. So I think it will work correctly if we remove the unused violations? Or is there a case I'm missing?

mystor commented 1 year ago

I had written up a full comment about this but apparently never submitted it so it ended up disappearing.

This was originally an intentional design decision with the first design where we wanted to pull in updates when needed, and always pulled in everything from a peer when needed. Because violations were considered important, we would forcefully import them every time similar to how we always imported criteria back then.

This wasn't changed when doing the other changes to only import what we need. Technically speaking we will never actually import a violation and store it in imports.lock as we only update imports.lock after a successful vet, and we'll never successfully vet with a relevant violation, so just switching this to always return false would roughly have the expected effect.

I'm not sure if that's the behaviour we'd want though. We might instead want to have it always import violations for third-party crates you're using whether or not they end up relevant for the audit.

repi commented 1 year ago

thanks for filing this! noticed it also in our repos after we (well I) created the violation entry in our registry, but missed filing an issue on it.