Feature/multiple filters, fix dedup CLI config values & other fixes #1165

aleksanderkrauze · 2024-04-07T13:38:30Z

This PR:

implements Allow to specify multiple --target-os and --target-arch #1160
fixes BinCommand duplicates options of AuditCommand in cargo-audit #1164
fixes parsing of CLI --color flag

I have formulated a couple of questions whilst I was reading code and would like to ask them here.

Should cargo-audit --color support value termcolor::ColorChoice::AlwaysAnsi? Before I refactored --color flag, this value was not supported, so I left it that way. But should this remain?

After I removed duplicated flags from BinCommand I have noticed that FixCommand also duplicates flag --file of AuditCommand, which allows to specify path to Cargo.lock file. I wanted to remove it as well, and use --file passed to AuditCommand, when running FixCommand, but I don't think this is easy to do, because FixCommand implements Runnable and its method run does not allow to pass arguments from external context.

I have never before worked with Abscissa before, so I don't know what is the best possible solution. Maybe FixCommand doesn't need to implement Command? Maybe --file should be also added to AuditConfig? Maybe FixCommand.file should be changed to RefCell<Option<PathBuf>>, skipped in clap, and then set in AuditCommand::run before it invokes FixCommand::run? I would be happy to hear a suggestion on how to implement it best. And I think it should be fixed, the same as duplicated arguments in BinCommand should have been fixed.

I would also like to do something with another issue I have created - #1161. As I understand, crate platforms is auto-generated, and so are values for target Arch and OS. But where exactly are they generated from? And how to point users to an exhaustive list of those values?

Shnatsel · 2024-04-07T15:03:07Z

Thanks! So CI is going to fail but it's not your fault - it also fails on master due to reliance on a yanked crate, #1156

I should probably fix that first so that we get working CI again.

aleksanderkrauze · 2024-04-07T17:49:56Z

Ok. I will wait for an update info and then I will rebase.

And I will be waiting concurrently for any review comments, or answers to the questions I asked.

Shnatsel · 2024-04-09T17:54:45Z

I've fixed CI in #1167, please rebase or merge

Shnatsel · 2024-04-09T17:56:22Z

I have never before worked with Abscissa before, so I don't know what is the best possible solution. Maybe FixCommand doesn't need to implement Command? Maybe --file should be also added to AuditConfig? Maybe FixCommand.file should be changed to RefCell<Option<PathBuf>>, skipped in clap, and then set in AuditCommand::run before it invokes FixCommand::run? I would be happy to hear a suggestion on how to implement it best. And I think it should be fixed, the same as duplicated arguments in BinCommand should have been fixed.

@tarcieri any thoughts on the interaction between Clap and Abscissa here?

tarcieri · 2024-04-10T12:50:14Z

clap was retrofit into abscissa which previously used gumdrop as its command line arguments parser.

I don't know exactly what changes are needed. If someone wants to open a PR I can take a look.

aleksanderkrauze · 2024-04-16T07:15:00Z

Oh, sorry. I've missed your message, that I can now rebase. I apologize for the delay.

`cargo-audit` can now be configured to filter vulnerabilities by multiple target operating systems and CPU architectures. This can by done both in config file and in CLI. Support for old config file format (one where `target.os` and `target.arch` have a value of string) is kept.

CLI arguments defined in `AuditCommand` were also duplicated in `BinCommand`, with those in `BinCommand` taking precedence. This commit removes all duplicated flags. CliConfig was used to "unify" CLI flags defined in both `AuditCommand` and `BinCommand`. However since now those flags are only present in `AuditCommand`, `CliConfig` is no longer necessary, and as such was removed. If in future a need arises for subcommands to have their specific configuration, then overriding of `AuditConfig` can be done directly in `impl Override<AuditConfig> for AuditCommand`.

This fixes panic that were occurring when invalid color value was passed as an CLI argument. Now `clap` handles validating possible values.

aleksanderkrauze · 2024-04-16T07:29:02Z

I have rebased.

What about my question regarding FixCommand? I would like to propose some solution, so it can be discussed as @tarcieri suggested, but I would prefer to do it after current changes are accepted. Or should this be split off to another PR?

aleksanderkrauze · 2024-05-04T07:43:11Z

@Shnatsel Hi. I don't want to sound pushy, but could someone perhaps review this PR, please? I have ideas how to dedup --file flag, but I would like to be firstly sure, that current patches are accepted.

Shnatsel · 2024-05-04T14:42:18Z

Oh! Thanks for the ping, the notification about the PR update got lost in my inbox. Let me take a look

Shnatsel · 2024-05-04T14:46:42Z

CI fails due to latest rustc adding more lints. I've fixed it in #1181; CI should pass if you merge main into this branch.

Shnatsel · 2024-05-04T14:52:12Z

Should cargo-audit --color support value termcolor::ColorChoice::AlwaysAnsi? Before I refactored --color flag, this value was not supported, so I left it that way. But should this remain?

I cannot think of any uses for it. We haven't seen any user requests for it either. Let's leave it out to keep the user interface simple, and only add it if someone asks for it.

What about my question regarding FixCommand? I would like to propose some solution, so it can be discussed as @tarcieri suggested, but I would prefer to do it after current changes are accepted. Or should this be split off to another PR?

I think the bottom line is "none of the maintainers have a clue", so let's leave that to a follow-up PR.

I would also like to do something with another issue I have created - #1161. As I understand, crate platforms is auto-generated, and so are values for target Arch and OS. But where exactly are they generated from? And how to point users to an exhaustive list of those values?

Yes, that is correct. I have explained where they come from in the linked issue. We should list the recognized values somewhere; either in the help text, or have a flag akin to rustc --print=target-list. Clap has long and short help texts; we could put the list into the long one I suppose? Anyway, that is also best to leave to another PR; I'd like to get this merged.

Shnatsel

Looks good!

The only issues I could spot are some typos in comments. Thanks a lot for this PR!

Shnatsel · 2024-05-04T14:53:15Z

cargo-audit/audit.toml.example

+arch = ["x86_64"] # Ignore advisories for CPU architectures other than this ones
+os = ["linux", "windows"] # Ignore advisories for operating systems other than this ones


Suggested change

arch = ["x86_64"] # Ignore advisories for CPU architectures other than this ones

os = ["linux", "windows"] # Ignore advisories for operating systems other than this ones

arch = ["x86_64"] # Ignore advisories for CPU architectures other than these

os = ["linux", "windows"] # Ignore advisories for operating systems other than these

Shnatsel · 2024-05-04T14:54:55Z

cargo-audit/tests/audit.toml.legacy

+# Legacy audit config file
+#
+# This is an older version of audit.toml, that we want to still parse
+# and support.


Excellent to have this tested 👍

Shnatsel · 2024-05-04T14:55:15Z

cargo-audit/tests/config.rs

+}
+
+/// Ensure `target.arch` and `target.os` continue to parse when they
+/// are specified as a string, and not a list. This is the legacy behovior.


Suggested change

/// are specified as a string, and not a list. This is the legacy behovior.

/// are specified as a string, and not a list. This is the legacy behavior.

Shnatsel · 2024-05-05T14:16:27Z

I've rebased this myself in #1185 and merged that PR so that this doesn't stall out again.

Thanks a lot for your contribution!

I'd be happy to have more PRs from you, and hopefully I won't miss the notification next time.

aleksanderkrauze added 4 commits April 16, 2024 09:22

Make Query's target arch & os a Vec<T> instead of Option<T>

7c5745e

Use clap to properly parse --color argument

0be7612

This fixes panic that were occurring when invalid color value was passed as an CLI argument. Now `clap` handles validating possible values.

aleksanderkrauze force-pushed the feature/multiple-filters branch from f90cf8f to 0be7612 Compare April 16, 2024 07:23

Shnatsel approved these changes May 4, 2024

View reviewed changes

Shnatsel mentioned this pull request May 5, 2024

Feature/multiple filters, rebased #1185

Merged

Shnatsel closed this May 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/multiple filters, fix dedup CLI config values & other fixes #1165

Feature/multiple filters, fix dedup CLI config values & other fixes #1165

aleksanderkrauze commented Apr 7, 2024

Shnatsel commented Apr 7, 2024

aleksanderkrauze commented Apr 7, 2024 •

edited

Shnatsel commented Apr 9, 2024

Shnatsel commented Apr 9, 2024

tarcieri commented Apr 10, 2024

aleksanderkrauze commented Apr 16, 2024

aleksanderkrauze commented Apr 16, 2024

aleksanderkrauze commented May 4, 2024

Shnatsel commented May 4, 2024

Shnatsel commented May 4, 2024

Shnatsel commented May 4, 2024

Shnatsel left a comment

Shnatsel May 4, 2024

Shnatsel May 4, 2024

Shnatsel May 4, 2024

Shnatsel commented May 5, 2024

		arch = ["x86_64"] # Ignore advisories for CPU architectures other than this ones
		os = ["linux", "windows"] # Ignore advisories for operating systems other than this ones

	/// are specified as a string, and not a list. This is the legacy behovior.
	/// are specified as a string, and not a list. This is the legacy behavior.

Feature/multiple filters, fix dedup CLI config values & other fixes #1165

Feature/multiple filters, fix dedup CLI config values & other fixes #1165

Conversation

aleksanderkrauze commented Apr 7, 2024

Shnatsel commented Apr 7, 2024

aleksanderkrauze commented Apr 7, 2024 • edited

Shnatsel commented Apr 9, 2024

Shnatsel commented Apr 9, 2024

tarcieri commented Apr 10, 2024

aleksanderkrauze commented Apr 16, 2024

aleksanderkrauze commented Apr 16, 2024

aleksanderkrauze commented May 4, 2024

Shnatsel commented May 4, 2024

Shnatsel commented May 4, 2024

Shnatsel commented May 4, 2024

Shnatsel left a comment

Choose a reason for hiding this comment

Shnatsel May 4, 2024

Choose a reason for hiding this comment

Shnatsel May 4, 2024

Choose a reason for hiding this comment

Shnatsel May 4, 2024

Choose a reason for hiding this comment

Shnatsel commented May 5, 2024

aleksanderkrauze commented Apr 7, 2024 •

edited