-
Notifications
You must be signed in to change notification settings - Fork 6
security: rate limiting & SSRF allowlists (sessions throttle, OTP rate limit, provider base_url) #1520
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
dgilperez
wants to merge
8
commits into
we-promise:main
Choose a base branch
from
dgilperez:security/pr5-ratelimit-ssrf
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
security: rate limiting & SSRF allowlists (sessions throttle, OTP rate limit, provider base_url) #1520
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
d411364
fix(security): throttle POST /sessions via Rack::Attack
dgilperez 47c5f17
fix(security): F-06 rate limit OTP attempts on API login (Rack::Attack)
dgilperez dc09bb5
fix(security): F-08 SSRF allowlist for Mercury/Lunchflow base_url
dgilperez 55dea37
fix(security): address CodeRabbit review
dgilperez cbb0e42
fix(security): keep BaseUrlAllowlistable allowlist + validator in sync
dgilperez fa92355
fix(security): validate allowed_base_urls input at declaration time
dgilperez fe0511d
docs(security): correct BaseUrlAllowlistable concern header wording
dgilperez 717da1b
fix(security): fail-closed on malformed allowed_base_urls at declaration
dgilperez File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| # 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: "[email protected]", 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: "[email protected]", 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 | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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:[email protected]/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 |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.