Dependency hardening: patch bumps, Ruby 3.4, Rails 8.1 - #15
Merged
Conversation
Ran bundler-audit against the lockfile: 174 known-CVE advisory entries across 21 gems, mostly Rails-core ReDoS/XSS/path-traversal issues fixed by moving within the existing 7.2.x line, plus a large batch of nokogiri memory-safety issues. Used `bundle update --patch` rather than a plain `bundle update` after the latter silently pulled several unpinned direct dependencies (searchkick, rspec-rails, shoulda-matchers, rubocop) up whole major versions - those are explicitly deferred to their own follow-up work, not bundled in here. Beyond the patch-only pass, targeted a few specific fixes for CVEs a patch-level bump couldn't reach: nokogiri (1.16->1.19, several High-severity memory-safety issues), loofah/rails-html-sanitizer (both back this app's own RichTextField sanitizer), rexml, websocket-driver, net-imap, msgpack, and addressable (needed relaxing its Gemfile pin from `~> 2.8` to `~> 2.9`). Two gems still have open advisories, both requiring a major-version bump this pass deliberately defers: - httparty 0.20.0: High-severity SSRF/API-key-leakage CVE, fixed in >= 0.24.0. Every call site (lib/import.rb, app/jobs/import_data_job.rb) hits a single hardcoded host with no user- or redirect-controlled URL, so practical exploitability here looks low, but flagging clearly rather than silently leaving it. - puma 5.6.9: two High-severity PROXY-protocol-parsing CVEs, fixed in ~> 7.2.1 or >= 8.0.2. config/puma.rb never enables proxy_protocol on any bind directive, so the vulnerable code path isn't reachable as configured. Also fixed 5 mechanical rubocop offenses (frozen_string_literal comments, string style, ENV.fetch) that the toolchain patch bump newly surfaced. Left 3 Metrics/MethodLength and 1 Rails/I18nLocaleTexts finding alone - pre-existing code the stricter toolchain now flags differently, not something to refactor silently as a side effect of a dependency bump. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Rails 8 only requires Ruby >= 3.2 (this app is already well past that), so this is independent of the Rails bump - isolating "does the app work on new Ruby" from "does the app work on new Rails" as separate questions before compounding both. Re-adds the csv gem, removed earlier as an unused direct dependency: csv stopped being a Ruby default gem in 3.4+, and httparty 0.20.0 requires it internally without declaring it as its own dependency (confirmed - the app booted with a real deprecation warning on 3.3 before this bump, and boots silently now). Verified: full suite (304 examples, 0 failures), rubocop (same 4 pre-existing findings as before, nothing new from the Ruby bump itself), clean app boot. Still on Rails 7.2.3.2 for this commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
acts-as-taggable-on had to move too: 11.0.0 hard-pins activerecord < 8.0, which makes it impossible to install alongside Rails 8 at all - not a discretionary bump like the others deferred in the earlier patch-bump commit, a forced one. Bumped to ~> 13.0 (12->13 has no breaking changes to the tag_list/for_context API ActsAsTaggableField depends on, per its CHANGELOG - only dropped-Ruby-version and added-Rails-version support). Ran `bin/rails app:update` and reviewed its diff file-by-file rather than accepting it wholesale: declined every config file overwrite (all just show cosmetic/new-default-flag differences from this app's deliberate customizations - CORS rules, custom filter_parameters, Sidekiq queue adapter, etc. - confirmed by diffing against a fresh Rails 8.1 scaffold app, nothing there is actually required for Rails 8 to work), dropped the generated bin/ci + config/ci.rb (assumes MiniTest via `bin/rails test`, this app uses RSpec with its own GitHub Actions workflow), and deleted 3 generated ActiveStorage migrations that are pure no-ops here (this app's existing single `create_active_storage_tables` migration, dated 2022, already creates everything they would). Keeps the generated, all-commented-out new_framework_defaults_8_1.rb checklist and config.load_defaults at 7.0 - deliberately not adopting new defaults as part of this bump, per Rails' own recommended upgrade path. Manual verification in a real browser (RAILS_ENV=test server, since dev credentials aren't available here) caught a real regression the automated suite had missed: ActsAsTaggableField's own field spec constructed the field directly with pre-fetched data, and the mentions request spec's factory had no tags by default, so neither ever exercised Administrate's actual field-construction path. Administrate::Field::Base#read_value (new in 1.0, works differently than in 0.20.1) lazily fetches data via `resource.try(attribute)` when data is nil - and that `attribute` call is polymorphic, landing on ActsAsTaggableField's own #attribute override (returns "tag_list", for form-building) instead of the raw :tags key. That fetched mention.tag_list (name strings) instead of mention.tags (real Tag records), and _show.html.erb's `tag.name` raised NoMethodError on a String - reproducible on develop today with any tagged mention, not something this Rails bump introduced. Fixed by overriding #read_value to use the raw @Attribute ivar instead of the overridden method, and added regression coverage at both the field-unit level (constructing the field the way Administrate really does) and the request-spec level (a tagged mention through index/show/edit). Verified: full suite (308 examples, 0 failures - reset the local test DB after manual browser testing polluted it with non-transactional data), system specs passing (letters entity picker jQuery/selectize still initializes correctly), rubocop clean, and a real browser click-through across letters/mentions/entities admin pages. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three sequential, independently-verified phases - matching the same discipline as the administrate 1.0 upgrade (isolate each risk, verify with the real test suite plus a real browser pass).
Patch and security bumps (0049ef3)
Ran
bundler-audit: 174 known-CVE advisory entries across 21 gems. Usedbundle update --patchafter a plainbundle updatesilently pulled several unpinned direct dependencies (searchkick, rspec-rails, shoulda-matchers, rubocop) up whole major versions - those stay deferred to their own follow-up work. Beyond the patch-only pass, targeted nokogiri (1.16→1.19, several High-severity memory-safety issues), loofah/rails-html-sanitizer (back this app's ownRichTextFieldsanitizer), rexml, websocket-driver, net-imap, msgpack, and addressable.Two gems still have open advisories, both requiring a major bump this deliberately defers, with reasoning for why they look low-risk as currently configured:
config/puma.rbnever enablesproxy_protocolon any bind directive.Ruby 3.3.4 -> 3.4.10 (cfc3e36)
Independent of the Rails bump - isolates "does the app work on new Ruby" from "does the app work on new Rails." Re-adds the
csvgem (removed earlier as unused-by-us;csvstops being a Ruby default gem in 3.4+, andhttpartyneeds it internally without declaring it).Rails 7.2.3.2 -> 8.1.3.1 (597f06f)
acts-as-taggable-onhad to move too - 11.0.0 hard-pinsactiverecord < 8.0, making it impossible to install alongside Rails 8 at all. Bumped to~> 13.0(no breaking API changes per its CHANGELOG).Ran
bin/rails app:updateand reviewed its diff file-by-file: declined every config file overwrite (confirmed via diffing against a fresh Rails 8.1 scaffold that nothing there is actually required, just this app's deliberate customizations vs. new cosmetic defaults), dropped the generatedbin/ci/config/ci.rb(assumes MiniTest, this app uses RSpec), and deleted 3 generated ActiveStorage migrations that are pure no-ops (this app's existing 2022create_active_storage_tablesmigration already covers them). Keepsconfig.load_defaultsat7.0deliberately - not adopting new framework defaults as part of this bump.Manual browser verification caught a real regression the automated suite had missed:
ActsAsTaggableField's own spec constructed the field directly with pre-fetched data, and the mentions request spec's factory had no tags by default, so neither ever exercised Administrate's actual field-construction path.Administrate::Field::Base#read_value(new in 1.0) lazily fetches data viaresource.try(attribute)when data is nil, and thatattributecall is polymorphic - it lands onActsAsTaggableField's own#attributeoverride (returns"tag_list", for form-building) instead of the raw:tagskey. Fixed by overriding#read_valueto use the raw@attributeivar, with new regression coverage at both the field-unit and request-spec level. This bug is reproducible ondeveloptoday with any tagged mention - not something this Rails bump introduced, just found by it.Test plan
🤖 Generated with Claude Code