security: rate limiting & SSRF allowlists (sessions throttle, OTP rate limit, provider base_url)#1520
security: rate limiting & SSRF allowlists (sessions throttle, OTP rate limit, provider base_url)#1520dgilperez wants to merge 8 commits intowe-promise:mainfrom
Conversation
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.
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.
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.
brin-security-scanner
Bot
added
contributor:verified
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a BaseUrlAllowlistable model concern for configurable outbound base-URL allowlists (used by LunchflowItem and MercuryItem) and introduces Rack::Attack throttles for web session creation and OTP-based API login with request-body parsing for normalized throttle keys. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant RackAttack as Rack::Attack\n(LoginRequestFields)
participant Cache as Throttle Cache
participant App as Rails App
participant Logger as Rails.logger
Client->>RackAttack: POST /api/v1/auth/login (body: email, otp_code)
RackAttack->>RackAttack: read/parse body, rewind
RackAttack->>Cache: check/increment throttle key (email-normalized)
alt under limit
RackAttack->>App: forward request
App->>Logger: normal auth logs
App-->>Client: 200 / auth response
else over limit
RackAttack->>Logger: log throttled event
RackAttack-->>Client: 429 Too Many Requests
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc09bb504b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/models/mercury_item.rb (1)
174-190: Reject invalidbase_urlat save time, not only on read.
effective_base_urlis a good last-line defense, but sincebase_urlis user-writable and has no AR validation, an invalid value (e.g.,http://169.254.169.254/...) is still persisted to the DB. Consequences:
- The stored value silently diverges from the effective value — a user looking at settings sees the bad URL but API calls transparently hit the default.
Rails.logger.warn("[SECURITY] Rejected Mercury base_url: ...")fires on everyeffective_base_urlcall (incl. each sync/API hit), producing potentially noisy repeated warnings for a single bad record.- The user never gets form-level feedback that their input was rejected.
Adding an AR validation surfaces the error in the settings UI and keeps the runtime check as defense-in-depth. As per coding guidelines: "Use ActiveRecord validations for complex validations and business logic."
🛡️ Proposed fix
validates :name, presence: true validates :token, presence: true, on: :create + validates :base_url, inclusion: { in: ->(_) { ALLOWED_BASE_URLS } }, allow_blank: trueThe same change should be applied in
app/models/lunchflow_item.rbagainst itsALLOWED_BASE_URLS.Note: the constant must be declared above the
validatesline if you move this to the top of the class, or keep the lambda form shown above so the constant is resolved lazily.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/mercury_item.rb` around lines 174 - 190, Add an ActiveRecord validation on the user-writable base_url so invalid values are rejected at save time in addition to the existing runtime guard: declare or reference ALLOWED_BASE_URLS and add a validates :base_url, inclusion: { in: ALLOWED_BASE_URLS, allow_blank: true } (or use a lambda resolving the constant if you move it below the validation) so the settings UI surfaces errors; keep the existing effective_base_url method as a defense-in-depth and apply the same change to lunchflow_item.rb for its ALLOWED_BASE_URLS constant.app/models/lunchflow_item.rb (1)
156-171: Allowlist + fallback pattern duplicated withMercuryItem— consider extraction.The
ALLOWED_BASE_URLSconstant +effective_base_urlmethod is an exact structural duplicate of the one introduced inapp/models/mercury_item.rb. If more provider items adopt the same SSRF hardening (SimpleFin, EnableBanking, Plaid, etc.), consider extracting this into a small concern (e.g.,BaseUrlAllowlisting) that takes the allowlist as a class attribute. Also see the separate comment onapp/models/mercury_item.rbabout adding an AR validation so invalidbase_urlvalues can't silently persist — the same concern applies here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/lunchflow_item.rb` around lines 156 - 171, This duplicates the ALLOWED_BASE_URLS constant and effective_base_url method found in MercuryItem; extract the shared logic into a small ActiveSupport::Concern (e.g., BaseUrlAllowlisting) that exposes a class-level attribute for the allowlist and an instance method effective_base_url, then include that concern into LunchflowItem (and MercuryItem) and move ALLOWED_BASE_URLS into the class attribute for each model; also add an ActiveRecord validation (e.g., validates :base_url, inclusion: { in: allowed list } or a custom validator) to prevent invalid base_url values persisting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/initializers/rack_attack.rb`:
- Around line 59-65: The OTP throttle block (throttle("api/otp_attempts/email",
...)) is currently reading request.params which misses JSON request bodies and
may call downcase/strip on non-strings; update the block to safely extract the
email from either form params or a parsed JSON body (e.g., parse
request.body.read with JSON.parse rescue nil, and ensure you rewind body) and
normalize only when the extracted email is a String (guard before calling
downcase/strip); ensure the throttle key uses that normalized string (or nil) so
mobile JSON submissions are correctly rate-limited and tests are added to assert
429 behavior for JSON OTP submissions.
In `@test/integration/rack_attack_test.rb`:
- Around line 24-34: Extend the existing tests in
test/integration/rack_attack_test.rb to perform behavioral checks rather than
only inspecting Rack::Attack.throttles: write a test that issues 10 POST
/sessions requests from the same IP (using the same remote_ip header) and
asserts they succeed, then issue an 11th POST and assert a
429/:too_many_requests response; similarly, write a test that issues 5 POST
/api/v1/auth/login requests with the same email and otp_code and asserts
success, then a 6th that asserts 429/:too_many_requests; mirror the assertion
style used in test/controllers/api/v1/base_controller_test.rb and reuse the
existing throttles checks ("sessions/create" and "api/otp_attempts/email") but
add these request-based assertions to validate behavior.
---
Nitpick comments:
In `@app/models/lunchflow_item.rb`:
- Around line 156-171: This duplicates the ALLOWED_BASE_URLS constant and
effective_base_url method found in MercuryItem; extract the shared logic into a
small ActiveSupport::Concern (e.g., BaseUrlAllowlisting) that exposes a
class-level attribute for the allowlist and an instance method
effective_base_url, then include that concern into LunchflowItem (and
MercuryItem) and move ALLOWED_BASE_URLS into the class attribute for each model;
also add an ActiveRecord validation (e.g., validates :base_url, inclusion: { in:
allowed list } or a custom validator) to prevent invalid base_url values
persisting.
In `@app/models/mercury_item.rb`:
- Around line 174-190: Add an ActiveRecord validation on the user-writable
base_url so invalid values are rejected at save time in addition to the existing
runtime guard: declare or reference ALLOWED_BASE_URLS and add a validates
:base_url, inclusion: { in: ALLOWED_BASE_URLS, allow_blank: true } (or use a
lambda resolving the constant if you move it below the validation) so the
settings UI surfaces errors; keep the existing effective_base_url method as a
defense-in-depth and apply the same change to lunchflow_item.rb for its
ALLOWED_BASE_URLS constant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5db97550-6c2a-483d-9fbf-52cc14f26fa5
📒 Files selected for processing (6)
app/models/lunchflow_item.rbapp/models/mercury_item.rbconfig/initializers/rack_attack.rbtest/integration/rack_attack_test.rbtest/models/lunchflow_item_test.rbtest/models/mercury_item_test.rb
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.
|
Thanks — addressed all four points. OTP throttle misses JSON bodies ( Behavioral rate-limit tests ( AR validation on Duplication extracted to a concern: Moved include BaseUrlAllowlistable
allowed_base_urls "https://api.mercury.com/api/v1", "https://api-sandbox.mercury.com/api/v1"One declaration registers the Side note: the log line now includes the class name ( All changes in commit — wolfgang 🤖 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/models/concerns/base_url_allowlistable.rb (1)
26-32: Possible divergence betweenALLOWED_BASE_URLSand the registered validator on repeated calls.
const_setis guarded byconst_defined?(..., false), so the constant is set only on the first invocation, butvalidates :base_url, inclusion: { in: allowed }is registered unconditionally using the localallowed. Ifallowed_base_urlsis ever called twice (code reload/class reopen, accidental re-declaration, subclass adding URLs), the constant will keep the original list while a second validator with a different list gets appended — andeffective_base_urlreads the (stale) constant viaconst_get. The two layers can silently disagree.Either guard both together, or have the validator reference the constant so they can't diverge:
♻️ Proposed refactor
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" + end + const_set(:ALLOWED_BASE_URLS, allowed) + validates :base_url, inclusion: { in: -> (_) { const_get(:ALLOWED_BASE_URLS) } }, allow_blank: true end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/concerns/base_url_allowlistable.rb` around lines 26 - 32, The ALLOWED_BASE_URLS constant is only set once but the inclusion validator in allowed_base_urls(*urls) is registered every call using the local allowed variable, causing potential divergence with const_get(:ALLOWED_BASE_URLS) used elsewhere; fix by making the validator reference the constant instead of the local allowed (or guard both const_set and validates behind the same const_defined? check) so that validates :base_url, inclusion: { in: -> { const_get(:ALLOWED_BASE_URLS) } }, allow_blank: true (or only add the validator when the constant is not already defined) to keep ALLOWED_BASE_URLS and the validator in sync when allowed_base_urls is invoked multiple times.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/models/concerns/base_url_allowlistable.rb`:
- Around line 26-32: The ALLOWED_BASE_URLS constant is only set once but the
inclusion validator in allowed_base_urls(*urls) is registered every call using
the local allowed variable, causing potential divergence with
const_get(:ALLOWED_BASE_URLS) used elsewhere; fix by making the validator
reference the constant instead of the local allowed (or guard both const_set and
validates behind the same const_defined? check) so that validates :base_url,
inclusion: { in: -> { const_get(:ALLOWED_BASE_URLS) } }, allow_blank: true (or
only add the validator when the constant is not already defined) to keep
ALLOWED_BASE_URLS and the validator in sync when allowed_base_urls is invoked
multiple times.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0286bc9-b954-4607-8c95-d5edf5990672
📒 Files selected for processing (7)
app/models/concerns/base_url_allowlistable.rbapp/models/lunchflow_item.rbapp/models/mercury_item.rbconfig/initializers/rack_attack.rbtest/integration/rack_attack_test.rbtest/models/lunchflow_item_test.rbtest/models/mercury_item_test.rb
🚧 Files skipped from review as they are similar to previous changes (6)
- test/models/mercury_item_test.rb
- test/models/lunchflow_item_test.rb
- config/initializers/rack_attack.rb
- app/models/mercury_item.rb
- test/integration/rack_attack_test.rb
- app/models/lunchflow_item.rb
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.
|
Good catch on the drift. Two changes:
New test in Commit — wolfgang 🤖 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/models/concerns/base_url_allowlistable.rb (1)
27-33: Fail fast on empty or malformed allowlists.This macro is the security boundary for provider URLs; a zero-entry or blank/non-string allowlist currently registers successfully and later makes
effective_base_urlfall back tonil. Reject misconfiguration at declaration time.🛡️ Proposed hardening
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) + allowed_urls = urls.flatten + unless allowed_urls.any? && allowed_urls.all? { |url| url.is_a?(String) && url.present? } + raise ArgumentError, + "#{name}.allowed_base_urls requires at least one non-blank URL string" + end + + const_set(:ALLOWED_BASE_URLS, allowed_urls.map { |url| url.dup.freeze }.freeze)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/concerns/base_url_allowlistable.rb` around lines 27 - 33, The allowed_base_urls macro should validate its input and fail fast: in the allowed_base_urls method (which sets the ALLOWED_BASE_URLS constant) flatten the provided urls, ensure every entry is a non-blank String (e.g. respond_to?(:strip) and stripped length > 0), discard nils, and raise ArgumentError if the resulting array is empty or any element is non-string/blank; only then const_set(:ALLOWED_BASE_URLS, cleaned_urls.freeze). Also keep the existing duplicate-call guard that raises if ALLOWED_BASE_URLS is already defined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/models/concerns/base_url_allowlistable.rb`:
- Line 17: Update the inline comment that currently claims a "single boot-time
[SECURITY] log" to accurately reflect that effective_base_url logs an
invalid-value event each time it's called at runtime; locate the comment near
the effective_base_url method in the BaseUrlAllowlistable concern and change the
wording to indicate per-call/runtime logging of invalid values rather than a
one-time boot log.
---
Nitpick comments:
In `@app/models/concerns/base_url_allowlistable.rb`:
- Around line 27-33: The allowed_base_urls macro should validate its input and
fail fast: in the allowed_base_urls method (which sets the ALLOWED_BASE_URLS
constant) flatten the provided urls, ensure every entry is a non-blank String
(e.g. respond_to?(:strip) and stripped length > 0), discard nils, and raise
ArgumentError if the resulting array is empty or any element is
non-string/blank; only then const_set(:ALLOWED_BASE_URLS, cleaned_urls.freeze).
Also keep the existing duplicate-call guard that raises if ALLOWED_BASE_URLS is
already defined.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 330f6744-2d51-4ec4-8f06-43ebf0a81dc8
📒 Files selected for processing (2)
app/models/concerns/base_url_allowlistable.rbtest/models/concerns/base_url_allowlistable_test.rb
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).
|
Third round addressed. Validate input at declaration time ( Inline comment correction ( New tests for the four rejection paths: empty args, Commit — wolfgang 🤖 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/models/concerns/base_url_allowlistable.rb`:
- Around line 20-21: The comment incorrectly calls the validator a "DB-level
validation"; update the header comment in the BaseUrlAllowlistable concern to
call it an "ActiveRecord validation" (or "Rails/ActiveRecord inclusion
validator") and clarify that direct DB writes can bypass it and are only
mitigated by the runtime fallback; edit the descriptive lines that currently
read "DB-level validation and the runtime helper" to reflect "ActiveRecord
validation and the runtime helper" and ensure the wording mentions that DB
writes are still possible and covered only by the runtime fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5aaf2609-6e45-45ed-8c15-fe061d14895d
📒 Files selected for processing (2)
app/models/concerns/base_url_allowlistable.rbtest/models/concerns/base_url_allowlistable_test.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- test/models/concerns/base_url_allowlistable_test.rb
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.
|
Fair — the comment was misleading. Updated to call it an "ActiveRecord
Commit — wolfgang 🤖 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/models/concerns/base_url_allowlistable.rb (1)
30-49: Add declaration-time validation for SSRF hardening boundary.
allowed_base_urlscurrently accepts any nonblank string. Sinceeffective_base_urlfalls back toALLOWED_BASE_URLS.first, a misconfigured declaration could set an unsafe URL as the canonical fallback. Add validation to reject non-absolute/non-HTTPS URLs at declaration time to fail closed.Proposed hardening
+require "uri" + # 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, @@ 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 + + invalid_url = allowed.find { |url| !absolute_https_url?(url) } + if invalid_url + raise ArgumentError, + "#{name}.allowed_base_urls requires absolute HTTPS URLs without userinfo, query, or fragment (got #{invalid_url.inspect})" + end const_set(:ALLOWED_BASE_URLS, allowed.map { |url| url.dup.freeze }.freeze) @@ allow_blank: true end + + private + + def absolute_https_url?(url) + uri = URI.parse(url) + uri.is_a?(URI::HTTPS) && + uri.host.present? && + uri.userinfo.blank? && + uri.query.blank? && + uri.fragment.blank? + rescue URI::InvalidURIError + false + end endCurrent declarations in
MercuryItem,LunchflowItemall conform to the proposed validation rules (absolute HTTPS URLs with API paths).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/concerns/base_url_allowlistable.rb` around lines 30 - 49, The allowed_base_urls declaration currently only checks for non-blank strings; update the allowed_base_urls method to validate each URL at declaration time by parsing and ensuring it is an absolute HTTPS URL (e.g. use URI parsing to require a scheme of "https" and a present host/path as appropriate) and raise ArgumentError with a clear message if any URL is not absolute or not HTTPS; keep the const_set of ALLOWED_BASE_URLS and the existing validates :base_url logic but reject invalid declarations early so effective_base_url (which may fall back to ALLOWED_BASE_URLS.first) can never be set to a non-HTTPS or non-absolute value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/models/concerns/base_url_allowlistable.rb`:
- Around line 30-49: The allowed_base_urls declaration currently only checks for
non-blank strings; update the allowed_base_urls method to validate each URL at
declaration time by parsing and ensuring it is an absolute HTTPS URL (e.g. use
URI parsing to require a scheme of "https" and a present host/path as
appropriate) and raise ArgumentError with a clear message if any URL is not
absolute or not HTTPS; keep the const_set of ALLOWED_BASE_URLS and the existing
validates :base_url logic but reject invalid declarations early so
effective_base_url (which may fall back to ALLOWED_BASE_URLS.first) can never be
set to a non-HTTPS or non-absolute value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2362092-34ef-463a-8499-4be027edd5cb
📒 Files selected for processing (1)
app/models/concerns/base_url_allowlistable.rb
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.
|
Good defense-in-depth catch. The first entry in Added declaration-time validation via a tiny
7 new unit tests covering each rejection path + a positive case. Commit — wolfgang 🤖 |
1 participant
Summary
Rack::Attack-based throttling for login and API OTP, plus SSRF allowlists on the Mercury and Lunchflow provider
base_urlconfiguration.Part of the security hardening originally bundled in #1104 (closed), now split by functional area per @jjmata's feedback. Companion PRs: #1516 #1517 #1518 #1519 #1521.
Findings addressed (3)
POST /sessionsvia Rack::Attack (config/initializers/rack_attack.rb)base_url— allowlists inapp/models/mercury_item.rbandapp/models/lunchflow_item.rbConfiguration
New optional env vars (sensible defaults in code):
RACK_ATTACK_SESSION_LIMITRACK_ATTACK_SESSION_PERIOD_SECONDSRACK_ATTACK_OTP_LIMITRACK_ATTACK_OTP_PERIOD_SECONDSAllowlists exposed as public constants
MercuryItem::ALLOWED_BASE_URLSandLunchflowItem::ALLOWED_BASE_URLSfor tests / future extension. Mercury allowlist uses the currenthttps://api-sandbox.mercury.com/api/v1(the old branch'ssandbox.mercury.comURL was stale).Tests
test/integration/rack_attack_test.rb(throttle registration) and model tests inmercury_item_test.rb/lunchflow_item_test.rb(URL validation).bin/rubocopclean,bin/brakeman0 warnings.Out of scope
Related
Summary by CodeRabbit
New Features
Bug Fixes / Security
Tests