Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for cargo version range #84

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Conversation

ziadhany
Copy link
Contributor

Signed-off-by: ziad ziadhany2016@gmail.com

@TG1999
Copy link
Member

TG1999 commented Sep 12, 2022

@ziadhany please add tests

@ziadhany
Copy link
Contributor Author

@ziadhany please add tests
I think this pull request needs some changes to make it fit with docs and I will start by adding some tests
https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html

@ziadhany ziadhany marked this pull request as draft September 26, 2022 10:58
@TG1999
Copy link
Member

TG1999 commented Oct 18, 2022

gentle ping!

Signed-off-by: ziadhany <ziadhany2016@gmail.com>
@pombredanne
Copy link
Member

@ziadhany I chatted with @keshav-space and here is a suggestion on how to handle the star in versions... this is not tested but this should help you deal with such a thing:

def get_constraints_from_star_version(version):
    """
    Return a list of VersionConstraint from a Cargo version with stars.

    See https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#wildcard-requirements
    for details. We support these constructions:
        *     := >=0.0.0
        1.*   := >=1.0.0, <2.0.0
        1.2.* := >=1.2.0, <1.3.0
    """
    if "*" not in version:
        raise TypeError(f"Not a star version: {version}")

    ## CargoVersion SHOULD be a subclass of univers.versions.SemverVersion
    if version == "*":
        return [VersionConstraint(comparator="*", version_class=CargoVersion)]

    if "*" in version and not version.endswith("*"):
        raise TypeError(f"Unsupported star in the middle of a version: it should be a trailing star only: {version}")

    segments_count = len(version.split("."))
    lower_bound = CargoVersion(version.replace("*", "0"))
    upper_bound = None
    if segments_count == 2:
        # bump minor
        upper_bound = lower_bound.next_major()
    elif segments_count == 3:
        # bump patch
        upper_bound = lower_bound.next_minor()
    else:
        raise TypeError(f"Invalid version: not a semver version: {version}")
    return [
        VersionConstraint(comparator=">=", version=lower_bound),
        VersionConstraint(comparator="<", version=upper_bound)
    ]

This should get you things more or less this way:

>>> get_constraints_from_star_version("1.2.3")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 12, in get_constraints_from_star_version
TypeError: Not a star version: 1.2.3
>>> get_constraints_from_star_version("1.2.*")
[VersionConstraint(comparator='>=', version=CargoVersion(string='1.2.0')), VersionConstraint(comparator='<', version=SemverVersion(string='1.3.0'))]
>>> get_constraints_from_star_version("1.*")
[VersionConstraint(comparator='>=', version=CargoVersion(string='1.0')), VersionConstraint(comparator='<', version=SemverVersion(string='2.0.0'))]

@ziadhany
Copy link
Contributor Author

@pombredanne I thought of the same implementation but what if we have two stars like this
https://github.com/dtolnay/semver/blob/master/tests/test_version_req.rs#L371

#[test]
fn test_cargo3202() {
    let ref r = req("0.*.*");
    assert_to_string(r, "0.*");
    assert_match_all(r, &["0.5.0"]);

    let ref r = req("0.0.*");
    assert_to_string(r, "0.0.*");
}

@pombredanne
Copy link
Member

@pombredanne I thought of the same implementation but what if we have two stars like this https://github.com/dtolnay/semver/blob/master/tests/test_version_req.rs#L371

#[test]
fn test_cargo3202() {
    let ref r = req("0.*.*");
    assert_to_string(r, "0.*");
    assert_match_all(r, &["0.5.0"]);

    let ref r = req("0.0.*");
    assert_to_string(r, "0.0.*");
}

Use this:

while version.endswith(".*.*"):
    version = version.replace(".*.*", ".*")
>>> version = "1.*.*"
>>> while version.endswith(".*.*"):
...     version = version.replace(".*.*", ".*")
... 
>>> version
'1.*'

Signed-off-by: ziadhany <ziadhany2016@gmail.com>
@TG1999
Copy link
Member

TG1999 commented Aug 22, 2023

@ziadhany gentle ping

Copy link
Member

@TG1999 TG1999 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add ABOUT file and NOTICE for the tests copied from cargo

Signed-off-by: Keshav Priyadarshi <git@keshav.space>
@keshav-space
Copy link
Member

@ziadhany, all CIs are passing now.

Comment on lines 37 to 46
# https://github.com/dtolnay/semver/blob/master/tests/test_version_req.rs#L73
# ["=0.1.0+meta", [["=", "0.1.0+meta"]], ["0.1.0", "0.1.0+meta", "0.1.0+any"], []],
# test_greater_than
# [">= 1.0.0", [[]], ["1.0.0", "2.0.0"], ["0.1.0", "0.0.1", "1.0.0-pre", "2.0.0-pre"]],
# [">= 2.1.0-alpha2", [[]], ["2.1.0-alpha2", "2.1.0-alpha3", "2.1.0", "3.0.0"], ["2.0.0", "2.1.0-alpha1", "2.0.0-alpha2", "3.0.0-alpha2"]],
# test_less_than
# ["<1.0.0", [[]], ["0.1.0", "0.0.1"], ["1.0.0", "1.0.0-beta", "1.0.1", "0.9.9-alpha"]],
# ["<= 2.1.0-alpha2", [[]], ["2.1.0-alpha2", "2.1.0-alpha1", "2.0.0", "1.0.0"], ["2.1.0", "2.2.0-alpha1", "2.0.0-alpha2", "1.0.0-alpha2"]],
# [">1.0.0-alpha, <1.0.0", [[">", "2.1.0-alpha2"], ["<", "1.0.0"]], ["1.0.0-beta"], []],
# [">1.0.0-alpha, <1.0", [[">", "1.0.0-alpha"], ["<", "1.0"]]], ["1.0.0-beta"], []],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ziadhany why're we not testing these Cargo version ranges?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I commented # all the tests that generate errors , so I can try to bypass the test one by one .

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ziadhany It appears like we're having issues with the pre-release versions. The Cargo Semver library (https://github.com/dtolnay/semver) is primarily meant for processing version range in the Cargo manifest. Consequently, any version range will not include the pre-release version unless one of the range boundaries is a pre-release (e.g., ">1.0.0-alpha, <1.0"). See https://doc.rust-lang.org/cargo/reference/resolver.html#pre-releases for more details.

However, for our use case in VulnerableCode, we do want the pre-release to be included in the version range. We have encountered similar behavior with NPM ranges, and it's not surprising that the Cargo version range is mostly based on NPM (dtolnay/semver#58).

Let's discuss this in our upcoming community call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes the main problem with Cargo version ranges is the pre-release versions

rustsec/advisory-db use the pre-release version range :
https://github.com/rustsec/advisory-db/blob/a5fb72de318a74eb69a2c241c0e46705684a35d0/crates/lettre/RUSTSEC-2021-0069.md?plain=1#L12

so I think we should have a way to parse the pre-release versions in Univers

Add ABOUT file and NOTICE

Signed-off-by: ziadhany <ziadhany2016@gmail.com>
Copy link
Member

@keshav-space keshav-space left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ziadhany I've entered the issue to properly control the pre-release behavior in VersionRange. #130

Since the default behavior in VersionRange is to include pre-release versions, we need to modify the test case coming from https://github.com/dtolnay/semver/blob/f9cc2df9415c880bd3610c2cdb6785ac7cad31ea/tests/test_version_req.rs to include pre-release versions in versions_in field. See the suggestion below. All test cases involving pre-release versions will require similar adjustments.

Comment on lines +63 to +68
[
"<1.0.0",
[[]],
["0.1.0", "0.0.1"],
["1.0.0", "1.0.0-beta", "1.0.1", "0.9.9-alpha"],
],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[
"<1.0.0",
[[]],
["0.1.0", "0.0.1"],
["1.0.0", "1.0.0-beta", "1.0.1", "0.9.9-alpha"],
],
[
"<1.0.0",
[[]],
["0.1.0", "0.0.1", "0.9.9-alpha", "1.0.0-beta", ],
["1.0.0", "1.0.1"],
],

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants