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

Disallow pushing gems with unresolved deps #5344

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
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
15 changes: 15 additions & 0 deletions app/models/pusher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ def process
authorize &&
verify_gem_scope &&
verify_mfa_requirement &&
verify_dependencies_resolvable &&
validate &&
save
end
Expand All @@ -46,6 +47,20 @@ def verify_mfa_requirement
notify("Rubygem requires owners to enable MFA. You must enable MFA before pushing new version.", 403)
end

def verify_dependencies_resolvable
return true if spec.dependencies.empty?

dependency_names = spec.dependencies.map(&:name)
existing_gems = Rubygem.where(name: dependency_names).pluck(:name)
missing_gems = dependency_names - existing_gems

if missing_gems.any?
return notify("There was a problem saving your gem: \nThe following dependencies don't exist: #{missing_gems.join(', ')}", 422)
end

true
end

def validate
unless validate_signature_exists?
return notify("There was a problem saving your gem: \nYou have added cert_chain in gemspec but signature was empty", 403)
Expand Down
8 changes: 1 addition & 7 deletions test/integration/push_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -236,13 +236,7 @@ class PushTest < ActionDispatch::IntegrationTest

push_gem "sandworm-1.0.0.gem"

assert_response :success

get rubygem_path("sandworm")

assert_response :success
assert page.has_content?("mauddib")
assert page.has_content?("> 1")
assert_response :unprocessable_entity
end

test "pushing a gem with a new platform for the same version" do
Expand Down
32 changes: 30 additions & 2 deletions test/models/pusher_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ class PusherTest < ActiveSupport::TestCase
@cutter.stubs(:authorize).returns true
@cutter.stubs(:verify_mfa_requirement).returns true
@cutter.stubs(:verify_gem_scope).returns true
@cutter.stubs(:verify_dependencies_resolvable).returns true
@cutter.stubs(:validate).returns true
@cutter.stubs(:verify_sigstore).returns true
@cutter.stubs(:sign_sigstore).returns true
Expand Down Expand Up @@ -100,12 +101,26 @@ class PusherTest < ActiveSupport::TestCase
@cutter.process
end

should "not attempt to validate if mfa check failed" do
should "not attempt to verify dependencies resolve if mfa check failed" do
@cutter.stubs(:pull_spec).returns true
@cutter.stubs(:find).returns true
@cutter.stubs(:authorize).returns true
@cutter.stubs(:verify_gem_scope).returns true
@cutter.stubs(:verify_mfa_requirement).returns false
@cutter.stubs(:verify_dependencies_resolvable).never
@cutter.stubs(:validate).never
@cutter.stubs(:save).never

@cutter.process
end

should "not attempt to validate if dependencies don't resolve" do
@cutter.stubs(:pull_spec).returns true
@cutter.stubs(:find).returns true
@cutter.stubs(:authorize).returns true
@cutter.stubs(:verify_gem_scope).returns true
@cutter.stubs(:verify_mfa_requirement).returns true
@cutter.stubs(:verify_dependencies_resolvable).returns false
@cutter.stubs(:validate).never
@cutter.stubs(:save).never

Expand All @@ -118,6 +133,7 @@ class PusherTest < ActiveSupport::TestCase
@cutter.stubs(:authorize).returns true
@cutter.stubs(:verify_gem_scope).returns true
@cutter.stubs(:verify_mfa_requirement).returns true
@cutter.stubs(:verify_dependencies_resolvable).returns true
@cutter.stubs(:validate).returns false
@cutter.stubs(:save).never

Expand Down Expand Up @@ -202,7 +218,7 @@ class PusherTest < ActiveSupport::TestCase
.instance_variable_set(:@requirements, [["!!!", "0"]])
end)
@cutter.stubs(:validate_signature_exists?).returns(true)

@cutter.stubs(:verify_dependencies_resolvable).returns true
@cutter.process

assert_match(/requirements must be list of valid requirements/, @cutter.message)
Expand All @@ -214,13 +230,25 @@ class PusherTest < ActiveSupport::TestCase
s.add_runtime_dependency "\nother"
end)
@cutter.stubs(:validate_signature_exists?).returns(true)
@cutter.stubs(:verify_dependencies_resolvable).returns true

@cutter.process

assert_match(/Dependency unresolved name can only include letters, numbers, dashes, and underscores/, @cutter.message)
assert_equal 403, @cutter.code
end

should "not be able to save a gem if the dependencies are not resolvable" do
@cutter.stubs(:spec).returns(new_gemspec("gem-with-nonexistent-dep", "1.0.0", "Summary", "ruby") do |s|
s.add_runtime_dependency "non-existent-gem", "~> 1.0"
end)

@cutter.process

assert_match(/There was a problem saving your gem: \nThe following dependencies don't exist: non-existent-gem/, @cutter.message)
assert_equal 422, @cutter.code
end

should "not be able to save a gem if the metadata has incorrect values" do
@cutter.stubs(:spec).returns(new_gemspec("bad-metadata", "1.0.0", "Summary", "ruby") do |s|
s.metadata["foo"] = []
Expand Down
Loading