Remove generate-lockfile To Prevent Overwriting Checked-In Lock Files

[tillmann-crabnebula]

In CI builds, Rust applications or libraries are usually built with --locked or --frozen which uses the checked-in Cargo.lock file in the repository to decide which exact dependency version is used.

The current audit-check action does not respect this checked-in Cargo.lock file as it automatically overwrites the versions based on the Cargo.toml policies. This means the audited library or application is using different dependencies than the release/production build in most cases.

The cargo audit readme mentions to use cargo generate-lockfile but we couldn't figure out why this should be done in CI builds or in cases when the application or library is built with the --locked or --freeze argument.

cargo-audit implements logic to generate a Cargo.lock file if not already existing in locate_or_generate. The additional lock file generation seems not needed and causes checked-in lock files to be ignored and overwritten.

We tested the behavior of cargo audit with and without lock files in the following action runs:

Unmodified Action

outdated lock file is ignored, no vulns are found
no lock file exists, no vulns are found

Modified Action

Removed generate-lockfile invocation from audit-check action.

outdated lock file is respected, vulns are found
no lock file exists, no vulns are found

Manual Action

This action uses dtolnay/rust-toolchain to install Rust and runs plain cargo audit. It was used to compare behavior.

outdated lockfile is respected, vulns are found

[sveitser]

We also just experienced the action missing a potential vulnerability in our binaries because the lock file was regenerated.

However, I think it may make sense to regenerate a lock file for library crates because the lock file will be ignored if the library is used as a dependency. How about making the lock file (re)generation optional?

[tillmann-crabnebula]

However, I think it may make sense to regenerate a lock file for library crates because the lock file will be ignored if the library is used as a dependency. How about making the lock file (re)generation optional?

It is generally recommended to check in lock files for binaries but not for libraries.
If you don't check in the lock file (for libraries as mentioned) it will automatically generate the Cargo.lock file when calling the cargo audit binary.

For checked in lock files in binary projects and CI actions I don't think it should be re-generated during auditing workflows and absolutely respected.

[sveitser]

The official recommendations regarding checking in lock files for libraries were changed last year: https://blog.rust-lang.org/2023/08/29/committing-lockfiles.html

My thinking was that as a library author you may want to know if using your library may leave a project vulnerable regardless of whether you choose to commit the lock file or not.

[tillmann-crabnebula]

Ah yeah this change is still encouraging to check in for binaries but does not use "enforcing" language from my perspective.

My thinking was that as a library author you may want to know if using your library may leave a project vulnerable regardless of whether you choose to commit the lock file or not.

Totally agree on this and it was not part of my perspective previously.
Making it configurable would be a good addition to this PR but to be fair I am also happy if it is part of another PR, so we get the "more correct" behavior into this action rather sooner than later - the original issue is over 3 years old.

[tillmann-crabnebula]

Re-built with latest upstream changes (upgrade to node 20) to resolve merge conflicts.

[tarcieri]

Why are there changes to index.js and package-lock.js?

[tillmann-crabnebula]

As far as I understand how this action works is that the minified and bundled index.js is used to create the action in the first place (See https://github.com/rustsec/audit-check/blob/fe0359b3e165e4d83e11ffab7715e56c43cc2d76/action.yml#L16C1-L17C24), so all changes to actions behavior need to be built and checked into the repo (you merged this in #16 as well).

I mentioned in Zulip that this makes review harder and is whacky for concurrent PRs and would be open to contribute another PR where the repository workflows automatically builds the dist/index.js on merges to main.

I also updated the package-lock.json with running npm audit fix as there was a vulnerability in one of the dependencies ( I didn't really investigate if affected).
If it's better to review I can undo the package update but not the dist/index.js.

[tarcieri]

Okay, in the future it would be good to keep any changes to dependencies completely isolated and with a rationale for why they're being changed/updated, as they're an easy to introduce surreptitious supply chain attacks.

Automating the index.js builds (and more generally, automating deployments) would be good as well.

That said, approved.

[elichai]

@tarcieri Can we get a release with this change please?

[tarcieri]

@elichai see #22

I just need to figure out how to do a release, but am very, very busy