Feature: filter returned vulnerabilites and warnings to subtree of a …

[dkcumming]

…target package

Passing -t followed by the name of a target package omits any vulnerabilities or warnings found that are not from packages in the subgraph formed from the target node. This is achieved by a depth first search for the vulnerable packages, from the target node.

Currently misses for the target name, and multiple targets returned for the target name are handled naively by ignoring flag and continuing with the full graph.

If -t is not provided, no change to complexity. Providing -t increases complexity by $O(P \times (V' + E'))$ where $V'$ is the vertices in the subgraph (packages), $E'$ is the edges in the subgraph (dependencies), $P$ is the sum of all vulnerabilites and warnings found from the audit.

[Shnatsel]

Thank you for the PR!

Could you clarify why such a feature is desirable? This introduces additional complexity to the codebase, and I'm not sure what are the use cases for it.

If we do decide to merge this, I'd also expect all TODOs to be done and tests to be added.

[tarcieri]

We'd previously discussed this a bit over email, although I thought the intent was to enable it at the library-level, not necessarily something that would be in cargo-audit

[dkcumming]

Hi @Shnatsel, the motivation for the feature that auditing large packages can result in a large amount of vulnerabilities being output. Sometimes it is beneficial to isolate a particular part and see what vulnerabilities exist for that component of the package. Previously when auditing a large code base I was using cargo audit in combination with cargo tree to determine if the returned vulnerabilities were dependent in the subtree of a target package, but it felt appropriate to add the feature to cargo audit directly.

[dkcumming]

Hi @Shnatsel @tarcieri, Sorry for the long time between commits. I have had other work. I implemented some solutions to the requested changes, however this push is to get some feedback from you. Some details of this push are:

Features:

• Refactored the parameters to make use of query.
• Added feedback to user if target package doesn't exist, or multiple packages match the identifier. I had a look at cargo tree -p package for this as they had nice feedback for this. I pulled and edited some of the code (credited), extending the functionality useful for this case and stripping what wasn't relevant. But as it is now, The user provides a package identifier as a combination of name, version, url. If no package matches the identifier then the user receives error: No target package found matching identifier: not-there And if multiple packages match the identifier then the user receives

  error: Multiple packages found matching identifier: hydra-dx-math
  These packages were found that could match:
      https://crates.io/foo#[email protected]
      https://crates.io/foo#[email protected]
  

These strings that are returned are able to be copy pasted for the next cargo audit -t command and will parse correctly.

Known Problems:

• The location of the files is not in final place. As I was moving them I thought it better to get some feedback from you.
• Formatting of error messages can be improved, just wanted to see if you had anything to say on this.
• I expect there is some sub optimal rust code in here, I am not an expert rust developer by any means, so feedback is appreciated.
• The cargo tree -p method does not support if people screw up the versioning. For example if name, version, url are the same but the commit at url is different. I haven't looked into it deeper if that is intended or not.
• The tree does not print from the target package node. I missed this as I only use the json implementation, but I will implement it.

Let me know what you think when you get some time. Thank you.

[dkcumming]

Further progression. I refactored and added test cases to show the restriction of output for different branches. These can be run in the from the test directory like so:

• No filtering multi_vulns$ ../../../../target/debug/cargo-audit audit -t multi_vuln or multi_vulns$ ../../../../target/debug/cargo-audit audit -Filter base64 branch multi_vulns$ ../../../../target/debug/cargo-audit audit -t base64 -Filter chrono branch multi_vulns$ ../../../../target/debug/cargo-audit audit -t chrono or multi_vulns$ ../../../../target/debug/cargo-audit audit -t time (for the time dependency vuln directly)

Previously I thought there was an issue with the layout when printing the tree "The tree does not print from the target package node. I missed this as I only use the json implementation, but I will implement it.". After using this a few times I realised the current output is correct as is.

@Shnatsel @tarcieri
Are you able to provide feedback, or what you would need to see for a merge? This change has been very useful for RV with audits, and we believe is likely useful to others who use cargo audit, it would be great to make it available.

[tarcieri]

I worked through reviewing a rather large backlog of PRs this weekend and didn't quite get to this one. Maybe next weekend.

[dkcumming]

No problem, I see that some CI tests failed. Looks like related to the lockfile. Is that related to anything on my end that I need to address?

[tarcieri]

You can try rebasing

[dkcumming]

I synced the main branch with upstream/main. The feature branch was already merged with the upstream/main. Is this what you meant?

[ACassimiro]

@Shnatsel @tarcieri I would just like to leave a ping here and say that this was a really helpful feature for going through specific packages in a huge repository. It saved me quite a bit of effort.

[tarcieri]

I should have some time this weekend to take a look

[dkcumming]

@tarcieri If there would be any benefit from a meeting to go over anything, I'm available pretty much anytime weekend or weekday for a meeting.

[tarcieri]

@dkcumming can you rebase? that should fix the test failures

[dkcumming]

@tarcieri Should be updated now

[tarcieri]

@dkcumming looks like there's a test failure

[dkcumming]

@Shnatsel @tarcieri I haven't actually needed to use this feature on any projects since I have only been working with smaller trees since I did this. But I had some spare time this weekend to go and fix the problems that were on CI. If I get time another weekend I might add some test cases, but since I haven't needed this feature I am just going to put that on hold for now.

[Shnatsel]

Thank you for updating the PR!

I admit I am hesitant to merge this, because we are struggling to maintain the code for the basic functionality as it is. Taking on more complex features would make it even harder, and is probably not something we should be doing at this stage, unless someone is willing to provide funding to the project. @tarcieri WDYT?

[tarcieri]

I think my general concern remains that this contains logic in cargo-audit that would probably make more sense in cargo-lock and rustsec