diff --git a/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core.Test/MockNuGetPackage.cs b/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core.Test/MockNuGetPackage.cs index 0284d67dc2c..9b5402a2f02 100644 --- a/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core.Test/MockNuGetPackage.cs +++ b/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core.Test/MockNuGetPackage.cs @@ -1,3 +1,4 @@ +using System.Collections.Immutable; using System.IO.Compression; using System.Security.Cryptography; using System.Text; @@ -95,7 +96,7 @@ public static MockNuGetPackage CreateSimplePackage( /// Creates a mock NuGet package with a single assembly in the appropriate `lib/` directory. The assembly will /// contain the appropriate `AssemblyVersion` attribute and nothing else. /// - public static MockNuGetPackage CreatePackageWithAssembly(string id, string version, string targetFramework, string assemblyVersion, (string? TargetFramework, (string Id, string Version)[] Packages)[]? dependencyGroups = null) + public static MockNuGetPackage CreatePackageWithAssembly(string id, string version, string targetFramework, string assemblyVersion, ImmutableArray? assemblyPublicKey = null, (string? TargetFramework, (string Id, string Version)[] Packages)[]? dependencyGroups = null) { return new( id, @@ -104,7 +105,7 @@ public static MockNuGetPackage CreatePackageWithAssembly(string id, string versi DependencyGroups: dependencyGroups, Files: [ - ($"lib/{targetFramework}/{id}.dll", CreateAssembly(id, assemblyVersion)) + ($"lib/{targetFramework}/{id}.dll", CreateAssembly(id, assemblyVersion, assemblyPublicKey)) ] ); } @@ -271,9 +272,13 @@ public Stream GetZipStream() return _stream; } - private static byte[] CreateAssembly(string assemblyName, string assemblyVersion) + private static byte[] CreateAssembly(string assemblyName, string assemblyVersion, ImmutableArray? assemblyPublicKey = null) { CSharpCompilationOptions compilationOptions = new(OutputKind.DynamicallyLinkedLibrary); + if (assemblyPublicKey is not null) + { + compilationOptions = compilationOptions.WithCryptoPublicKey(assemblyPublicKey.Value); + } CSharpCompilation compilation = CSharpCompilation.Create(assemblyName, options: compilationOptions) .AddReferences(MetadataReference.CreateFromFile(typeof(object).Assembly.Location)) .AddSyntaxTrees(CSharpSyntaxTree.ParseText($"[assembly: System.Reflection.AssemblyVersionAttribute(\"{assemblyVersion}\")]")); diff --git a/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core.Test/Update/UpdateWorkerTests.PackagesConfig.cs b/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core.Test/Update/UpdateWorkerTests.PackagesConfig.cs index b5d1a6e4bf2..91ec30ac65f 100644 --- a/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core.Test/Update/UpdateWorkerTests.PackagesConfig.cs +++ b/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core.Test/Update/UpdateWorkerTests.PackagesConfig.cs @@ -1,3 +1,5 @@ +using System.Collections.Immutable; + using NuGetUpdater.Core.Updater; using Xunit; @@ -1013,6 +1015,307 @@ await TestUpdateForProject("Some.Package", "7.0.1", "13.0.1", ); } + [Fact] + public async Task UpdateBindingRedirect_DuplicateRedirectsForTheSameAssemblyAreRemoved() + { + await TestUpdateForProject("Some.Package", "7.0.1", "13.0.1", + packages: + [ + MockNuGetPackage.CreatePackageWithAssembly("Some.Package", "7.0.1", "net45", "7.0.0.0"), + MockNuGetPackage.CreatePackageWithAssembly("Some.Package", "13.0.1", "net45", "13.0.0.0"), + ], + projectContents: """ + + + + v4.5 + + + + + + + + + + packages\Some.Package.7.0.1\lib\net45\Some.Package.dll + True + + + + + """, + packagesConfigContents: """ + + + + """, + additionalFiles: + [ + ("app.config", """ + + + + + + + + + + + + + + + """) + ], + expectedProjectContents: """ + + + + v4.5 + + + + + + + + + + packages\Some.Package.13.0.1\lib\net45\Some.Package.dll + True + + + + + """, + expectedPackagesConfigContents: """ + + + + + """, + additionalFilesExpected: + [ + ("app.config", """ + + + + + + + + + + + """) + ] + ); + } + + [Fact] + public async Task UpdateBindingRedirect_ExistingRedirectForAssemblyPublicKeyTokenDiffersByCase() + { + // Generated using "sn -k keypair.snk && sn -p keypair.snk public.snk" then converting public.snk to base64 + // https://learn.microsoft.com/en-us/dotnet/standard/assembly/create-public-private-key-pair + var assemblyStrongNamePublicKey = Convert.FromBase64String( + "ACQAAASAAACUAAAABgIAAAAkAABSU0ExAAQAAAEAAQAJJW4hmKpxa9pU0JPDvJ9KqjvfQuMUovGtFjkZ9b0i1KQ/7kqEOjW3Va0eGpU7Kz0qHp14iYQ3SsMzBZU3mZ2Ezeqg+dCVuDk7o2lp++4m1FstHsebtXBetyOzWkneo+3iKSzOQ7bOXj2s5M9umqRPk+yj0ZBILf+HvfAd07iIuQ==" + ).ToImmutableArray(); + + await TestUpdateForProject("Some.Package", "7.0.1", "13.0.1", + packages: + [ + MockNuGetPackage.CreatePackageWithAssembly("Some.Package", "7.0.1", "net45", "7.0.0.0", assemblyPublicKey: assemblyStrongNamePublicKey), + MockNuGetPackage.CreatePackageWithAssembly("Some.Package", "13.0.1", "net45", "13.0.0.0", assemblyPublicKey: assemblyStrongNamePublicKey), + ], + projectContents: """ + + + + v4.5 + + + + + + + + + + packages\Some.Package.7.0.1\lib\net45\Some.Package.dll + True + + + + + """, + packagesConfigContents: """ + + + + """, + additionalFiles: + [ + ("app.config", """ + + + + + + + + + + + """) + ], + expectedProjectContents: """ + + + + v4.5 + + + + + + + + + + packages\Some.Package.13.0.1\lib\net45\Some.Package.dll + True + + + + + """, + expectedPackagesConfigContents: """ + + + + + """, + additionalFilesExpected: + [ + ("app.config", """ + + + + + + + + + + + """) + ] + ); + } + + [Fact] + public async Task UpdateBindingRedirect_ConfigXmlDeclarationNodeIsPreserved() + { + await TestUpdateForProject("Some.Package", "7.0.1", "13.0.1", + packages: + [ + MockNuGetPackage.CreatePackageWithAssembly("Some.Package", "7.0.1", "net45", "7.0.0.0"), + MockNuGetPackage.CreatePackageWithAssembly("Some.Package", "13.0.1", "net45", "13.0.0.0"), + ], + projectContents: """ + + + + v4.5 + + + + + + + + + + packages\Some.Package.7.0.1\lib\net45\Some.Package.dll + True + + + + + """, + packagesConfigContents: """ + + + + """, + additionalFiles: + [ + ("app.config", """ + + + + + + + + + + + + + + + + """) + ], + expectedProjectContents: """ + + + + v4.5 + + + + + + + + + + packages\Some.Package.13.0.1\lib\net45\Some.Package.dll + True + + + + + """, + expectedPackagesConfigContents: """ + + + + + """, + additionalFilesExpected: + [ + ("app.config", """ + + + + + + + + + + + + """) + ] + ); + } + [Fact] public async Task PackagesConfigUpdateCanHappenEvenWithMismatchedVersionNumbers() { diff --git a/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core/Updater/BindingRedirectManager.cs b/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core/Updater/BindingRedirectManager.cs index 892d506ef8f..2b6011c9910 100644 --- a/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core/Updater/BindingRedirectManager.cs +++ b/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core/Updater/BindingRedirectManager.cs @@ -149,42 +149,6 @@ static bool IsConfigFile(IXmlElementSyntax element) return (element.Name == "None" && string.Equals(path, "app.config", StringComparison.OrdinalIgnoreCase)) || (element.Name == "Content" && string.Equals(path, "web.config", StringComparison.OrdinalIgnoreCase)); } - - static string GetConfigFileName(XmlDocumentSyntax document) - { - var guidValue = document.Descendants() - .Where(static x => x.Name == "PropertyGroup") - .SelectMany(static x => x.Elements.Where(static x => x.Name == "ProjectGuid")) - .FirstOrDefault() - ?.GetContentValue(); - return guidValue switch - { - "{E24C65DC-7377-472B-9ABA-BC803B73C61A}" or "{349C5851-65DF-11DA-9384-00065B846F21}" => "Web.config", - _ => "App.config" - }; - } - - static string GenerateDefaultAppConfig(XmlDocumentSyntax document) - { - var frameworkVersion = GetFrameworkVersion(document); - return $""" - - - - - - - """; - } - - static string? GetFrameworkVersion(XmlDocumentSyntax document) - { - return document.Descendants() - .Where(static x => x.Name == "PropertyGroup") - .SelectMany(static x => x.Elements.Where(static x => x.Name == "TargetFrameworkVersion")) - .FirstOrDefault() - ?.GetContentValue(); - } } private static string AddBindingRedirects(ConfigurationFile configFile, IEnumerable bindingRedirects) @@ -213,10 +177,24 @@ private static string AddBindingRedirects(ConfigurationFile configFile, IEnumera foreach (var bindingRedirect in bindingRedirects) { - // Look to see if we already have this in the list of bindings already in config. - if (currentBindings.TryGetValue((bindingRedirect.Name, bindingRedirect.PublicKeyToken), out var existingBinding)) + // If the binding redirect already exists in config, update it. Otherwise, add it. + var bindingAssemblyIdentity = new AssemblyIdentity(bindingRedirect.Name, bindingRedirect.PublicKeyToken); + if (currentBindings.Contains(bindingAssemblyIdentity)) { - UpdateBindingRedirectElement(existingBinding, bindingRedirect); + // Check if there are multiple bindings in config for this assembly and remove all but the first one. + // Normally there should only be one binding per assembly identity unless the config is malformed, which we'll fix here like NuGet.exe would. + var existingBindings = currentBindings[bindingAssemblyIdentity]; + if (existingBindings.Any()) + { + // Remove all but the first element + foreach (var bindingElement in existingBindings.Skip(1)) + { + RemoveElement(bindingElement); + } + + // Update the first one with the new binding + UpdateBindingRedirectElement(existingBindings.First(), bindingRedirect); + } } else { @@ -228,7 +206,10 @@ private static string AddBindingRedirects(ConfigurationFile configFile, IEnumera } } - return document.ToString(); + return string.Concat( + document.Declaration?.ToString() ?? String.Empty, // Ensure the declaration node is preserved, if present + document.ToString() + ); static XDocument GetConfiguration(string configFileContent) { @@ -278,7 +259,7 @@ static void UpdateBindingRedirectElement( } } - static Dictionary<(string Name, string PublicKeyToken), XElement> GetAssemblyBindings(XElement runtime) + static ILookup GetAssemblyBindings(XElement runtime) { var dependencyAssemblyElements = runtime.Elements(AssemblyBindingName) .Elements(DependentAssemblyName); @@ -291,7 +272,12 @@ static void UpdateBindingRedirectElement( }); // Return a mapping from binding to element - return assemblyElementPairs.ToDictionary(p => (p.Binding.Name, p.Binding.PublicKeyToken), p => p.Element); + // It is possible that multiple elements exist for the same assembly identity, so use a lookup (1:*) instead of a dictionary (1:1) + return assemblyElementPairs.ToLookup( + p => new AssemblyIdentity(p.Binding.Name, p.Binding.PublicKeyToken), + p => p.Element, + new AssemblyIdentityIgnoreCaseComparer() + ); } static XElement GetAssemblyBindingElement(XElement runtime) @@ -309,4 +295,21 @@ static XElement GetAssemblyBindingElement(XElement runtime) return assemblyBinding; } } + + internal sealed record AssemblyIdentity(string Name, string PublicKeyToken); + + // Case-insensitive comparer. This helps avoid creating duplicate binding redirects when there is a case form mismatch between assembly identities. + // Especially important for PublicKeyToken which is typically lowercase (using NuGet.exe), but can also be uppercase when using other tools (e.g. Visual Studio auto-resolve assembly conflicts feature). + internal sealed class AssemblyIdentityIgnoreCaseComparer : IEqualityComparer + { + public bool Equals(AssemblyIdentity? x, AssemblyIdentity? y) => + string.Equals(x?.Name, y?.Name, StringComparison.OrdinalIgnoreCase) && + string.Equals(x?.PublicKeyToken, y?.PublicKeyToken, StringComparison.OrdinalIgnoreCase); + + public int GetHashCode(AssemblyIdentity obj) => + HashCode.Combine( + obj.Name?.ToLowerInvariant(), + obj.PublicKeyToken?.ToLowerInvariant() + ); + } } diff --git a/nuget/lib/dependabot/nuget/file_fetcher.rb b/nuget/lib/dependabot/nuget/file_fetcher.rb index f3e0ab40a84..c8993a8e0f6 100644 --- a/nuget/lib/dependabot/nuget/file_fetcher.rb +++ b/nuget/lib/dependabot/nuget/file_fetcher.rb @@ -51,20 +51,22 @@ def initialize(source:, credentials:, repo_contents_path: nil, options: {}) @fetched_files = T.let({}, T::Hash[String, T::Array[Dependabot::DependencyFile]]) @nuget_config_files = T.let(nil, T.nilable(T::Array[Dependabot::DependencyFile])) @packages_config_files = T.let(nil, T.nilable(T::Array[Dependabot::DependencyFile])) + @assembly_binding_redirect_config_files = T.let(nil, T.nilable(T::Array[Dependabot::DependencyFile])) end sig { override.returns(T::Array[DependencyFile]) } def fetch_files - fetched_files = [] - fetched_files += project_files - fetched_files += directory_build_files - fetched_files += imported_property_files - - fetched_files += packages_config_files - fetched_files += nuget_config_files - fetched_files << global_json if global_json - fetched_files << dotnet_tools_json if dotnet_tools_json - fetched_files << packages_props if packages_props + fetched_files = [ + *project_files, + *directory_build_files, + *imported_property_files, + *packages_config_files, + *assembly_binding_redirect_config_files, + *nuget_config_files, + global_json, + dotnet_tools_json, + packages_props + ].compact # dedup files based on their absolute path fetched_files = fetched_files.uniq do |fetched_file| @@ -128,6 +130,23 @@ def packages_config_files end end + sig { returns(T::Array[Dependabot::DependencyFile]) } + def assembly_binding_redirect_config_files + return @assembly_binding_redirect_config_files if @assembly_binding_redirect_config_files + + candidate_paths = + [*project_files.map { |f| File.dirname(f.name) }, "."].uniq + + # Assembly binding redirects can appear in any app/web.config file for a .NET Framework project + # https://learn.microsoft.com/en-us/dotnet/framework/configure-apps/redirect-assembly-versions#specify-assembly-binding-in-configuration-files + @assembly_binding_redirect_config_files = + candidate_paths.filter_map do |dir| + file = repo_contents(dir: dir) + .find { |f| f.name.match?(/^(app|web)\.config$/i) } + fetch_file_from_host(File.join(dir, file.name)) if file + end + end + # rubocop:disable Metrics/PerceivedComplexity sig { returns(T.nilable(T::Array[T.untyped])) } def sln_file_names diff --git a/nuget/lib/dependabot/nuget/file_updater.rb b/nuget/lib/dependabot/nuget/file_updater.rb index 10b5d758cea..38ff5089a63 100644 --- a/nuget/lib/dependabot/nuget/file_updater.rb +++ b/nuget/lib/dependabot/nuget/file_updater.rb @@ -21,6 +21,8 @@ def self.updated_files_regex [ %r{^[^/]*\.([a-z]{2})?proj$}, /^packages\.config$/i, + /^app\.config$/i, + /^web\.config$/i, /^global\.json$/i, /^dotnet-tools\.json$/i, /^Directory\.Build\.props$/i,