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

Update version comments for SHA-pinned GitHub Actions #5951

Merged
Merged
Show file tree
Hide file tree
Changes from 2 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
4 changes: 4 additions & 0 deletions common/lib/dependabot/git_commit_checker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ def pinned_ref_looks_like_version?

def pinned_ref_looks_like_commit_sha?
ref = dependency_source_details.fetch(:ref)
ref_looks_like_commit_sha?(ref)
end

def ref_looks_like_commit_sha?(ref)
return false unless ref&.match?(/^[0-9a-f]{6,40}$/)

return false unless pinned?
Expand Down
28 changes: 24 additions & 4 deletions github_actions/lib/dependabot/github_actions/file_updater.rb
Original file line number Diff line number Diff line change
Expand Up @@ -65,17 +65,37 @@ def updated_workflow_file_content(file)
gsub(/@.*+/, "@#{new_req.fetch(:source).fetch(:ref)}")

# Replace the old declaration that's preceded by a non-word character
# and followed by a whitespace character (comments) or EOL
# and followed by a whitespace character (comments) or EOL.
# If the declaration is followed by a comment, attempt to update
# any version comments associated with SHA source refs.
jeffwidman marked this conversation as resolved.
Show resolved Hide resolved
updated_content =
updated_content.
gsub(
/(?<=\W|"|')#{Regexp.escape(old_declaration)}(?=\s|"|'|$)/,
new_declaration
)
/(?<=\W|"|')#{Regexp.escape(old_declaration)}(?<comment>\s+#.*)?(?=\s|"|'|$)/
) do |match|
comment = Regexp.last_match(:comment)
match.gsub!(old_declaration, new_declaration)
if comment && (updated_comment = updated_version_comment(comment, new_req))
match.gsub!(comment, updated_comment)
end
match
end
end

updated_content
end

def updated_version_comment(comment, new_req)
raise "No comment!" unless comment
comment = comment.rstrip
return unless dependency.previous_version && dependency.version
return unless comment.end_with? dependency.previous_version

git_checker = Dependabot::GitCommitChecker.new(dependency: dependency, credentials: credentials)
return unless git_checker.ref_looks_like_commit_sha?(new_req.fetch(:source).fetch(:ref))

comment.gsub(dependency.previous_version, dependency.version)
end
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,105 @@
expect(subject.content).not_to include "actions/[email protected]\n"
end
end

context "with pinned SHA hash and version in comment" do
let(:service_pack_url) do
"https://github.com/actions/checkout.git/info/refs" \
"?service=git-upload-pack"
end
before do
stub_request(:get, service_pack_url).
to_return(
status: 200,
body: fixture("git", "upload_packs", "checkout"),
headers: {
"content-type" => "application/x-git-upload-pack-advertisement"
}
)
end

let(:workflow_file_body) do
fixture("workflow_files", "pinned_sources_version_comments.yml")
end
let(:dependency) do
Dependabot::Dependency.new(
name: "actions/checkout",
version: "2.2.0",
package_manager: "github_actions",
previous_version: "2.1.0",
previous_requirements: [{
requirement: nil,
groups: [],
file: ".github/workflows/workflow.yml",
source: {
type: "git",
url: "https://github.com/actions/checkout",
ref: "01aecccf739ca6ff86c0539fbc67a7a5007bbc81",
branch: nil
},
metadata: { declaration_string: "actions/checkout@01aecccf739ca6ff86c0539fbc67a7a5007bbc81" }
}, {
requirement: nil,
groups: [],
file: ".github/workflows/workflow.yml",
source: {
type: "git",
url: "https://github.com/actions/checkout",
ref: "v2.1.0",
branch: nil
},
metadata: { declaration_string: "actions/[email protected]" }
}],
requirements: [{
requirement: nil,
groups: [],
file: ".github/workflows/workflow.yml",
source: {
type: "git",
url: "https://github.com/actions/checkout",
ref: "aabbfeb2ce60b5bd82389903509092c4648a9713",
branch: nil
},
metadata: { declaration_string: "actions/checkout@aabbfeb2ce60b5bd82389903509092c4648a9713" }
}, {
requirement: nil,
groups: [],
file: ".github/workflows/workflow.yml",
source: {
type: "git",
url: "https://github.com/actions/checkout",
ref: "v2.2.0",
branch: nil
},
metadata: { declaration_string: "actions/[email protected]" }
}]
)
end

it "updates SHA version" do
old_sha = dependency.previous_requirements.first.dig(:source, :ref)
expect(subject.content).to include "#{dependency.name}@#{dependency.requirements.first.dig(:source, :ref)}"
expect(subject.content).not_to match(/#{old_sha}\s+#.*#{dependency.previous_version}/)
end
it "updates version comment" do
new_sha = dependency.requirements.first.dig(:source, :ref)
expect(subject.content).not_to match(/@#{new_sha}\s+#.*#{dependency.previous_version}\s*$/)

expect(subject.content).to include "# v#{dependency.version}"
expect(subject.content).to include "# #{dependency.version}"
expect(subject.content).to include "# @v#{dependency.version}"
expect(subject.content).to include "# pin @v#{dependency.version}"
expect(subject.content).to include "# tag=v#{dependency.version}"
end
it "doesn't update version comments when @ref is not a SHA" do
old_version = dependency.previous_requirements[1].dig(:source, :ref)
expect(subject.content).not_to match(/@#{old_version}\s+#.*#{dependency.version}/)
end
it "doesn't update version comments in the middle of sentences" do
expect(subject.content).to include "Versions older than v#{dependency.previous_version} have a security vulnerability"
expect(subject.content).not_to include "Versions older than v#{dependency.version} have a security vulnerability"
end
end
end
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
on: [push]

name: Integration
jobs:
chore:
steps:
- uses: actions/checkout@01aecccf739ca6ff86c0539fbc67a7a5007bbc81 # v2.1.0
- uses: actions/checkout@01aecccf739ca6ff86c0539fbc67a7a5007bbc81 # 2.1.0
- uses: actions/checkout@01aecccf739ca6ff86c0539fbc67a7a5007bbc81 # @v2.1.0
- uses: actions/checkout@01aecccf739ca6ff86c0539fbc67a7a5007bbc81 # pin @v2.1.0
- uses: actions/checkout@01aecccf739ca6ff86c0539fbc67a7a5007bbc81 # tag=v2.1.0
- uses: actions/checkout@01aecccf739ca6ff86c0539fbc67a7a5007bbc81 # v2.1.0
- uses: actions/checkout@01aecccf739ca6ff86c0539fbc67a7a5007bbc81 #v2.1.0
# The comment on the next line has a trailing tab. The version should still be updated.
- uses: actions/checkout@01aecccf739ca6ff86c0539fbc67a7a5007bbc81 #v2.1.0
- uses: actions/checkout@01aecc # v2.1.0
Copy link
Member

Choose a reason for hiding this comment

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

Major 💖 for phenom set of test cases.

Two more cases:

  1. What about the case where a comment includes the version, but then some text?
- uses: actions/checkout@01aecccf739ca6ff86c0539fbc67a7a5007bbc81 # Versions older than v2.1.0 have a security vulnerability
  1. What if the version string is repeated twice?
- uses: actions/checkout@01aecccf739ca6ff86c0539fbc67a7a5007bbc81 # v2.1.0 - Versions older than v2.1.0 have a security vulnerability

I don't want to throw up roadblocks on this awesome PR, so if for simplicity neither case is bumped, that's perfectly acceptable. So one simple heuristic could be if there's any text after the version string, then don't bump the version string... Users that still want it bumped can instead leave their comment as a full-line comment above the actual code line:

# Versions older than v2.1.0 have a security vulnerability
- uses: actions/checkout@01aecccf739ca6ff86c0539fbc67a7a5007bbc81 # v2.1.0

But we do need to ensure we never auto-bump the comment to start incorrectly saying "Versions older than v2.2.0 have a security vulnerability"...

So can you add these two test cases to ensure no future regressions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback. I was worried the simple search-and-replace was too broad but I couldn't come up with a sample comment where we wouldn't want to bump the version. The cases you mentioned are exactly what I was looking for!

I've updated the PR with the simple fix to only change comments if there's no text after the version string.

integration:
- uses: actions/[email protected] # comments that include the version (v2.1.0) shouldn't be updated for non-SHA refs
- uses: actions/checkout@01aecc#v2.1.0 # this shouldn't be updated, because the version is part of the ref, not a comment.

# The version in the comment for the next action shouldn't be updated
# because it refers to past behavior.
- uses: actions/checkout@01aecccf739ca6ff86c0539fbc67a7a5007bbc81 # Versions older than v2.1.0 have a security vulnerability

# The versions in the comment for the next action won't be updated.
# The first version could be updated, but it's difficult to create
# a heuristic that recognizes the first version as a version alias
# for the SHA commit, and the second version as a concrete version
# that shouldn't change. For simplicity, we don't update either.
- uses: actions/checkout@01aecccf739ca6ff86c0539fbc67a7a5007bbc81 # v2.1.0 - Versions older than v2.1.0 have a security vulnerability