Feature: filter returned vulnerabilites and warnings to subtree of a … #862
Conversation
|
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. |
|
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 |
rustsec/src/report.rs
Outdated
| /// Generate a report for the given advisory database and lockfile | ||
| pub fn generate(db: &Database, lockfile: &Lockfile, settings: &Settings) -> Self { | ||
| /// Generate a report for the given advisory database and lockfile, optional narrowing of subgraph from target package | ||
| pub fn generate(db: &Database, lockfile: &Lockfile, settings: &Settings, tree:&dependency::Tree, target:Option<&Package>) -> Self { |
There was a problem hiding this comment.
There are already two types for carrying additional state so you don't have to add parameters to this method: Settings and its Settings::query field which contains a database::Query.
I would think you could access the dependency tree via the Lockfile, and this feature could potentially set some option on the database::Query.
There was a problem hiding this comment.
Yes, this makes more sense. I will do a refactor using Query
|
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 |
…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.
…s` occurs in `query_vulnerabilities`. `Warnings` that are `Yanked` are still occuring in `check_for_yanked`.
- Accepts target package identifier, and validates correct name, version, source. - Filters packages in lockfile by the name, version, source looking for unique package matching target. - If no package matches, the identifier is retuned with the error. - If multiple packages could match the identifier, then the provided identifier and the possible targets are returned with the error. - If a unique target is found the package is returned.
dkcumming
force-pushed
the
feature-target-package
branch
from
4360c3d
to
6bc1087
Compare
|
Hi @Shnatsel @tarcieri, Features:
These strings that are returned are able to be copy pasted for the next Known Problems:
Let me know what you think when you get some time. Thank you. |
cargo-audit/src/lockfile.rs
Outdated
|
|
||
| // ! Package identifiers. | ||
| // ! | ||
| // ! Adapted from Cargo's `package_id_spec.rs`: | ||
| // ! | ||
| // ! <https://github.com/rust-lang/cargo/blob/master/src/cargo/core/package_id_spec.rs> | ||
| // ! | ||
| // ! Copyright (c) 2014 The Rust Project Developers | ||
| // ! Licensed under the same terms as the `cargo-lock` crate: Apache 2.0 + MIT |
There was a problem hiding this comment.
This does not belong here, but I was wondering what structure you would recommend.
Running `cargo audit` alone, or with `-t multi-vulns` will show both vulnerabilities. Running `cargo audit` with `-t base64` will show the `base64` vulnerability, and omit the `chrono` branch with `time` vulnerability. Running `cargo audit` with `-t chrono`, or `-t time` will show the `time` vulnerability, and omit the `base64` vulnerability.
|
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:
Previously I thought there was an issue with the layout when printing the tree " @Shnatsel @tarcieri |
|
I worked through reviewing a rather large backlog of PRs this weekend and didn't quite get to this one. Maybe next weekend. |
|
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? |
|
You can try rebasing |
|
I synced the |
|
I should have some time this weekend to take a look |
|
@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. |
|
@dkcumming can you rebase? that should fix the test failures |
|
@tarcieri Should be updated now |
|
@dkcumming looks like there's a test failure |
…target package
Passing
-tfollowed 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$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.
-tis not provided, no change to complexity.Providing
-tincreases complexity by