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.

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

[npm] Flag indirect transitive updates to be ignored by the FileUpdater #5873

Merged
merged 4 commits into from
Oct 12, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 7 additions & 0 deletions common/lib/dependabot/dependency.rb
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,13 @@ def all_versions
all_versions.filter_map(&:version)
end

# This dependency is being indirectly updated by an update to another
# dependency. We don't need to try and update it ourselves but want to
# surface it to the user in the PR.
def informational_only?
metadata[:information_only]
end

def ==(other)
other.instance_of?(self.class) && to_h == other.to_h
end
Expand Down
2 changes: 1 addition & 1 deletion hex/spec/dependabot/hex/update_checker_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@
[{ file: "mix.exs", requirement: "~> 1.2.1", groups: [], source: nil }]
end

it { is_expected.to eq(Gem::Version.new("1.3.4")) }
it { is_expected.to eq(Gem::Version.new("1.3.5")) }
end

context "when the user is ignoring the latest version" do
Expand Down
14 changes: 9 additions & 5 deletions npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -190,16 +190,18 @@ def conflicting_updated_dependencies
end
# rubocop:enable Metrics/AbcSize

# We don't need to update this but need to include it so it's described
# in the PR and we'll pass validation that this dependency is at a
# non-vulnerable version.
# We don't need to directly update the target dependency if it will
# be updated as a side effect of updating the parent. However, we need
# to include it so it's described in the PR and we'll pass validation
# that this dependency is at a non-vulnerable version.
if updated_deps.none? { |dep| dep.name == dependency.name }
target_version = vulnerability_audit["target_version"]
updated_deps << build_updated_dependency(
dependency: dependency,
version: target_version,
previous_version: dependency.version,
removed: target_version.nil?
removed: target_version.nil?,
metadata: { information_only: true } # Instruct updater to not directly update this dependency
)
end

Expand All @@ -223,6 +225,7 @@ def build_updated_dependency(update_details)
removed = update_details.fetch(:removed, false)
version = update_details.fetch(:version).to_s unless removed
previous_version = update_details.fetch(:previous_version)&.to_s
metadata = update_details.fetch(:metadata, {})

Dependency.new(
name: original_dep.name,
Expand All @@ -236,7 +239,8 @@ def build_updated_dependency(update_details)
previous_version: previous_version,
previous_requirements: original_dep.requirements,
package_manager: original_dep.package_manager,
removed: removed
removed: removed,
metadata: metadata
)
end

Expand Down
41 changes: 31 additions & 10 deletions npm_and_yarn/spec/dependabot/npm_and_yarn/update_checker_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1361,6 +1361,22 @@
)
end

# Dependency doesn't consider metadata as part of equality checks
# so this allows us to check that the metadata is updated in tests.
RSpec::Matchers.define :including_metadata do |expected|
match do |actual|
actual == expected && actual.metadata == expected.metadata
end
end

def contain_exactly_including_metadata(*expected)
contain_exactly(*expected.map { |e| including_metadata(e) })
end

def eq_including_metadata(expected_array)
eq(expected_array).and contain_exactly_including_metadata(*expected_array)
end

context "for a security update for a locked transitive dependency" do
let(:dependency_files) { project_dependency_files("npm8/locked_transitive_dependency") }
let(:registry_listing_url) { "https://registry.npmjs.org/locked-transitive-dependency" }
Expand All @@ -1385,14 +1401,15 @@

it "correctly updates the transitive dependency" do
expect(checker.send(:updated_dependencies_after_full_unlock)).
to eq([
to eq_including_metadata([
Dependabot::Dependency.new(
name: "@dependabot-fixtures/npm-transitive-dependency",
version: "1.0.1",
package_manager: "npm_and_yarn",
previous_version: "1.0.0",
requirements: [],
previous_requirements: []
previous_requirements: [],
metadata: { information_only: true }
),
Dependabot::Dependency.new(
name: "@dependabot-fixtures/npm-parent-dependency",
Expand Down Expand Up @@ -1426,14 +1443,15 @@
let(:registry_listing_url) { "https://registry.npmjs.org/transitive-dependency-locked-by-intermediate" }

it "correctly updates the transitive dependency" do
expect(checker.send(:updated_dependencies_after_full_unlock)).to eq([
expect(checker.send(:updated_dependencies_after_full_unlock)).to eq_including_metadata([
Dependabot::Dependency.new(
name: "@dependabot-fixtures/npm-transitive-dependency",
package_manager: "npm_and_yarn",
previous_requirements: [],
previous_version: "1.0.0",
requirements: [],
version: "1.0.1"
version: "1.0.1",
metadata: { information_only: true }
),
Dependabot::Dependency.new(
name: "@dependabot-fixtures/npm-intermediate-dependency",
Expand All @@ -1452,7 +1470,7 @@
let(:registry_listing_url) { "https://registry.npmjs.org/transitive-dependency-locked-by-multiple" }

it "correctly updates the transitive dependency" do
expect(checker.send(:updated_dependencies_after_full_unlock)).to contain_exactly(
expect(checker.send(:updated_dependencies_after_full_unlock)).to contain_exactly_including_metadata(
Dependabot::Dependency.new(
name: "@dependabot-fixtures/npm-parent-dependency",
package_manager: "npm_and_yarn",
Expand Down Expand Up @@ -1531,7 +1549,8 @@
previous_requirements: [],
previous_version: "1.0.0",
requirements: [],
version: "1.0.1"
version: "1.0.1",
metadata: { information_only: true }
)
)
end
Expand All @@ -1547,14 +1566,15 @@
end

it "correctly updates the parent dependency and removes the transitive because removal is enabled" do
expect(checker.send(:updated_dependencies_after_full_unlock)).to contain_exactly(
expect(checker.send(:updated_dependencies_after_full_unlock)).to contain_exactly_including_metadata(
Dependabot::Dependency.new(
name: "@dependabot-fixtures/npm-transitive-dependency",
package_manager: "npm_and_yarn",
previous_requirements: [],
previous_version: "1.0.0",
requirements: [],
removed: true
removed: true,
metadata: { information_only: true }
),
Dependabot::Dependency.new(
name: "@dependabot-fixtures/npm-remove-dependency",
Expand Down Expand Up @@ -1608,14 +1628,15 @@
end

it "correctly updates the transitive dependency by unlocking the parent" do
expect(checker.send(:updated_dependencies_after_full_unlock)).to eq([
expect(checker.send(:updated_dependencies_after_full_unlock)).to eq_including_metadata([
Dependabot::Dependency.new(
name: "@dependabot-fixtures/npm-transitive-dependency-with-more-versions",
package_manager: "npm_and_yarn",
previous_requirements: [],
previous_version: "1.0.0",
requirements: [],
version: "2.0.0"
version: "2.0.0",
metadata: { information_only: true }
),
Dependabot::Dependency.new(
name: "@dependabot-fixtures/npm-parent-dependency-with-more-versions",
Expand Down
7 changes: 4 additions & 3 deletions updater/lib/dependabot/updater.rb
Original file line number Diff line number Diff line change
Expand Up @@ -733,9 +733,10 @@ def generate_dependency_files_for(updated_dependencies)
logger_info("Updating #{dependency_names.join(', ')}")
end

# Removal is only supported for transitive dependencies which are removed as a
# side effect of the parent update
deps_to_update = updated_dependencies.reject(&:removed?)
# Ignore dependencies that are tagged as information_only. These will be
# updated indirectly as a result of a parent dependency update and are
# only included here to be included in the PR info.
deps_to_update = updated_dependencies.reject(&:informational_only?)
updater = file_updater_for(deps_to_update)
updater.updated_dependency_files
end
Expand Down