Fix security findings from looper-defend hunt

.gitignore

@@ -71,3 +71,6 @@ yarn-debug.log*
 
 # RubyCritic code analysis
 /rubycritic
+
+# local files
+local/

Gemfile

@@ -45,11 +45,14 @@ gem "passwordless"
 # The authorization Gem for Ruby on Rails [https://github.com/CanCanCommunity/cancancan]
 gem "cancancan"
 
+# Throttle and block abusive requests [https://github.com/rack/rack-attack]
+gem "rack-attack", require: "rack/attack"
+
 # A view helper for adding Gravatars to your Ruby on Rails app [https://github.com/nschneble/gravatar_image_tag]
 gem "gravatar_image_tag", github: "nschneble/gravatar_image_tag", ref: "71b32fcffe461252aa4bb4fb9586ca0797af09ff"
 
 # A full-featured admin dashboard inspired by Administrate and ActiveAdmin [https://github.com/ThibautBaissac/super_admin]
-gem "super_admin", github: "nschneble/super_admin", ref: "f77c28bd6e1371d6b84aadbe31c2f2ce9bde67e2"
+gem "super_admin", github: "nschneble/super_admin", ref: "12f9c8c914ff8d66caebb1186b76a8385b9f1b7f"
 
 # Ruby wrapper for the CommonMark parser [https://github.com/gjtorikian/commonmarker]
 gem "commonmarker"

Gemfile.lock

@@ -34,8 +34,8 @@ GIT
 
 GIT
   remote: https://github.com/nschneble/super_admin.git
-  revision: f77c28bd6e1371d6b84aadbe31c2f2ce9bde67e2
-  ref: f77c28bd6e1371d6b84aadbe31c2f2ce9bde67e2
+  revision: 12f9c8c914ff8d66caebb1186b76a8385b9f1b7f
+  ref: 12f9c8c914ff8d66caebb1186b76a8385b9f1b7f
   specs:
     super_admin (0.2.1)
       csv
@@ -735,6 +735,7 @@ DEPENDENCIES
   pom-component
   propshaft
   puma (~> 8.0)
+  rack-attack
   rails (~> 8.1)
   rails-graphql!
   resque

app/graphql/schemas/app_schema.rb

@@ -17,7 +17,7 @@ class AppSchema < BaseSchema
       field(:users, "User", array: true, null: false)
         .authorize(&require_authentication)
         .resolve {
-          User.accessible_by(request.context.current_ability).order(:id)
+          User.accessible_by(request.context.current_ability).order(:id).limit(100)
         }
     end
   end

app/graphql/schemas/demo/app_schema.rb

@@ -15,7 +15,7 @@ class Demo::AppSchema < BaseSchema
       field(:mac_guffins, "MacGuffin", array: true, null: false)
         .authorize(&require_authentication)
         .resolve {
-          ::Demo::MacGuffin.accessible_by(request.context.current_ability).order(:id)
+          ::Demo::MacGuffin.accessible_by(request.context.current_ability).order(:id).limit(100)
         }
     end
   end

app/services/billing/create_checkout_session_service.rb

@@ -5,7 +5,11 @@ class CreateCheckoutSessionService < BillingService
     SECONDS_PER_DAY = 86400
     TRIAL_PERIOD_IN_DAYS = FREE_TRIAL_DURATION.to_i / SECONDS_PER_DAY
 
+    ALLOWED_PRICE_IDS = -> { [ Figaro.env.stripe_price_pro_monthly, Figaro.env.stripe_price_pro_yearly ] }
+
     def call(user:, price_id:, urls:)
+      return log_error_and_fail(:invalid_price_id, "Unknown Stripe price id: #{price_id}") unless ALLOWED_PRICE_IDS.call.include?(price_id)
+
       session_params = {
         customer: stripe_customer_id(user),
         line_items: [ { price: price_id, quantity: 1 } ],

config/initializers/rack_attack.rb

@@ -0,0 +1,50 @@
+# Throttles abuse-prone unauthenticated/low-cost endpoints: sign-in (which
+# creates a User row and sends a magic-link email per request), email-change
+# confirmation, and the GraphQL endpoint.
+class Rack::Attack
+  # Dedicated store so throttle counts aren't tied to the app's cache
+  # eviction policy. Production/development use solid_cache/memory_store for
+  # app caching; test uses :null_store, which would silently make every
+  # throttle here a no-op if it shared Rails.cache.
+  self.cache.store = ActiveSupport::Cache::MemoryStore.new
+
+  throttle("sign_in/ip", limit: 5, period: 20) do |req|
+    req.ip if req.path == "/sign_in" && req.post?
+  end
+
+  throttle("sign_in/email", limit: 5, period: 60) do |req|
+    if req.path == "/sign_in" && req.post?
+      EmailNormalizer.call(req.params.dig("passwordless", "email")).presence
+    end
+  end
+
+  throttle("email_change/ip", limit: 5, period: 60) do |req|
+    req.ip if req.path == "/email_change" && req.post?
+  end
+
+  throttle("graphql/ip", limit: 30, period: 10) do |req|
+    req.ip if req.path == "/graphql" && req.post?
+  end
+end
+
+# Disabled in test by default since the request-spec suite reuses the same
+# IP/emails across many examples in one process; spec/requests/rack_attack_spec.rb
+# explicitly re-enables it to verify these throttles.
+Rack::Attack.enabled = !Rails.env.test?
+
+# The vendored super_admin engine (lib/super_admin/engine.rb:46, see
+# config/application.rb) loads its own bundled rack-attack config once
+# Rack::Attack is defined. That config's throttles/blocklists are scoped to
+# /super_admin (inert here -- this app mounts the engine at /admin), but it
+# also sets a GLOBAL Rack::Attack.throttled_responder returning a JSON body,
+# which would silently apply to the throttles above too -- including the
+# HTML sign-in form, where a raw JSON response is a poor error page. That
+# engine initializer runs after this whole file, so re-asserting our own
+# responder in after_initialize (which runs after every named initializer)
+# is what makes ours win instead of being silently overridden.
+Rails.application.config.after_initialize do
+  Rack::Attack.throttled_responder = lambda do |request|
+    period = request.env["rack.attack.match_data"]&.fetch(:period, nil)
+    [ 429, { "Content-Type" => "text/plain", "Retry-After" => period.to_s }, [ "Too many requests. Please try again later.\n" ] ]
+  end
+end

config/initializers/super_admin.rb

@@ -24,13 +24,16 @@
   config.user_class = "User"
 
   # Authorization
-  # Configure authorization adapter (:auto, :pundit, :cancancan, or :custom)
-  config.authorization_adapter = :cancancan
-
-  # Custom authorization block (when using :custom adapter)
-  # config.authorize_with = proc { |controller|
-  #   redirect_to main_app.root_path unless current_user&.admin?
-  # }
+  # Configure authorization adapter (:auto, :pundit, :proc, or :custom).
+  # NOTE: this app uses CanCanCan (lib/abilities/), but the installed
+  # super_admin gem (0.2.1) has no CancanAdapter -- only default, pundit, and
+  # proc adapters exist. Setting :cancancan here silently no-ops to
+  # DefaultAdapter (which happens to check current_user.admin? directly,
+  # since no super_admin_check is configured below). Using :proc instead
+  # makes that same check explicit rather than relying on an undocumented
+  # fallback. See spec/requests/super_admin_authorization_spec.rb.
+  config.authorization_adapter = :proc
+  config.authorize_with = proc { current_user&.admin? }
 
   # What to do when authorization fails
   config.on_unauthorized = proc { |controller|

lib/abilities/users.rb

@@ -4,7 +4,6 @@ module Abilities::Users
   def self.apply(ability, user)
     if user.present?
       ability.can :manage, User, id: user.id
-      ability.can :read, User, role: :user
 
       # define any additional permissions for signed-in users here
       # e.g. ability.can :write, Post, user: user

spec/requests/graphql_execute_spec.rb

@@ -53,9 +53,9 @@
         expect(response).to have_http_status(:unprocessable_content)
       end
 
-      it "returns users ordered by id" do
+      it "returns only the signed-in user's own record" do
         user_a = create(:user)
-        user_b = create(:user)
+        create(:user) # another user must exist to prove they're excluded
 
         passwordless_sign_in(user_a)
 
@@ -66,15 +66,14 @@
         end
 
         expect(response).to have_http_status(:ok)
-        expect(parsed_body.dig("data", "users")).to include(
-          { "id" => user_a.id.to_s, "email" => user_a.email, "role" => user_a.role },
-          { "id" => user_b.id.to_s, "email" => user_b.email, "role" => user_b.role }
+        expect(parsed_body.dig("data", "users")).to contain_exactly(
+          { "id" => user_a.id.to_s, "email" => user_a.email, "role" => user_a.role }
         )
       end
 
       it "excludes admins when authenticated user is not an admin" do
         user = create(:user)
-        admin = create(:user, :admin)
+        create(:user, :admin) # an admin must exist to prove they're excluded
 
         passwordless_sign_in(user)
 
@@ -85,16 +84,16 @@
         end
 
         expect(response).to have_http_status(:ok)
-        expect(parsed_body.dig("data", "users")).to include(
+        expect(parsed_body.dig("data", "users")).to contain_exactly(
           { "id" => user.id.to_s, "email" => user.email, "role"  => user.role }
         )
       end
     end
 
     context "with token authentication" do
-      it "returns users ordered by id" do
+      it "returns only the token-owning user's own record" do
         user_a = create(:user)
-        user_b = create(:user)
+        create(:user) # another user must exist to prove they're excluded
 
         token = ApiToken.issue!(user: user_a, name: "Spec Token")
 
@@ -105,9 +104,8 @@
         end
 
         expect(response).to have_http_status(:ok)
-        expect(parsed_body.dig("data", "users")).to include(
-          { "id" => user_a.id.to_s, "email" => user_a.email, "role" => user_a.role },
-          { "id" => user_b.id.to_s, "email" => user_b.email, "role" => user_b.role }
+        expect(parsed_body.dig("data", "users")).to contain_exactly(
+          { "id" => user_a.id.to_s, "email" => user_a.email, "role" => user_a.role }
         )
       end
 
@@ -129,6 +127,22 @@
           { "id" => admin.id.to_s, "email" => admin.email, "role" => admin.role }
         )
       end
+
+      it "caps the number of returned users at 100" do
+        user = create(:user, :admin)
+        create_list(:user, 101) # rubocop:disable FactoryBot/ExcessiveCreateList -- must exceed the cap under test
+
+        token = ApiToken.issue!(user:, name: "Admin Spec Token")
+
+        with_forgery_protection do
+          post "/graphql",
+            params: { query: "{ users { id } }" },
+            headers: { Authorization: "Bearer #{token.plaintext_token}" }
+        end
+
+        expect(response).to have_http_status(:ok)
+        expect(parsed_body.dig("data", "users").size).to eq(100)
+      end
     end
   end
 

spec/requests/rack_attack_spec.rb

@@ -0,0 +1,48 @@
+require "rails_helper"
+
+RSpec.describe "Rack::Attack throttling", type: :request do
+  around do |example|
+    original_enabled = Rack::Attack.enabled
+    original_store = Rack::Attack.cache.store
+
+    Rack::Attack.enabled = true
+    Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new
+
+    example.run
+
+    Rack::Attack.enabled = original_enabled
+    Rack::Attack.cache.store = original_store
+  end
+
+  it "throttles repeated sign-in attempts from the same IP" do
+    5.times { post auth_sign_in_path, params: { passwordless: { email: "throttle-ip@example.com" } } }
+    expect(response).not_to have_http_status(:too_many_requests)
+
+    post auth_sign_in_path, params: { passwordless: { email: "throttle-ip@example.com" } }
+    expect(response).to have_http_status(:too_many_requests)
+  end
+
+  it "throttles repeated sign-in attempts for the same email regardless of casing" do
+    5.times { post auth_sign_in_path, params: { passwordless: { email: "Throttle-Email@Example.com" } }, headers: { "REMOTE_ADDR" => "10.0.0.1" } }
+    post auth_sign_in_path, params: { passwordless: { email: "throttle-email@example.com" } }, headers: { "REMOTE_ADDR" => "10.0.0.2" }
+
+    expect(response).to have_http_status(:too_many_requests)
+  end
+
+  it "throttles repeated GraphQL requests from the same IP" do
+    30.times { post "/graphql", params: { query: "{ health { status } }" } }
+    expect(response).not_to have_http_status(:too_many_requests)
+
+    post "/graphql", params: { query: "{ health { status } }" }
+    expect(response).to have_http_status(:too_many_requests)
+  end
+
+  it "returns a plain-text response, not the vendored super_admin engine's JSON responder" do
+    5.times { post auth_sign_in_path, params: { passwordless: { email: "throttle-responder@example.com" } } }
+    post auth_sign_in_path, params: { passwordless: { email: "throttle-responder@example.com" } }
+
+    expect(response).to have_http_status(:too_many_requests)
+    expect(response.content_type).to start_with("text/plain")
+    expect(response.body).to eq("Too many requests. Please try again later.\n")
+  end
+end

spec/requests/super_admin_authorization_spec.rb

@@ -0,0 +1,23 @@
+require "rails_helper"
+
+RSpec.describe "SuperAdmin authorization", type: :request do
+  it "denies a non-admin authenticated user" do
+    user = create(:user)
+    passwordless_sign_in(user)
+
+    get "/admin/users"
+
+    expect(response).to redirect_to(root_path)
+    follow_redirect!
+    expect(flash[:alert]).to be_present
+  end
+
+  it "allows an admin user" do
+    admin = create(:user, :admin)
+    passwordless_sign_in(admin)
+
+    get "/admin/users"
+
+    expect(response).to have_http_status(:ok)
+  end
+end

spec/services/billing/create_checkout_session_service_spec.rb

@@ -9,7 +9,7 @@
   let(:call_args) do
     {
       user:,
-      price_id: "price_test",
+      price_id: "price_monthly_test",
       urls: {
         success: "https://example.com/success",
         cancel:  "https://example.com/cancel"
@@ -18,6 +18,10 @@
   end
 
   before do
+    allow(Figaro.env).to receive_messages(
+      stripe_price_pro_monthly: "price_monthly_test",
+      stripe_price_pro_yearly:  "price_yearly_test"
+    )
     allow(fake_customers).to receive(:create).and_return(fake_customer)
     allow(fake_checkout_sessions).to receive(:create).and_return(fake_session)
   end
@@ -77,5 +81,13 @@
       expect(result).to be_failure
       expect(result.error).to eq(:stripe_error)
     end
+
+    it "returns a failure result for a price_id outside the configured plans" do
+      result = described_class.call(**call_args.merge(price_id: "price_not_a_real_plan"))
+
+      expect(result).to be_failure
+      expect(result.error).to eq(:invalid_price_id)
+      expect(fake_checkout_sessions).not_to have_received(:create)
+    end
   end
 end