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.

[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.

It is definitely not that easy. Here are things, based empirically on vulns in the advisory DB, that alone fails to capture:

• When the vuln was introduced
• Backported vulns
• Backported fixes (i.e. previous minor versions an author is still maintaining)
• Deleted/reverted code

Here are some real-world VersionReqs from https://github.com/RustSec/advisory-db.git

• >= 0.6.3, ^0.3.4, ^0.4.5, ^0.5.1
• < 0.6.0, ^0.6.2, >= 0.7.6
• >= 0.10.2, < 0.10.0, >= 0.9.18

I am going to attempt to solve this by making a modified VersionReq type that respects the matching rules RustSec needs in regard to prereleases. If that works out, I think this issue can be closed.

[tarcieri]

RustSec PR which vendors a minified/modified set of the VersionReq predicate matching logic here in case anyone is interested in reviewing:

https://github.com/RustSec/rustsec-crate/pull/69

[tarcieri]

I ended up ripping out the vendored code from semver in this PR:

https://github.com/RustSec/rustsec-crate/pull/217

We've managed to upgrade the rustsec crate to semver v0.10, but it'd be great to figure out a more minimal/upgrade-friendly solution than https://github.com/RustSec/rustsec-crate/pull/69, or ideally something implemented upstream in the semver crate which provides similar logic.

(Sidebar: in general I've found the way prereleases are handled by this crate somewhat confusing)

[Diggsey]

I still think VersionReq is the wrong type for this: it's a version requirement whereas what rustsec needs is a version range.

Of your examples, all can be represented by a Vec<Range<Version>>, and the existing PartialEq implementation on Version works fine:

let patched_versions: Vec<Range<Version>> = ...;

patched_versions.iter().any(|range| range.contains(locked_version))

[tarcieri]

I still think VersionReq is the wrong type for this: it's a version requirement whereas what rustsec needs is a version range.

patched_versions is a full-blown version requirement which can contain multiple comma-delimited version ranges. It's not just a range: it's a set of ranges, any of which satisfy the "patched" requirement.

As it were, it seems some of the requirements we have no longer parse with semver v0.11:

Error { kind: Parse, msg: "the given version requirement is invalid for key `versions.patched` at line 17 column 12" }

[Diggsey]

patched_versions is a full-blown version requirement which can contain multiple comma-delimited version ranges. It's not just a range: it's a set of ranges, any of which satisfy the "patched" requirement.

Right, that's why I modelled it as a Vec<Range<Version>>. My point was that it's semantically different from a version requirement. You just happen to have borrowed the same syntax for defining them, since they are quite similar.

[tarcieri]

Can you state concretely what you think the difference is?

Ultimately we’re trying to test whether a concrete Version satisfies a requirement expressing versions which patched. Why isn’t that a VersionReq?

As a counterpoint, I can state what I feel is weird about the current prerelease handling: given a requirement for 1, that requirement is satisfied by a 1.0.0-pre. I would otherwise think that 1 means >= 1.0.0, and prereleases don't count and are otherwise considered to be < 1.0.0.

[Diggsey]

Sure: version requirements are about compatibility and stability of the API. I would expect 1.0.0-pre to already have the majority of breaking changes that are going to be part of the 1.0 release, and therefore it makes more sense to group pre-releases with the releases that follow them (so that 0.9.0 and 1.0.0-pre can coexist, but 1.0.0 and 1.0.0-pre cannot coexist).

A version range is about the development timeline. It makes perfect sense to say that versions >0.1.2, <0.1.8 are patched, but that would not make a whole lot of sense as a version requirement, since 0.1.9 is supposed to be compatible with all patch versions before it.

[tarcieri]

Sure: version requirements are about compatibility and stability of the API.

Note that we also leverage version requirements as part of cargo audit fix. That command quite literally needs to select a VersionReq that goes into Cargo.toml.

We can consider implementing our own version requirement type which differs from the one in this crate (again, the previous one was largely copy paste from semver v0.9.0), but IMO we are very much looking for a "version requirement" which satisfies a set of constraints. Semantically "version requirement" seems correct to me, even if the precise semantics of what RustSec needs and what SemVer ordinarily needs differ.

I would expect 1.0.0-pre to already have the majority of breaking changes that are going to be part of the 1.0 release, and therefore it makes more sense to group pre-releases with the releases that follow them (so that 0.9.0 and 1.0.0-pre can coexist, but 1.0.0 and 1.0.0-pre cannot coexist).

I think it would be more sense if prereleases weren't grouped into either category and considered distinct from a SemVer perspective, lexicographically located in between 0.9.0 and 1.0.0.

A major notable drawback of a 1.0.0-pre prerelease matching 1.0.0 is it means someone using a 1.0.0-pre in their Cargo.toml may experience a breakage if a 1.0.0 release includes any SemVer breaking changes from any prerelease. Personally I would consider prereleases the last time to make breaking changes prior to a final release, but this prerelease scheme expects that prereleases and final releases are compatible (or implicitly assumes that users of prereleases should expect breakages if any are made).

[sify21]

Node.js's security working group also use VersionReq as vulnerable_versions and patched_versions in their data files. For example: https://github.com/nodejs/security-wg/blob/master/vuln/npm/104.json

[tarcieri]

I just noticed something else impacted by this issue: https://deps.rs thinks prerelease dependencies are out-of-date

[dtolnay]

I'll go ahead and close this because I believe this matching behavior is intentional as described by section 5 of https://github.com/semver/semver/blob/efcff2c838c9945f79bfd21c1df0073271bcc29c/ranges.md.

As of 1.0.0 though, the VersionReq's operators/comparators are publicly exposed and so it's possible to implement "nontraditional" matching logic downstream. https://docs.rs/semver/1.0.0-rc.1/semver/struct.VersionReq.html

[tarcieri]

For reasons unrelated to this issue, we'll be migrating away from VersionReqs (to one implemented using Version comparators, which work correctly for our purposes), so this no longer concerns me, but honestly this seems like a "bug" in the spec. I agree that this behavior does appear to match the spec as written, however.

The rationale for the criteria in the spec all center around something which does make sense: making sure that prereleases don't match as a sort of boundary condition in a "less than" expression. As it were, I opened a separate issue related to that: #236.

But this leads to this rather confusing behavior for anything other than that particular edge case...

For the match expression >= 2.0.0, here's the output of VersionReq::matches for the following versions:

• 2.0.0-rc.0: false
• 2.0.0: true
• 3.0.0-rc.0: false
• 3.0.0: true
• 4.0.0-rc.0: false
• 4.0.0: true

Of the prereleases returning false, I'd argue only the first one makes sense. I don't think it makes sense for prereleases to cease to match after a major version bump (but for the corresponding release version to start matching again, only for the next major version's prereleases to cease to match, and so on for each major version), but again, I'd agree this is the "correct" behavior per the spec as written.

[dtolnay]

Locking because this is not the right/productive place to rant about the spec, especially not about deliberate and sensible features of the spec.