Fix security findings from looper-defend hunt#93
Merged
Conversation
Remediates the high/medium findings from a proactive whole-repo vulnerability hunt (bundler-audit + brakeman clean; findings from targeted LLM review of billing, auth, authorization, GraphQL, and the demo namespace). - GraphQL `users` query let any authenticated non-admin read every other non-admin user's id/email (lib/abilities/users.rb granted `read` on all `role: :user` records instead of just the caller's own). Removed the redundant, over-broad grant -- self-read already comes from the existing `can :manage, User, id: user.id` rule. - Stripe checkout accepted a client-supplied `price_id` with no allowlist check, letting a user request a checkout session for any price ID rather than only the configured plans. - Added rack-attack throttles on sign-in (IP + email), email-change, and GraphQL requests -- none of these had any rate limiting, so an unauthenticated actor could mail-bomb arbitrary addresses via the sign-in flow or brute-force the GraphQL endpoint. This also covers GraphQL query-cost abuse, since the app's GraphQL gem (a custom rails-graphql fork, not graphql-ruby) has no complexity/depth-limit primitive to configure directly. - Capped the two unbounded GraphQL list fields at 100 records. - Corrected a dangling `super_admin` authorization_adapter config: it was set to `:cancancan`, but the installed gem version has no CancanAdapter, so it silently fell back to DefaultAdapter. Made the real check (admin-only) explicit via the `:proc` adapter instead of relying on an undocumented rescue fallback. Adding rack-attack exposed a pre-existing bug in the vendored super_admin gem fork: its engine initializer runs `require "rack-attack"` (wrong path) the moment Rack::Attack is defined, which always crashes boot. Since that gem is pulled via a pinned git ref, vendor/rack_attack_shim/ supplies a shim satisfying that broken require until upstream fixes the typo. Every fix has corresponding RSpec coverage written/updated alongside it; full suite (483 examples) and rubocop pass clean.
nschneble/super_admin#2 fixes the require-path typo directly (and removes the now-provably-dead require entirely, since the guard it sat behind already proves Rack::Attack is loaded). Bumping to that commit removes the need for vendor/rack_attack_shim/ altogether. Full suite (483 examples) passes with the shim gone, confirming the upstream fix actually resolves the crash rather than just relocating where it's worked around.
nschneble/super_admin#2 merged. Moving off the branch-commit pin now that main carries the fix.
nschneble
marked this pull request as ready for review
July 23, 2026 17:15
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.
Remediates 5 findings from a
looper-defendwhole-repo vulnerability hunt.Changed:
lib/abilities/users.rb: Removedcan :read, User, role: :user, which let any authenticated non-admin read every other non-admin user's id/email through the GraphQLusersqueryapp/services/billing/create_checkout_session_service.rb: Validatesprice_idagainst the two configured Stripe plan prices before calling Stripe, instead of trusting the client-supplied value outrightconfig/initializers/rack_attack.rb: Throttles sign-in, email-change, and GraphQL requestsapp/graphql/schemas/app_schema.rb,app/graphql/schemas/demo/app_schema.rb: Capped the two unbounded root list fields at 100 records.config/initializers/super_admin.rb:authorization_adapterwas set to:cancancan, but the installedsuper_admingem version has noCancanAdapter, so it silently fell back toDefaultAdapterUpstream:
Adding rack-attack originally exposed a real, pre-existing bug in the vendored
super_admingem fork: its engine initializer ranrequire "rack-attack"(wrong path) the momentRack::Attackwas defined, always crashing boot. Fixed properly upstream instead of carrying a workaround here.