Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix handling of NuGet transitive dependencies #10449

Merged
merged 1 commit into from
Aug 16, 2024
Merged

fix handling of NuGet transitive dependencies #10449

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core.Test/Analyze/AnalyzeWorkerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,57 @@ await TestAnalyzeAsync(
);
}

[Fact]
public async Task AnalyzeVulnerableTransitiveDependencies()
{
await TestAnalyzeAsync(
packages:
[
MockNuGetPackage.CreateSimplePackage("Some.Transitive.Dependency", "1.0.0", "net8.0"),
MockNuGetPackage.CreateSimplePackage("Some.Transitive.Dependency", "1.0.1", "net8.0"),
],
discovery: new()
{
Path = "/",
Projects = [
new()
{
FilePath = "project.csproj",
TargetFrameworks = ["net8.0"],
Dependencies = [
new("Some.Transitive.Dependency", "1.0.0", DependencyType.Unknown, TargetFrameworks: ["net8.0"], IsTransitive: true),
]
}
]
},
dependencyInfo: new()
{
Name = "Some.Transitive.Dependency",
Version = "1.0.0",
IsVulnerable = true,
IgnoredVersions = [],
Vulnerabilities = [
new()
{
DependencyName = "Some.Transitive.Dependency",
PackageManager = "nuget",
VulnerableVersions = [Requirement.Parse("<= 1.0.0")],
SafeVersions = [Requirement.Parse("= 1.0.1")],
}
]
},
expectedResult: new()
{
UpdatedVersion = "1.0.1",
CanUpdate = true,
VersionComesFromMultiDependencyProperty = false,
UpdatedDependencies = [
new("Some.Transitive.Dependency", "1.0.1", DependencyType.Unknown, TargetFrameworks: ["net8.0"]),
],
}
);
}

[Fact]
public async Task IgnoredVersionsCanHandleWildcardSpecification()
{
Expand Down
23 changes: 19 additions & 4 deletions nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core/Analyze/AnalyzeWorker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ public async Task RunAsync(string repoRoot, string discoveryPath, string depende
discovery,
dependenciesToUpdate,
updatedVersion,
dependencyInfo,
nugetContext,
_logger,
CancellationToken.None);
Expand Down Expand Up @@ -359,6 +360,7 @@ internal static async Task<ImmutableArray<Dependency>> FindUpdatedDependenciesAs
WorkspaceDiscoveryResult discovery,
ImmutableHashSet<string> packageIds,
NuGetVersion updatedVersion,
DependencyInfo dependencyInfo,
NuGetContext nugetContext,
Logger logger,
CancellationToken cancellationToken)
Expand All @@ -379,10 +381,23 @@ internal static async Task<ImmutableArray<Dependency>> FindUpdatedDependenciesAs
.Select(NuGetFramework.Parse)
.ToImmutableArray();

// When updating peer dependencies, we only need to consider top-level dependencies.
var projectDependencyNames = projectsWithDependency
.SelectMany(p => p.Dependencies)
.Where(d => !d.IsTransitive)
// When updating dependencies, we only need to consider top-level dependencies _UNLESS_ it's specifically vulnerable
var relevantDependencies = projectsWithDependency.SelectMany(p => p.Dependencies)
.Where(d =>
{
if (string.Compare(d.Name, dependencyInfo.Name, StringComparison.OrdinalIgnoreCase) == 0 &&
dependencyInfo.IsVulnerable)
{
// if this dependency is one we're specifically updating _and_ if it's vulnerable, always update it
return true;
}
else
{
// otherwise only update if it's a top-level dependency
return !d.IsTransitive;
}
});
var projectDependencyNames = relevantDependencies
.Select(d => d.Name)
.ToImmutableHashSet(StringComparer.OrdinalIgnoreCase);

Expand Down
5 changes: 4 additions & 1 deletion nuget/lib/dependabot/nuget/file_parser.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,10 @@ def parse
# cache discovery results
NativeDiscoveryJsonReader.set_discovery_from_dependency_files(dependency_files: dependency_files,
discovery: discovery_json_reader)
discovery_json_reader.dependency_set.dependencies
# we only return top-level dependencies and requirements here
dependency_set = discovery_json_reader.dependency_set(dependency_files: dependency_files,
top_level_only: true)
dependency_set.dependencies
end

T.must(self.class.file_dependency_cache[key])
Expand Down
181 changes: 84 additions & 97 deletions nuget/lib/dependabot/nuget/file_updater.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,16 @@ module Nuget
class FileUpdater < Dependabot::FileUpdaters::Base
extend T::Sig

DependencyDetails = T.type_alias do
{
file: String,
name: String,
version: String,
previous_version: String,
is_transitive: T::Boolean
}
end

sig { override.params(allowlist_enabled: T::Boolean).returns(T::Array[Regexp]) }
def self.updated_files_regex(allowlist_enabled = false)
if allowlist_enabled
Expand Down Expand Up @@ -65,9 +75,21 @@ def self.differs_in_more_than_blank_lines?(original_content, updated_content)
def updated_dependency_files
base_dir = "/"
SharedHelpers.in_a_temporary_repo_directory(base_dir, repo_contents_path) do
dependencies.each do |dependency|
try_update_projects(dependency) || try_update_json(dependency)
expanded_dependency_details.each do |dep_details|
file = T.let(dep_details.fetch(:file), String)
name = T.let(dep_details.fetch(:name), String)
version = T.let(dep_details.fetch(:version), String)
previous_version = T.let(dep_details.fetch(:previous_version), String)
is_transitive = T.let(dep_details.fetch(:is_transitive), T::Boolean)
NativeHelpers.run_nuget_updater_tool(repo_root: T.must(repo_contents_path),
proj_path: file,
dependency_name: name,
version: version,
previous_version: previous_version,
is_transitive: is_transitive,
credentials: credentials)
end

updated_files = dependency_files.filter_map do |f|
updated_content = File.read(dependency_file_path(f))
next if updated_content == f.content
Expand All @@ -87,104 +109,69 @@ def updated_dependency_files

private

sig { params(dependency: Dependabot::Dependency).returns(T::Boolean) }
def try_update_projects(dependency)
update_ran = T.let(false, T::Boolean)
checked_files = Set.new

# run update for each project file
project_files.each do |project_file|
project_dependencies = project_dependencies(project_file)
proj_path = dependency_file_path(project_file)

next unless project_dependencies.any? { |dep| dep.name.casecmp?(dependency.name) }

next unless repo_contents_path

checked_key = "#{project_file.name}-#{dependency.name}#{dependency.version}"
call_nuget_updater_tool(dependency, proj_path) unless checked_files.include?(checked_key)

checked_files.add(checked_key)
# We need to check the downstream references even though we're already evaluated the file
downstream_files = referenced_project_paths(project_file)
downstream_files.each do |downstream_file|
checked_files.add("#{downstream_file}-#{dependency.name}#{dependency.version}")
# rubocop:disable Metrics/AbcSize
sig { returns(T::Array[DependencyDetails]) }
def expanded_dependency_details
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was pulled out into a separate method because it made it really easy to unit test.

discovery_json_reader = NativeDiscoveryJsonReader.get_discovery_from_dependency_files(dependency_files)
dependency_set = discovery_json_reader.dependency_set(dependency_files: dependency_files, top_level_only: false)
all_dependencies = dependency_set.dependencies
dependencies.map do |dep|
# if vulnerable metadata is set, re-fetch all requirements from discovery
is_vulnerable = T.let(dep.metadata.fetch(:is_vulnerable, false), T::Boolean)
relevant_dependencies = all_dependencies.filter { |d| d.name.casecmp?(dep.name) }
candidate_vulnerable_dependency = T.must(relevant_dependencies.first)
relevant_dependency = is_vulnerable ? candidate_vulnerable_dependency : dep
relevant_details = relevant_dependency.requirements.filter_map do |req|
dependency_details_from_requirement(dep.name, req, is_vulnerable: is_vulnerable)
end
update_ran = true
end
update_ran
end

sig { params(dependency: Dependabot::Dependency).returns(T::Boolean) }
def try_update_json(dependency)
if dotnet_tools_json_dependencies.any? { |dep| dep.name.casecmp?(dependency.name) } ||
global_json_dependencies.any? { |dep| dep.name.casecmp?(dependency.name) }

# We just need to feed the updater a project file, grab the first
project_file = T.must(project_files.first)
proj_path = dependency_file_path(project_file)

return false unless repo_contents_path

call_nuget_updater_tool(dependency, proj_path)
return true
end

false
end

sig { params(dependency: Dependency, proj_path: String).void }
def call_nuget_updater_tool(dependency, proj_path)
NativeHelpers.run_nuget_updater_tool(repo_root: T.must(repo_contents_path), proj_path: proj_path,
dependency: dependency, is_transitive: !dependency.top_level?,
credentials: credentials)

# Tests need to track how many times we call the tooling updater to ensure we don't recurse needlessly
# Ideally we should find a way to not run this code in prod
# (or a better way to track calls made to NativeHelpers)
@update_tooling_calls ||= T.let({}, T.nilable(T::Hash[String, Integer]))
key = "#{proj_path.delete_prefix(T.must(repo_contents_path))}+#{dependency.name}"
@update_tooling_calls[key] =
if @update_tooling_calls[key]
T.must(@update_tooling_calls[key]) + 1
else
1
next relevant_details if relevant_details.any?

# If we didn't find anything to update, we're in a very specific corner case: we were explicitly asked to
# (1) update a certain dependency, (2) it wasn't listed as a security update, but (3) it only exists as a
# transitive dependency. In this case, we need to rebuild the dependency requirements as if this were a
# security update so that we can perform the appropriate update.
candidate_vulnerable_dependency.requirements.filter_map do |req|
rebuilt_req = {
file: req[:file], # simple copy
requirement: relevant_dependency.version, # the newly available version
metadata: {
is_transitive: T.let(req[:metadata], T::Hash[Symbol, T.untyped])[:is_transitive], # simple copy
previous_requirement: req[:requirement] # the old requirement's "current" version is now the "previous"
}
}
dependency_details_from_requirement(dep.name, rebuilt_req, is_vulnerable: true)
end
end

# Don't call this from outside tests, we're only checking that we aren't recursing needlessly
sig { returns(T.nilable(T::Hash[String, Integer])) }
def testonly_update_tooling_calls
@update_tooling_calls
end

sig { returns(T.nilable(NativeWorkspaceDiscovery)) }
def workspace
discovery_json_reader = NativeDiscoveryJsonReader.get_discovery_from_dependency_files(dependency_files)
discovery_json_reader.workspace_discovery
end

sig { params(project_file: Dependabot::DependencyFile).returns(T::Array[String]) }
def referenced_project_paths(project_file)
workspace&.projects&.find { |p| p.file_path == project_file.name }&.referenced_project_paths || []
end

sig { params(project_file: Dependabot::DependencyFile).returns(T::Array[NativeDependencyDetails]) }
def project_dependencies(project_file)
workspace&.projects&.find do |p|
full_project_file_path = File.join(project_file.directory, project_file.name)
p.file_path == full_project_file_path
end&.dependencies || []
end

sig { returns(T::Array[NativeDependencyDetails]) }
def global_json_dependencies
workspace&.global_json&.dependencies || []
end

sig { returns(T::Array[NativeDependencyDetails]) }
def dotnet_tools_json_dependencies
workspace&.dotnet_tools_json&.dependencies || []
end.flatten
end
# rubocop:enable Metrics/AbcSize

sig do
params(
name: String,
requirement: T::Hash[Symbol, T.untyped],
is_vulnerable: T::Boolean
).returns(T.nilable(DependencyDetails))
end
def dependency_details_from_requirement(name, requirement, is_vulnerable:)
metadata = T.let(requirement.fetch(:metadata), T::Hash[Symbol, T.untyped])
current_file = T.let(requirement.fetch(:file), String)
return nil unless current_file.match?(/\.(cs|vb|fs)proj$/)

is_transitive = T.let(metadata.fetch(:is_transitive), T::Boolean)
return nil if !is_vulnerable && is_transitive

version = T.let(requirement.fetch(:requirement), String)
previous_version = T.let(metadata[:previous_requirement], String)
return nil if version == previous_version

{
file: T.let(requirement.fetch(:file), String),
name: name,
version: version,
previous_version: previous_version,
is_transitive: is_transitive
}
end

# rubocop:disable Metrics/PerceivedComplexity
Expand Down
10 changes: 4 additions & 6 deletions nuget/lib/dependabot/nuget/native_discovery/native_dependency_file_discovery.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,22 +106,20 @@ def build_dependency(file_name, dependency_details)
.returns(T.nilable(T::Hash[Symbol, T.untyped]))
end
def build_requirement(file_name, dependency_details)
return if dependency_details.is_transitive

version = dependency_details.version
version = nil if version&.empty?
metadata = { is_transitive: dependency_details.is_transitive }

requirement = {
requirement: version,
file: file_name,
groups: [dependency_details.is_dev_dependency ? "devDependencies" : "dependencies"],
source: nil
source: nil,
metadata: metadata
}

property_name = dependency_details.evaluation&.root_property_name
return requirement unless property_name

requirement[:metadata] = { property_name: property_name }
metadata[:property_name] = property_name if property_name
requirement
end
end
Expand Down
Loading
Loading