Dependency hardening: roo, httparty, searchkick, image_processing, puma, dev toolchain - #17
Merged
Merged
Conversation
Both deferred from the earlier dependency-hardening PR since they're major version bumps needing their own verification. roo 3.0 has no API-level breaking changes to spreadsheet reading (open/cell access/sheet iteration) per its changelog - just needs Ruby 3.1+ (have 3.4.10) and pulls rubyzip to 3.x, unconstrained by anything else in the Gemfile. httparty 0.24.2 fixes a High-severity SSRF/API-key-leakage CVE (GHSA-hm5p-x4rq-38w4) by restricting credential-forwarding on redirects to same-host. Every call site in this app (lib/import.rb, app/jobs/import_data_job.rb) hits a single hardcoded host directly, with no base_uri set and no credentials passed, so no behavior change expected - verified via full suite + clean boot (no dedicated spec coverage exists for either file either way). Verified: full suite (308 examples, 0 failures), rubocop clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
No breaking changes to the .search API (where/load/order) per the
changelog. Removed its hard dependency on the elasticsearch gem (now
optional, bring-your-own client) - decoupled from any future ES9 bump,
not blocked by it.
Surfaced two real, pre-existing bugs neither caught by tests before:
- LettersController/EntitiesController's `before_action :reindex` calls
`Letter.reindex`/`Entity.reindex` (class-level, full reindex) with no
explicit mode, relying on Searchkick's default mode resolution to pick a
synchronous reindex before the search that immediately follows. That
resolution (Searchkick::Index#reindex_records) falls back to the model's
own `callbacks:` searchkick option when no mode is given - and Searchable
sets `callbacks: :async`. Whether that fallback applied to explicit
`.reindex` calls the same way in 5.x is unclear, but it does now: the
reindex enqueued an async job instead of running inline, and the search
that followed sometimes ran before it completed, intermittently missing
the ES mapping entirely ("No mapping found for [date]"). Fixed by passing
`mode: :inline` explicitly in both controllers' reindex methods.
- LettersController#letters_from_results called `letter.orgins` (typo for
`origins`) on a Searchkick::HashWrapper. 5.x's HashWrapper silently
returned nil for the typo'd key; 6.x raises NoMethodError instead.
Fixed the typo and added regression coverage - nothing had ever asserted
on the `origins` key in a letters index response before.
Verified: full suite (309 examples, 0 failures), rubocop clean.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Fixes several real RCE vulnerabilities (remote code execution via user-influenced operation names through #operation/#method_missing/#send, and unsafe minimagick loader/saver option names). mini_magick/ruby-vips became soft dependencies in 2.0 and must be added explicitly now - image_processing 1.x's gemspec hard-required both unconditionally. Initially added mini_magick, assuming it was the active processor since Rails defaults ActiveStorage.variant_processor to :mini_magick. That was wrong: this app calls config.load_defaults 7.0 (config/application.rb), and Rails' 7.0 defaults block (config/application/configuration.rb) unconditionally sets config.active_storage.variant_processor = :vips - vips has been the actually-active processor the whole time, with mini_magick present only as an unused transitive dependency. Confirmed by generating a real image variant, not just booting cleanly. Added ruby-vips instead. Verified: full suite (309 examples, 0 failures), rubocop clean, and an actual end-to-end variant generation via `rails runner` (attach a real image, call .variant(...).processed, confirm it produces a real blob) - not just a clean boot, since this exact class of bug (config silently depending on a gem that happened to be present transitively) only shows up when the feature is actually exercised. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Reviewed all four majors' upgrade guides (6.0/7.0/8.0-Upgrade.md) for
anything that could affect this app: hook renames, removed Server#min/
max_threads, HTTP method allowlisting, response header lowercasing - none
of it touches config/puma.rb (no hooks, no workers/cluster mode configured)
or anything this app depends on.
The one real behavioral change: Puma 8 defaults to binding IPv6 (::)
instead of IPv4 (0.0.0.0) when a non-loopback IPv6 interface is available.
This app deploys to AWS ECS/Fargate, and whether that networking setup
handles the new default cleanly isn't verifiable from here. Puma's own
upgrade guide recommends pinning the bind explicitly to preserve exact
current behavior - added `port ENV.fetch('PORT', 3000), '0.0.0.0'` to
config/puma.rb rather than gambling on it. Confirmed via a real (non-test-
suite) server boot that it actually listens on 0.0.0.0 as pinned.
No dependency conflicts - puma itself has no rack version constraint at
all, unlike sidekiq 8 (which hard-requires rack >= 3.2.0 and was deferred
alongside rack-cors for that reason).
Verified: full suite (309 examples, 0 failures) including system specs,
which boot a real Puma server via Capybara/Selenium - the exact code path
Puma's own 6.0 changelog warned could break for gems depending on removed
Server internals. rubocop clean.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
rspec-rails 6.1.5 -> 8.0.4, shoulda-matchers 6.4.0 -> 8.0.1, rubocop
1.65.1 -> 1.89.0, rubocop-{rails,rspec,factory_bot,rspec_rails} to latest.
All dev/test-only, no production runtime impact - verified by running the
actual test suite and linter rather than reading changelogs, since a
failure here surfaces immediately as a spec or lint failure.
Applied the handful of newly-surfaced, genuinely safe rubocop fixes
(redundant safe-navigation chains, a no-op Array#flatten after a
string-returning #map, order('position') -> order(:position)) - all
verified equivalent by reading the surrounding code, not blindly
autocorrected.
Deliberately left Rails/StrongParametersExpect alone (18 call sites across
6 controllers) even though most are auto-correctable: params.expect(...)
raises ActionController::ParameterMissing/ParameterInvalid where bare
params[:key] silently returns nil, a real behavior change across every
public-facing endpoint in this app. That's its own deliberate audit, not a
mechanical sweep. Also left Naming/PredicateMethod (flags a pre-existing
dead `# TODO: Add authentication logic here` stub) and RSpec/Output
(a false positive - it's rails_helper.rb boot-time boilerplate that prints
and hard-exits on pending migrations, not a leftover debug print in a
spec) - both pre-existing, out of scope here.
Verified: full suite (309 examples, 0 failures) both before and after the
manual fixes, rubocop clean except the deliberately-deferred items above.
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
Bumps the direct-dependency majors deferred from the earlier Rails 8 dependency-hardening pass, per user decision.
elasticsearch/rack-corsexplicitly excluded (ES9 needs a server upgrade first; rack-cors needs Rack 3, deferred as its own dedicated ecosystem-wide upgrade alongside sidekiq - see below).roo 2.10.1 -> 3.0.0, httparty 0.20.0 -> 0.24.2 (befe33d)
No API breaking changes to either. httparty fixes a High-severity SSRF CVE; every call site hits a hardcoded host with no credentials, so no behavior change expected.
searchkick 5.3.1 -> 6.1.2 (41caef7)
No breaking changes to
.search. Surfaced two real, pre-existing bugs neither caught by tests before:LettersController/EntitiesController'sbefore_action :reindexrelied on Searchkick's default reindex-mode resolution to pick synchronous - that resolution now falls back to the model'scallbacks: :asyncoption, so the reindex sometimes ran async and the search immediately after raced against it, intermittently missing the ES mapping entirely. Fixed with explicitmode: :inline.letter.orgins(typo fororigins) - 5.x'sHashWrappersilently returned nil for the typo, 6.x raisesNoMethodError. Fixed + added regression coverage.image_processing 1.14.0 -> 2.0.3 (49a1d67)
Fixes several real RCE vulnerabilities. mini_magick/ruby-vips became soft dependencies in 2.0. Initially assumed mini_magick was the active processor (Rails' bare default) - wrong. This app's
config.load_defaults 7.0unconditionally setsvariant_processor = :vips, which has been the actually-active processor the whole time (mini_magick was present only as an unused transitive dependency of image_processing 1.x). Addedruby-vipsinstead, confirmed via an actual end-to-end variant generation, not just a clean boot.puma 5.6.9 -> 8.0.2 (a4de628)
Reviewed all four majors' upgrade guides. The one real change: Puma 8 defaults to binding IPv6 instead of IPv4 when available. Pinned
config/puma.rb's bind explicitly to0.0.0.0to preserve current behavior regardless of Fargate's IPv6 support (unverifiable from here). Confirmed via a real server boot that it listens on 0.0.0.0 as pinned.Dev/test toolchain (252111f)
rspec-rails, shoulda-matchers, rubocop + plugins to latest. Dev-only, no production impact. Fixed a handful of newly-surfaced, individually-verified-safe rubocop offenses; deliberately left
Rails/StrongParametersExpectalone (18 call sites -params.expectraises where bareparams[:key]silently returns nil, a real behavior change deserving its own audit, not a mechanical sweep).Explicitly deferred, not in this PR
rack >= 3.2.0, the same Rack 3 bump just deferred via rack-cors. Per your decision, staying on 7.x until Rack 3 is tackled as its own dedicated upgrade (both gems can ride together then).Test plan
🤖 Generated with Claude Code