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

Fix handling of the database.fetch option #1302

Merged
merged 3 commits into from
Jan 13, 2025

Conversation

brucelay
Copy link

Issue and fix overview

This PR fixes #1072 and personal issues using cargo audit in an environment without access to the Internet. cargo audit will still fetch even if the config file has set fetch to false, requiring the --no-fetch option to always be supplied in the CLI.

The problems are that:

  1. As the issue describes, database.fetch is set to false by default rather than true as the documentation says
  2. The logic to calculate the fetch option is buggy:
    config.database.fetch |= !self.no_fetch;

When the config file is set to false, and the --no-fetch option is not supplied, this computes config.database.fetch = false | true = true, when the expected behaviour is that as no CLI flag is provided to overwrite the config file, the config file should be respected.

This PR fixes the problem by:

  • Setting the default value of database.fetch to true
  • Correcting the above logic

This PR adjusts the logic to use bitwise AND config.database.fetch &= !self.no_fetch, so that the following results are computed:

Respect the config file saying no fetch when no CLI flag is provided
config.database.fetch=FALSE self.no_fetch=FALSE => False & !False => False & True -> False

Config and CLI both say no fetch:
config.database.fetch=FALSE self.no_fetch=TRUE => False & !True => False & False -> False

Config says fetch and CLI flag not provided:
config.database.fetch=TRUE self.no_fetch=FALSE => True & !False => True & True -> True

Config says fetch but the CLI flag overwrites the config file
config.database.fetch=TRUE self.no_fetch=TRUE => True & !True => True & False -> False

Issue reproduction

The issue can be observed by performing the following:

  1. Copy the new test case from this PR into a fresh, unmodified copy of this repo.
  2. Run the test case cargo test override_default_fetch_option
  3. Observe that the test fails because database.fetch is false rather than true by default
  4. Copy cargo-audit/src/config.rs from this PR and run the test case again
  5. Observe that the test case fails because of the incorrect bitwise OR logic described above

@Shnatsel
Copy link
Member

Thank you for the PR and for such a detailed description!

I'm sorry it took so long to merge, CI was broken for unrelated reasons deep in the dependency tree and it took us a while to fix. I've merged main into your branch, I'll merge as soon as CI passes.

@Shnatsel Shnatsel merged commit 3792525 into rustsec:main Jan 13, 2025
9 checks passed
@brucelay
Copy link
Author

No worries! Thanks for merging and a belated happy new year

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Documentation on database.fetch config is confusing
2 participants