From e25a21c558ba3c52d497f3536fa66ea15451511e Mon Sep 17 00:00:00 2001 From: Jason Rudolph Date: Tue, 2 Mar 2021 14:29:24 -0500 Subject: [PATCH 1/2] Add a failing test to demonstrate gemspec sanitization bug Given a gemspec like this: Spec.new do |s| s.version = "0.1.0" s.post_install_message = <<~DESCRIPTION.strip.downcase My description DESCRIPTION end The sanitized output should look like this: Spec.new do |s| s.version = "0.1.0" "sanitized" end But it actually looks like this: Spec.new do |s| s.version = "0.1.0" "sanitized" My description DESCRIPTION end I'll look into a fix for this bug in a follow-up commit. --- .../file_updater/gemspec_sanitizer_spec.rb | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/bundler/spec/dependabot/bundler/file_updater/gemspec_sanitizer_spec.rb b/bundler/spec/dependabot/bundler/file_updater/gemspec_sanitizer_spec.rb index 2c68420de5..6e3324b081 100644 --- a/bundler/spec/dependabot/bundler/file_updater/gemspec_sanitizer_spec.rb +++ b/bundler/spec/dependabot/bundler/file_updater/gemspec_sanitizer_spec.rb @@ -136,6 +136,23 @@ ) end end + + context "that uses a heredoc with methods chained onto it" do + let(:content) do + %(Spec.new do |s| + s.version = "0.1.0" + s.post_install_message = <<~DESCRIPTION.strip.downcase + My description + DESCRIPTION + end) + end + it "removes the whole heredoc" do + expect(rewrite).to eq( + "Spec.new do |s|\n s.version = \"0.1.0\""\ + "\n \"sanitized\"\n end" + ) + end + end end describe "version assignment" do From 8101b774c566525faffc9532c5383eea01a0ef54 Mon Sep 17 00:00:00 2001 From: Jason Rudolph Date: Thu, 4 Mar 2021 10:55:50 -0500 Subject: [PATCH 2/2] Fix gemspec sanitization bug when heredoc has methods chained onto it Co-authored-by: Philip Harrison --- .../bundler/file_updater/gemspec_sanitizer.rb | 31 ++++++++++++++++--- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/bundler/lib/dependabot/bundler/file_updater/gemspec_sanitizer.rb b/bundler/lib/dependabot/bundler/file_updater/gemspec_sanitizer.rb index bb9a6915a8..5852247421 100644 --- a/bundler/lib/dependabot/bundler/file_updater/gemspec_sanitizer.rb +++ b/bundler/lib/dependabot/bundler/file_updater/gemspec_sanitizer.rb @@ -234,11 +234,8 @@ def node_calls_find_dot_find?(node) def remove_unnecessary_assignments(node) return unless node.is_a?(Parser::AST::Node) - if unnecessary_assignment?(node) && - node.children.last&.location.respond_to?(:heredoc_end) - range_to_remove = node.loc.expression.join( - node.children.last.location.heredoc_end - ) + if unnecessary_assignment?(node) && node_includes_heredoc?(node) + range_to_remove = node.loc.expression.join(find_heredoc_end_range(node)) return replace(range_to_remove, '"sanitized"') elsif unnecessary_assignment?(node) return replace(node.loc.expression, '"sanitized"') @@ -249,6 +246,30 @@ def remove_unnecessary_assignments(node) end end + def node_includes_heredoc?(node) + find_heredoc_end_range(node) + end + + # Performs a depth-first search for the first heredoc in the given + # Parser::AST::Node. + # + # Returns a Parser::Source::Range identifying the location of the end + # of the heredoc, or nil if no heredoc was found. + def find_heredoc_end_range(node) + return unless node.is_a?(Parser::AST::Node) + + node.children.each do |child| + next unless child.is_a?(Parser::AST::Node) + + return child.location.heredoc_end if child.location.respond_to?(:heredoc_end) + + range = find_heredoc_end_range(child) + return range if range + end + + nil + end + def unnecessary_assignment?(node) return false unless node.is_a?(Parser::AST::Node) return false unless node.children.first.is_a?(Parser::AST::Node)