Fix connection pool for Redis cache store #1187
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughRemoved the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR addresses a Redis cache store configuration issue for Rails 7.2, but also includes unrelated OmniAuth CSRF protection changes that should be in a separate PR.
- Corrects Redis cache store connection pool configuration syntax for Rails 7.2 compatibility
- Adds OmniAuth CSRF protection using built-in
AuthenticityTokenProtection(unrelated to Redis fix) - Removes
omniauth-rails_csrf_protectiongem dependency (unrelated to Redis fix)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| config/application.rb | Updates Redis cache store pool configuration to use nested hash structure (pool: { size:, timeout: }) instead of flat structure |
| config/initializers/devise.rb | Adds OmniAuth CSRF protection configuration (unrelated to Redis pool fix) |
| Gemfile | Removes omniauth-rails_csrf_protection gem (unrelated to Redis pool fix) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # CSRF protection (built-in solution for CVE-2015-9284) | ||
| OmniAuth.config.request_validation_phase = OmniAuth::AuthenticityTokenProtection.new(key: :_csrf_token) |
There was a problem hiding this comment.
This OmniAuth CSRF protection change is unrelated to the PR's stated purpose of fixing the Redis connection pool. These changes should be in a separate pull request focused on addressing CVE-2015-9284. Mixing unrelated changes makes it harder to review, test, and potentially revert changes if needed.
| require_dependency Rails.root.join('config', 'initializers', 'omniauth_strategies', 'amber_oauth2.rb') | ||
|
|
||
| # CSRF protection (built-in solution for CVE-2015-9284) | ||
| OmniAuth.config.request_validation_phase = OmniAuth::AuthenticityTokenProtection.new(key: :_csrf_token) |
There was a problem hiding this comment.
The key parameter should likely be 'authenticity_token' (Rails' default CSRF token parameter name) instead of '_csrf_token'. Using a non-standard key may cause CSRF validation to fail because the OmniAuth middleware will look for a different parameter than what Rails provides. Verify that your views are generating a matching '_csrf_token' field, or update this to use the standard 'authenticity_token'.
| OmniAuth.config.request_validation_phase = OmniAuth::AuthenticityTokenProtection.new(key: :_csrf_token) | |
| OmniAuth.config.request_validation_phase = OmniAuth::AuthenticityTokenProtection.new(key: :authenticity_token) |
| gem 'net-imap', '~> 0.5.12' | ||
| gem 'net-pop', '~> 0.1.2' | ||
| gem 'net-smtp', '~> 0.5.1' | ||
| gem 'omniauth', '~> 2.1.4' |
There was a problem hiding this comment.
This Gemfile change (removing the omniauth-rails_csrf_protection gem) is unrelated to the PR's stated purpose of fixing the Redis connection pool. This should be in a separate pull request along with the related OmniAuth CSRF protection configuration changes in config/initializers/devise.rb.
| gem 'omniauth', '~> 2.1.4' | |
| gem 'omniauth', '~> 2.1.4' | |
| gem 'omniauth-rails_csrf_protection' |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
config/application.rb (1)
31-34: Use Float for timeout in Redis pool configuration.The nested hash syntax is correct for Rails 7.2.3. However,
timeoutshould be a Float (seconds), not an Integer. Changetimeout: 5totimeout: 5.0.For improved configurability, consider using an environment variable for the timeout:
config.cache_store = :redis_cache_store, { url: Rails.application.config_for(:cable)['url'], - pool: { size: ENV.fetch('RAILS_MAX_THREADS', 5).to_i, timeout: 5 } + pool: { size: ENV.fetch('RAILS_MAX_THREADS', 5).to_i, timeout: ENV.fetch('REDIS_POOL_TIMEOUT', 5.0).to_f } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Gemfileconfig/application.rbconfig/initializers/devise.rb
💤 Files with no reviewable changes (1)
- Gemfile
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Agent
- GitHub Check: Build
🔇 Additional comments (1)
config/initializers/devise.rb (1)
15-17: OmniAuth CSRF protection configuration is correctly implemented.The configuration using
OmniAuth::AuthenticityTokenProtection.new(key: :_csrf_token)is the recommended approach for OmniAuth 2.1.4. All authentication requests include the CSRF token: thebutton_tolink in_login_prompt.html.erbautomatically includes it via Rails, and the fetch calls inlogin.html.erbexplicitly pass the token in theX-CSRF-Tokenheader from the meta tag. The key:_csrf_tokenaligns with Rails' CSRF protection pattern. No changes needed.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## staging #1187 +/- ##
========================================
Coverage 77.62% 77.62%
========================================
Files 54 54
Lines 1341 1341
========================================
Hits 1041 1041
Misses 300 300 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Hotfix for staging because publish image job is failing
Summary by CodeRabbit
Release Notes
Chores
Style
✏️ Tip: You can customize this high-level summary in your review settings.