mozilla / cargo-vet

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

Local `cargo vet diff` should ignore `target` directory #491

Closed afranchuk closed 1 year ago

afranchuk commented 1 year ago

When doing a cargo vet diff between a published cargo version and a specific git rev (i.e. a local diff by default, rather than using a browser-based viewer), the diff includes the target directory. In this case the published version had been compiled so had a ton of files, whereas the git-based version had no target directory. This produced a ton of noise in the diff that was irrelevant to the audit.

There may be some other directories that should be ignored, but target is the most obvious one.

afranchuk commented 1 year ago

It looks like the "Audit Size" is also counting the target dir. These stats must be cached somewhere, because after deleting the target directories they remain the same (but the cargo vet diff command shows the correct diff).

alilleybrinker commented 1 year ago

Good to note that the "target" directory can be configured in a .cargo/config.toml file (it's actually even slightly more complicated: see the docs). So it would probably be best for this ignoring to take that into account to figure out the correct "target" directory to ignore.

mystor commented 1 year ago

IIRC in general the fetched checkouts of crates from crates.io shouldn't be built within those directories, so I wouldn't expect a target directory to be present. It's surprising to me you ended up in a situation where there was a target directory present in either the cargo cache or within the cargo-vet cache.

Assuming this is a thing which happens normally (I'm not sure how you ended up in this situation. Perhaps rust-analyzer built it when you were looking at some files in the background?), fixing this poorly is relatively straightforward, as we could add target as a directory to the skipped path list: https://github.com/mozilla/cargo-vet/blob/088586cdf13a9959ce9ca4e1321203b34e467656/src/storage.rs#L90-L91

That's not perfectly foolproof though. As mentioned above, it is technically possible to reconfigure the target directory. Fixing this "properly" would require keeping track of the original packed tarball, and using it's directory listing to narrow down which files are considered when doing the diff (though this still wouldn't be immune to someone modifying the checkout - to fix that we'd need to run everything off of the tarball). This also wouldn't fix git checkouts, where we'd also have to keep track of that fact, and probably use the git index or something like that to determine whether files should be considered as included.

afranchuk commented 1 year ago

I'm okay with closing this as user error, albeit indirect. It would be nice if such mistakes (or even mistakenly changing code in those files) could be avoided with filesystem permissions, but that'd be up to cargo to address.