Fix gemspec sanitization bug when heredoc has methods chained onto it #3220
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
|
Looks like this is where we try and do the sanitisation: https://github.com/dependabot/dependabot-core/blob/main/bundler/lib/dependabot/bundler/file_updater/gemspec_sanitizer.rb#L237-L242 haven't worked on this code but would be up for spending a short block on trying to fix it. |
Co-authored-by: Philip Harrison <feelepxyz@github.com>
jasonrudolph
commented
Mar 4, 2021
Comment on lines
+258
to
+271
| 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 |
There was a problem hiding this comment.
We (@feelepxyz and I) suspect that there's a more readable way to implement this. We explored a few options, but the other options were a bit harder to follow in our opinion. So, if you have ideas for an alternative implementation that would be easier to read/maintain, definitely let us know. 🙇
|
Just merged #3229 👌
…On Thu, 4 Mar 2021 at 18:07, Jason Rudolph ***@***.***> wrote:
The npm_and_yarn CI job is failing due to a flaky test. That flaky test is
fixed in #3229 <#3229>.
Once #3229 <#3229> is
merged, I'll merge main into this branch, and that should give us a green
build.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3220 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAE5RPBL55W6UFK6VXY42DTB7D7ZANCNFSM4YPT2FFA>
.
|
…specs-with-heredocs-with-method-chains
xlgmokha
approved these changes
Mar 5, 2021
xlgmokha
deleted the
gracefully-handle-gemspecs-with-heredocs-with-method-chains
branch
This was referenced Mar 9, 2021
This was referenced Mar 16, 2021
This was referenced Mar 17, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When dependabot is processing a gemspec, it performs some sanitization of the gemspec's content. In most situations, it handles a heredoc by replacing the heredoc's content as seen in this test.
Given a gemspec like this:
The sanitized output should look like this:
That's the current behavior, and it meets our needs. However, if the heredoc has methods chained onto it, the sanitzation currently produces invalid Ruby code.
Given a gemspec like this:
We should get the same sanitized output that we got in the previous example, but we currently get the following output instead:
Later on in the dependabot-core workflow, when we ask bundler to evaluate the sanitized gemspec, it fails with a parser error because the output above is invalid Ruby code.
TODO