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

Fix GitHub actions updater crashing and cloning the wrong repository #6052

Merged
merged 4 commits into from
Nov 11, 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
14 changes: 7 additions & 7 deletions common/lib/dependabot/git_commit_checker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,11 @@ def pinned_ref_looks_like_commit_sha?
ref_looks_like_commit_sha?(ref)
end

def head_commit_for_pinned_ref
ref = dependency_source_details.fetch(:ref)
local_repo_git_metadata_fetcher.head_commit_for_ref_sha(ref)
end

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

Expand All @@ -85,13 +90,8 @@ def branch_or_ref_in_release?(version)
def head_commit_for_current_branch
ref = ref_or_branch || "HEAD"

if pinned?
return dependency.version ||
local_repo_git_metadata_fetcher.head_commit_for_ref(ref)
end

sha = local_repo_git_metadata_fetcher.head_commit_for_ref(ref)
return sha if sha
sha = head_commit_for_local_branch(ref)
return sha if pinned? || sha

raise Dependabot::GitDependencyReferenceNotFound, dependency.name
end
Expand Down
6 changes: 6 additions & 0 deletions common/lib/dependabot/git_metadata_fetcher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,12 @@ def head_commit_for_ref(ref)
commit_sha
end

def head_commit_for_ref_sha(ref)
refs_for_upload_pack.
find { |r| r.ref_sha == ref }&.
commit_sha
end

private

attr_reader :url, :credentials
Expand Down
41 changes: 21 additions & 20 deletions common/spec/dependabot/git_commit_checker_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -510,31 +510,32 @@
ref: "v1.0.0"
}
end
it { is_expected.to eq(dependency.version) }

context "without a version" do
let(:version) { nil }
let(:git_header) do
{ "content-type" => "application/x-git-upload-pack-advertisement" }
end
let(:auth_header) { "Basic eC1hY2Nlc3MtdG9rZW46dG9rZW4=" }

let(:git_header) do
{ "content-type" => "application/x-git-upload-pack-advertisement" }
end
let(:auth_header) { "Basic eC1hY2Nlc3MtdG9rZW46dG9rZW4=" }
let(:git_url) do
"https://github.com/gocardless/business.git" \
"/info/refs?service=git-upload-pack"
end

let(:git_url) do
"https://github.com/gocardless/business.git" \
"/info/refs?service=git-upload-pack"
context "that can be reached just fine" do
before do
stub_request(:get, git_url).
with(headers: { "Authorization" => auth_header }).
to_return(
status: 200,
body: fixture("git", "upload_packs", "business"),
headers: git_header
)
end

context "that can be reached just fine" do
before do
stub_request(:get, git_url).
with(headers: { "Authorization" => auth_header }).
to_return(
status: 200,
body: fixture("git", "upload_packs", "business"),
headers: git_header
)
end
it { is_expected.to eq(dependency.version) }

context "without a version" do
let(:version) { nil }

it { is_expected.to eq("df9f605d7111b6814fe493cf8f41de3f9f0978b2") }

Expand Down
3 changes: 0 additions & 3 deletions github_actions/lib/dependabot/github_actions.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,3 @@
require "dependabot/dependency"
Dependabot::Dependency.
register_production_check("github_actions", ->(_) { true })

require "dependabot/utils"
Dependabot::Utils.register_always_clone("github_actions")
35 changes: 25 additions & 10 deletions github_actions/lib/dependabot/github_actions/update_checker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -83,12 +83,27 @@ def fetch_latest_version_for_git_dependency
end

def latest_commit_for_pinned_ref
@latest_commit_for_pinned_ref ||=
SharedHelpers.in_a_temporary_repo_directory("/", repo_contents_path) do
ref_branch = find_container_branch(current_commit)
@latest_commit_for_pinned_ref ||= begin
head_commit_for_ref_sha = git_commit_checker.head_commit_for_pinned_ref
if head_commit_for_ref_sha
head_commit_for_ref_sha
else
url = dependency_source_details[:url]
source = Source.from_url(url)

git_commit_checker.head_commit_for_local_branch(ref_branch)
SharedHelpers.in_a_temporary_directory(File.dirname(source.repo)) do |temp_dir|
repo_contents_path = File.join(temp_dir, File.basename(source.repo))

SharedHelpers.run_shell_command("git clone --no-recurse-submodules #{url} #{repo_contents_path}")

Dir.chdir(repo_contents_path) do
ref_branch = find_container_branch(dependency_source_details[:ref])

git_commit_checker.head_commit_for_local_branch(ref_branch)
end
end
end
end
end

def latest_version_tag
Expand Down Expand Up @@ -184,17 +199,17 @@ def shortened_semver_eq?(base, other)
end

def find_container_branch(sha)
SharedHelpers.run_shell_command("git fetch #{current_commit}")

branches_including_ref = SharedHelpers.run_shell_command("git branch --contains #{sha}").split("\n")
branches_including_ref = SharedHelpers.run_shell_command(
"git branch --remotes --contains #{sha}"
).split("\n").map { |branch| branch.strip.gsub("origin/", "") }

current_branch = branches_including_ref.find { |line| line.start_with?("* ") }
current_branch = branches_including_ref.find { |branch| branch.start_with?("HEAD -> ") }

if current_branch
current_branch.delete_prefix("* ")
current_branch.delete_prefix("HEAD -> ")
elsif branches_including_ref.size > 1
# If there are multiple non default branches including the pinned SHA, then it's unclear how we should proceed
raise "Multiple ambiguous branches (#{branches_including_ref.join(', ')}) include #{current_commit}!"
raise "Multiple ambiguous branches (#{branches_including_ref.join(', ')}) include #{sha}!"
else
branches_including_ref.first
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,67 @@
end
end

context "given a realworld repository", :vcr do
let(:dependency) do
Dependabot::Dependency.new(
name: dependency_name,
version: dependency_version,
requirements: [{
requirement: nil,
groups: [],
file: ".github/workflows/main.yml",
source: dependency_source
}],
package_manager: "github_actions"
)
end
let(:dependency_name) { "dependabot-fixtures/github-action-push-to-another-repository" }
let(:dependency_version) { nil }
let(:dependency_source) do
{
type: "git",
url: "https://github.com/dependabot-fixtures/github-action-push-to-another-repository",
ref: reference,
branch: nil
}
end

let(:latest_commit_in_main) { "9e487f29582587eeb4837c0552c886bb0644b6b9" }
let(:latest_commit_in_devel) { "c7563454dd4fbe0ea69095188860a62a19658a04" }

context "when pinned to an up to date commit in the default branch" do
let(:reference) { latest_commit_in_main }

it "returns the expected value" do
expect(subject).to eq(latest_commit_in_main)
end
end

context "when pinned to an out of date commit in the default branch" do
let(:reference) { "f4b9c90516ad3bdcfdc6f4fcf8ba937d0bd40465" }

it "returns the expected value" do
expect(subject).to eq(latest_commit_in_main)
end
end

context "when pinned to an up to date commit in a non default branch" do
let(:reference) { latest_commit_in_devel }

it "returns the expected value" do
expect(subject).to eq(latest_commit_in_devel)
end
end

context "when pinned to an out of date commit in a non default branch" do
let(:reference) { "96e7dec17bbeed08477b9edab6c3a573614b829d" }

it "returns the expected value" do
expect(subject).to eq(latest_commit_in_devel)
end
end
end

context "that is a git commit SHA not pointing to the tip of a branch" do
let(:reference) { "1c24df3" }
let(:exit_status) { double(success?: true) }
Expand All @@ -401,14 +462,18 @@
allow(git_commit_checker).to receive(:branch_or_ref_in_release?).and_return(false)
allow(git_commit_checker).to receive(:head_commit_for_current_branch).and_return(reference)

allow(Open3).to receive(:capture2e).with(anything, "git fetch #{reference}").and_return(["", exit_status])
allow(Dir).to receive(:chdir).and_yield

allow(Open3).to receive(:capture2e).
with(anything, %r{git clone --no-recurse-submodules https://github\.com/actions/setup-node}).
and_return(["", exit_status])
end

context "and it's in the current (default) branch" do
before do
allow(Open3).to receive(:capture2e).
with(anything, "git branch --contains #{reference}").
and_return(["* master\n", exit_status])
with(anything, "git branch --remotes --contains #{reference}").
and_return([" origin/HEAD -> origin/master\n origin/master", exit_status])
end

it "can update to the latest version" do
Expand All @@ -421,8 +486,8 @@

before do
allow(Open3).to receive(:capture2e).
with(anything, "git branch --contains #{reference}").
and_return(["releases/v1\n", exit_status])
with(anything, "git branch --remotes --contains #{reference}").
and_return([" origin/releases/v1\n", exit_status])
end

it "can update to the latest version" do
Expand All @@ -433,8 +498,8 @@
context "and multiple branches include it, the current (default) branch among them" do
before do
allow(Open3).to receive(:capture2e).
with(anything, "git branch --contains #{reference}").
and_return(["* master\nv1.1\n", exit_status])
with(anything, "git branch --remotes --contains #{reference}").
and_return([" origin/HEAD -> origin/master\n origin/master\n origin/v1.1\n", exit_status])
end

it "can update to the latest version" do
Expand All @@ -445,8 +510,8 @@
context "and multiple branches include it, the current (default) branch NOT among them" do
before do
allow(Open3).to receive(:capture2e).
with(anything, "git branch --contains #{reference}").
and_return(["3.3-stable\nproduction\n", exit_status])
with(anything, "git branch --remotes --contains #{reference}").
and_return([" origin/3.3-stable\n origin/production\n", exit_status])
end

it "raises an error" do
Expand Down
Loading