Bug: Pre-release versions not considered matching

[dbrgn]

This test fails:

diff --git a/tests/regression.rs b/tests/regression.rs
index a47a777..041bd7c 100644
--- a/tests/regression.rs
+++ b/tests/regression.rs
@@ -28,3 +28,22 @@ fn test_regressions() {
         }
     }
 }
+
+// See https://github.com/RustSec/cargo-audit/issues/17
+#[test]
+fn test_suffix() {
+    let req = semver::VersionReq::parse(">= 0.10.2").unwrap();
+    let version = semver::Version::parse("0.12.0-pre.0").unwrap();
+
+    // Test parsing
+    assert_eq!(version.major, 0);
+    assert_eq!(version.minor, 12);
+    assert_eq!(version.patch, 0);
+    assert_eq!(version.pre, vec![
+        semver::Identifier::AlphaNumeric("pre".into()),
+        semver::Identifier::Numeric(0),
+    ]);
+
+    // Test matching
+    assert!(req.matches(&version));
+}

It matches the req if I remove the "-pre.0" suffix though.

This causes cargo-audit to report wrong vulnerabilities.

I assume this is a bug, and does not follow the spec, right?

[dbrgn]

Hm, I just found this comment in the code:

    // https://docs.npmjs.com/misc/semver#prerelease-tags                                           
    fn pre_tag_is_compatible(&self, ver: &Version) -> bool {                                           
        // If a version has a prerelease tag (for example, 1.2.3-alpha.3) then it will                 
        // only be                                                                                     
        // allowed to satisfy comparator sets if at least one comparator with the same                 
        // [major,                                                                                     
        // minor, patch] tuple also has a prerelease tag.                                              
        !ver.is_prerelease() ||                                                                        
            (self.major == ver.major && self.minor == Some(ver.minor) &&                            
                 self.patch == Some(ver.patch) && !self.pre.is_empty())                             
    }                                                                                               

That may be correct for version resolution, but not for a case like vulnerability checking.

Maybe the matches method should accept a flag indicating whether to allow prereleases or not?

[steveklabnik]

Yes, it's not what we want for resolution, for sure. That being said, if it doesn't make it for resolution, how would it make it into a vulnerability? Wouldn't you want those checks to follow resolution?

[dbrgn]

Hm, I can't quite follow.

In cargo-audit, an advisory is published as a file containing the semver specifiers for versions where the vulnerability is patched. Example.

Vulnerability checking is done by testing whether a certain version is included in the patched range or not.

[steveklabnik]

Right, so if I had ">= 0.10.2, and the fix was in 0.12.0-pre.0, I wouldn't get the fix, thanks to resolution. So I'm vulnerable.

[dbrgn]

No, it's the other way around. If a program has 0.12.0-pre.0 in its lockfile, then cargo-audit checks whether it matches >= 0.10.2 (which should return true since 0.12.0-pre.0 contains the fix).

I'm not sure how checking is done for libraries without a lockfile, maybe @tarcieri knows more.

[tarcieri]

cargo audit presently requires a lock file, as it relies on cargo to do the dependency resolution

[dbrgn]

@steveklabnik are there any news on this issue? Unfortunately it renders cargo audit useless for all projects that use a pre-release of hyper, since it reports a patched version as vulnerable...

[steveklabnik]

News is basically, "I have no time to prioritize work on SemVer right now, I hope to get back to it soon, the lack of 1.0 is really bugging me, but I'm also only human."

[steveklabnik]

That said, I am also still not 100% sure what this issue is saying; I haven't dug into the exact details, but not matching pre-releases is a feature, not a bug.

[dbrgn]

I understand about the workload, that's no problem. If it's clear what needs to be done I might even be able to come up with a pull request.

But regarding bug vs feature: Independently from package managers, why would a query for 0.12.0-pre.1 >= 0.10.2 be false? I understand that package managers won't want to offer pre-releases as candidates, but the logical statement is clearly true.

[steveklabnik]

I understand that package managers won't want to offer pre-releases as candidates,

That is exactly why. release candidates should be opt-in. All major semver implementations I'm aware of work this way, for this reason.

If we changed the API to let cargo audit do this, it would report that it's been fixed, but you wouldn't actually get the fix, which seems quite bad.

[tarcieri]

It sounds like the simplest option is to explicitly enumerate the prereleases that contain the fix in the advisory's patched versions.

I can also see how it's entirely possible there'd be prereleases which don't contain a security fix despite there being a fix in the final release.

[dbrgn]

@steveklabnik I think we're still not talking about the same thing 🙂

The concrete use case I mentioned above:

• My Cargo.lock pins hyper to 0.12.0-pre.1
• There was a patch that fixed this for versions >= 0.10.2 (this is how the vulnerability db registers fixes)
• I am using 0.12.0-pre.1
• I want to know if the version I'm using is inside the range that contains the fix
• In this case, since the patch is in >= 0.10.2, the version I am using right now (0.12.0-pre.1) is not affected by the vulnerability.

[steveklabnik]

(this is how the vulnerability db registers fixes)

Ah ha! This must be the misunderstanding. So the vulnerability DB uses a different conception of versions than Cargo does? You’re not expecting someone who puts >= 0.10.2 in their Cargo.toml to get the fix?

[dbrgn]

You’re not expecting someone who puts >= 0.10.2 in their Cargo.toml to get the fix?

According to @tarcieri:

cargo audit presently requires a lock file, as it relies on cargo to do the dependency resolution

This means that we're always dealing with a pinned, concrete version from Cargo.lock, not with version ranges. For example, the question could be: "Is version 0.12.0-pre.1 (installed by the user) in the range >= 0.10.2 (patched range)". I'd argue, that for simple semver version checking, not in the context of a package manager, the answer should be "yes".

[steveklabnik]

Okay, we're finally on the same page. Whew. Thanks :)

What do you think about

I can also see how it's entirely possible there'd be prereleases which don't contain a security fix despite there being a fix in the final release.

[tarcieri]

If there were a mode where prereleases were in lexographic order between the release versions, you could do e.g. patched_versions = [">= 0.11.0-pre.2"], which could signal all 0.11 releases are fixed and all prereleases except 0.11.0-pre.1

[tarcieri]

@steveklabnik do you think this is something semver should handle, or should we add special logic for this case in the rustsec vulnerability checker?

[tarcieri]

Here's the current logic we're using to select vulnerable crates, for what it's worth:

https://github.com/RustSec/rustsec-client/blob/13968fb1e198593b791af7c98bd9427a9dd65d40/src/db.rs#L111

self.find_by_crate(crate_name)
    .iter()
    .filter(|advisory| {
        !advisory
            .patched_versions
            .iter()
            .any(|req| req.matches(version))
    })
    .map(|a| *a)
    .collect()

patched_versions is a Vec<VersionReq>s, so for each of those it calls matches, and if any of them match the given Version, the crate is selected.

Perhaps there should be some special case handling of prereleases here?

[Diggsey]

@tarcieri I think rustsec should not be using VersionReq for this, it should simply store the Version when a fix was applied: the existing PartialOrd implementation on Version behaves the way you want.

Maybe you need something more complicated to handle regressions, eg. Vec<(Version, Option<Version>)>, but this is already more flexible than using VersionReq.

[dwijnand]

Though the fact that Version's ordering doesn't match VersionReq's ordering is also a source of confusion. I think I'd expect semver's Version ordering to be consistent throughout the crate.