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

Raise DependencyFileUnsupported error when API returns matching 400 error #11438

Merged
merged 3 commits into from
Jan 30, 2025
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
7 changes: 7 additions & 0 deletions common/lib/dependabot/errors.rb
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,11 @@ def self.updater_error_details(error)
"file-path": error.file_path
}
}
when Dependabot::DependencyFileNotSupported
{
"error-type": "dependency_file_not_supported",
"error-detail": { message: error.message }
}
when Dependabot::GitDependenciesNotReachable
{
"error-type": "git_dependencies_not_reachable",
Expand Down Expand Up @@ -616,6 +621,8 @@ class DependencyFileNotEvaluatable < DependabotError; end

class DependencyFileNotResolvable < DependabotError; end

class DependencyFileNotSupported < DependabotError; end

class BadRequirementError < Gem::Requirement::BadRequirementError; end

#######################
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@
it "returns the correct package manager" do
expect(package_manager.name).to eq "devcontainers"
expect(package_manager.requirement).to be_nil
expect(package_manager.version.to_s).to eq "0.72.0"
expect(package_manager.version.to_s).to match(/\d+.\d+.\d+/)
end
end

Expand All @@ -244,7 +244,7 @@
it "returns the correct language" do
expect(language.name).to eq "node"
expect(language.requirement).to be_nil
expect(language.version.to_s).to eq "18.20.6"
expect(language.version.to_s).to match(/\d+.\d+.\d+/)
end
end
end
Expand Down
21 changes: 19 additions & 2 deletions updater/lib/dependabot/api_client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
require "dependabot/job"
require "dependabot/opentelemetry"
require "sorbet-runtime"
require "dependabot/errors"

# Provides a client to access the internal Dependabot Service's API
#
Expand All @@ -18,10 +19,11 @@
module Dependabot
class ApiError < StandardError; end

class ApiClient
class ApiClient # rubocop:disable Metrics/ClassLength
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, we could satisfy the cop if we extracted some logic out into it's own method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the class length so we would have to extract a mixin but then extracting a mixin just for this would probably not be worth it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should really make an effort to adhere by the suggestions from rubocop rather than disable them. Especially for a core class that is shared across ecosystems.

extend T::Sig

MAX_REQUEST_RETRIES = 3
INVALID_REQUEST_MSG = /The request contains invalid or unauthorized changes/
Copy link
Member

Choose a reason for hiding this comment

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

do you know why we do this?

Copy link
Contributor Author

@amazimbe amazimbe Jan 29, 2025

Choose a reason for hiding this comment

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

To quote the API code, it's because we are trying to update GitHub Actions workflow files when our package manager is not github_actions or the dependency file is not one of the files specified in the allowed list .

It was added as a fix to prevent any potential security issues raised in issue

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. Adding @honeyankit as a reviewer.


sig { params(base_url: String, job_id: T.any(String, Integer), job_token: String).void }
def initialize(base_url, job_id, job_token)
Expand All @@ -41,7 +43,12 @@ def create_pull_request(dependency_change, base_commit_sha)
api_url = "#{base_url}/update_jobs/#{job_id}/create_pull_request"
data = create_pull_request_data(dependency_change, base_commit_sha)
response = http_client.post(api_url, json: { data: data })
raise ApiError, response.body if response.code >= 400

if response.code >= 400 && dependency_file_not_supported_error?(response.body.to_s)
raise Dependabot::DependencyFileNotSupported, response.body.to_s
elsif response.code >= 400
raise ApiError, response.body
end
rescue HTTP::ConnectionError, OpenSSL::SSL::SSLError
retry_count ||= 0
retry_count += 1
Expand Down Expand Up @@ -416,5 +423,15 @@ def create_pull_request_data(dependency_change, base_commit_sha)
data["pr-body"] = dependency_change.pr_message.pr_message
data
end

sig { params(response: String).returns(T::Boolean) }
def dependency_file_not_supported_error?(response)
body = JSON.parse(response)

return false unless body.is_a?(Hash)
return false unless body["errors"]

INVALID_REQUEST_MSG.match? body["errors"].first["detail"]
end
end
end
22 changes: 22 additions & 0 deletions updater/spec/dependabot/api_client_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,28 @@
end)
end
end

context "when API returns a 400 Bad Request" do
let(:body) do
<<~ERROR
{ "errors": [{
"status": 400,
"title": "Bad Request",
"detail": "The request contains invalid or unauthorized changes"}]
}
ERROR
end

before do
stub_request(:post, create_pull_request_url).to_return(status: 400, body: body)
end

it "raises the correct error" do
expect do
client.create_pull_request(dependency_change, base_commit)
end.to raise_error(Dependabot::DependencyFileNotSupported)
end
end
end

describe "update_pull_request" do
Expand Down
Loading