From 9297f055e6650e838be90c310356cd3efcf33d1b Mon Sep 17 00:00:00 2001 From: Paul Dorsch <107068277+pauld-msft@users.noreply.github.com> Date: Thu, 22 Aug 2024 10:07:36 -0400 Subject: [PATCH] Pauldorsch/fix invalid version bug (#1232) * catch exceptions thrown from manual dependency scanning * handle argument exceptions thrown, skipping those packages * whitespace * pr feedback --- .../Contracts/PipDependencySpecification.cs | 87 ++++++++++++++----- .../pip/PipComponentDetector.cs | 3 +- .../pip/PipReportComponentDetector.cs | 22 +++-- .../pip/SharedPipUtilities.cs | 7 +- .../pip/SimplePipComponentDetector.cs | 2 +- .../PipDependencySpecifierTests.cs | 39 +++++++++ .../PipReportComponentDetectorTests.cs | 72 +++++++++++++-- .../resources/pip/fallback/requirements.txt | 1 + 8 files changed, 195 insertions(+), 38 deletions(-) diff --git a/src/Microsoft.ComponentDetection.Detectors/pip/Contracts/PipDependencySpecification.cs b/src/Microsoft.ComponentDetection.Detectors/pip/Contracts/PipDependencySpecification.cs index 878b631b..c0d3b08a 100644 --- a/src/Microsoft.ComponentDetection.Detectors/pip/Contracts/PipDependencySpecification.cs +++ b/src/Microsoft.ComponentDetection.Detectors/pip/Contracts/PipDependencySpecification.cs @@ -5,6 +5,7 @@ using System.Collections.Generic; using System.Diagnostics; using System.Linq; using System.Text.RegularExpressions; +using Microsoft.Extensions.Logging; /// /// Represents a package and a list of dependency specifications that the package must be. @@ -59,6 +60,37 @@ public class PipDependencySpecification /// The to parse. /// The package format. public PipDependencySpecification(string packageString, bool requiresDist = false) + => this.Initialize(packageString, requiresDist); + + /// + /// Constructs a dependency specification from a string in one of two formats (Requires-Dist: a (==1.3)) OR a==1.3. + /// + /// The to parse. + /// The package format. + /// The logger used for events realting to the pip dependency specification. + public PipDependencySpecification(ILogger logger, string packageString, bool requiresDist = false) + { + this.Logger = logger; + this.Initialize(packageString, requiresDist); + } + + /// + /// Gets or sets the package (ex: pyyaml). + /// + public string Name { get; set; } + + /// + /// Gets or sets the set of dependency specifications that constrain the overall dependency request (ex: ==1.0, >=2.0). + /// + public IList DependencySpecifiers { get; set; } = new List(); + + public IList ConditionalDependencySpecifiers { get; set; } = new List(); + + private string DebuggerDisplay => $"{this.Name} ({string.Join(';', this.DependencySpecifiers)})"; + + private ILogger Logger { get; set; } + + private void Initialize(string packageString, bool requiresDist) { if (requiresDist) { @@ -116,20 +148,6 @@ public class PipDependencySpecification .ToList(); } - /// - /// Gets or sets the package (ex: pyyaml). - /// - public string Name { get; set; } - - /// - /// Gets or sets the set of dependency specifications that constrain the overall dependency request (ex: ==1.0, >=2.0). - /// - public IList DependencySpecifiers { get; set; } = new List(); - - public IList ConditionalDependencySpecifiers { get; set; } = new List(); - - private string DebuggerDisplay => $"{this.Name} ({string.Join(';', this.DependencySpecifiers)})"; - /// /// Whether or not the package is safe to resolve based on the packagesToIgnore. /// @@ -142,7 +160,7 @@ public class PipDependencySpecification /// /// Whether or not the package is safe to resolve based on the packagesToIgnore. /// - /// True if the package is unsafe, otherwise false. + /// True if the package meets all conditions. public bool PackageConditionsMet(Dictionary pythonEnvironmentVariables) { var conditionalRegex = new Regex(@"(and|or)?\s*(\S+)\s*(<=|>=|<|>|===|==|!=|~=)\s*['""]?([^'""]+)['""]?", RegexOptions.Compiled); @@ -166,7 +184,18 @@ public class PipDependencySpecification if (pythonVersion.Valid) { var conditionalSpec = $"{conditionalOperator}{conditionalValue}"; - conditionMet = PythonVersionUtilities.VersionValidForSpec(pythonEnvironmentVariables[conditionalVar], new List { conditionalSpec }); + try + { + conditionMet = PythonVersionUtilities.VersionValidForSpec(pythonEnvironmentVariables[conditionalVar], new List { conditionalSpec }); + } + catch (ArgumentException ae) + { + conditionMet = false; + if (this.Logger != null) + { + this.Logger.LogDebug("Could not create pip dependency: {ErrorMessage}", ae.Message); + } + } } else { @@ -214,14 +243,26 @@ public class PipDependencySpecification .Where(x => !string.IsNullOrEmpty(x)) .ToList(); - var topVersion = versions - .Where(x => PythonVersionUtilities.VersionValidForSpec(x, this.DependencySpecifiers)) - .Select(x => (Version: x, PythonVersion: PythonVersion.Create(x))) - .Where(x => x.PythonVersion.Valid) - .OrderByDescending(x => x.PythonVersion) - .FirstOrDefault(); + try + { + var topVersion = versions + .Where(x => PythonVersionUtilities.VersionValidForSpec(x, this.DependencySpecifiers)) + .Select(x => (Version: x, PythonVersion: PythonVersion.Create(x))) + .Where(x => x.PythonVersion.Valid) + .OrderByDescending(x => x.PythonVersion) + .FirstOrDefault((null, null)); - return topVersion.Version; + return topVersion.Version; + } + catch (ArgumentException ae) + { + if (this.Logger != null) + { + this.Logger.LogDebug("Could not create pip dependency: {ErrorMessage}", ae.Message); + } + + return null; + } } /// diff --git a/src/Microsoft.ComponentDetection.Detectors/pip/PipComponentDetector.cs b/src/Microsoft.ComponentDetection.Detectors/pip/PipComponentDetector.cs index 2fe75539..f50175c0 100644 --- a/src/Microsoft.ComponentDetection.Detectors/pip/PipComponentDetector.cs +++ b/src/Microsoft.ComponentDetection.Detectors/pip/PipComponentDetector.cs @@ -75,7 +75,8 @@ public class PipComponentDetector : FileComponentDetector, IDefaultOffComponentD var initialPackages = await this.pythonCommandService.ParseFileAsync(file.Location, pythonExePath); var listedPackage = SharedPipUtilities.ParsedPackagesToPipDependencies( initialPackages, - this.pythonResolver.GetPythonEnvironmentVariables()) + this.pythonResolver.GetPythonEnvironmentVariables(), + this.Logger) .ToList(); var roots = await this.pythonResolver.ResolveRootsAsync(singleFileComponentRecorder, listedPackage); diff --git a/src/Microsoft.ComponentDetection.Detectors/pip/PipReportComponentDetector.cs b/src/Microsoft.ComponentDetection.Detectors/pip/PipReportComponentDetector.cs index f964ed51..0a24d9fa 100644 --- a/src/Microsoft.ComponentDetection.Detectors/pip/PipReportComponentDetector.cs +++ b/src/Microsoft.ComponentDetection.Detectors/pip/PipReportComponentDetector.cs @@ -294,8 +294,18 @@ public class PipReportComponentDetector : FileComponentDetector // if pipreport fails, try to at least list the dependencies that are found in the source files if (this.GetPipReportOverrideBehavior() != PipReportOverrideBehavior.SourceCodeScan && !this.PipReportSkipFallbackOnFailure()) { - this.Logger.LogInformation("PipReport: Trying to Manually compile dependency list for '{File}' without reaching out to a remote feed.", file.Location); - await this.RegisterExplicitComponentsInFileAsync(singleFileComponentRecorder, file.Location, pythonExePath); + try + { + this.Logger.LogInformation( + "PipReport: Trying to manually compile package list for '{File}' without reaching out to a remote feed. " + + "This will NOT create a dependency graph, so should be avoided unless absolutely necessary.", + file.Location); + await this.RegisterExplicitComponentsInFileAsync(singleFileComponentRecorder, file.Location, pythonExePath); + } + catch (Exception ex) + { + this.Logger.LogWarning(ex, "PipReport: Failed to manually compile package list for '{File}'.", file.Location); + } } } finally @@ -366,7 +376,7 @@ public class PipReportComponentDetector : FileComponentDetector // cffi (>=1.12) // futures; python_version <= \"2.7\" // sphinx (!=1.8.0,!=3.1.0,!=3.1.1,>=1.6.5) ; extra == 'docs' - var dependencySpec = new PipDependencySpecification($"Requires-Dist: {dependency}", requiresDist: true); + var dependencySpec = new PipDependencySpecification(this.Logger, $"Requires-Dist: {dependency}", requiresDist: true); if (!dependencySpec.IsValidParentPackage(pythonEnvVars)) { continue; @@ -459,7 +469,8 @@ public class PipReportComponentDetector : FileComponentDetector var listedPackage = SharedPipUtilities.ParsedPackagesToPipDependencies( initialPackages, - this.pythonResolver.GetPythonEnvironmentVariables()) + this.pythonResolver.GetPythonEnvironmentVariables(), + this.Logger) .ToList(); listedPackage.Select(x => (x.Name, Version: x.GetHighestExplicitPackageVersion())) @@ -486,7 +497,8 @@ public class PipReportComponentDetector : FileComponentDetector var initialPackages = await this.pythonCommandService.ParseFileAsync(filePath, pythonExePath); var listedPackage = SharedPipUtilities.ParsedPackagesToPipDependencies( initialPackages, - this.pythonResolver.GetPythonEnvironmentVariables()) + this.pythonResolver.GetPythonEnvironmentVariables(), + this.Logger) .Select(x => x.Name) .ToImmutableSortedSet(); diff --git a/src/Microsoft.ComponentDetection.Detectors/pip/SharedPipUtilities.cs b/src/Microsoft.ComponentDetection.Detectors/pip/SharedPipUtilities.cs index 3a714b50..05ae5334 100644 --- a/src/Microsoft.ComponentDetection.Detectors/pip/SharedPipUtilities.cs +++ b/src/Microsoft.ComponentDetection.Detectors/pip/SharedPipUtilities.cs @@ -3,6 +3,7 @@ namespace Microsoft.ComponentDetection.Detectors.Pip; using System.Collections.Generic; using System.Linq; using Microsoft.ComponentDetection.Contracts.TypedComponent; +using Microsoft.Extensions.Logging; public static class SharedPipUtilities { @@ -12,14 +13,16 @@ public static class SharedPipUtilities /// /// List of packages and git components. /// Python environment specifiers. + /// Logger for pip dependency specification. /// Enumerable containing the converted, sanitized Pip dependency specs. public static IEnumerable ParsedPackagesToPipDependencies( IList<(string PackageString, GitComponent Component)> parsedPackages, - Dictionary pythonEnvironmentVariables) => + Dictionary pythonEnvironmentVariables, + ILogger logger) => parsedPackages.Where(tuple => tuple.PackageString != null) .Select(tuple => tuple.PackageString) .Where(x => !string.IsNullOrWhiteSpace(x)) - .Select(x => new PipDependencySpecification(x)) + .Select(x => new PipDependencySpecification(logger, x, false)) .Where(x => !x.PackageIsUnsafe()) .Where(x => x.PackageConditionsMet(pythonEnvironmentVariables)); } diff --git a/src/Microsoft.ComponentDetection.Detectors/pip/SimplePipComponentDetector.cs b/src/Microsoft.ComponentDetection.Detectors/pip/SimplePipComponentDetector.cs index 21d084e6..47baf7f2 100644 --- a/src/Microsoft.ComponentDetection.Detectors/pip/SimplePipComponentDetector.cs +++ b/src/Microsoft.ComponentDetection.Detectors/pip/SimplePipComponentDetector.cs @@ -68,7 +68,7 @@ public class SimplePipComponentDetector : FileComponentDetector, IDefaultOffComp var listedPackage = initialPackages.Where(tuple => tuple.PackageString != null) .Select(tuple => tuple.PackageString) .Where(x => !string.IsNullOrWhiteSpace(x)) - .Select(x => new PipDependencySpecification(x)) + .Select(x => new PipDependencySpecification(x, false)) .Where(x => !x.PackageIsUnsafe()) .ToList(); diff --git a/test/Microsoft.ComponentDetection.Detectors.Tests/PipDependencySpecifierTests.cs b/test/Microsoft.ComponentDetection.Detectors.Tests/PipDependencySpecifierTests.cs index 8fb2707d..d7717f30 100644 --- a/test/Microsoft.ComponentDetection.Detectors.Tests/PipDependencySpecifierTests.cs +++ b/test/Microsoft.ComponentDetection.Detectors.Tests/PipDependencySpecifierTests.cs @@ -161,4 +161,43 @@ public class PipDependencySpecifierTests VerifyPipConditionalDependencyParsing(specs, pythonEnvironmentVariables, true); } + + [TestMethod] + public void TestPipDependencyGetHighestExplicitPackageVersion_Valid() + { + var spec = new PipDependencySpecification + { + Name = "TestPackage", + DependencySpecifiers = new List { ">=1.0", "<=3.0", "!=2.0", "!=4.0" }, + }; + + var highestVersion = spec.GetHighestExplicitPackageVersion(); + highestVersion.Should().Be("3.0"); + } + + [TestMethod] + public void TestPipDependencyGetHighestExplicitPackageVersion_SingleInvalidSpec() + { + var spec = new PipDependencySpecification + { + Name = "TestPackage", + DependencySpecifiers = new List { ">=1.0", "info", "!=2.0", "!=4.0" }, + }; + + var highestVersion = spec.GetHighestExplicitPackageVersion(); + highestVersion.Should().BeNull(); + } + + [TestMethod] + public void TestPipDependencyGetHighestExplicitPackageVersion_AllInvalidSpec() + { + var spec = new PipDependencySpecification + { + Name = "TestPackage", + DependencySpecifiers = new List { "info" }, + }; + + var highestVersion = spec.GetHighestExplicitPackageVersion(); + highestVersion.Should().BeNull(); + } } diff --git a/test/Microsoft.ComponentDetection.Detectors.Tests/PipReportComponentDetectorTests.cs b/test/Microsoft.ComponentDetection.Detectors.Tests/PipReportComponentDetectorTests.cs index 58eb4834..7c323334 100644 --- a/test/Microsoft.ComponentDetection.Detectors.Tests/PipReportComponentDetectorTests.cs +++ b/test/Microsoft.ComponentDetection.Detectors.Tests/PipReportComponentDetectorTests.cs @@ -522,12 +522,72 @@ public class PipReportComponentDetectorTests : BaseDetectorTest x.Log( - LogLevel.Information, - It.IsAny(), - It.Is((o, t) => o.ToString().StartsWith("PipReport: Found PipReportOverrideBehavior environment variable set to SourceCodeScan.")), - It.IsAny(), - (Func)It.IsAny())); + this.mockLogger.Verify( + x => x.Log( + LogLevel.Information, + It.IsAny(), + It.Is((o, t) => o.ToString().StartsWith("PipReport: Found PipReportOverrideBehavior environment variable set to SourceCodeScan.")), + It.IsAny(), + (Func)It.IsAny()), + Times.Exactly(2)); + } + + [TestMethod] + public async Task TestPipReportDetector_FallbackAsync() + { + this.pythonCommandService.Setup(x => x.PythonExistsAsync(It.IsAny())).ReturnsAsync(true); + + var baseSetupPyDependencies = this.ToGitTuple(new List { "a==1.0", "b>=2.0,!=2.1,<3.0.0", "c!=1.1", "y==invalidversion" }); + var baseRequirementsTextDependencies = this.ToGitTuple(new List { "d~=1.0", "e<=2.0", "f===1.1", "g<3.0", "h>=1.0,<=3.0,!=2.0,!=4.0", "z==anotherinvalidversion" }); + baseRequirementsTextDependencies.Add((null, new GitComponent(new Uri("https://github.com/example/example"), "deadbee"))); + + this.pythonCommandService.Setup(x => x.ParseFileAsync(Path.Join(Path.GetTempPath(), "setup.py"), null)).ReturnsAsync(baseSetupPyDependencies); + this.pythonCommandService.Setup(x => x.ParseFileAsync(Path.Join(Path.GetTempPath(), "requirements.txt"), null)).ReturnsAsync(baseRequirementsTextDependencies); + + this.pipCommandService.Setup(x => x.GenerateInstallationReportAsync( + It.Is(s => s.Contains("requirements.txt", StringComparison.OrdinalIgnoreCase)), + It.IsAny(), + It.IsAny(), + It.IsAny())) + .ThrowsAsync(new InvalidOperationException("Want to fallback, so fail initial report generation")); + + var (result, componentRecorder) = await this.DetectorTestUtility + .WithFile("setup.py", string.Empty) + .WithFile("requirements.txt", string.Empty) + .ExecuteDetectorAsync(); + + result.ResultCode.Should().Be(ProcessingResultCode.Success); + + var detectedComponents = componentRecorder.GetDetectedComponents(); + detectedComponents.Should().HaveCount(7); + + var pipComponents = detectedComponents.Where(detectedComponent => detectedComponent.Component.Id.Contains("pip")).ToList(); + ((PipComponent)pipComponents.Single(x => ((PipComponent)x.Component).Name == "a").Component).Version.Should().Be("1.0"); + ((PipComponent)pipComponents.Single(x => ((PipComponent)x.Component).Name == "b").Component).Version.Should().Be("2.0"); + ((PipComponent)pipComponents.Single(x => ((PipComponent)x.Component).Name == "d").Component).Version.Should().Be("1.0"); + ((PipComponent)pipComponents.Single(x => ((PipComponent)x.Component).Name == "e").Component).Version.Should().Be("2.0"); + ((PipComponent)pipComponents.Single(x => ((PipComponent)x.Component).Name == "f").Component).Version.Should().Be("1.1"); + ((PipComponent)pipComponents.Single(x => ((PipComponent)x.Component).Name == "h").Component).Version.Should().Be("3.0"); + pipComponents.Should().NotContain(x => ((PipComponent)x.Component).Name == "c"); + pipComponents.Should().NotContain(x => ((PipComponent)x.Component).Name == "g"); + pipComponents.Should().NotContain(x => ((PipComponent)x.Component).Name == "y"); + pipComponents.Should().NotContain(x => ((PipComponent)x.Component).Name == "z"); + + this.mockLogger.Verify( + x => x.Log( + LogLevel.Debug, + It.IsAny(), + It.Is((o, t) => o.ToString().StartsWith("Could not create pip dependency: invalidversion is not a valid python version")), + It.IsAny(), + (Func)It.IsAny()), + Times.Once); + + var gitComponents = detectedComponents.Where(detectedComponent => detectedComponent.Component.Type == ComponentType.Git); + gitComponents.Should().ContainSingle(); + var gitComponent = (GitComponent)gitComponents.Single().Component; + + gitComponent.RepositoryUrl.Should().Be("https://github.com/example/example"); + gitComponent.CommitHash.Should().Be("deadbee"); } [TestMethod] diff --git a/test/Microsoft.ComponentDetection.VerificationTests/resources/pip/fallback/requirements.txt b/test/Microsoft.ComponentDetection.VerificationTests/resources/pip/fallback/requirements.txt index 8e711674..3b77b4b0 100644 --- a/test/Microsoft.ComponentDetection.VerificationTests/resources/pip/fallback/requirements.txt +++ b/test/Microsoft.ComponentDetection.VerificationTests/resources/pip/fallback/requirements.txt @@ -1,2 +1,3 @@ fakepythonpackage==1.2.3 zipp>=3.2.0,<4.0 +badpackage==info