diff --git a/.gitignore b/.gitignore index 01c1592..6fdf89d 100644 --- a/.gitignore +++ b/.gitignore @@ -71,3 +71,6 @@ yarn-debug.log* # RubyCritic code analysis /rubycritic + +# local files +local/ diff --git a/Gemfile b/Gemfile index a69aac9..902974b 100644 --- a/Gemfile +++ b/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" diff --git a/Gemfile.lock b/Gemfile.lock index d176afe..e32ab0f 100644 --- a/Gemfile.lock +++ b/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 diff --git a/app/graphql/schemas/app_schema.rb b/app/graphql/schemas/app_schema.rb index c03491e..22c6ad2 100644 --- a/app/graphql/schemas/app_schema.rb +++ b/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 diff --git a/app/graphql/schemas/demo/app_schema.rb b/app/graphql/schemas/demo/app_schema.rb index 11af3d8..b8ed4c2 100644 --- a/app/graphql/schemas/demo/app_schema.rb +++ b/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 diff --git a/app/services/billing/create_checkout_session_service.rb b/app/services/billing/create_checkout_session_service.rb index 0bce9d0..3f9255d 100644 --- a/app/services/billing/create_checkout_session_service.rb +++ b/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 } ], diff --git a/config/initializers/rack_attack.rb b/config/initializers/rack_attack.rb new file mode 100644 index 0000000..1bdc75e --- /dev/null +++ b/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 diff --git a/config/initializers/super_admin.rb b/config/initializers/super_admin.rb index 4330ca7..f2f1cfc 100644 --- a/config/initializers/super_admin.rb +++ b/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| diff --git a/lib/abilities/users.rb b/lib/abilities/users.rb index 2238b53..910af13 100644 --- a/lib/abilities/users.rb +++ b/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 diff --git a/spec/requests/graphql_execute_spec.rb b/spec/requests/graphql_execute_spec.rb index 873410e..70fffaf 100644 --- a/spec/requests/graphql_execute_spec.rb +++ b/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 diff --git a/spec/requests/rack_attack_spec.rb b/spec/requests/rack_attack_spec.rb new file mode 100644 index 0000000..d399815 --- /dev/null +++ b/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 diff --git a/spec/requests/super_admin_authorization_spec.rb b/spec/requests/super_admin_authorization_spec.rb new file mode 100644 index 0000000..1ee70ef --- /dev/null +++ b/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 diff --git a/spec/services/billing/create_checkout_session_service_spec.rb b/spec/services/billing/create_checkout_session_service_spec.rb index 676f6d7..5affcbe 100644 --- a/spec/services/billing/create_checkout_session_service_spec.rb +++ b/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