Dependabot for Ruby libraries should behave just like for dependabot for applications

deivid-rodriguez commented 2 years ago

Sorry for the long ticket, sometimes I feel I am too verbose, but I tried my best to justify my proposal

Context

I commit Gemfile.lock files to source control when developing Ruby libraries. This is somewhat controversial and not widely adopted, but it's currently the official recommendation of the Bundler team, and other package managers also agree (https://classic.yarnpkg.com/blog/2016/11/24/lockfiles-for-all/).

Anyways, that currently means that I get security advisories and dependabot PRs to warn me about vulnerabilities in my Gemfile.lock files and to update them.

However, sometimes dependabot is unable to create PRs for some of these security advisories, because of some special behavior it has for libraries.

The special behavior is that if it's updating a Gemfile.lock file inside a library, dependabot will parse the required_ruby_version field in the gemspec file, and use the lower bound for it as the required ruby version for the resolution. That means that sometimes it will not be able to update a Gemfile.lock file to a secure version because the secure version no longer supports the oldest Ruby version supported by the library.

An example of this is https://github.com/formtastic/formtastic/security/dependabot/15:

Captura de Pantalla 2022-04-12 a las 12 42 08

Dependabot is only able to update to nokogiri-1.12.5, because it adds a requirement on "Ruby 2.5" to the resolution (the gemspec includes required_ruby_version >= 2.5), and 1.12.5 is the newest nokogiri version that still supports Ruby 2.5.

Why it currently works like this?

This feature was added a very long time ago with https://github.com/dependabot/dependabot-core/commit/9d08fc2df2c05873fb20445596b9bd83542993c3. I think the reason is the following:

As you probably know, Gemfile and Gemfile.lock files in libraries are not used at all by end users of the libraries, it's the Gemfile and Gemfile.lock files in end applications using the library that get considered. The only thing actually meaningful for end users are the dependency requirements in the gemspec file of the library.
Gemfile and Gemfile.lock files are important though for developers of the library, since they usually determine the exact set of dependencies and their versions the library is tested against. It's normally impractical to test a library against all possible combinations of its dependencies, so Gemfile and Gemfile.lock files help choosing what's worth testing against.
I think adding this extra requirement on the Ruby version is intended to help making sure that the set of dependencies the library is tested against is compatible with the oldest Ruby version supported by the library. And I think that's intended to avoid introducing changes that unintentionally drop support for old Rubies.

Why I think it should be changed?

I think there are a few reasons to change this:

Simplicity. We can get rid of a lot of "library specific code". Code that's also very dependent on Bundler internals and likely to have issues, like the one fixed by https://github.com/dependabot/dependabot-core/pull/4820.
Consistency. Currently dependabot does this, but only "sometimes". For example, if it finds any issues caused by the extra ruby requirement, it will retry without it. This was introduced by https://github.com/dependabot/dependabot-core/commit/fcd56828cc6b82a54d5184f9f04851d78a99d96c.
Not necessary most of the times. Most libraries test against all major Rubies they support, and if they have to choose, they definitely test against the oldest supported Ruby. So even if a PR created by dependabot due to this change made the library incompatible with that Ruby version, it will be cached by CI.
Clarity. As can be seen in the above screenshot, it's currently very hard to figure out why Dependabot could not update the dependency, because this extra requirement on the Ruby version is added "silently".

And most importantly

Security. Security issues are most important for production environments, but can also affect developer environments. By leaving Gemfile.lock files out there with insecure versions of gems, we leave room for those issues hitting developer environments.

If the above makes sense, I'm happy to create a PR for this.

Thanks for reading!

jurre commented 2 years ago

Thanks for the detailed issue @deivid-rodriguez! Overall I think you make a good point, but I am wondering if we don't end up breaking users setups when doing this, if someone is using the lower bound ruby version as specified in the Gemfile, and dependabot now performs this update, all of a sudden we've broken their development environment in a way they wouldn't have if they'd performed the update manually.

I wonder if we should instead update the ruby requirement in the Gemfile in this case, but it might also be confusing and not what users want 🤔

All in all, I am not sure what the right thing to do is, pinging other folks on the team to gather their thoughts.

jurre commented 2 years ago

One improvement we could make if we do decide to stick with the current approach is to attempt explaining why that's the latest version that could be installed. We already do this for cases when there's a transitive dependency conflict, and might be able to do the same thing here. Not saying that this is the right thing to do, just dumping my thoughts

brrygrdn commented 2 years ago

I think there are a few reasons to change this

@deivid-rodriguez I agree with the rationale, especially Simplicity/Consistency as I think those are general improvement targets we should aim for 👍🏻

I am wondering if we don't end up breaking users setups when doing this

This is a concern I share, I think there are two aspects to separate out:

What is the objectively correct - or least surprising - behaviour for Dependabot as a library?
Assuming we do make a change, how do we ship a behavioural change like this to the service?

I think @jurre suggestion above that we attempt to explain that the latest version is pinned by the gemspec is optimised for solving the second and is likely something we can deliver more easily.

If we were to optimise for (1) by changing this behaviour I think we'd need to look at:

How frequently do we encounter this kind of pinning in the wild? If it's a sufficiently small percentage we might have more appetite to just change it?
If we were to change the behaviour, is there anything we should add to the PR to make it obvious we have ignored required_ruby_version?

brrygrdn commented 2 years ago

🤔 We could isolate the old/new behaviour behind a configuration value and graduate the improved behaviour as optional at first and then deprecate it by flipping the configuration flag to an opt in to the old behaviour

jurre commented 2 years ago

Yeah good call on how to go about rolling out such a change @brrygrdn

🤔 We could isolate the old/new behaviour behind a configuration value and graduate the improved behaviour as optional at first and then deprecate it by flipping the configuration flag to an opt in to the old behaviour

Would really love to avoid this sort of configuration option as it's overly specific and I'd hope we can instead identify the Right Thing To Do™

deivid-rodriguez commented 2 years ago

Hei, thanks for your responses!

Just to clarify the current and proposed behavior.

The required_ruby_version in the gemspec won't be ignored, it will still be respected, before and after this proposal.
If there's a ruby directive in the Gemfile, it will still be respected, before and after this proposal.

The difference between the current and proposed behavior is that if you have, say, required_ruby_version = ">= 2.5.0" in your gemspec file, and no ruby directive in your Gemfile. dependabot currently adds the following on top of your Gemfile

ruby "~> 2.5.0"

So, dependabot is currently forcing the resolution to be valid on Ruby 2.5, while with the new behavior, dependabot would find a resolution valid on some Ruby >= 2.5.0.

Could this proposal break some developer setup? I guess, yeah. If a developer of a gem is developing using the oldest ruby the gem supports, and have Gemfile (and possibly Gemfile.lock) files reflecting that, dependabot could create a PR that breaks that user's setup. But only if the PR is merged, and I think the great majority of the gems out there test in CI against the oldest Ruby they support. So I think it's an assumable risk, and in any case it might help the gem author realize that her CI is missing some coverage.

jurre commented 2 years ago

Thanks David, I guess my main concern is users getting upset that Dependabot is opening a PR that breaks the assumptions of their setup, when it could have figured out the "right" thing to do (although "right" is dubious in this case, I totally see your point).

I'm worried that if users have a setup like this, but they don't have CI set up for their oldest ruby version and they now release an incompatible version for the ruby version they're supposedly supporting, they might end up being frustrated with Dependabot's behavior.

I wonder if in this case we should bump the required_ruby_version directive in the users gemspec to a range that is valid together with the update? This might be complex, but at least we're being clear about what accepting the updated PR means in that case?

I'm not necessarily convinced that what I'm saying is "right" here, I'm mostly just thinking out loud to make sure we end up making a well thought-out decision, I really appreciate the perspective you're bringing!

deivid-rodriguez commented 2 years ago

I totally understand your concern @jurre.

The idea of bumping the required_ruby_version directive in the gemspec is interesting, but it seems to me that would be mixing two separate concerns in the same PR. Dropping support for an old ruby is one thing, bump the version of a dependency seems something unrelated. Mainly because the former actually affect end users using the dropped Ruby (who won't be able to use future releases), while the latter only affects developers of the gem.

To be honest, what bothers me the most about the current behavior is that dependabot creates a security alert but it's not able to offer a solution nor to properly explain what went wrong. I think improving the error message to explain that Dependabot tried to upgrade the dependency using the oldest supported Ruby, and that was not possible, and to recommend to drop the old Ruby in order to let dependabot upgrade the dependency would go a long way.

jurre commented 2 years ago

The idea of bumping the required_ruby_version directive in the gemspec is interesting, but it seems to me that would be mixing two separate concerns in the same PR. Dropping support for an old ruby is one thing, bump the version of a dependency seems something unrelated. Mainly because the former actually affect end users using the dropped Ruby (who won't be able to use future releases), while the latter only affects developers of the gem.

That's totally fair, yeah, although bumping to a version of a dependency that doesn't support their version of ruby also effects them in the same way, right?

This is probably what I would do manually when running into this situation though

To be honest, what bothers me the most about the current behavior is that dependabot creates a security alert but it's not able to offer a solution nor to properly explain what went wrong. I think improving the error message to explain that Dependabot tried to upgrade the dependency using the oldest supported Ruby, and that was not possible, and to recommend to drop the old Ruby in order to let dependabot upgrade the dependency would go a long way.

Yeah this is a much bigger problem, we've been able to tackle explaining some failed updates, but this one might be a good one to tackle

deivid-rodriguez commented 2 years ago

That's totally fair, yeah, although bumping to a version of a dependency that doesn't support their version of ruby also effects them in the same way, right?

Only if the gemspec is changed to not allow the previous version. If dependabot proposes changes only to Gemfile and/or Gemfile.lock files, those won't affect end users no matter what. And even if it changes the gemspec, that only results in support drop if the versioning strategy has been changed from the default, since widen, which is the default for Ruby libraries, does not result in bumping the lower bound for the dependency.

Now that I think about it, the current implementation does make sense to prevent the situation you're mentioning: avoid a dependency being bumped in the gemspec, causing a implicit support drop of the oldest Ruby. Perhaps it would be acceptable to accept my proposal for "development updates" (those that don't affect end users), but keep the current behavior in other situations.

schinery commented 2 years ago

Don't know if this is relevant to this conversation or a separate issue but I have two private GitHub package (rubygems.pkg.github.com) gems, both having spec.required_ruby_version = ">= 3.1" in their .gemspec.

One of the gems has ruby "3.1.1" defined in its Gemfile and works as expected when any new gems are found that need updating. However, the other gem doesn't have the Ruby version defined in the gem file, and when it finds any gems to update it throws a Bundler found conflicting requirements for the Ruby version error.

updater | ERROR <job_343082656> Error processing nokogiri (Dependabot::SharedHelpers::HelperSubprocessFailed)
updater | ERROR <job_343082656> Bundler found conflicting requirements for the Ruby version:
updater | <job_343082656>   In Gemfile:
updater | <job_343082656>     my_gem was resolved to 4.63.1, which depends on
updater | <job_343082656>       Ruby (>= 3.1)
updater | <job_343082656> 
updater | <job_343082656>     pry-byebug (~> 3.9) was resolved to 3.9.0, which depends on
updater | <job_343082656>       byebug (~> 11.0) was resolved to 11.1.3, which depends on
updater | <job_343082656>         Ruby (>= 2.4.0)
updater | <job_343082656> 
updater | <job_343082656>     faker (~> 2.20) was resolved to 2.20.0, which depends on
updater | <job_343082656>       Ruby (>= 2.5)
updater | <job_343082656> 
updater | <job_343082656>     i18n-tasks (~> 1.0) was resolved to 1.0.9, which depends on
updater | <job_343082656>       Ruby (< 4.0, >= 2.6)
updater | <job_343082656> 
updater | <job_343082656>     net-ftp (~> 0.1) was resolved to 0.1.3, which depends on
updater | <job_343082656>       Ruby (>= 2.3.0)
updater | <job_343082656> 
updater | <job_343082656>     pry-rails (~> 0.3) was resolved to 0.3.9, which depends on
updater | <job_343082656>       Ruby (>= 1.9.1)
updater | <job_343082656> 
updater | <job_343082656>     rspec-rails (~> 5.1) was resolved to 5.1.1, which depends on
updater | <job_343082656>       Ruby (>= 2.2.0)
updater | <job_343082656> 
updater | <job_343082656>     my_rubocop (= 0.0.2) was resolved to 0.0.2, which depends on
updater | <job_343082656>       Ruby (>= 2.7)
updater | <job_343082656> 
updater | <job_343082656>     m (~> 1.5) was resolved to 1.6.0, which depends on
updater | <job_343082656>       Ruby (>= 1.9)
updater | <job_343082656> 
updater | <job_343082656>     minitest-macos-notification (~> 1.0) was resolved to 1.0.1, which depends on
updater | <job_343082656>       minitest (~> 5.14) was resolved to 5.15.0, which depends on
updater | <job_343082656>         Ruby (< 4.0, >= 2.2)
updater | <job_343082656> 
updater | <job_343082656>     minitest-reporters (~> 1.5) was resolved to 1.5.0, which depends on
updater | <job_343082656>       Ruby (>= 1.9.3)
updater | <job_343082656> 
updater | <job_343082656>     mocha (~> 1.11) was resolved to 1.13.0, which depends on
updater | <job_343082656>       Ruby (>= 1.8.7)
updater | <job_343082656> 
updater | <job_343082656>     spy (~> 1.0) was resolved to 1.0.2, which depends on
updater | <job_343082656>       Ruby (>= 2.1.0)
updater | <job_343082656> 
updater | <job_343082656>     with_model (~> 2.1) was resolved to 2.1.6, which depends on
updater | <job_343082656>       Ruby (>= 2.6)
updater | <job_343082656> 
updater | <job_343082656>     nokogiri (= 1.13.4) was resolved to 1.13.4, which depends on
updater | <job_343082656>       Ruby (< 3.2.dev, >= 2.6)
updater | <job_343082656> 
updater | <job_343082656>     nokogiri (= 1.13.4) was resolved to 1.13.4, which depends on
updater | <job_343082656>       Ruby (< 3.2.dev, >= 2.6)
updater | <job_343082656> 
updater | <job_343082656>     nokogiri (= 1.13.4) was resolved to 1.13.4, which depends on
updater | <job_343082656>       Ruby (< 3.2.dev, >= 2.6)
updater | <job_343082656> 
updater | <job_343082656>     Ruby
updater | <job_343082656> 
updater | <job_343082656> Bundler could not find compatible versions for gem "my_gem":
updater | <job_343082656>   In snapshot (Gemfile.lock):
updater | <job_343082656>     my_gem (= 4.63.1)
updater | <job_343082656> 
updater | <job_343082656>   In Gemfile:
updater | <job_343082656>     my_gem
updater | <job_343082656> 
updater | <job_343082656> Running `bundle update` will rebuild your snapshot from scratch, using only
updater | <job_343082656> the gems in your Gemfile, which may resolve the conflict.
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/vendor/ruby/2.7.0/gems/dependabot-common-0.180.5/lib/dependabot/shared_helpers.rb:121:in `run_helper_subprocess'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/vendor/ruby/2.7.0/gems/dependabot-bundler-0.180.5/lib/dependabot/bundler/native_helpers.rb:44:in `block in run_bundler_subprocess'
updater | ERROR <job_343082656> /usr/lib/ruby/gems/2.7.0/gems/bundler-2.3.10/lib/bundler.rb:382:in `block in with_original_env'
updater | ERROR <job_343082656> /usr/lib/ruby/gems/2.7.0/gems/bundler-2.3.10/lib/bundler.rb:698:in `with_env'
updater | ERROR <job_343082656> /usr/lib/ruby/gems/2.7.0/gems/bundler-2.3.10/lib/bundler.rb:382:in `with_original_env'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/vendor/ruby/2.7.0/gems/dependabot-bundler-0.180.5/lib/dependabot/bundler/native_helpers.rb:40:in `run_bundler_subprocess'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/vendor/ruby/2.7.0/gems/dependabot-bundler-0.180.5/lib/dependabot/bundler/file_updater/lockfile_updater.rb:68:in `block in build_updated_lockfile'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/vendor/ruby/2.7.0/gems/dependabot-common-0.180.5/lib/dependabot/shared_helpers.rb:48:in `block in in_a_temporary_directory'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/vendor/ruby/2.7.0/gems/dependabot-common-0.180.5/lib/dependabot/shared_helpers.rb:48:in `chdir'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/vendor/ruby/2.7.0/gems/dependabot-common-0.180.5/lib/dependabot/shared_helpers.rb:48:in `in_a_temporary_directory'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/vendor/ruby/2.7.0/gems/dependabot-common-0.180.5/lib/dependabot/shared_helpers.rb:37:in `in_a_temporary_repo_directory'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/vendor/ruby/2.7.0/gems/dependabot-bundler-0.180.5/lib/dependabot/bundler/file_updater/lockfile_updater.rb:62:in `build_updated_lockfile'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/vendor/ruby/2.7.0/gems/dependabot-bundler-0.180.5/lib/dependabot/bundler/file_updater/lockfile_updater.rb:46:in `updated_lockfile_content'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/vendor/ruby/2.7.0/gems/dependabot-bundler-0.180.5/lib/dependabot/bundler/file_updater.rb:157:in `updated_lockfile_content'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/vendor/ruby/2.7.0/gems/dependabot-bundler-0.180.5/lib/dependabot/bundler/file_updater.rb:41:in `updated_dependency_files'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/lib/dependabot/updater.rb:678:in `generate_dependency_files_for'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/lib/dependabot/updater.rb:274:in `check_and_create_pull_request'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/lib/dependabot/updater.rb:82:in `check_and_create_pr_with_error_handling'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/lib/dependabot/updater.rb:56:in `block in run'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/lib/dependabot/updater.rb:56:in `each'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/lib/dependabot/updater.rb:56:in `run'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/lib/dependabot/update_files_job.rb:17:in `perform_job'
updater | ERROR <job_343082656> /home/dependabot/dependabot-updater/lib/dependabot/base_job.rb:35:in `run'
updater | ERROR <job_343082656> bin/update_files.rb:22:in `<main>'

Again, I don't know if this is relevant and am happy to open a separate issue for this if it is not. I've also added ruby "3.1.1" to the gem that didn't have it to check if it is that which is causing the issue, just waiting for another gem bump to confirm.

Also worth noting the the gem without Ruby defined in the Gemfile did work at some point, and don't think anything dramatic has changed with it recently.

deivid-rodriguez commented 2 years ago

Interesting @schinery! It seems definitely related and I think it's worth looking into it and fixing your issue before proceeding with anything here. It might shed some light and what the best/right/expected behavior is.

One guess would be a conflict between your gem requirements (Ruby >= 3.1) and the Ruby version run by Dependabot internally (2.7)? Dependabot definitely has some solution for that since the gem with ruby in the Gemfile is working, but maybe it's not getting applied when there's no ruby directive.

Also, since you said this only stopped working recently, I wonder if #4820 affected this somehow...

schinery commented 2 years ago

I think it's worth looking into it and fixing your issue before proceeding with anything here

@deivid-rodriguez I can confirm that adding ruby "3.1.1" to the Gemfile does appear to resolve my issue.

I just pushed a patch version of a public gem the problematic gem depends on and triggered a "Check for updates" in GitHub and it has generated a PR for that gem update.

deivid-rodriguez commented 2 years ago

@schinery I've been investigating this and I have some findings.

Regarding your issue, I'm pretty sure this is a regression of 2462acb73e223fb2fdcf4a8b8cd4b0ad463d4773. I think reverting that commit and applying a fix similar to e9a759b0db817034277d65d4bbeea5f082214004 on top of it might fix the issue.

While investigating your issue, I realized that the behavior I'm proposing to remove here might actually be doing more than I thought. Currently Bundler can only resolve against the running Ruby version, because it adds an implicit requirement on it and passes that to the resolver. So if there's a gem with required_ruby_version incompatible with the running Ruby version, Bundler generates a resolution error. This is what's happening to you: Dependabot uses Ruby 2.7.5 for resolving, but your gem specifies required_ruby_version >= 3.1. So why does Dependabot work when you add "ruby 3.1.1" to your Gemfile? I think the key is this monkey patch to Bundler:

https://github.com/dependabot/dependabot-core/blob/2df7590acf54bb38aadc6fc14d7bbe36840c81f6/bundler/helpers/v2/monkey_patches/definition_ruby_version_patch.rb

The monkey patch means that if the Gemfile includes the ruby directive, a Ruby version matching that directive will be added to the list of Rubies available for resolution (in addition to the one being run). So I think that would explain why adding ruby "3.1.1" to the Gemfile fixes your issue.

Assuming this is what's going on, if we were to accept my proposal to remove this thing, we would need to adapt the monkey patch to add the minimum required_ruby_version to the list of Rubies available for resolution instead.

Something slightly surprising is that this kind of issue is not coming up more for applications without the ruby directive in the Gemfile. But I guess most application do add that, and even if they don't, since Dependabot's Ruby (2.7) is reasonably up to date, it does not seem frequent to find dependencies out there have already dropped Ruby 2.7 support 🤷.

deivid-rodriguez commented 2 years ago

Also, quick update related to this, I'm breaking the mentioned monkey patch in https://github.com/rubygems/rubygems/pull/5472, so it will need to be adapted when upgrading to Bundler 2.3.12. https://github.com/dependabot/dependabot-core/blob/2df7590acf54bb38aadc6fc14d7bbe36840c81f6/bundler/helpers/v2/monkey_patches/definition_ruby_version_patch.rb#L8

should be changed to

requested_version = ruby_version.gem_version

deivid-rodriguez commented 2 years ago

Just another thought about this. Even if we don't remove this feature, there's another simple situation where it can be "skipped'. Let's take the original error I presented here:

In this situation, the locked version of nokogiri was already 1.13.3, but dependabot is saying that 1.12.5 is a far as it can go. If this situation happens, we can clearly retry without the ruby Gemfile directive, since the Gemfile already does not support the lowest required_ruby_version, so there's nothing to be broken that was not already broken.

schinery commented 2 years ago

Something slightly surprising is that this kind of issue is not coming up more for applications without the ruby directive in the Gemfile. But I guess most application do add that, and even if they don't, since Dependabot's Ruby (2.7) is reasonably up to date, it does not seem frequent to find dependencies out there have already dropped Ruby 2.7 support 🤷.

Yeah I suspect the >= 3.1 fails will be pretty edge case. The private gems are set to use the same version of Ruby as their parent apps as that is all the gems are used by (everyone wants to use that new Hash short hand syntax!). I have some public gems that are set to >= 2.7 that don't have the Ruby version defined in the Gemfile which generate PRs fine using the Dependabot default.

deivid-rodriguez commented 2 years ago

@schinery I reproduced your issue locally and confirmed that reverting https://github.com/dependabot/dependabot-core/commit/2462acb73e223fb2fdcf4a8b8cd4b0ad463d4773 and applying a fix similar to https://github.com/dependabot/dependabot-core/commit/e9a759b0db817034277d65d4bbeea5f082214004 on top of it fixes the issue.

I will work on a spec and try to provide a PR.

deivid-rodriguez commented 2 years ago

Done at #5019!

Next thing I'll try to address is https://github.com/dependabot/dependabot-core/issues/4987#issuecomment-1100589437.

github-actions[bot] commented 6 months ago

👋 This issue has been marked as stale because it has been open for 2 years with no activity. You can comment on the issue to hold stalebot off for a while, or do nothing. If you do nothing, this issue will be closed eventually by the stalebot. Please see CONTRIBUTING.md for more policy details.

dependabot / dependabot-core