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.

Sign up for GitHub

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

Disallow pushing gems with unresolved deps #5344

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7ab8241
Disallow pushing of gems with unresolved dependencies
Kuanchiliao1 Dec 18, 2024
0bc7b90
Modify existing integration test and add model test
Kuanchiliao1 Dec 18, 2024
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
View file
Open in desktop
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 &&
Copy link
Member

Choose a reason for hiding this comment

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

what do you think of the idea of adding an on: :create validation to the Dependency model?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@segiddins Just took a look - Could be off but my understanding is that the on create validation in Dependency would have to prevent the push if the name is unresolved. It seems like for other contexts(not pushing), there is a validation already called use_existing_rubygem but it just sets the unresolved_name of the dependency if its not found. If we were to go this route, would it make sense to add a condition into this validation to check whether a gem is being pushed rather than creating a new validation?

I'm leaning towards having it in Pusher because it checks all dependencies in one query and can fail early before creating Dependency objects, which seems more efficient than adding per-dependency validations in the context of pushing. I could totally be missing something though - what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

which seems more efficient than adding per-dependency validations in the context of pushing

We're already doing the query though to find the rubygem in Dependency#use_existing_rubygem. Adding a validation on: :create wouldn't require any additional queries. Version#update_dependencies! is already doing the lookup one-by-one

Since use_existing_rubygem is a before_validation hook, I think we would want a new validation, so it becomes easy to scope to on: :create, but writing one that gives an error message saying which gem name isn't resolved seems both easy and a good idea

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@segiddins
Ah I see, I think I was mistaken in thinking that the additional Dependency validation would add a query for each dependency. If I'm understanding correctly now, you're saying the new validation wouldn't create extra queries since it would use the results already found by use_existing_rubygem?

Also wanted to circle back to my point about early validation in Pusher. Say a gem being pushed has 30 dependencies and the last one is invalid, was thinking the validation in Dependency would do something like this:

1. Create 30 Dependency objects
2. Run use_existing_rubygem 30 times (30 queries)
3. Fail on the last validation

While the Pusher validation would go something like this:

1. Run one query to check for and return all missing dependencies
2. Fail early since last one is invalid
3. Skip creating any Dependency objects since validation fails (no additional queries)

What are your thoughts on this aspect? I do agree the validation fits more naturally in the Dependency model, but it seems like the early check could save some work. I'm not sure how significant the savings would be in practice though, perhaps a dependency typo is rare event not worth worrying about

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's significant enough in practice, because we have to do all that work anyways when there isn't a typo

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
View file
Open in desktop
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
View file
Open in desktop
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