Restrict nuget version range allows incorrect ranges #3917

erdembayar · 2021-02-23T22:03:12Z

Regression? Last working version:

Description

See: https://nugettoolsdev.azurewebsites.net/5.5.0-preview.2.6382/parse-version-range?versionRange=%5B10.0.0%2C+9.0.0%29

[10.0.0, 9.0.0)

This range is empty and there's no reasonable scenario where it's necessary.

This has always been the case, all the way to 2.5 https://nugettoolsdev.azurewebsites.net/2.5.0/parse-version-range?versionRange=%5B10.0.0%2C+9.0.0%29.

Now we'll throw exception in above case. Also we consider [9.0.0, 9.0.0), (9.0.0, 9.0.0] as illogical now.
But we're not touch (9.0.0, 9.0.0) even though it's technically illogical range, it's used as empty range in many unit test cases, let's not overdo it.

PR Checklist

erdembayar · 2021-02-24T00:20:13Z

@nkolev92
How about "[1.0.0, 1.0.0)"? Some tests are failing.
Personally, I want to treat as bad range.

nkolev92 · 2021-02-24T00:23:04Z

That seems like a bad range, but I'm not sure what you are to tell me by "some tests are failing".

erdembayar · 2021-02-24T00:49:34Z

That seems like a bad range, but I'm not sure what you are to tell me by "some tests are failing".

For example below test is now failing, because input is actually [2.3.1-RC+srv01-a5c5ff9, 2.3.1-RC+srv02-dbf5ec0) means [2.3.1-RC, 2.3.1-RC) ~ [a,a) is here. I believe [a,a) is illogical range. Something can't be exluding and including same thing a same time.

NuGet.Client/test/NuGet.Core.Tests/NuGet.Versioning.Test/ExternalComparerTests.cs

Lines 15 to 19 in ea6602c

    
           [InlineData("[2.3.1-RC+srv01-a5c5ff9, 2.3.1-RC+srv02-dbf5ec0)", "2.3.1-RC+srv00-a5c5ff9")] 
        
           public void NuGetVersionRangeWithGitCommit(string verSpec, string ver) 
        
           { 
        
               // Arrange 
        
               var versionInfo = VersionRange.Parse(verSpec);

nkolev92 · 2021-02-24T02:34:00Z

I think that the test could be better.

erdembayar · 2021-02-25T00:03:42Z

I think that the test could be better.

Removed only that case, it's illogical test input.

erdembayar · 2021-02-25T21:34:55Z

@nkolev92
[2.0.0) it it illogical range? I guess it's. I believe it should be [2.0.0, ) or [2.0.0, *) valid, yeah?

nkolev92 · 2021-02-25T22:14:42Z

[2.0.0) likely normalizes to [2.0.0,2.0.0) which is equivalent to the one you mentioned above.
[2.0.0, ) is different and it's the normalized version of 2.0.0.

[2.0.0, *) is not a valid range, because the floating part cannot be part of the upper range. Only a min range can be floating.

erdembayar · 2021-02-26T19:00:58Z

@NuGet/nuget-client
Hi.
Ready for review, please review.

nkolev92

The general idea seems good.
Maybe we can try simplifying the code a bit?

Why does your checklist have tests exception selected?

nkolev92 · 2021-02-26T22:43:36Z

test/NuGet.Core.Tests/NuGet.Versioning.Test/VersionRangeTests.cs

+        [InlineData("[2.0.0)")]
+        [InlineData("(2.0.0]")]
+        [InlineData("[2.0.0, *)")]
+        public void VersionRange_LogicallyIncorrectRanges_Throws(string range)


The correct name according to the guide would be Parse_****_****

Ok . Renamed.

test/NuGet.Core.Tests/NuGet.Versioning.Test/VersionRangeTests.cs

src/NuGet.Core/NuGet.Versioning/VersionRangeFactory.cs

erdembayar · 2021-02-27T01:14:03Z

The general idea seems good.
Maybe we can try simplifying the code a bit?

Why does your checklist have tests exception selected?

Thank you for your review. I addressed your comments and made it more easy to reason. Please review again.

nkolev92

I think we can go even simpler than what we have right now :)

nkolev92 · 2021-03-03T03:02:26Z

src/NuGet.Core/NuGet.Versioning/VersionRangeFactory.cs

+                && minVersion >= maxVersion
+                && !(minVersion == maxVersion && isMinInclusive && isMaxInclusive))  // Exclude (9.0.0,9.0.0) since it's used as empty version range.
+            {
+                if (partsLength == 1)


This check can still be a lot simpler.

All comments aside, I'd expect return false covers all scenarios here and you don't need any of the parts checks.

Ok. I addressed this one. Please check.

src/NuGet.Core/NuGet.Versioning/VersionRangeFactory.cs

kartheekp-ms · 2021-03-05T04:03:09Z

src/NuGet.Core/NuGet.Versioning/VersionRangeFactory.cs

+            // Illogical version range detection
+            if (minVersion != null && maxVersion != null
+                && minVersion >= maxVersion
+                && !(minVersion == maxVersion && isMinInclusive && isMaxInclusive))  // Exclude (9.0.0,9.0.0) since it's used as empty version range.
+            {
+                if (partsLength == 1)
+                {
+                    var onePartValidation = isMinInclusive ^ isMaxInclusive; // (1.0.0] and [1.0.0) are invalid
+                    onePartValidation |= !isMinInclusive && !isMaxInclusive; ; // or (1.0.0) is invalid
+                    if (onePartValidation)
+                    {
+                        return false;
+                    }
+                }
+                else
+                {
+                    var manyPartsValidation = minVersion > maxVersion // If a > b then (a, b) range is invalid. But below 2 cases tests are a = b (equals).
+                                              | isMinInclusive // [1.0.0, 1.0.0) is invalid
+                                              | isMaxInclusive; // (1.0.0, 1.0.0] and [1.0.0, 1.0.0] are invalid
+                    if (manyPartsValidation)
+                    {
+                        return false;
+                    }
+                }
+            }
+


I think we can change the above logic as follows. Hope it covers all edge cases :)

Add below logic after https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Versioning/VersionRangeFactory.cs#L187

//(1.0.0) , (1.0.0] and [1.0.0) invalid [1.0.0] is the only valid version number if (parts.Length == 1 && !(isMinInclusive && isMaxInclusive)) { return false; }

Add below logic after https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Versioning/VersionRangeFactory.cs#L235

if (minVersion != null && maxVersion != null) { int result = minVersion.CompareTo(maxVersion); //minVersion is equal to maxVersion (1.0.0, 1.0.0], [1.0.0, 1.0.0) if (result == 0 && (isMinInclusive ^ isMaxInclusive)) return false; //minVersion > maxVersion if (result > 0) return false; }

Ok. Taken your suggestion. Please review again now.

erdembayar · 2021-03-26T00:30:48Z

Addressed PR review comments. Ready for review.

nkolev92

1 clean-up nit.

Nicely done! 👏

nkolev92 · 2021-03-26T01:57:45Z

src/NuGet.Core/NuGet.Versioning/VersionRangeFactory.cs

@@ -123,6 +123,7 @@ public static bool TryParse(string value, bool allowFloating, out VersionRange v
            NuGetVersion minVersion = null;
            NuGetVersion maxVersion = null;
            FloatRange floatRange = null;
+            int partsLength = 0;


you probably don't need this here anymore. Everything is in the internal { }

erdembayar · 2021-03-26T21:15:15Z

@kartheekp-ms
Can review again please?

erdembayar force-pushed the dev-eryondon-9145NuGetVersionRangeAllowsIncorrectRanges branch from 8bd1959 to da47744 Compare February 24, 2021 19:54

erdembayar marked this pull request as ready for review February 24, 2021 23:58

erdembayar requested a review from a team as a code owner February 24, 2021 23:58

erdembayar changed the title ~~Be restict on nuget version range allows incorrect ranges~~ Restrict nuget version range allows incorrect ranges Feb 25, 2021

nkolev92 reviewed Feb 26, 2021

View reviewed changes

nkolev92 reviewed Mar 3, 2021

View reviewed changes

kartheekp-ms reviewed Mar 5, 2021

View reviewed changes

erdembayar force-pushed the dev-eryondon-9145NuGetVersionRangeAllowsIncorrectRanges branch from 59f5cc4 to 5e6a2ba Compare March 23, 2021 04:18

erdembayar added 13 commits March 23, 2021 09:52

Handle illogical Nuget version range when parsing.

cf2546f

Include more illogical versions.

85641dd

Make logic simple.

f30cf7d

Add edge case handling.

45c6111

Not treat (9.0.0, 9.0.0) as illogical range.

594c197

Fix failing unit tests

c9df627

Improve unit test name.

f8e4c5e

Add (1.0.0) not valid case.

c6ee001

Fix failing test with bad version range.

3f760cd

Fix bad unit tests

529d83d

Remove bad unit test I added.

f8c7c9e

Revert change made by mistake.

545f268

Simplify validation logic.

f106167

erdembayar added 6 commits March 23, 2021 09:52

More simplify validation.

4ac0da9

Address code review comment

1542c89

More address

bfbc28c

2

8145ce0

3

72a755c

4

b34ad7b

erdembayar force-pushed the dev-eryondon-9145NuGetVersionRangeAllowsIncorrectRanges branch from 5e6a2ba to b34ad7b Compare March 23, 2021 16:52

erdembayar requested review from kartheekp-ms and nkolev92 March 26, 2021 00:29

nkolev92 approved these changes Mar 26, 2021

View reviewed changes

Remove partsLength variable.

65b8992

kartheekp-ms approved these changes Mar 27, 2021

View reviewed changes

erdembayar merged commit 44210f1 into dev Mar 27, 2021

erdembayar deleted the dev-eryondon-9145NuGetVersionRangeAllowsIncorrectRanges branch March 27, 2021 16:06

joelverhagen mentioned this pull request Jul 20, 2021

Consider blocking non-sense version range: [X, Y) where X > Y NuGet/NuGetGallery#7839

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restrict nuget version range allows incorrect ranges #3917

Restrict nuget version range allows incorrect ranges #3917

erdembayar commented Feb 23, 2021 •

edited

Loading

erdembayar commented Feb 24, 2021

nkolev92 commented Feb 24, 2021

erdembayar commented Feb 24, 2021

nkolev92 commented Feb 24, 2021

erdembayar commented Feb 25, 2021

erdembayar commented Feb 25, 2021

nkolev92 commented Feb 25, 2021

erdembayar commented Feb 26, 2021

nkolev92 left a comment

nkolev92 Feb 26, 2021

erdembayar Feb 27, 2021

erdembayar commented Feb 27, 2021

nkolev92 left a comment

nkolev92 Mar 3, 2021

erdembayar Mar 23, 2021

kartheekp-ms Mar 5, 2021 •

edited

Loading

erdembayar Mar 23, 2021 •

edited

Loading

erdembayar commented Mar 26, 2021

nkolev92 left a comment

nkolev92 Mar 26, 2021 •

edited

Loading

erdembayar commented Mar 26, 2021

Restrict nuget version range allows incorrect ranges #3917

Restrict nuget version range allows incorrect ranges #3917

Conversation

erdembayar commented Feb 23, 2021 • edited Loading

Description

PR Checklist

erdembayar commented Feb 24, 2021

nkolev92 commented Feb 24, 2021

erdembayar commented Feb 24, 2021

nkolev92 commented Feb 24, 2021

erdembayar commented Feb 25, 2021

erdembayar commented Feb 25, 2021

nkolev92 commented Feb 25, 2021

erdembayar commented Feb 26, 2021

nkolev92 left a comment

Choose a reason for hiding this comment

nkolev92 Feb 26, 2021

Choose a reason for hiding this comment

erdembayar Feb 27, 2021

Choose a reason for hiding this comment

erdembayar commented Feb 27, 2021

nkolev92 left a comment

Choose a reason for hiding this comment

nkolev92 Mar 3, 2021

Choose a reason for hiding this comment

erdembayar Mar 23, 2021

Choose a reason for hiding this comment

kartheekp-ms Mar 5, 2021 • edited Loading

Choose a reason for hiding this comment

erdembayar Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

erdembayar commented Mar 26, 2021

nkolev92 left a comment

Choose a reason for hiding this comment

nkolev92 Mar 26, 2021 • edited Loading

Choose a reason for hiding this comment

erdembayar commented Mar 26, 2021

erdembayar commented Feb 23, 2021 •

edited

Loading

kartheekp-ms Mar 5, 2021 •

edited

Loading

erdembayar Mar 23, 2021 •

edited

Loading

nkolev92 Mar 26, 2021 •

edited

Loading