forked from maybe-finance/maybe
-
Notifications
You must be signed in to change notification settings - Fork 893
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 5 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,52 @@ | ||
| # 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 | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
| # 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) | ||
| 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 | ||
|
|
||
| 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
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,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 |
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 |
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
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.