From dcd728cf9ff33223cead100938a75d35773cd9c3 Mon Sep 17 00:00:00 2001 From: Shishir H Date: Tue, 28 Mar 2017 00:19:17 -0700 Subject: [PATCH] Add strict version check for dependencies in NuspecReader (#1262) --- .../NuGet.Packaging/NuspecReader.cs | 125 +++++++++-------- .../NuGet.Packaging/Strings.Designer.cs | 9 ++ src/NuGet.Core/NuGet.Packaging/Strings.resx | 6 + .../NuGet.Packaging.Test/NuspecReaderTests.cs | 132 ++++++++++++++++++ 4 files changed, 217 insertions(+), 55 deletions(-) diff --git a/src/NuGet.Core/NuGet.Packaging/NuspecReader.cs b/src/NuGet.Core/NuGet.Packaging/NuspecReader.cs index 50332b881dc..cc442426d48 100644 --- a/src/NuGet.Core/NuGet.Packaging/NuspecReader.cs +++ b/src/NuGet.Core/NuGet.Packaging/NuspecReader.cs @@ -3,7 +3,6 @@ using System; using System.Collections.Generic; -using System.Diagnostics; using System.Globalization; using System.IO; using System.Linq; @@ -106,46 +105,36 @@ public NuspecReader(XDocument xml, IFrameworkNameProvider frameworkProvider) /// Read package dependencies for all frameworks /// public IEnumerable GetDependencyGroups() + { + return GetDependencyGroups(useStrictVersionCheck: false); + } + + /// + /// Read package dependencies for all frameworks + /// + public IEnumerable GetDependencyGroups(bool useStrictVersionCheck) { var ns = MetadataNode.GetDefaultNamespace().NamespaceName; + var dependencyNode = MetadataNode + .Elements(XName.Get(Dependencies, ns)); var groupFound = false; + var dependencyGroups = dependencyNode + .Elements(XName.Get(Group, ns)); - foreach (var depGroup in MetadataNode.Elements(XName.Get(Dependencies, ns)).Elements(XName.Get(Group, ns))) + foreach (var depGroup in dependencyGroups) { groupFound = true; var groupFramework = GetAttributeValue(depGroup, TargetFramework); - var packages = new HashSet(); - - foreach (var depNode in depGroup.Elements(XName.Get(Dependency, ns))) - { - VersionRange range = null; - - var rangeNode = GetAttributeValue(depNode, Version); - - if (!String.IsNullOrEmpty(rangeNode)) - { - VersionRange.TryParse(rangeNode, out range); - - Debug.Assert(range != null, "Unable to parse range: " + rangeNode); - } - - var includeFlags = GetFlags(GetAttributeValue(depNode, IncludeFlags)); - var excludeFlags = GetFlags(GetAttributeValue(depNode, ExcludeFlags)); - - var dependency = new PackageDependency( - GetAttributeValue(depNode, Id), - range, - includeFlags, - excludeFlags); + var dependencies = depGroup + .Elements(XName.Get(Dependency, ns)); - packages.Add(dependency); - } + var packages = GetPackageDependencies(dependencies, useStrictVersionCheck); - var framework = String.IsNullOrEmpty(groupFramework) - ? NuGetFramework.AnyFramework + var framework = string.IsNullOrEmpty(groupFramework) + ? NuGetFramework.AnyFramework : NuGetFramework.Parse(groupFramework, _frameworkProvider); yield return new PackageDependencyGroup(framework, packages); @@ -154,34 +143,16 @@ public IEnumerable GetDependencyGroups() // legacy behavior if (!groupFound) { - var depNodes = MetadataNode.Elements(XName.Get(Dependencies, ns)) + var legacyDependencies = dependencyNode .Elements(XName.Get(Dependency, ns)); - var packages = new HashSet(); - - foreach (var depNode in depNodes) - { - VersionRange range = null; - - var rangeNode = GetAttributeValue(depNode, Version); - - if (!String.IsNullOrEmpty(rangeNode)) - { - VersionRange.TryParse(rangeNode, out range); - - Debug.Assert(range != null, "Unable to parse range: " + rangeNode); - } - - packages.Add(new PackageDependency(GetAttributeValue(depNode, Id), range)); - } + var packages = GetPackageDependencies(legacyDependencies, useStrictVersionCheck); if (packages.Any()) { yield return new PackageDependencyGroup(NuGetFramework.AnyFramework, packages); } } - - yield break; } /// @@ -199,9 +170,9 @@ public IEnumerable GetReferenceGroups() var groupFramework = GetAttributeValue(group, TargetFramework); - var items = group.Elements(XName.Get(Reference, ns)).Select(n => GetAttributeValue(n, File)).Where(n => !String.IsNullOrEmpty(n)).ToArray(); + var items = group.Elements(XName.Get(Reference, ns)).Select(n => GetAttributeValue(n, File)).Where(n => !string.IsNullOrEmpty(n)).ToArray(); - var framework = String.IsNullOrEmpty(groupFramework) ? NuGetFramework.AnyFramework : NuGetFramework.Parse(groupFramework, _frameworkProvider); + var framework = string.IsNullOrEmpty(groupFramework) ? NuGetFramework.AnyFramework : NuGetFramework.Parse(groupFramework, _frameworkProvider); yield return new FrameworkSpecificGroup(framework, items); } @@ -210,7 +181,7 @@ public IEnumerable GetReferenceGroups() if (!groupFound) { var items = MetadataNode.Elements(XName.Get(References, ns)) - .Elements(XName.Get(Reference, ns)).Select(n => GetAttributeValue(n, File)).Where(n => !String.IsNullOrEmpty(n)).ToArray(); + .Elements(XName.Get(Reference, ns)).Select(n => GetAttributeValue(n, File)).Where(n => !string.IsNullOrEmpty(n)).ToArray(); if (items.Length > 0) { @@ -239,7 +210,7 @@ public IEnumerable GetFrameworkReferenceGroups() var frameworks = new List(); // Empty frameworks go under Any - if (String.IsNullOrEmpty(group.Key)) + if (string.IsNullOrEmpty(group.Key)) { frameworks.Add(NuGetFramework.AnyFramework); } @@ -247,7 +218,7 @@ public IEnumerable GetFrameworkReferenceGroups() { foreach (var fwString in group.Key.Split(CommaArray, StringSplitOptions.RemoveEmptyEntries)) { - if (!String.IsNullOrEmpty(fwString)) + if (!string.IsNullOrEmpty(fwString)) { frameworks.Add(NuGetFramework.Parse(fwString.Trim(), _frameworkProvider)); } @@ -265,7 +236,7 @@ public IEnumerable GetFrameworkReferenceGroups() } // Merge items and ignore duplicates - items.UnionWith(group.Select(item => GetAttributeValue(item, AssemblyName)).Where(item => !String.IsNullOrEmpty(item))); + items.UnionWith(group.Select(item => GetAttributeValue(item, AssemblyName)).Where(item => !string.IsNullOrEmpty(item))); } } @@ -465,6 +436,7 @@ private static string GetAttributeValue(XElement element, string attributeName) } private static readonly List EmptyList = new List(); + private static List GetFlags(string flags) { if (string.IsNullOrEmpty(flags)) @@ -479,5 +451,48 @@ private static List GetFlags(string flags) return set.OrderBy(s => s, StringComparer.OrdinalIgnoreCase).ToList(); } + + private HashSet GetPackageDependencies(IEnumerable nodes, bool useStrictVersionCheck) + { + var packages = new HashSet(); + + foreach (var depNode in nodes) + { + VersionRange range = null; + + var rangeNode = GetAttributeValue(depNode, Version); + + if (!string.IsNullOrEmpty(rangeNode)) + { + var versionParsedSuccessfully = VersionRange.TryParse(rangeNode, out range); + if (!versionParsedSuccessfully && useStrictVersionCheck) + { + // Invalid version + var dependencyId = GetAttributeValue(depNode, Id); + var message = string.Format( + CultureInfo.CurrentCulture, + Strings.ErrorInvalidPackageVersionForDependency, + dependencyId, + GetIdentity(), + rangeNode); + + throw new PackagingException(message); + } + } + + var includeFlags = GetFlags(GetAttributeValue(depNode, IncludeFlags)); + var excludeFlags = GetFlags(GetAttributeValue(depNode, ExcludeFlags)); + + var dependency = new PackageDependency( + GetAttributeValue(depNode, Id), + range, + includeFlags, + excludeFlags); + + packages.Add(dependency); + } + + return packages; + } } } diff --git a/src/NuGet.Core/NuGet.Packaging/Strings.Designer.cs b/src/NuGet.Core/NuGet.Packaging/Strings.Designer.cs index cafdfce809a..855ebfe0da7 100644 --- a/src/NuGet.Core/NuGet.Packaging/Strings.Designer.cs +++ b/src/NuGet.Core/NuGet.Packaging/Strings.Designer.cs @@ -104,6 +104,15 @@ public static string ErrorInvalidPackageVersion { } } + /// + /// Looks up a localized string similar to Invalid package version for a dependency with id '{0}' in package '{1}': '{2}'. + /// + public static string ErrorInvalidPackageVersionForDependency { + get { + return ResourceManager.GetString("ErrorInvalidPackageVersionForDependency", resourceCulture); + } + } + /// /// Looks up a localized string similar to Null or empty package id. /// diff --git a/src/NuGet.Core/NuGet.Packaging/Strings.resx b/src/NuGet.Core/NuGet.Packaging/Strings.resx index ee53f7c37a1..64b8f5a77c2 100644 --- a/src/NuGet.Core/NuGet.Packaging/Strings.resx +++ b/src/NuGet.Core/NuGet.Packaging/Strings.resx @@ -129,6 +129,12 @@ Invalid package version for package id '{0}': '{1}' + + Invalid package version for a dependency with id '{0}' in package '{1}': '{2}' + {0} is the dependency's package id. +{1} is the package's identity. +{2} is the dependency's version range. + Null or empty package id diff --git a/test/NuGet.Core.Tests/NuGet.Packaging.Test/NuspecReaderTests.cs b/test/NuGet.Core.Tests/NuGet.Packaging.Test/NuspecReaderTests.cs index 19a56cd6c8d..21476639cac 100644 --- a/test/NuGet.Core.Tests/NuGet.Packaging.Test/NuspecReaderTests.cs +++ b/test/NuGet.Core.Tests/NuGet.Packaging.Test/NuspecReaderTests.cs @@ -7,6 +7,9 @@ using System.Text; using NuGet.Frameworks; using Xunit; +using NuGet.Packaging.Core; +using System.Collections.Generic; +using NuGet.Versioning; namespace NuGet.Packaging.Test { @@ -255,6 +258,73 @@ public class NuspecReaderTests "; + private const string VersionRangeInDependency = @" + + + PackageA + 2.0.1.0 + Package A + ownera, ownerb + Package A description + ServiceStack Serilog + + + + + "; + + public static IEnumerable GetValidVersions() + { + return GetVersionRange(validVersions: true); + } + + public static IEnumerable GetInValidVersions() + { + return GetVersionRange(validVersions: false); + } + + private static IEnumerable GetVersionRange(bool validVersions) + { + IEnumerable range = validVersions + ? ValidVersionRange() + : InvalidVersionRange(); + + foreach (var s in range) + { + yield return new object[] { s }; + } + } + + private static IEnumerable ValidVersionRange() + { + yield return null; + yield return string.Empty; + yield return "0.0.0"; + yield return "1.0.0-beta"; + yield return "1.0.1-alpha.1.2.3"; + yield return "1.0.1-alpha.1.2.3+a.b.c.d"; + yield return "1.0.1+metadata"; + yield return "1.0.1--"; + yield return "1.0.1-a.really.long.version.release.label"; + yield return "00.00.00.00-alpha"; + yield return "0.0-alpha.1"; + yield return "(1.0.0-alpha.1, )"; + yield return "[1.0.0-alpha.1+metadata]"; + yield return "[1.0, 2.0.0+metadata)"; + yield return "[1.0+metadata, 2.0.0+metadata)"; + } + + private static IEnumerable InvalidVersionRange() + { + yield return "~1"; + yield return "~1.0.0"; + yield return "0.0.0-~4"; + yield return "$version$"; + yield return "Invalid"; + yield return "[15.106.0.preview]"; + yield return "15.106.0-preview.01"; // no leading zeros in numeric identifiers of release label + } + [Fact] public void NuspecReaderTests_NamespaceOnMetadata() { @@ -307,6 +377,68 @@ public void NuspecReaderTests_DuplicateDependencyGroups() Assert.Equal(2, dependencies.Count); } + [Theory] + [MemberData(nameof(GetValidVersions))] + [MemberData(nameof(GetInValidVersions))] + public void NuspecReaderTests_NonStrictCheckInDependencyShouldNotThrowException(string versionRange) + { + // Arrange + var formattedNuspec = string.Format(VersionRangeInDependency, versionRange); + var nuspecReader = GetReader(formattedNuspec); + + // Act + var dependencies = nuspecReader.GetDependencyGroups().ToList(); + + // Assert + Assert.Equal(1, dependencies.Count); + } + + [Theory] + [MemberData(nameof(GetInValidVersions))] + public void NuspecReaderTests_NonStrictCheckInDependencyShouldFallbackToAllRangeForInvalidVersions( + string versionRange) + { + // Arrange + var formattedNuspec = string.Format(VersionRangeInDependency, versionRange); + var nuspecReader = GetReader(formattedNuspec); + + // Act + var dependencies = nuspecReader.GetDependencyGroups().ToList(); + + // Assert + Assert.Equal(VersionRange.All, dependencies.First().Packages.First().VersionRange); + } + + [Theory] + [MemberData(nameof(GetInValidVersions))] + public void NuspecReaderTests_InvalidVersionRangeInDependencyThrowsExceptionForStrictCheck(string versionRange) + { + // Arrange + var formattedNuspec = string.Format(VersionRangeInDependency, versionRange); + var nuspecReader = GetReader(formattedNuspec); + Action action = () => nuspecReader.GetDependencyGroups(useStrictVersionCheck: true).ToList(); + + // Act & Assert + Assert.Throws(action); + } + + [Theory] + [MemberData(nameof(GetValidVersions))] + public void NuspecReaderTests_ValidVersionRangeInDependencyReturnsResultForStrictCheck(string versionRange) + { + // Arrange + var formattedNuspec = string.Format(VersionRangeInDependency, versionRange); + var nuspecReader = GetReader(formattedNuspec); + var expectedVersionRange = string.IsNullOrEmpty(versionRange) ? VersionRange.All : VersionRange.Parse(versionRange); + + // Act + var dependencyGroups = nuspecReader.GetDependencyGroups(useStrictVersionCheck: true).ToList(); + + // Assert + Assert.Equal(1, dependencyGroups.Count); + Assert.Equal(expectedVersionRange, dependencyGroups.First().Packages.First().VersionRange); + } + [Fact] public void NuspecReaderTests_FrameworkGroups() {