From 6a9b6910d286bbbe79c9929704429285b6584806 Mon Sep 17 00:00:00 2001 From: James Roberts Date: Thu, 20 Oct 2022 21:32:19 +0000 Subject: [PATCH 1/4] Update version comments for SHA-pinned GitHub Actions GitHub encourages pinning third-party GitHub Actions to a full length commit SHA. It's common for actions pinned by commit SHA to include a comment specifying the version associated with the commit. For example: - uses: actions/checkout@01aecc # v2.1.0 This change updates the GitHub Actions manager to bump versions in comments that follow SHA-pinned actions, so the comment stays up-to-date with the SHA being updated. The file_updater now searches the comment string for all references to the previous version and replaces them with the new version. To avoid changing unrelated comments, the comment updater only updates dependencies that pin SHA refs. --- common/lib/dependabot/git_commit_checker.rb | 4 + .../dependabot/github_actions/file_updater.rb | 26 ++++- .../github_actions/file_updater_spec.rb | 95 +++++++++++++++++++ .../pinned_sources_version_comments.yml | 17 ++++ 4 files changed, 138 insertions(+), 4 deletions(-) create mode 100644 github_actions/spec/fixtures/workflow_files/pinned_sources_version_comments.yml diff --git a/common/lib/dependabot/git_commit_checker.rb b/common/lib/dependabot/git_commit_checker.rb index d91db6c1d3..804a952882 100644 --- a/common/lib/dependabot/git_commit_checker.rb +++ b/common/lib/dependabot/git_commit_checker.rb @@ -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? diff --git a/github_actions/lib/dependabot/github_actions/file_updater.rb b/github_actions/lib/dependabot/github_actions/file_updater.rb index 65372f677b..bbe741733e 100644 --- a/github_actions/lib/dependabot/github_actions/file_updater.rb +++ b/github_actions/lib/dependabot/github_actions/file_updater.rb @@ -65,17 +65,35 @@ 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. updated_content = updated_content. gsub( - /(?<=\W|"|')#{Regexp.escape(old_declaration)}(?=\s|"|'|$)/, - new_declaration - ) + /(?<=\W|"|')#{Regexp.escape(old_declaration)}(?\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 + return unless dependency.previous_version && dependency.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 diff --git a/github_actions/spec/dependabot/github_actions/file_updater_spec.rb b/github_actions/spec/dependabot/github_actions/file_updater_spec.rb index 9262046b52..7f41064aba 100644 --- a/github_actions/spec/dependabot/github_actions/file_updater_spec.rb +++ b/github_actions/spec/dependabot/github_actions/file_updater_spec.rb @@ -293,6 +293,101 @@ expect(subject.content).not_to include "actions/cache@v2.1.2\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/checkout@v2.1.0" } + }], + 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/checkout@v2.2.0" } + }] + ) + 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}/) + + 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 + end end end end diff --git a/github_actions/spec/fixtures/workflow_files/pinned_sources_version_comments.yml b/github_actions/spec/fixtures/workflow_files/pinned_sources_version_comments.yml new file mode 100644 index 0000000000..8e9c2db8e0 --- /dev/null +++ b/github_actions/spec/fixtures/workflow_files/pinned_sources_version_comments.yml @@ -0,0 +1,17 @@ +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 + - uses: actions/checkout@01aecc # v2.1.0 + integration: + - uses: actions/checkout@v2.1.0 # 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. From a459c88271e582ee16618a56a6fa24365b6e2efd Mon Sep 17 00:00:00 2001 From: James Roberts Date: Sat, 29 Oct 2022 22:05:53 +0000 Subject: [PATCH 2/4] Only update comments that end with the version If the version in a comment is followed by additional text, it could be documentation explaining some behavior, rather than a tag corresponding to the SHA in the action version. To be safe, don't update the version unless it's the end of the comment. --- .../lib/dependabot/github_actions/file_updater.rb | 2 ++ .../dependabot/github_actions/file_updater_spec.rb | 6 +++++- .../pinned_sources_version_comments.yml | 13 +++++++++++++ 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/github_actions/lib/dependabot/github_actions/file_updater.rb b/github_actions/lib/dependabot/github_actions/file_updater.rb index bbe741733e..a54635c82f 100644 --- a/github_actions/lib/dependabot/github_actions/file_updater.rb +++ b/github_actions/lib/dependabot/github_actions/file_updater.rb @@ -87,7 +87,9 @@ def updated_workflow_file_content(file) 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)) diff --git a/github_actions/spec/dependabot/github_actions/file_updater_spec.rb b/github_actions/spec/dependabot/github_actions/file_updater_spec.rb index 7f41064aba..30243cb58c 100644 --- a/github_actions/spec/dependabot/github_actions/file_updater_spec.rb +++ b/github_actions/spec/dependabot/github_actions/file_updater_spec.rb @@ -375,7 +375,7 @@ 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}/) + 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}" @@ -387,6 +387,10 @@ 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 diff --git a/github_actions/spec/fixtures/workflow_files/pinned_sources_version_comments.yml b/github_actions/spec/fixtures/workflow_files/pinned_sources_version_comments.yml index 8e9c2db8e0..3128412273 100644 --- a/github_actions/spec/fixtures/workflow_files/pinned_sources_version_comments.yml +++ b/github_actions/spec/fixtures/workflow_files/pinned_sources_version_comments.yml @@ -11,7 +11,20 @@ jobs: - 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 integration: - uses: actions/checkout@v2.1.0 # 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 From 784b2b8bf6c19baf5675740803f13da7b491f25c Mon Sep 17 00:00:00 2001 From: James Roberts Date: Mon, 31 Oct 2022 01:41:47 +0000 Subject: [PATCH 3/4] Update comment --- .../lib/dependabot/github_actions/file_updater.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/github_actions/lib/dependabot/github_actions/file_updater.rb b/github_actions/lib/dependabot/github_actions/file_updater.rb index a54635c82f..451e5dea80 100644 --- a/github_actions/lib/dependabot/github_actions/file_updater.rb +++ b/github_actions/lib/dependabot/github_actions/file_updater.rb @@ -66,8 +66,11 @@ def updated_workflow_file_content(file) # Replace the old declaration that's preceded by a non-word character # 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. + # If the declaration is followed by a comment that lists the version associated + # with the SHA source ref, then update the comment to the human-readable new version. + # However, if the comment includes additional text beyond the version, for safety + # we skip updating the comment in case it's a custom note, todo, warning etc of some kind. + # See the related unit tests for examples. updated_content = updated_content. gsub( From a894ab3b9902b64a4c8853c281dd3129808d0748 Mon Sep 17 00:00:00 2001 From: James Roberts Date: Mon, 31 Oct 2022 01:46:24 +0000 Subject: [PATCH 4/4] Fix and silence linter offenses --- github_actions/lib/dependabot/github_actions/file_updater.rb | 1 + .../spec/dependabot/github_actions/file_updater_spec.rb | 2 ++ 2 files changed, 3 insertions(+) diff --git a/github_actions/lib/dependabot/github_actions/file_updater.rb b/github_actions/lib/dependabot/github_actions/file_updater.rb index 451e5dea80..d6aa9a47ec 100644 --- a/github_actions/lib/dependabot/github_actions/file_updater.rb +++ b/github_actions/lib/dependabot/github_actions/file_updater.rb @@ -90,6 +90,7 @@ def updated_workflow_file_content(file) 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 diff --git a/github_actions/spec/dependabot/github_actions/file_updater_spec.rb b/github_actions/spec/dependabot/github_actions/file_updater_spec.rb index 30243cb58c..a679a3b1c5 100644 --- a/github_actions/spec/dependabot/github_actions/file_updater_spec.rb +++ b/github_actions/spec/dependabot/github_actions/file_updater_spec.rb @@ -388,8 +388,10 @@ expect(subject.content).not_to match(/@#{old_version}\s+#.*#{dependency.version}/) end it "doesn't update version comments in the middle of sentences" do + # rubocop:disable Layout/LineLength 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" + # rubocop:enable Layout/LineLength end end end