🚨 [security] Update rails 7.1.6 → 8.0.2.1 (major) #560
Conversation
depfu
bot
force-pushed
the
depfu/update/group/rails-8.0.2.1
branch
from
615e103 to
9cbb15f
Compare
mayorova
force-pushed
the
depfu/update/group/rails-8.0.2.1
branch
from
b3589df to
53c5665
Compare
depfu
bot
force-pushed
the
depfu/update/group/rails-8.0.2.1
branch
from
53c5665 to
1f70b27
Compare
depfu
bot
force-pushed
the
depfu/update/group/rails-8.0.2.1
branch
from
1f70b27 to
b58c09a
Compare
depfu
bot
changed the title
depfu
bot
force-pushed
the
depfu/update/group/rails-8.0.2.1
branch
from
b58c09a to
511ad57
Compare
bin/dev
Outdated
There was a problem hiding this comment.
I don't think we need it. It was created automatically by rails app:update
jlledom
force-pushed
the
depfu/update/group/rails-8.0.2.1
branch
2 times, most recently
from
ef7f8d7 to
e89c22d
Compare
|
Jira issue for this upgrade: https://issues.redhat.com/browse/THREESCALE-12186 I followed the migration guides for 7.2 and 8.0 and went through the whole list of deprecations for both updates. This is what I did:
I also tested locally, Both normal and TLS DB:
I comment further details in each file. About the rails defaults, I think we should set |
| end | ||
|
|
||
| gem 'rails', '~> 7.1.6' | ||
| gem "mutex_m", "~> 0.3.0" |
There was a problem hiding this comment.
Fix a deprecation warning:
/home/jlledom/.asdf/installs/ruby/3.3.1/lib/ruby/gems/3.3.0/gems/bootsnap-1.16.0/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:32: warning: /home/jlledom/.asdf/installs/ruby/3.3.1/lib/ruby/3.3.0/mutex_m.rb was loaded from the standard library, but will no longer be part of the default gems since Ruby 3.4.0. Add mutex_m to your Gemfile or gemspec.
|
|
||
| gem 'rails', '~> 7.1.6' | ||
| gem "mutex_m", "~> 0.3.0" | ||
| gem "csv", "~> 3.3" |
There was a problem hiding this comment.
Deprecation warning:
/home/jlledom/.asdf/installs/ruby/3.3.1/lib/ruby/gems/3.3.0/gems/bootsnap-1.16.0/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:32: warning: /home/jlledom/.asdf/installs/ruby/3.3.1/lib/ruby/3.3.0/csv.rb was loaded from the standard library, but will no longer be part of the default gems since Ruby 3.4.0. Add csv to your Gemfile or gemspec. Also contact author of license_finder-7.0.1 to add csv into its gemspec.
| has_many :integration_states, dependent: :destroy | ||
|
|
||
| enum state: %i[active disabled].index_with{ |status| status.to_s } | ||
| enum :state, %i[active disabled].index_with{ |status| status.to_s } |
| # Append comments with runtime information tags to SQL queries in logs. | ||
| config.active_record.query_log_tags_enabled = true |
There was a problem hiding this comment.
This was added by app:update
config/environments/production.rb
Outdated
| # Prevent health checks from clogging up the logs. | ||
| config.silence_healthcheck_path = "/up" |
There was a problem hiding this comment.
Added by the update task. Any idea which is our health check path?
config/environments/production.rb
Outdated
| # Only use :id for inspections in production. | ||
| config.active_record.attributes_for_inspect = [ :id ] |
There was a problem hiding this comment.
Added by app:update. It doesn't harm I guess.
| # Raise error when a before_action's only/except options reference missing actions. | ||
| config.action_controller.raise_on_missing_callback_actions = true | ||
|
|
||
| config.active_job.queue_adapter = :test |
There was a problem hiding this comment.
This is required from now on:
| # See the ActiveSupport::ParameterFilter documentation for supported notations and behaviors. | ||
| Rails.application.config.filter_parameters += [ | ||
| :password, :access_token | ||
| :password, :access_token, :passw, :email, :secret, :token, :_key, :crypt, :salt, :certificate, :otp, :ssn, :cvv, :cvc |
There was a problem hiding this comment.
Added by the update task, I think it's fine.
| # backends that use the same database as Active Record as a queue, hence they | ||
| # don't need this feature. | ||
| #++ | ||
| # Rails.application.config.active_job.enqueue_after_transaction_commit = :default |
There was a problem hiding this comment.
This one is pretty confusing: enqueue_after_transaction_commit. It tries to fix the scenario when job is enqueued inside a transaction but the transaction was rollbacked. In that scenario the job would actually run and lead to undefined behavior. It's confusing because Rails maintainers changed opinion about this several times:
- 7.2: Added, and took 3 values:
:always.:neverand:default. - 8.0: Deprecated, and took 2 values:
trueandfalse - 8.1: Removed
- 8.2: Added again, taking boolean values, and
trueby default.
8.2 Changelog: https://github.com/rails/rails/blob/6f39910d26eb590cb214a0fce5858fe0d7ddfff8/activejob/CHANGELOG.md?plain=1#L48-L58
The setting only makes sense when the queue is not stored in the same DB ActiveRecord uses. For instance the Sidekiq scenario using Redis. But Que stores the queue in the same DB as ActiveRecord so the changes in the queue are committed or rollbacked along with other changes in the transaction.
I think this doesn't affect us, so leaving it unset.
| # This is possible due to broad browser support for WebP, but older browsers and email clients may still not support | ||
| # WebP. Requires imagemagick/libvips built with WebP support. | ||
| #++ | ||
| # Rails.application.config.active_storage.web_image_content_types = %w[image/png image/jpeg image/gif image/webp] |
There was a problem hiding this comment.
Not relevant, we don't have assets in Zync, right?
| # Applications with existing timestamped migrations that do not adhere to the | ||
| # expected format can disable validation by setting this config to `false`. | ||
| #++ | ||
| Rails.application.config.active_record.validate_migration_timestamps = true |
There was a problem hiding this comment.
It doesn't seem we'll face the scenario this tries to protect from, right?
| # | ||
| # This query used to return a `String`. | ||
| #++ | ||
| Rails.application.config.active_record.postgresql_adapter_decode_dates = true |
There was a problem hiding this comment.
We don't use the affected syntax in our codebase AFAIK
| # Enables YJIT as of Ruby 3.3, to bring sizeable performance improvements. If you are | ||
| # deploying to a memory constrained environment you may want to set this to `false`. | ||
| #++ | ||
| Rails.application.config.yjit = true |
There was a problem hiding this comment.
Apparently this will get us 15-20% performance boost if enabled, it requires Ruby 3.3 which we use.
| # If set to `:offset`, `to_time` methods will use the UTC offset. | ||
| # If `false`, `to_time` methods will convert to the local system UTC offset instead. | ||
| #++ | ||
| Rails.application.config.active_support.to_time_preserves_timezone = :zone |
There was a problem hiding this comment.
This won't affect us I think. Anyway, we still see the deprecation warning about this because the warning is shown before this initializer runs. So the proper way to set this value would be setting load_defaults to 8.0.
| # only consider `If-None-Match` as specified by RFC 7232 Section 6. | ||
| # If set to `false` both conditions need to be satisfied. | ||
| #++ | ||
| Rails.application.config.action_dispatch.strict_freshness = true |
There was a problem hiding this comment.
I don't think we return 304 from zync, right? This should have any effect. Anyway, if the setting forces following an RFC, then it's good I think.
| ### | ||
| # Set `Regexp.timeout` to `1`s by default to improve security over Regexp Denial-of-Service attacks. | ||
| #++ | ||
| Regexp.timeout = 1 |
There was a problem hiding this comment.
Not Rails class, but I agree on setting this.
|
|
||
| execute do |connection| | ||
| connection.select_all(relation.to_sql) | ||
| connection.select_all(relation) |
There was a problem hiding this comment.
For some reason, to_sql doesn't bind values in Rails 8, which generates SQL conditions like error_count = $1. This fixes it.
| def with_que_adapter(&block) | ||
| ApplicationJob.stub(:queue_adapter, ActiveJob::QueueAdapters::QueAdapter.new, &block) | ||
| end | ||
|
|
There was a problem hiding this comment.
All tests run now using the default :test queue adapter I set in the environments/test.rb file. However, this particular test suite actually requires using the Que adapter, Claude suggested this.
| class ModelTest < ActiveSupport::TestCase | ||
| self.use_transactional_tests = false | ||
|
|
||
| def test_weak_lock |
There was a problem hiding this comment.
The original idea for this test was simulating a race condition and ensure the corresponding exception is raised. But this test had a few problems:
- Instead of simulating different threads belonging to different requests, it used Fibers within a single thread.
- It was using a stub that didn't work because the stub was set only when the fiber was created, but remove just before the fiber starts to run.
- The test tries to simulate an unfinished transaction, but the whole test itself ran inside a parent transaction.
- It relied on stubbing some rails internals, but those internals changed in Deprecate
ConnectionPool#connectionrails/rails#51230
I rewrote it in a way it more closely reproduces reality.
|
Before even reviewing the PR, @jlledom, a question - is it actually safe to jump straight from 7.1 to 8.0 ? |
Maybe I should have questioned that also before starting to work on it. But Depfu did the skip and I took it without question. I don't know, everything seems to work for me, zync is a small project and we have QE. WDYT? |
I also kind of forgot that there was a 7.2 in-between 🙃 But I think it would make sense to first upgrade to 7.2, and then to 8.0. It's probably OK to do it within the same product release. But for the sake of sanity, we probably should split them in 2 PRs? 🤔 Unless @akostadinov has a different opinion. |
jlledom
force-pushed
the
depfu/update/group/rails-8.0.2.1
branch
from
e89c22d to
5857eab
Compare
|
|
||
| # Build connection URL with SSL parameters from database config | ||
| # Workaround for https://github.com/que-rb/que/issues/442 | ||
| db_config = ActiveRecord::Base.connection_db_config.configuration_hash | ||
| connection_url = Que::DBConnectionURL.build_connection_url(db_config) | ||
|
|
||
| Que.locker = Que::Locker.new(connection_url: connection_url, **Rails.application.config.x.que) |
There was a problem hiding this comment.
This is not directly related to Rails 8, but rather #573.
It's just I tried to run the tests against an SSL protected DB and found them failing because of this.
There was a problem hiding this comment.
Hm, not sure I understand. Does it mean that #573 didn't fix the issue completely?
Or it did work with the old Rails, but after upgrade some extra code is now needed?
There was a problem hiding this comment.
The issue was solved in #573. This change is only to run the test suite against an SSL database. It only affects tests.
Not related to Rails 8, the rails upgrade has no effect in SSL connections. It's just I was testing things locally and found the suite not working over SSL databases, and I thought: why not just push this to the current branch I'm working on, which happens to be Rails 8, rather than having to open a new PR? And I told myself it was a good idea.
There was a problem hiding this comment.
Still not getting it 🙃
How does it "only affects tests", if it's actually changing the initializer code? 🤔
Also, if you run this suite against master - do the tests also fail without the change? If they do - then I'd say #573 did not in fact fix the issue completely.
Or am I missing something?
Overall, if this would be also needed for master for whatever test to pass, I would split it into another PR.
There was a problem hiding this comment.
How does it "only affects tests", if it's actually changing the initializer code? 🤔
It's in an initializer, but it's a method defined by us, not part of Que gem. I don't know why it's defined in the initializer, but shrug. The thing is that method is currently only called for tests.
This is the sequence:
- Que is installed: 2a5f898
- The method is defined in the intializer
- It's called from
config.rufor production - And from
test_helper.rbfor tests
- Que is updated: daae239
- Stops being called from
config.ru, instead, it's called from a callback defined in the initializer
- Stops being called from
- Que moved to it's own rake task: https://github.com/3scale/zync/pull/181/changes
- My guess is, at the beginning Que was started by rails but after this PR it started to be a separated rake task.
- The PR stops calling the method from the rails callback, so the only remaining call is from
test_helper.rb - 150faa7
So the only usage left for this method is from tests. In #573 I fixed SSL for actual que execution but not for tests. Now this change fixes it.
Also, if you run this suite against master - do the tests also fail without the change? If they do - then I'd say #573 did not in fact fix the issue completely.
The tests work in master... unless you run the tests against an SSL protected DB:
$ DATABASE_SSL_CA=/home/jlledom/3scale-services/redis-configs/certs/ca-root-cert.pem DATABASE_SSL_CERT=/home/jlledom/3scale-services/redis-configs/certs/redis-client.crt DATABASE_SSL_KEY=/home/jlledom/3scale-services/redis-configs/certs/redis-client.key DATABASE_SSL_MODE=verify-full DATABASE_URL=postgresql://porta:@localhost:25432 RAILS_ENV=test bundle exec rails test
connection to server at "::1", port 25432 failed: FATAL: connection requires a valid client certificate (PG::ConnectionBad)
So basically, que is initialized differently for two scenarios:
- Rake taks
- Tests
In #573 I only fixed the rake task. Now the test scenario is also available to read ssl params.
There was a problem hiding this comment.
Thank you for the explanation! I understand now.
So, Que.start! and Que.stop! method is not the actual Que interface - maybe at some point (original versions) it was, but now it's only used for tests.
So, I see that previously que was actually being started as bundle exec que - which basically executes the gem's Que::CommandLineInterface directly.
Then it was changed to bundle exec rake que - which then calls the que executable anyway, so the rake task is just a wrapper that adds some extra parameters to the executable.
But bundle exec que is still a valid command that works. However, as que by default doesn't support TLS for DB, I guess adding the correct config to the rake task is correct and works for production (as our pods execute the rake task).
There was a problem hiding this comment.
Maybe the question is can we call it in tests in the same way as tasks? Or define this custom thing in the test_helper or somewhere. If not too hard.
|
@mayorova @akostadinov I rebased this PR onto master after merging Rails 7.2 |
| @@ -1,5 +1,3 @@ | |||
| require "active_support/core_ext/integer/time" | |||
There was a problem hiding this comment.
Actually a good question, I guess the rake app removed this and I didn't notice. Let me investigate, but we should probably revert this change.
There was a problem hiding this comment.
Got it, this was to make 1.hour.to_i work. But it works anyway after removing the require, so I guess it was not needed.
| # Specifies the number of `workers` to boot in clustered mode. | ||
| # Workers are forked web server processes. If using threads and workers together | ||
| # the concurrency of the application would be max `threads` * `workers`. | ||
| # Workers do not work on JRuby or Windows (both of which do not support | ||
| # processes). | ||
| # | ||
| # workers ENV.fetch("WEB_CONCURRENCY") { 2 } | ||
|
|
||
| # Use the `preload_app!` method when specifying a `workers` number. | ||
| # This directive tells Puma to first boot the application and load code | ||
| # before forking the application. This takes advantage of Copy On Write | ||
| # process behavior so workers use less memory. | ||
| # | ||
| # preload_app! |
There was a problem hiding this comment.
any insight of why the above is not needed and how do we set workers ENV.fetch("WEB_CONCURRENCY") { 2 }, was this unsupported or something?
| db_config = ActiveRecord::Base.connection_db_config.configuration_hash | ||
| connection_url = Que::DBConnectionURL.build_connection_url(db_config) | ||
|
|
||
| Que.locker = Que::Locker.new(connection_url: connection_url, **Rails.application.config.x.que) |
There was a problem hiding this comment.
I still think this deserves its own PR, as it doesn't have anything to do with this Rails upgrade 🙃
Also, we might need to evaluate whether we actually need:
Que.start!andQue.stop!- maybe we can just add something likeQue.locker = Que::Locker.newdirectly in the test helper, and callQue.locker.stop!where needed.- maybe we don't need that initializer at all
- also, maybe we don't need the
que.ymlfile and the corresponding code in theapplication.rb- I can't find where they are used (except for the tests)
But well, that's all optional, I just noticed it while investigating.
3 participants
Jira issue: https://issues.redhat.com/browse/THREESCALE-12186
🚨 Your current dependencies have known security vulnerabilities 🚨
This dependency update fixes known security vulnerabilities. Please see the details below and assess their impact carefully. We recommend to merge and deploy this as soon as possible!
Here is everything you need to know about this upgrade. Please take a good look at what changed and the test results before merging this pull request.
What changed?
✳️ rails (7.1.6 → 8.0.2.1) · Repo
Release Notes
Too many releases to show here. View the full release notes.
Commits
See the full diff on Github. The new version differs by more commits than we can show here.
✳️ webrick (1.8.2 → 1.9.2) · Repo
Sorry, we couldn't find anything useful about this release.
Release Notes
8.0.1 (from changelog)
8.0.0.1 (from changelog)
8.0.0 (from changelog)
7.2.2.1 (from changelog)
7.2.2 (from changelog)
7.2.1.2 (from changelog)
7.2.1.1 (from changelog)
7.2.1 (from changelog)
7.2.0 (from changelog)
Does any of this look wrong? Please let us know.
Commits
See the full diff on Github. The new version differs by more commits than we can show here.
Security Advisories 🚨
🚨 Possible ReDoS vulnerability in block_format in Action Mailer
Release Notes
8.0.1 (from changelog)
8.0.0.1 (from changelog)
8.0.0 (from changelog)
7.2.2.1 (from changelog)
7.2.2 (from changelog)
7.2.1.2 (from changelog)
7.2.1.1 (from changelog)
7.2.1 (from changelog)
7.2.0 (from changelog)
Does any of this look wrong? Please let us know.
Commits
See the full diff on Github. The new version differs by more commits than we can show here.
Security Advisories 🚨
🚨 Possible Content Security Policy bypass in Action Dispatch
🚨 Possible Content Security Policy bypass in Action Dispatch
🚨 Possible ReDoS vulnerability in HTTP Token authentication in Action Controller
🚨 Possible ReDoS vulnerability in query parameter filtering in Action Dispatch
Release Notes
8.0.1 (from changelog)
8.0.0.1 (from changelog)
8.0.0 (from changelog)
7.2.2.1 (from changelog)
7.2.2 (from changelog)
7.2.1.2 (from changelog)
7.2.1.1 (from changelog)
7.2.1 (from changelog)
7.2.0 (from changelog)
Does any of this look wrong? Please let us know.
Commits
See the full diff on Github. The new version differs by more commits than we can show here.
Security Advisories 🚨
🚨 Possible ReDoS vulnerability in plain_text_for_blockquote_node in Action Text
Release Notes
8.0.1 (from changelog)
8.0.0.1 (from changelog)
8.0.0 (from changelog)
7.2.2.1 (from changelog)
7.2.2 (from changelog)
7.2.1.2 (from changelog)
7.2.1.1 (from changelog)
7.2.1 (from changelog)
7.2.0 (from changelog)
Does any of this look wrong? Please let us know.
Commits
See the full diff on Github. The new version differs by more commits than we can show here.
Release Notes
8.0.1 (from changelog)
8.0.0.1 (from changelog)
8.0.0 (from changelog)
7.2.2.1 (from changelog)
7.2.2 (from changelog)
7.2.1.2 (from changelog)
7.2.1.1 (from changelog)
7.2.1 (from changelog)
7.2.0 (from changelog)
Does any of this look wrong? Please let us know.
Commits
See the full diff on Github. The new version differs by more commits than we can show here.
Release Notes
8.0.1 (from changelog)
8.0.0.1 (from changelog)
8.0.0 (from changelog)
7.2.2.1 (from changelog)
7.2.2 (from changelog)
7.2.1.2 (from changelog)
7.2.1.1 (from changelog)
7.2.1 (from changelog)
7.2.0 (from changelog)
Does any of this look wrong? Please let us know.
Commits
See the full diff on Github. The new version differs by more commits than we can show here.
Release Notes
8.0.1 (from changelog)
8.0.0.1 (from changelog)
8.0.0 (from changelog)
7.2.2.1 (from changelog)
7.2.2 (from changelog)
7.2.1.2 (from changelog)
7.2.1.1 (from changelog)
7.2.1 (from changelog)
7.2.0 (from changelog)
Does any of this look wrong? Please let us know.
Commits
See the full diff on Github. The new version differs by more commits than we can show here.
Security Advisories 🚨
🚨 Active Record logging vulnerable to ANSI escape injection
🚨 Active Record logging vulnerable to ANSI escape injection
Release Notes
8.0.1 (from changelog)
8.0.0.1 (from changelog)
8.0.0 (from changelog)
7.2.2.1 (from changelog)
7.2.2 (from changelog)
7.2.1.2 (from changelog)
7.2.1.1 (from changelog)
7.2.1 (from changelog)
7.2.0 (from changelog)
Does any of this look wrong? Please let us know.
Commits
See the full diff on Github. The new version differs by more commits than we can show here.
Security Advisories 🚨
🚨 Active Storage allowed transformation methods that were potentially unsafe
🚨 Active Storage allowed transformation methods that were potentially unsafe
Release Notes
8.0.1 (from changelog)
8.0.0.1 (from changelog)
8.0.0 (from changelog)
7.2.2.1 (from changelog)
7.2.2 (from changelog)
7.2.1.2 (from changelog)
7.2.1.1 (from changelog)
7.2.1 (from changelog)
7.2.0 (from changelog)
Does any of this look wrong? Please let us know.
Commits
See the full diff on Github. The new version differs by more commits than we can show here.
Release Notes
8.0.1 (from changelog)
8.0.0.1 (from changelog)
8.0.0 (from changelog)
7.2.2.1 (from changelog)
7.2.2 (from changelog)
7.2.1.2 (from changelog)
7.2.1.1 (from changelog)
7.2.1 (from changelog)
7.2.0 (from changelog)
Does any of this look wrong? Please let us know.
Commits
See the full diff on Github. The new version differs by more commits than we can show here.
Release Notes
3.0.2 (from changelog)
3.0.1 (from changelog)
3.0.0 (from changelog)
2.5.5 (from changelog)
Does any of this look wrong? Please let us know.
Commits
See the full diff on Github. The new version differs by more commits than we can show here.
Commits
See the full diff on Github. The new version differs by more commits than we can show here.
Release Notes
6.0.0
5.1.3
5.1.2
5.1.1 (from changelog)
5.1.0 (from changelog)
5.0.3 (from changelog)
5.0.2 (from changelog)
5.0.1 (from changelog)
5.0.0 (from changelog)
Does any of this look wrong? Please let us know.
Commits
See the full diff on Github. The new version differs by more commits than we can show here.
Commits
See the full diff on Github. The new version differs by more commits than we can show here.
Release Notes
8.0.1 (from changelog)
8.0.0.1 (from changelog)
8.0.0 (from changelog)
7.2.2.1 (from changelog)
7.2.2 (from changelog)
7.2.1.2 (from changelog)
7.2.1.1 (from changelog)
7.2.1 (from changelog)
7.2.0 (from changelog)
Does any of this look wrong? Please let us know.
Commits
See the full diff on Github. The new version differs by more commits than we can show here.
Release Notes
6.17.0
6.16.1
6.16.0
Does any of this look wrong? Please let us know.
Commits
See the full diff on Github. The new version differs by more commits than we can show here.
Release Notes
3.1.9
Does any of this look wrong? Please let us know.
Commits
See the full diff on Github. The new version differs by more commits than we can show here.
Release Notes
0.5.0
Does any of this look wrong? Please let us know.
Commits
See the full diff on Github. The new version differs by more commits than we can show here.
🆕 uri (added, 1.1.1)
🆕 useragent (added, 0.16.11)
🗑️ cgi (removed)
🗑️ mutex_m (removed)
Depfu will automatically keep this PR conflict-free, as long as you don't add any commits to this branch yourself. You can also trigger a rebase manually by commenting with
@depfu rebase.All Depfu comment commands