Block packages with illegal package dependency version ranges #3728

xavierdecoster · 2017-03-30T10:13:18Z

Using strictVersionCheck=true by default, to block packages from being uploaded to nuget.org if they contain package dependencies with an invalid version range.

joelverhagen · 2017-03-30T15:54:08Z

src/NuGetGallery/Controllers/PackagesController.cs

@@ -1149,7 +1150,8 @@ private ActionResult EditFailed(string id, EditPackageRequest formData, Package
                    package = await _packageService.CreatePackageAsync(nugetPackage, packageStreamMetadata, currentUser, commitChanges: false);
                    Debug.Assert(package.PackageRegistration != null);
                }
-                catch (EntityException ex)
+                catch(Exception ex) 
+                    when (ex is PackagingException || ex is EntityException)


nit new line looks weird here

skofman1 · 2017-03-30T15:54:25Z

tests/NuGetGallery.Core.Facts/Packaging/ManifestValidatorFacts.cs

-        private string[] GetErrors(Stream nuspecStream)
+        [Theory]
+        [InlineData("$version$")]
+        [InlineData("[15.106.0.preview]")]


is
<dependency id=""a.b.c"" version="" />
is legal?

The NuspecReader.GetDependencyGroups(useStrictVersionCheck: true) does not throw apparently when the dependency version is "". Should it? It currently still does a silent fallback to VersionRange.All - aka (,).

If it should throw, that means another fix is due on the NuGet.Packaging client lib.

Imho, when using useStrictVersionCheck: true, we should treat null and "" as invalid versions, and thus throw.

However, the PR that added the overload for strict version checks explicitly added null and "" in the test data for valid version ranges (highlight). An oversight? Or conscious?

Seems like a question @emgarten and @joelverhagen may want to comment on. The fix in the client lib is pretty trivial, but as you can tell, rather important :)

Our docs specifically state that you can leave the version range unspecified, and what will happen. IMO we should keep this behavior the same, or, at the bare minimum, investigate how many packages have this issue before changing it.

https://docs.microsoft.com/en-us/nuget/create-packages/dependency-versions#version-ranges

I spoke to @emgarten and @joelverhagen before implementing this change, and the conclusion was that, moving forward, we should be strict and block this from happening. The documented behavior was more a hack to support floating versions if I understood correctly. The change to reject empty dependency version ranges is also in line with the recommendation as per the docs.

For consistent behavior, it's recommended to always specify a version or version range for package dependencies.

If no version is specified for a dependency, NuGet behaves as follows:

NuGet v2.7.2 and earlier: The latest package version will be used

NuGet v2.8 and later: The lowest package version will be used

I think we should add:

NuGet v2.8 to v4.1.0: The lowest package version will be used

NuGet v4.3.0 and above: no longer supported and considered an invalid dependency version range

skofman1 · 2017-03-30T16:09:07Z

src/NuGetGallery/Services/PackageService.cs

+        /// <exception cref="PackagingException">
+        /// This exception will be thrown if an invalid package dependency version range is detected.
+        /// </exception>
+        /// <exception cref="EntityException">


This "contract" is a bit strange... we throw EntityException for every bad metadata value, but a different exception (PackagingException) for bad dependencies.
I suggest wrapping it here in InvalidPackageException. This way we will have a clear contract with the callers.
Also, noticed there are place where we call EnsureValid, but don't expose the failure message correctly:
https://github.com/NuGet/NuGetGallery/blob/master/src/NuGetGallery/Controllers/PackagesController.cs#L234

This is one example why having a single exception type thrown will be better.

scottbommarito · 2017-04-04T17:36:32Z

tests/NuGetGallery.Core.Facts/Packaging/ManifestValidatorFacts.cs

@@ -249,7 +249,7 @@ public class ManifestValidatorFacts
                  </metadata>
                </package>";

-        private const string NuSpecDependencySetContainsInvalidTargetFramework = @"<?xml version=""1.0""?>
+        private const string NuSpecDependencySetContainsInvalidVersionRange = @"<?xml version=""1.0""?>


With this change, are we no longer testing invalid target frameworks?

That string was not used anywhere, I merely put it to use in a different scenario :)

scottbommarito · 2017-04-04T17:39:16Z

tests/NuGetGallery.Core.Facts/Packaging/ManifestValidatorFacts.cs

-        private string[] GetErrors(Stream nuspecStream)
+        [Theory]
+        [InlineData("$version$")]
+        [InlineData("[15.106.0.preview]")]


Our docs specifically state that you can leave the version range unspecified, and what will happen. IMO we should keep this behavior the same, or, at the bare minimum, investigate how many packages have this issue before changing it.

https://docs.microsoft.com/en-us/nuget/create-packages/dependency-versions#version-ranges

scottbommarito · 2017-04-04T17:45:33Z

src/NuGetGallery.Core/Packaging/PackageMetadata.cs

        public static PackageMetadata FromNuspecReader(NuspecReader nuspecReader)
        {
            return new PackageMetadata(
                nuspecReader.GetMetadata().ToDictionary(kvp => kvp.Key, kvp => kvp.Value),
-                nuspecReader.GetDependencyGroups(),
+                nuspecReader.GetDependencyGroups(useStrictVersionCheck: true),


Not in the scope of this PR, but do we potentially have similar issues with our framework reference groups, packages types, or min client version?

…pendency version ranges + adjust tests for useStrictVersionCheck changes

xavierdecoster · 2017-04-05T18:45:04Z

Updated this PR to consume the fresh builds that include the null and empty check fixes. Had to modify a few tests that assumed the empty string fallback to VersionRange.All, which we now not only do not recommend, but explicitly block from being pushed/uploaded to the gallery.

Final pair of eyes please? :) @joelverhagen @scottbommarito @skofman1

xavierdecoster added the Area: Data Integrity label Mar 30, 2017

xavierdecoster added this to the S116 - 2017.3.27 milestone Mar 30, 2017

xavierdecoster self-assigned this Mar 30, 2017

xavierdecoster requested review from joelverhagen, chenriksson, skofman1 and scottbommarito March 30, 2017 10:13

dnfclas added the cla-already-signed label Mar 30, 2017

xavierdecoster changed the title ~~No longer accept packages with illegal package dependency version ranges~~ Block packages with illegal package dependency version ranges Mar 30, 2017

shishirx34 mentioned this pull request Mar 30, 2017

feed2catalog produces catalog items with invalid dependency version ranges #3482

Closed

joelverhagen reviewed Mar 30, 2017

View reviewed changes

skofman1 reviewed Mar 30, 2017

View reviewed changes

skofman1 requested changes Mar 30, 2017

View reviewed changes

skofman1 approved these changes Apr 4, 2017

View reviewed changes

scottbommarito approved these changes Apr 4, 2017

View reviewed changes

xavierdecoster added 4 commits April 5, 2017 11:21

Upgrade NuGet.Packaging dependency

32047aa

Use strict version checks when calling NuspecReader.GetDependencyGroups

d327010

Code review feedback

8dced95

Upgrade to proper NuGet.Packaging version containing fix for empty de…

e3ba44f

…pendency version ranges + adjust tests for useStrictVersionCheck changes

xavierdecoster force-pushed the dev-issue3482 branch from 7d8d2b7 to e3ba44f Compare April 5, 2017 18:22

joelverhagen approved these changes Apr 5, 2017

View reviewed changes

scottbommarito approved these changes Apr 5, 2017

View reviewed changes

This was referenced Apr 5, 2017

NuGet pack should warn when dependency version range is invalid/empty NuGet/Home#4985

Open

Documentation update: empty dependency version strings will no longer be accepted when pushing packages to nuget.org #3750

Closed

xavierdecoster merged commit f6dc9e9 into dev Apr 5, 2017

xavierdecoster deleted the dev-issue3482 branch April 5, 2017 20:02

skofman1 mentioned this pull request Apr 5, 2017

Deployment 2017.04.12 #3718

Closed

37 tasks

xavierdecoster added the Verified-Dev label Apr 7, 2017

chenriksson mentioned this pull request Apr 17, 2017

PR#3741 test fix, per integration with PR#3728 #3794

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Block packages with illegal package dependency version ranges #3728

Block packages with illegal package dependency version ranges #3728

xavierdecoster commented Mar 30, 2017

joelverhagen Mar 30, 2017

skofman1 Mar 30, 2017

xavierdecoster Mar 31, 2017

xavierdecoster Mar 31, 2017

scottbommarito Apr 4, 2017

xavierdecoster Apr 4, 2017 •

edited

Loading

skofman1 Mar 30, 2017

scottbommarito Apr 4, 2017

xavierdecoster Apr 4, 2017

scottbommarito Apr 4, 2017

scottbommarito Apr 4, 2017

xavierdecoster commented Apr 5, 2017

Block packages with illegal package dependency version ranges #3728

Block packages with illegal package dependency version ranges #3728

Conversation

xavierdecoster commented Mar 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xavierdecoster Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xavierdecoster commented Apr 5, 2017

xavierdecoster Apr 4, 2017 •

edited

Loading