From d4113641216cd4dfe8e0ae518c2f3d3ba0aa0c07 Mon Sep 17 00:00:00 2001
From: David Gil - Klaus <dgilperez@gmail.com>
Date: Sun, 19 Apr 2026 13:42:57 +0200
Subject: [PATCH 1/8] fix(security): throttle POST /sessions via Rack::Attack

Adds an IP-based Rack::Attack throttle on the Rails web session endpoint
(POST /sessions) to slow down brute-force and password-spraying attacks.
This complements the OAuth token endpoint throttle already in place for
API-based login flows.

Limit and period are configurable via:
- RACK_ATTACK_SESSION_LIMIT (default: 10)
- RACK_ATTACK_SESSION_PERIOD_SECONDS (default: 60)

Adds a coverage test asserting the throttle is registered.
---
 config/initializers/rack_attack.rb   | 11 +++++++++++
 test/integration/rack_attack_test.rb |  6 ++++++
 2 files changed, 17 insertions(+)

diff --git a/config/initializers/rack_attack.rb b/config/initializers/rack_attack.rb
index 4a16a69bb0f..639fcf287fc 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?
 
diff --git a/test/integration/rack_attack_test.rb b/test/integration/rack_attack_test.rb
index 37fc0b65e5d..43ab84cdf17 100644
--- a/test/integration/rack_attack_test.rb
+++ b/test/integration/rack_attack_test.rb
@@ -20,4 +20,10 @@ 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
 end

From 47c5f175b2e5471c93614daf644ad59ea3599b67 Mon Sep 17 00:00:00 2001
From: David Gil - Klaus <dgilperez@gmail.com>
Date: Sun, 19 Apr 2026 13:43:26 +0200
Subject: [PATCH 2/8] fix(security): F-06 rate limit OTP attempts on API login
 (Rack::Attack)

Previously the mobile/API login endpoint (POST /api/v1/auth/login) accepted
unlimited OTP attempts, while the web MFA flow enforced a 5-attempt / 5-minute
lockout (CWE-307). This adds a Rack::Attack throttle keyed on the normalized
email address of the login attempt so attackers cannot trivially rotate IPs
to bypass the lockout.

Defaults mirror web MFA (5 attempts per 5 minutes) and are configurable via:
- RACK_ATTACK_OTP_LIMIT (default: 5)
- RACK_ATTACK_OTP_PERIOD_SECONDS (default: 300)

The throttle is applied at the middleware layer so it short-circuits before
the controller runs, which is simpler and more robust than an in-controller
Rails.cache counter.

Adds a coverage test asserting the throttle is registered.
---
 config/initializers/rack_attack.rb   | 15 +++++++++++++++
 test/integration/rack_attack_test.rb |  6 ++++++
 2 files changed, 21 insertions(+)

diff --git a/config/initializers/rack_attack.rb b/config/initializers/rack_attack.rb
index 639fcf287fc..4fe125b8149 100644
--- a/config/initializers/rack_attack.rb
+++ b/config/initializers/rack_attack.rb
@@ -50,6 +50,21 @@ 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).
+  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? && request.params["otp_code"].present?
+      request.params["email"]&.downcase&.strip
+    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 43ab84cdf17..60118045e2c 100644
--- a/test/integration/rack_attack_test.rb
+++ b/test/integration/rack_attack_test.rb
@@ -26,4 +26,10 @@ class RackAttackTest < ActionDispatch::IntegrationTest
     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
 end

From dc09bb504b0a9e1350d9fa6abc75f634a1b20ed0 Mon Sep 17 00:00:00 2001
From: David Gil - Klaus <dgilperez@gmail.com>
Date: Sun, 19 Apr 2026 13:45:03 +0200
Subject: [PATCH 3/8] fix(security): F-08 SSRF allowlist for Mercury/Lunchflow
 base_url
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The Mercury and Lunchflow provider settings allow users to override the
provider base_url via a free-form text field. Without validation, a
malicious (or compromised) user could redirect outbound credentialed
requests at internal services — cloud metadata endpoints
(169.254.169.254), localhost, or internal DNS — resulting in
Server-Side Request Forgery.

Restrict `effective_base_url` to a known-good allowlist per provider.
Unknown values are rejected (logged under [SECURITY]) and the default
production endpoint is used instead. For Mercury both the production
and the current `api-sandbox.mercury.com` endpoint are allowed, since
the sandbox is a documented legitimate value in the Mercury settings
panel.

Adds tests covering default fallback, sandbox allowance (Mercury), and
rejection of metadata-style URLs for both providers.
---
 app/models/lunchflow_item.rb       | 15 ++++++++++++++-
 app/models/mercury_item.rb         | 16 +++++++++++++++-
 test/models/lunchflow_item_test.rb | 23 +++++++++++++++++++++++
 test/models/mercury_item_test.rb   | 11 +++++++++++
 4 files changed, 63 insertions(+), 2 deletions(-)
 create mode 100644 test/models/lunchflow_item_test.rb

diff --git a/app/models/lunchflow_item.rb b/app/models/lunchflow_item.rb
index ba9830c6b39..1be27224d71 100644
--- a/app/models/lunchflow_item.rb
+++ b/app/models/lunchflow_item.rb
@@ -153,7 +153,20 @@ def credentials_configured?
     api_key.present?
   end
 
+  # F-08: SSRF hardening. The `base_url` column is user-writable via the
+  # Lunchflow settings panel; without validation a malicious user could point
+  # outbound requests at internal services (169.254.169.254, localhost,
+  # internal DNS, etc.). Restrict to a known-good Lunchflow endpoint.
+  ALLOWED_BASE_URLS = [
+    "https://lunchflow.app/api/v1"
+  ].freeze
+
   def effective_base_url
-    base_url.presence || "https://lunchflow.app/api/v1"
+    url = base_url.presence || ALLOWED_BASE_URLS.first
+    unless ALLOWED_BASE_URLS.include?(url)
+      Rails.logger.warn("[SECURITY] Rejected Lunchflow base_url: #{url.inspect}")
+      return ALLOWED_BASE_URLS.first
+    end
+    url
   end
 end
diff --git a/app/models/mercury_item.rb b/app/models/mercury_item.rb
index eb062b2b918..28b97ceaf1e 100644
--- a/app/models/mercury_item.rb
+++ b/app/models/mercury_item.rb
@@ -171,7 +171,21 @@ def credentials_configured?
     token.present?
   end
 
+  # F-08: SSRF hardening. The `base_url` column is user-writable via the
+  # Mercury settings panel; without validation a malicious user could point
+  # outbound requests at internal services (169.254.169.254, localhost,
+  # internal DNS, etc.). Restrict to a known-good set of Mercury endpoints.
+  ALLOWED_BASE_URLS = [
+    "https://api.mercury.com/api/v1",
+    "https://api-sandbox.mercury.com/api/v1"
+  ].freeze
+
   def effective_base_url
-    base_url.presence || "https://api.mercury.com/api/v1"
+    url = base_url.presence || ALLOWED_BASE_URLS.first
+    unless ALLOWED_BASE_URLS.include?(url)
+      Rails.logger.warn("[SECURITY] Rejected Mercury base_url: #{url.inspect}")
+      return ALLOWED_BASE_URLS.first
+    end
+    url
   end
 end
diff --git a/test/models/lunchflow_item_test.rb b/test/models/lunchflow_item_test.rb
new file mode 100644
index 00000000000..0ac9352f6b2
--- /dev/null
+++ b/test/models/lunchflow_item_test.rb
@@ -0,0 +1,23 @@
+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 Lunchflow base_url/))
+    assert_equal LunchflowItem::ALLOWED_BASE_URLS.first, @lunchflow_item.effective_base_url
+  end
+end
diff --git a/test/models/mercury_item_test.rb b/test/models/mercury_item_test.rb
index 365697ec450..776368e7b2e 100644
--- a/test/models/mercury_item_test.rb
+++ b/test/models/mercury_item_test.rb
@@ -31,6 +31,17 @@ 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 Mercury 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 "mercury_provider returns Provider::Mercury instance" do
     provider = @mercury_item.mercury_provider
     assert_instance_of Provider::Mercury, provider

From 55dea37d037f6293c2bd28fb365f5de288aa77e4 Mon Sep 17 00:00:00 2001
From: David Gil - Klaus <dgilperez@gmail.com>
Date: Sun, 19 Apr 2026 17:05:57 +0200
Subject: [PATCH 4/8] fix(security): address CodeRabbit review
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Inline:
- OTP throttle for /api/v1/auth/login now extracts `email`/`otp_code` from
  either form params OR a JSON body (mobile clients POST JSON, which Rack::Attack
  does NOT see via `request.params` — it runs before Rails parses the body).
  Body is rewound so downstream middleware still reads it. Helper module
  LoginRequestFields keeps the logic tidy and testable.
- New behavioral integration tests: after N posts at the configured limit,
  the N+1 request returns 429. Covers both `/sessions` (form-encoded) and the
  JSON path on `/api/v1/auth/login` (regression for the body-parse fix above).
  Rack::Attack is toggled on per-test and the cache store is reset in ensure
  blocks so other tests aren't affected.

Nits:
- Extracted ALLOWED_BASE_URLS + effective_base_url into a
  BaseUrlAllowlistable concern used by MercuryItem and LunchflowItem.
  Eliminates duplication and makes it trivial to apply the same hardening
  to other provider items later.
- Added an AR `inclusion` validation on `base_url` (via the concern) so
  invalid values are rejected at save time. The settings UI now surfaces
  form-level errors instead of silently persisting a bad URL and only
  catching it at read time. The runtime fallback in effective_base_url is
  kept as defense-in-depth (values written via rake, console, or direct
  DB updates still get logged and coerced).

Adjusted the existing mercury/lunchflow test expectations to match the
concern-emitted log message ("Rejected MercuryItem base_url: ..." rather
than "Rejected Mercury base_url: ...") — the class name is more grep-able.
---
 app/models/concerns/base_url_allowlistable.rb | 43 ++++++++++++++++
 app/models/lunchflow_item.rb                  | 19 ++-----
 app/models/mercury_item.rb                    | 21 ++------
 config/initializers/rack_attack.rb            | 29 ++++++++++-
 test/integration/rack_attack_test.rb          | 51 +++++++++++++++++++
 test/models/lunchflow_item_test.rb            |  8 ++-
 test/models/mercury_item_test.rb              | 13 ++++-
 7 files changed, 147 insertions(+), 37 deletions(-)
 create mode 100644 app/models/concerns/base_url_allowlistable.rb

diff --git a/app/models/concerns/base_url_allowlistable.rb b/app/models/concerns/base_url_allowlistable.rb
new file mode 100644
index 00000000000..5d95f816fd7
--- /dev/null
+++ b/app/models/concerns/base_url_allowlistable.rb
@@ -0,0 +1,43 @@
+# 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 (array of 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
+#     with a single boot-time [SECURITY] log when an invalid value sneaks through
+#
+# Both the DB-level validation and the runtime helper are kept as
+# defense-in-depth: validation catches bad input at the UI boundary, the
+# helper guards against values written through rake tasks, console sessions,
+# or direct DB updates.
+module BaseUrlAllowlistable
+  extend ActiveSupport::Concern
+
+  class_methods do
+    def allowed_base_urls(*urls)
+      allowed = urls.flatten.freeze
+      const_set(:ALLOWED_BASE_URLS, allowed) unless const_defined?(:ALLOWED_BASE_URLS, false)
+      validates :base_url, inclusion: { in: allowed }, allow_blank: true
+    end
+  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 1be27224d71..f7a87d39852 100644
--- a/app/models/lunchflow_item.rb
+++ b/app/models/lunchflow_item.rb
@@ -153,20 +153,7 @@ def credentials_configured?
     api_key.present?
   end
 
-  # F-08: SSRF hardening. The `base_url` column is user-writable via the
-  # Lunchflow settings panel; without validation a malicious user could point
-  # outbound requests at internal services (169.254.169.254, localhost,
-  # internal DNS, etc.). Restrict to a known-good Lunchflow endpoint.
-  ALLOWED_BASE_URLS = [
-    "https://lunchflow.app/api/v1"
-  ].freeze
-
-  def effective_base_url
-    url = base_url.presence || ALLOWED_BASE_URLS.first
-    unless ALLOWED_BASE_URLS.include?(url)
-      Rails.logger.warn("[SECURITY] Rejected Lunchflow base_url: #{url.inspect}")
-      return ALLOWED_BASE_URLS.first
-    end
-    url
-  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 28b97ceaf1e..8847aa30b89 100644
--- a/app/models/mercury_item.rb
+++ b/app/models/mercury_item.rb
@@ -171,21 +171,8 @@ def credentials_configured?
     token.present?
   end
 
-  # F-08: SSRF hardening. The `base_url` column is user-writable via the
-  # Mercury settings panel; without validation a malicious user could point
-  # outbound requests at internal services (169.254.169.254, localhost,
-  # internal DNS, etc.). Restrict to a known-good set of Mercury endpoints.
-  ALLOWED_BASE_URLS = [
-    "https://api.mercury.com/api/v1",
-    "https://api-sandbox.mercury.com/api/v1"
-  ].freeze
-
-  def effective_base_url
-    url = base_url.presence || ALLOWED_BASE_URLS.first
-    unless ALLOWED_BASE_URLS.include?(url)
-      Rails.logger.warn("[SECURITY] Rejected Mercury base_url: #{url.inspect}")
-      return ALLOWED_BASE_URLS.first
-    end
-    url
-  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 4fe125b8149..9b29999e9d3 100644
--- a/config/initializers/rack_attack.rb
+++ b/config/initializers/rack_attack.rb
@@ -56,12 +56,37 @@ class Rack::Attack
   # 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? && request.params["otp_code"].present?
-      request.params["email"]&.downcase&.strip
+    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
 
diff --git a/test/integration/rack_attack_test.rb b/test/integration/rack_attack_test.rb
index 60118045e2c..041ce2586a1 100644
--- a/test/integration/rack_attack_test.rb
+++ b/test/integration/rack_attack_test.rb
@@ -32,4 +32,55 @@ class RackAttackTest < ActionDispatch::IntegrationTest
     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/lunchflow_item_test.rb b/test/models/lunchflow_item_test.rb
index 0ac9352f6b2..85ec679c613 100644
--- a/test/models/lunchflow_item_test.rb
+++ b/test/models/lunchflow_item_test.rb
@@ -17,7 +17,13 @@ def setup
 
   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 Lunchflow base_url/))
+    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 776368e7b2e..9cbdab10ca5 100644
--- a/test/models/mercury_item_test.rb
+++ b/test/models/mercury_item_test.rb
@@ -33,7 +33,7 @@ def setup
 
   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 Mercury base_url/))
+    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
 
@@ -42,6 +42,17 @@ def setup
     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

From cbb0e42d162f5322234d1613b00038c3d61e34af Mon Sep 17 00:00:00 2001
From: David Gil - Klaus <dgilperez@gmail.com>
Date: Sun, 19 Apr 2026 18:27:55 +0200
Subject: [PATCH 5/8] fix(security): keep BaseUrlAllowlistable allowlist +
 validator in sync
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

CodeRabbit follow-up: `const_set` was guarded by `const_defined?` but
`validates` was unconditional. A second call to `allowed_base_urls`
(class reload, subclass, accidental re-declaration) would silently
leave `ALLOWED_BASE_URLS` as the first invocation's list while appending
a second `inclusion` validator bound to a snapshot of the second list —
so `effective_base_url` (const_get) and the validator could disagree
about what's legal.

Two changes:
- `allowed_base_urls` now raises ArgumentError on a second call. Fail
  loud at boot instead of latent drift.
- The validator resolves the allow-list via `const_get` on each call
  instead of closing over the local `allowed`, so even if someone
  mutates the constant out-of-band (rake task, console), `effective_base_url`
  and the inclusion check stay in lockstep.

New concern test asserts the double-configuration raises.
---
 app/models/concerns/base_url_allowlistable.rb | 15 +++++++++---
 .../concerns/base_url_allowlistable_test.rb   | 23 +++++++++++++++++++
 2 files changed, 35 insertions(+), 3 deletions(-)
 create mode 100644 test/models/concerns/base_url_allowlistable_test.rb

diff --git a/app/models/concerns/base_url_allowlistable.rb b/app/models/concerns/base_url_allowlistable.rb
index 5d95f816fd7..709ad8849f4 100644
--- a/app/models/concerns/base_url_allowlistable.rb
+++ b/app/models/concerns/base_url_allowlistable.rb
@@ -25,9 +25,18 @@ module BaseUrlAllowlistable
 
   class_methods do
     def allowed_base_urls(*urls)
-      allowed = urls.flatten.freeze
-      const_set(:ALLOWED_BASE_URLS, allowed) unless const_defined?(:ALLOWED_BASE_URLS, false)
-      validates :base_url, inclusion: { in: allowed }, allow_blank: true
+      if const_defined?(:ALLOWED_BASE_URLS, false)
+        raise ArgumentError,
+          "#{name}.allowed_base_urls already configured — call it exactly once per class"
+      end
+
+      const_set(:ALLOWED_BASE_URLS, urls.flatten.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
 
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..5b30b2d88da
--- /dev/null
+++ b/test/models/concerns/base_url_allowlistable_test.rb
@@ -0,0 +1,23 @@
+require "test_helper"
+
+class BaseUrlAllowlistableTest < ActiveSupport::TestCase
+  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 = Class.new do
+      include ActiveModel::Validations
+      include BaseUrlAllowlistable
+      def self.name
+        "TestAllowlistItem"
+      end
+      allowed_base_urls "https://first.example.com"
+    end
+
+    assert_raises(ArgumentError, "should reject double configuration") do
+      klass.class_eval do
+        allowed_base_urls "https://second.example.com"
+      end
+    end
+  end
+end

From fa9235581691ca27ee7b51ab9b8ef6d32a6d087c Mon Sep 17 00:00:00 2001
From: David Gil - Klaus <dgilperez@gmail.com>
Date: Sun, 19 Apr 2026 18:53:24 +0200
Subject: [PATCH 6/8] fix(security): validate allowed_base_urls input at
 declaration time
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

CodeRabbit round 3: `allowed_base_urls` trusted its input. An empty call,
a nil or blank entry, or a non-String entry (e.g. symbol, URI object) was
silently accepted and later made `effective_base_url` return nil or blow
up at validation time. Reject misconfiguration where it's most
debuggable: at boot.

Raises ArgumentError with a descriptive message when the flattened list
is empty, contains non-Strings, or contains blank strings. Deep-freezes
each URL so they can't be mutated through the constant later.

Also corrected an inline comment that misrepresented the [SECURITY]
logging as a "single boot-time" event — it's per-call at runtime.

New tests cover the four rejection paths (empty, :symbol, "", nil).
---
 app/models/concerns/base_url_allowlistable.rb | 13 +++--
 .../concerns/base_url_allowlistable_test.rb   | 51 +++++++++++++++----
 2 files changed, 50 insertions(+), 14 deletions(-)

diff --git a/app/models/concerns/base_url_allowlistable.rb b/app/models/concerns/base_url_allowlistable.rb
index 709ad8849f4..1dd77fb5e85 100644
--- a/app/models/concerns/base_url_allowlistable.rb
+++ b/app/models/concerns/base_url_allowlistable.rb
@@ -11,10 +11,11 @@
 #   end
 #
 # Provides:
-#   - ALLOWED_BASE_URLS class-level constant (array of strings)
+#   - 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
-#     with a single boot-time [SECURITY] log when an invalid value sneaks through
+#     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)
 #
 # Both the DB-level validation and the runtime helper are kept as
 # defense-in-depth: validation catches bad input at the UI boundary, the
@@ -30,7 +31,13 @@ def allowed_base_urls(*urls)
           "#{name}.allowed_base_urls already configured — call it exactly once per class"
       end
 
-      const_set(:ALLOWED_BASE_URLS, urls.flatten.freeze)
+      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
+
+      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.)
diff --git a/test/models/concerns/base_url_allowlistable_test.rb b/test/models/concerns/base_url_allowlistable_test.rb
index 5b30b2d88da..d9332ae07b1 100644
--- a/test/models/concerns/base_url_allowlistable_test.rb
+++ b/test/models/concerns/base_url_allowlistable_test.rb
@@ -1,23 +1,52 @@
 require "test_helper"
 
 class BaseUrlAllowlistableTest < ActiveSupport::TestCase
-  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.
+  # 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
-      def self.name
-        "TestAllowlistItem"
-      end
-      allowed_base_urls "https://first.example.com"
     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 do
-        allowed_base_urls "https://second.example.com"
-      end
+      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
 end

From fe0511dad25b47b38eb2559480d91c8e0e08deaf Mon Sep 17 00:00:00 2001
From: David Gil - Klaus <dgilperez@gmail.com>
Date: Sun, 19 Apr 2026 19:21:21 +0200
Subject: [PATCH 7/8] docs(security): correct BaseUrlAllowlistable concern
 header wording
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

CodeRabbit round 4: the concern's doc comment called the inclusion
validator a 'DB-level validation', which is misleading — it's an
ActiveRecord validation that can be bypassed by any path that writes
without going through AR (update_columns, raw SQL, a rake task using
direct DB access). Corrected the wording and spelled out what each
layer actually catches.
---
 app/models/concerns/base_url_allowlistable.rb | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/app/models/concerns/base_url_allowlistable.rb b/app/models/concerns/base_url_allowlistable.rb
index 1dd77fb5e85..47abb641d4f 100644
--- a/app/models/concerns/base_url_allowlistable.rb
+++ b/app/models/concerns/base_url_allowlistable.rb
@@ -17,10 +17,12 @@
 #     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)
 #
-# Both the DB-level validation and the runtime helper are kept as
-# defense-in-depth: validation catches bad input at the UI boundary, the
-# helper guards against values written through rake tasks, console sessions,
-# or direct DB updates.
+# 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.
 module BaseUrlAllowlistable
   extend ActiveSupport::Concern
 

From 717da1bdd1f9ded38ee25acc69939dd1ab76fd1d Mon Sep 17 00:00:00 2001
From: David Gil - Klaus <dgilperez@gmail.com>
Date: Sun, 19 Apr 2026 19:43:56 +0200
Subject: [PATCH 8/8] fix(security): fail-closed on malformed allowed_base_urls
 at declaration

CodeRabbit round 5: the first entry in ALLOWED_BASE_URLS becomes the
canonical fallback returned by effective_base_url when an invalid value
is encountered. A misconfigured declaration (http://, relative path,
embedded credentials) would undermine the whole F-08 SSRF defense by
making a non-HTTPS or internal URL the implicit escape hatch.

Added declaration-time validation:
- Must be absolute HTTPS
- Must have a host
- Must NOT carry userinfo (e.g., https://admin:secret@host), a query,
  or a fragment
- Must parse as a valid URI

7 new unit tests cover each rejection path + a positive case.
---
 app/models/concerns/base_url_allowlistable.rb | 24 +++++++++
 .../concerns/base_url_allowlistable_test.rb   | 52 +++++++++++++++++++
 2 files changed, 76 insertions(+)

diff --git a/app/models/concerns/base_url_allowlistable.rb b/app/models/concerns/base_url_allowlistable.rb
index 47abb641d4f..3893f76b04e 100644
--- a/app/models/concerns/base_url_allowlistable.rb
+++ b/app/models/concerns/base_url_allowlistable.rb
@@ -23,6 +23,8 @@
 # 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
 
@@ -39,6 +41,17 @@ def allowed_base_urls(*urls)
           "#{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
@@ -49,6 +62,17 @@ def allowed_base_urls(*urls)
     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
diff --git a/test/models/concerns/base_url_allowlistable_test.rb b/test/models/concerns/base_url_allowlistable_test.rb
index d9332ae07b1..31a1397bf02 100644
--- a/test/models/concerns/base_url_allowlistable_test.rb
+++ b/test/models/concerns/base_url_allowlistable_test.rb
@@ -49,4 +49,56 @@ def anonymous_class(name: "TestAllowlistItem")
       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