Skip to content

Commit

Permalink
Add strict version check for dependencies in NuspecReader (#1262)
Browse files Browse the repository at this point in the history
  • Loading branch information
@shishirx34 @xavierdecoster
shishirx34 authored and xavierdecoster committed Mar 28, 2017
1 parent e578d50 commit dcd728c
Show file tree
Hide file tree
Showing 4 changed files with 217 additions and 55 deletions.
125 changes: 70 additions & 55 deletions src/NuGet.Core/NuGet.Packaging/NuspecReader.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Globalization;
using System.IO;
using System.Linq;
Expand Down Expand Up @@ -106,46 +105,36 @@ public NuspecReader(XDocument xml, IFrameworkNameProvider frameworkProvider)
/// Read package dependencies for all frameworks
/// </summary>
public IEnumerable<PackageDependencyGroup> GetDependencyGroups()
{
return GetDependencyGroups(useStrictVersionCheck: false);
}

/// <summary>
/// Read package dependencies for all frameworks
/// </summary>
public IEnumerable<PackageDependencyGroup> 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<PackageDependency>();

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);
Expand All @@ -154,34 +143,16 @@ public IEnumerable<PackageDependencyGroup> 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<PackageDependency>();

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;
}

/// <summary>
Expand All @@ -199,9 +170,9 @@ public IEnumerable<FrameworkSpecificGroup> 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);
}
Expand All @@ -210,7 +181,7 @@ public IEnumerable<FrameworkSpecificGroup> 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)
{
Expand Down Expand Up @@ -239,15 +210,15 @@ public IEnumerable<FrameworkSpecificGroup> GetFrameworkReferenceGroups()
var frameworks = new List<NuGetFramework>();

// Empty frameworks go under Any
if (String.IsNullOrEmpty(group.Key))
if (string.IsNullOrEmpty(group.Key))
{
frameworks.Add(NuGetFramework.AnyFramework);
}
else
{
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));
}
Expand All @@ -265,7 +236,7 @@ public IEnumerable<FrameworkSpecificGroup> 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)));
}
}

Expand Down Expand Up @@ -465,6 +436,7 @@ private static string GetAttributeValue(XElement element, string attributeName)
}

private static readonly List<string> EmptyList = new List<string>();

private static List<string> GetFlags(string flags)
{
if (string.IsNullOrEmpty(flags))
Expand All @@ -479,5 +451,48 @@ private static List<string> GetFlags(string flags)

return set.OrderBy(s => s, StringComparer.OrdinalIgnoreCase).ToList();
}

private HashSet<PackageDependency> GetPackageDependencies(IEnumerable<XElement> nodes, bool useStrictVersionCheck)
{
var packages = new HashSet<PackageDependency>();

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;
}
}
}
9 changes: 9 additions & 0 deletions src/NuGet.Core/NuGet.Packaging/Strings.Designer.cs
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions src/NuGet.Core/NuGet.Packaging/Strings.resx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,12 @@
<data name="ErrorInvalidPackageVersion" xml:space="preserve">
<value>Invalid package version for package id '{0}': '{1}'</value>
</data>
<data name="ErrorInvalidPackageVersionForDependency" xml:space="preserve">
<value>Invalid package version for a dependency with id '{0}' in package '{1}': '{2}'</value>
<comment>{0} is the dependency's package id.
{1} is the package's identity.
{2} is the dependency's version range.</comment>
</data>
<data name="ErrorNullOrEmptyPackageId" xml:space="preserve">
<value>Null or empty package id</value>
</data>
Expand Down
132 changes: 132 additions & 0 deletions test/NuGet.Core.Tests/NuGet.Packaging.Test/NuspecReaderTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down Expand Up @@ -255,6 +258,73 @@ public class NuspecReaderTests
</metadata>
</package>";

private const string VersionRangeInDependency = @"<?xml version=""1.0"" encoding=""utf-8"" standalone=""yes""?>
<package xmlns=""http://schemas.microsoft.com/packaging/2011/10/nuspec.xsd"">
<metadata>
<id>PackageA</id>
<version>2.0.1.0</version>
<title>Package A</title>
<authors>ownera, ownerb</authors>
<description>Package A description</description>
<tags>ServiceStack Serilog</tags>
<dependencies>
<dependency id=""PackageB"" version=""{0}"" />
</dependencies>
</metadata>
</package>";

public static IEnumerable<object[]> GetValidVersions()
{
return GetVersionRange(validVersions: true);
}

public static IEnumerable<object[]> GetInValidVersions()
{
return GetVersionRange(validVersions: false);
}

private static IEnumerable<object[]> GetVersionRange(bool validVersions)
{
IEnumerable<string> range = validVersions
? ValidVersionRange()
: InvalidVersionRange();

foreach (var s in range)
{
yield return new object[] { s };
}
}

private static IEnumerable<string> 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<string> 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()
{
Expand Down Expand Up @@ -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<PackagingException>(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()
{
Expand Down

0 comments on commit dcd728c

Please sign in to comment.