diff --git a/app/models/concerns/base_url_allowlistable.rb b/app/models/concerns/base_url_allowlistable.rb new file mode 100644 index 00000000000..3893f76b04e --- /dev/null +++ b/app/models/concerns/base_url_allowlistable.rb @@ -0,0 +1,85 @@ +# Shared F-08 SSRF hardening for provider items whose operators can configure +# an outbound `base_url` from the UI. Without validation a user could point +# server-side requests at internal endpoints (169.254.169.254 metadata, +# localhost, internal DNS, etc.). +# +# Usage: +# +# class FooItem < ApplicationRecord +# include BaseUrlAllowlistable +# allowed_base_urls "https://api.foo.com/api/v1" +# end +# +# Provides: +# - ALLOWED_BASE_URLS class-level constant (frozen array of frozen strings) +# - AR `inclusion` validation on `base_url` rejecting invalid values at save time +# - `effective_base_url` instance helper that falls back to the canonical URL +# and logs a per-call [SECURITY] warning whenever an invalid value is read +# (e.g. a row written through rake/console before this concern was in place) +# +# The ActiveRecord `inclusion` validation and the runtime `effective_base_url` +# helper are both kept as defense-in-depth: the validation catches bad input +# at the UI / AR-write boundary, while the helper guards against values +# written by paths that bypass AR — direct SQL (`UPDATE provider_items ...`), +# rake tasks using `update_columns`, or console sessions skipping validation. +# Neither is a full substitute for the other. +require "uri" + +module BaseUrlAllowlistable + extend ActiveSupport::Concern + + class_methods do + def allowed_base_urls(*urls) + if const_defined?(:ALLOWED_BASE_URLS, false) + raise ArgumentError, + "#{name}.allowed_base_urls already configured — call it exactly once per class" + end + + allowed = urls.flatten + unless allowed.any? && allowed.all? { |url| url.is_a?(String) && url.present? } + raise ArgumentError, + "#{name}.allowed_base_urls requires at least one non-blank URL string (got #{allowed.inspect})" + end + + # Fail-closed: the first entry becomes the canonical fallback in + # `effective_base_url`, so a misconfigured allow-list (e.g. `http://`, + # embedded credentials, or a relative path) would undermine the whole + # SSRF defense. Require every entry to be an absolute HTTPS URL with + # no userinfo/query/fragment at declaration time. + invalid = allowed.find { |url| !BaseUrlAllowlistable.absolute_https_url?(url) } + if invalid + raise ArgumentError, + "#{name}.allowed_base_urls requires absolute HTTPS URLs without userinfo, query, or fragment (got #{invalid.inspect})" + end + + const_set(:ALLOWED_BASE_URLS, allowed.map { |url| url.dup.freeze }.freeze) + # The validator resolves the allow-list via `const_get` on each call so + # the inclusion check and `effective_base_url` can never drift. (A + # literal `in: allowed` would freeze a snapshot at registration time.) + validates :base_url, + inclusion: { in: ->(record) { record.class.const_get(:ALLOWED_BASE_URLS) } }, + allow_blank: true + end + end + + def self.absolute_https_url?(url) + uri = URI.parse(url) + uri.is_a?(URI::HTTPS) && + uri.host.to_s.present? && + uri.userinfo.to_s.empty? && + uri.query.to_s.empty? && + uri.fragment.to_s.empty? + rescue URI::InvalidURIError + false + end + + def effective_base_url + allowed = self.class.const_get(:ALLOWED_BASE_URLS) + url = base_url.presence || allowed.first + unless allowed.include?(url) + Rails.logger.warn("[SECURITY] Rejected #{self.class.name} base_url: #{url.inspect}") + return allowed.first + end + url + end +end diff --git a/app/models/lunchflow_item.rb b/app/models/lunchflow_item.rb index ba9830c6b39..f7a87d39852 100644 --- a/app/models/lunchflow_item.rb +++ b/app/models/lunchflow_item.rb @@ -153,7 +153,7 @@ def credentials_configured? api_key.present? end - def effective_base_url - base_url.presence || "https://lunchflow.app/api/v1" - end + # F-08: SSRF hardening — see BaseUrlAllowlistable. + include BaseUrlAllowlistable + allowed_base_urls "https://lunchflow.app/api/v1" end diff --git a/app/models/mercury_item.rb b/app/models/mercury_item.rb index eb062b2b918..8847aa30b89 100644 --- a/app/models/mercury_item.rb +++ b/app/models/mercury_item.rb @@ -171,7 +171,8 @@ def credentials_configured? token.present? end - def effective_base_url - base_url.presence || "https://api.mercury.com/api/v1" - end + # F-08: SSRF hardening — see BaseUrlAllowlistable. + include BaseUrlAllowlistable + allowed_base_urls "https://api.mercury.com/api/v1", + "https://api-sandbox.mercury.com/api/v1" end diff --git a/config/initializers/rack_attack.rb b/config/initializers/rack_attack.rb index 4a16a69bb0f..9b29999e9d3 100644 --- a/config/initializers/rack_attack.rb +++ b/config/initializers/rack_attack.rb @@ -16,6 +16,17 @@ class Rack::Attack request.ip if request.path.start_with?("/admin/") end + # Throttle web session creation (login) to slow down brute-force/password-spraying. + # NOTE: this is the Rails web session endpoint, not the OAuth token endpoint. + # Configurable via ENV: RACK_ATTACK_SESSION_LIMIT (default: 10), + # RACK_ATTACK_SESSION_PERIOD_SECONDS (default: 60). + throttle("sessions/create", + limit: ENV.fetch("RACK_ATTACK_SESSION_LIMIT", 10).to_i, + period: ENV.fetch("RACK_ATTACK_SESSION_PERIOD_SECONDS", 60).to_i.seconds + ) do |request| + request.ip if request.post? && request.path == "/sessions" + end + # Determine limits based on self-hosted mode self_hosted = Rails.application.config.app_mode.self_hosted? @@ -39,6 +50,46 @@ class Rack::Attack request.ip if request.path.start_with?("/api/") end + # F-06: Per-user OTP rate limiting on API login (mirrors web MFA: 5 attempts / 5 min). + # Without this, the mobile/API login endpoint accepted unlimited OTP attempts + # while the web flow enforced a 5-attempt / 5-minute lockout. Throttling by + # normalized email means attackers can't trivially rotate IPs to bypass it. + # Configurable via ENV: RACK_ATTACK_OTP_LIMIT (default: 5), + # RACK_ATTACK_OTP_PERIOD_SECONDS (default: 300). + # Helper for extracting a field from either form params or a JSON body + # without consuming the body for downstream middleware. Mobile clients POST + # JSON to /api/v1/auth/login, and Rack::Attack runs before Rails parses the + # JSON body into request.params — so we parse it ourselves and rewind. + module LoginRequestFields + def self.read(request, field) + value = request.params[field] + return value if value.is_a?(String) && value.present? + + content_type = request.get_header("CONTENT_TYPE").to_s + return nil unless content_type.include?("json") + + body = request.body.read + request.body.rewind + return nil if body.blank? + + parsed = JSON.parse(body) + parsed.is_a?(Hash) ? parsed[field] : nil + rescue JSON::ParserError + nil + end + end + + throttle("api/otp_attempts/email", + limit: ENV.fetch("RACK_ATTACK_OTP_LIMIT", 5).to_i, + period: ENV.fetch("RACK_ATTACK_OTP_PERIOD_SECONDS", 300).to_i.seconds + ) do |request| + if request.path == "/api/v1/auth/login" && request.post? + email = LoginRequestFields.read(request, "email") + otp_code = LoginRequestFields.read(request, "otp_code") + email.to_s.downcase.strip if email.is_a?(String) && otp_code.is_a?(String) && otp_code.present? + end + end + # Block requests that appear to be malicious blocklist("block malicious requests") do |request| # Block requests with suspicious user agents diff --git a/test/integration/rack_attack_test.rb b/test/integration/rack_attack_test.rb index 37fc0b65e5d..041ce2586a1 100644 --- a/test/integration/rack_attack_test.rb +++ b/test/integration/rack_attack_test.rb @@ -20,4 +20,67 @@ class RackAttackTest < ActionDispatch::IntegrationTest throttles = Rack::Attack.throttles.keys assert_includes throttles, "api/requests", "API requests should have rate limiting" end + + test "POST /sessions has rate limiting configured" do + # F-04/login-throttle: brute-force/password-spraying mitigation + throttles = Rack::Attack.throttles.keys + assert_includes throttles, "sessions/create", "Web session login should have rate limiting" + end + + test "API OTP login has per-user rate limiting configured" do + # F-06: mirror web MFA (5 attempts / 5 min) for API login OTP submissions + throttles = Rack::Attack.throttles.keys + assert_includes throttles, "api/otp_attempts/email", "API OTP login should have per-user rate limiting" + end + + # Behavioral tests — enable Rack::Attack just for these cases (it's disabled + # in the test env by default). `ensure` blocks restore global state so + # downstream tests aren't affected. + + test "POST /sessions throttles after session limit from the same IP" do + Rack::Attack.enabled = true + Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new + limit = ENV.fetch("RACK_ATTACK_SESSION_LIMIT", 10).to_i + + limit.times do |i| + post sessions_path, + params: { email: "throttle-test-#{i}@example.com", password: "wrong" }, + headers: { "REMOTE_ADDR" => "10.0.0.77" } + assert_not_equal 429, response.status, "request #{i + 1} should not be throttled" + end + + post sessions_path, + params: { email: "throttle-test-final@example.com", password: "wrong" }, + headers: { "REMOTE_ADDR" => "10.0.0.77" } + + assert_response :too_many_requests + ensure + Rack::Attack.enabled = false + Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new + end + + test "POST /api/v1/auth/login throttles OTP attempts per email for JSON bodies" do + Rack::Attack.enabled = true + Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new + limit = ENV.fetch("RACK_ATTACK_OTP_LIMIT", 5).to_i + + payload = { email: "otp-throttle@example.com", password: "wrong", otp_code: "000000" } + + limit.times do |i| + post "/api/v1/auth/login", + params: payload.to_json, + headers: { "CONTENT_TYPE" => "application/json" } + assert_not_equal 429, response.status, "JSON OTP request #{i + 1} should not be throttled" + end + + post "/api/v1/auth/login", + params: payload.to_json, + headers: { "CONTENT_TYPE" => "application/json" } + + assert_response :too_many_requests, + "OTP throttle should count JSON-body submissions (mobile clients)" + ensure + Rack::Attack.enabled = false + Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new + end end diff --git a/test/models/concerns/base_url_allowlistable_test.rb b/test/models/concerns/base_url_allowlistable_test.rb new file mode 100644 index 00000000000..31a1397bf02 --- /dev/null +++ b/test/models/concerns/base_url_allowlistable_test.rb @@ -0,0 +1,104 @@ +require "test_helper" + +class BaseUrlAllowlistableTest < ActiveSupport::TestCase + # Helper: build a throwaway class that can host the concern without + # persistence — just enough to exercise the DSL surface. + def anonymous_class(name: "TestAllowlistItem") + klass = Class.new do + include ActiveModel::Validations + include BaseUrlAllowlistable + end + klass.define_singleton_method(:name) { name } + klass + end + + test "raises if allowed_base_urls is declared twice on the same class" do + # A second call would previously leave ALLOWED_BASE_URLS stale (const_set + # is guarded) while appending a second `validates` with the new list — + # the model and validator could silently disagree. Now we fail loudly. + klass = anonymous_class + klass.class_eval { allowed_base_urls "https://first.example.com" } + + assert_raises(ArgumentError, "should reject double configuration") do + klass.class_eval { allowed_base_urls "https://second.example.com" } + end + end + + test "raises when the allowlist is empty" do + klass = anonymous_class(name: "EmptyAllowlistItem") + assert_raises(ArgumentError) { klass.class_eval { allowed_base_urls } } + end + + test "raises when the allowlist contains non-string entries" do + klass = anonymous_class(name: "NonStringAllowlistItem") + assert_raises(ArgumentError) do + klass.class_eval { allowed_base_urls "https://ok.example.com", :symbol } + end + end + + test "raises when the allowlist contains blank strings" do + klass = anonymous_class(name: "BlankAllowlistItem") + assert_raises(ArgumentError) do + klass.class_eval { allowed_base_urls "https://ok.example.com", "" } + end + end + + test "raises when the allowlist contains nil" do + klass = anonymous_class(name: "NilAllowlistItem") + assert_raises(ArgumentError) do + klass.class_eval { allowed_base_urls "https://ok.example.com", nil } + end + end + + # URL-shape validation at declaration time — fail-closed against SSRF + # footguns in the fallback `effective_base_url` path. + + test "rejects http:// (non-HTTPS)" do + klass = anonymous_class(name: "HttpAllowlistItem") + assert_raises(ArgumentError) do + klass.class_eval { allowed_base_urls "http://insecure.example.com/api" } + end + end + + test "rejects relative paths" do + klass = anonymous_class(name: "RelativeAllowlistItem") + assert_raises(ArgumentError) do + klass.class_eval { allowed_base_urls "/api/v1" } + end + end + + test "rejects URLs with embedded userinfo" do + klass = anonymous_class(name: "UserinfoAllowlistItem") + assert_raises(ArgumentError) do + klass.class_eval { allowed_base_urls "https://admin:secret@internal.example.com/api" } + end + end + + test "rejects URLs with a query string" do + klass = anonymous_class(name: "QueryAllowlistItem") + assert_raises(ArgumentError) do + klass.class_eval { allowed_base_urls "https://api.example.com/v1?secret=1" } + end + end + + test "rejects URLs with a fragment" do + klass = anonymous_class(name: "FragmentAllowlistItem") + assert_raises(ArgumentError) do + klass.class_eval { allowed_base_urls "https://api.example.com/v1#frag" } + end + end + + test "rejects syntactically invalid URIs" do + klass = anonymous_class(name: "InvalidAllowlistItem") + assert_raises(ArgumentError) do + klass.class_eval { allowed_base_urls "https://exa mple.com/api" } + end + end + + test "accepts a plain absolute HTTPS URL with path" do + klass = anonymous_class(name: "ValidAllowlistItem") + assert_nothing_raised do + klass.class_eval { allowed_base_urls "https://api.example.com/v1" } + end + end +end diff --git a/test/models/lunchflow_item_test.rb b/test/models/lunchflow_item_test.rb new file mode 100644 index 00000000000..85ec679c613 --- /dev/null +++ b/test/models/lunchflow_item_test.rb @@ -0,0 +1,29 @@ +require "test_helper" + +class LunchflowItemTest < ActiveSupport::TestCase + def setup + @lunchflow_item = lunchflow_items(:one) + end + + test "effective_base_url returns default when base_url blank" do + @lunchflow_item.base_url = nil + assert_equal "https://lunchflow.app/api/v1", @lunchflow_item.effective_base_url + end + + test "effective_base_url returns base_url when in allowlist" do + @lunchflow_item.base_url = "https://lunchflow.app/api/v1" + assert_equal "https://lunchflow.app/api/v1", @lunchflow_item.effective_base_url + end + + test "effective_base_url rejects unknown base_url and falls back to default (F-08 SSRF)" do + @lunchflow_item.base_url = "http://169.254.169.254/latest/meta-data" + Rails.logger.expects(:warn).with(regexp_matches(/\[SECURITY\] Rejected LunchflowItem base_url/)) + assert_equal LunchflowItem::ALLOWED_BASE_URLS.first, @lunchflow_item.effective_base_url + end + + test "validates base_url against the allowlist at save time (F-08)" do + @lunchflow_item.base_url = "http://169.254.169.254/" + assert_not @lunchflow_item.valid?, "invalid base_url should fail AR validation" + assert_includes @lunchflow_item.errors[:base_url], "is not included in the list" + end +end diff --git a/test/models/mercury_item_test.rb b/test/models/mercury_item_test.rb index 365697ec450..9cbdab10ca5 100644 --- a/test/models/mercury_item_test.rb +++ b/test/models/mercury_item_test.rb @@ -31,6 +31,28 @@ def setup assert_equal "https://api.mercury.com/api/v1", @mercury_item.effective_base_url end + test "effective_base_url rejects unknown base_url and falls back to default (F-08 SSRF)" do + @mercury_item.base_url = "http://169.254.169.254/latest/meta-data" + Rails.logger.expects(:warn).with(regexp_matches(/\[SECURITY\] Rejected MercuryItem base_url/)) + assert_equal MercuryItem::ALLOWED_BASE_URLS.first, @mercury_item.effective_base_url + end + + test "effective_base_url allows sandbox URL (F-08 SSRF allowlist)" do + @mercury_item.base_url = "https://api-sandbox.mercury.com/api/v1" + assert_equal "https://api-sandbox.mercury.com/api/v1", @mercury_item.effective_base_url + end + + test "validates base_url against the allowlist at save time (F-08)" do + @mercury_item.base_url = "http://169.254.169.254/" + assert_not @mercury_item.valid?, "invalid base_url should fail AR validation" + assert_includes @mercury_item.errors[:base_url], "is not included in the list" + end + + test "allows blank base_url to fall back to the default at read time" do + @mercury_item.base_url = nil + assert @mercury_item.valid? + end + test "mercury_provider returns Provider::Mercury instance" do provider = @mercury_item.mercury_provider assert_instance_of Provider::Mercury, provider