Files
sure/test/integration/rack_attack_test.rb
T
78228e68dd security: throttle every credential-guessing endpoint, fix duplicate Rack::Attack middleware (#3263)
* security: throttle every credential-guessing endpoint, fix duplicate Rack::Attack middleware

Follow-up on #1087 (Findings H4, M7). PR 4 of the 6-PR series.

Enumerated every endpoint that checks a password, TOTP code, or backup
code (grepped for User.authenticate_by/#authenticate/#verify_otp? across
app/controllers, not just the ones named in the issue) — six in total,
none previously throttled:

- POST /sessions (SessionsController#create) — web login
- POST /mfa/verify (MfaController#verify_code) — TOTP + backup codes
  (#verify_otp? handles both internally, so no separate endpoint to add)
- POST /password_reset (PasswordResetsController#create) — also M7
- POST /api/v1/auth/login (Api::V1::AuthController#login) — mobile/API
- POST /oidc_account/create_link (OidcAccountsController#create_link) —
  password check gating SSO-identity linking, not sign-in; easy to miss
  grepping routes.rb for "session"/"login"
- POST /api/v1/auth/sso_link (Api::V1::AuthController#sso_link) — same
  as above for the mobile app

Each gets two throttles (ip AND normalized email, or ip AND the MFA
step-up's session-bound user id where there's no email param) so an
attacker can't bypass by rotating IPs against one target, nor by
spraying many emails from one IP — Rack::Attack requires every matching
throttle to pass. limit: 10/minute, matching the existing oauth/token
and admin/ip throttles already in this file.

Also fixed a latent, unrelated-but-adjacent bug found while confirming
these throttles would actually enforce the limits documented in their
own comments: config/application.rb had an explicit `config.middleware.use
Rack::Attack` alongside the gem's own Railtie doing the same thing (`bin/rails
middleware` listed it twice) — every throttle's counter was incrementing
twice per request, so all of them, old and new, were silently firing at
half their documented limit. Removed the redundant explicit registration.

Race-condition check (per standing instruction): Rack::Attack's counter
increments are atomic within its cache store, so concurrent requests at
the threshold don't undercount. No new race introduced.

New tests in test/integration/rack_attack_test.rb:
- Registration checks for all 6 new throttle keys (existing convention
  in this file).
- Direct block-level tests for the discriminator logic (right path
  matched, right value extracted, blank/missing input produces nil
  rather than a bogus key) — Rack::Attack's cache backs onto Rails.cache,
  which is :null_store in the test environment, so no amount of request
  volume in a normal integration test can ever actually trip a throttle
  here; calling the registered block directly against a constructed
  Rack::Attack::Request is what makes the assertions meaningful instead
  of just checking string keys exist.
- Regression test asserting Rack::Attack appears exactly once in the
  middleware stack.

Verified against the NAS sure_test_web container: full restart, bin/rails
test (8/8 rack_attack tests green; ran the full test/integration suite
plus sessions/mfa/password_resets/api-auth/oidc_accounts controller tests
too — 6 pre-existing failures, confirmed identical on the unmodified
baseline before concluding they're the known WebAuthn-RP-ID-mismatch and
AI-disabled environmental categories, not a regression), bin/rubocop,
bin/brakeman. Also did a live demonstration against the running container
(which runs RAILS_ENV=production, where Rack::Attack is actually enabled):
12 rapid POSTs to /sessions with bad credentials — requests 1-10 got 422,
11 and 12 got 429, exactly matching limit: 10. Container restored to its
original state and restarted afterward.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* security: extract email from JSON bodies for credential-guess throttles

Rack::Attack runs before Rails' JSON parameter parsing, so request.params
only exposed query/form fields. The documented api/v1/auth/login and
.../sso_link JSON format bypassed the per-email throttle entirely,
letting an attacker rotate IPs against one target's account.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* security: guard JSON email peek against non-rewindable input and non-object payloads

Rack 3 no longer requires rack.input to be rewindable, and a bare
JSON.parse(body)["email"] raises NoMethodError on valid non-Hash JSON
(null, arrays, scalars) — either would 500 the request instead of just
skipping the email throttle.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* security: assert non-rewindable JSON bodies stay readable by the controller

Only checking that the throttle discriminator returned nil left a gap: an
implementation that read the body and then discarded the result on error
would pass the same assertion while leaving the controller with an
exhausted stream.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* security: close credential-guessing throttle bypass via format-suffixed paths

request.path == "/sessions" (etc.) never matched "/sessions.json", which
Rails still routes to the same controller action since none of these
routes are declared format: false. Match the optional format suffix
explicitly instead, per jjmata's review on PR #3263.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* security: match Rails' actual format-segment charset in credential_guess_path

\w excludes hyphens, but Rails' default (.:format) segment matches
[^./?]+, which does include them — e.g. "/api/v1/auth/login.rate-limit"
still routed and bypassed the throttle. Match the real charset instead,
per CodeRabbit's follow-up on PR #3263.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

---------

Co-authored-by: Gerald <248542187+gfr-free@users.noreply.github.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
2026-08-31 07:48:58 +02:00

202 lines
9.8 KiB
Ruby

# frozen_string_literal: true
require "test_helper"
class RackAttackTest < ActionDispatch::IntegrationTest
test "rack attack is configured" do
# Verify Rack::Attack is enabled in middleware stack
middleware_classes = Rails.application.middleware.map(&:klass)
assert_includes middleware_classes, Rack::Attack, "Rack::Attack should be in middleware stack"
end
test "rack attack is only inserted into the middleware stack once" do
# Regression guard: Rack::Attack's own Railtie already inserts it, and
# config/application.rb previously also called config.middleware.use
# Rack::Attack explicitly — the counters incremented twice per request,
# so every throttle limit fired at half its documented value.
middleware_classes = Rails.application.middleware.map(&:klass)
assert_equal 1, middleware_classes.count(Rack::Attack)
end
test "oauth token endpoint has rate limiting configured" do
# Test that the throttle is configured (we don't need to trigger it)
throttles = Rack::Attack.throttles.keys
assert_includes throttles, "oauth/token", "OAuth token endpoint should have rate limiting"
end
test "api requests have rate limiting configured" do
# Test that API rate limiting is configured
throttles = Rack::Attack.throttles.keys
assert_includes throttles, "api/requests", "API requests should have rate limiting"
end
test "credential-guessing surfaces have rate limiting configured" do
throttles = Rack::Attack.throttles.keys
%w[
logins/ip logins/email
mfa/verify/ip mfa/verify/user
password_resets/ip password_resets/email
oidc_account_link/ip oidc_account_link/email
api_login/ip api_login/email
api_sso_link/ip api_sso_link/email
].each do |name|
assert_includes throttles, name, "#{name} should have rate limiting configured"
end
end
# Rack::Attack's counters rely on Rails.cache, which is :null_store in the
# test environment (config/environments/test.rb) — a throttle can never
# actually fire here regardless of request volume, which is why the tests
# above only check registration. To still verify the matching logic itself
# (right path, right discriminator, blank-input handling), call each
# throttle's block directly against a constructed request instead of
# sending real requests through the stack.
test "login throttles discriminate by ip and normalized email, and ignore unrelated requests" do
ip_block = Rack::Attack.throttles["logins/ip"].block
email_block = Rack::Attack.throttles["logins/email"].block
login_request = throttle_request("/sessions", method: "POST", params: { "email" => " User@Example.com " })
assert_equal "203.0.113.5", ip_block.call(login_request)
assert_equal "user@example.com", email_block.call(login_request)
get_request = throttle_request("/sessions", method: "GET")
assert_nil ip_block.call(get_request), "GET requests must not count toward the throttle"
unrelated_path_request = throttle_request("/mfa/verify", method: "POST", params: { "email" => "x@example.com" })
assert_nil ip_block.call(unrelated_path_request)
blank_email_request = throttle_request("/sessions", method: "POST", params: {})
assert_nil email_block.call(blank_email_request), "a missing email must not produce a throttle key"
end
test "mfa verify throttle discriminates by the pending session user id, not email" do
ip_block = Rack::Attack.throttles["mfa/verify/ip"].block
user_block = Rack::Attack.throttles["mfa/verify/user"].block
request = throttle_request("/mfa/verify", method: "POST", session: { mfa_user_id: "abc-123" })
assert_equal "203.0.113.5", ip_block.call(request)
assert_equal "abc-123", user_block.call(request)
no_session_request = throttle_request("/mfa/verify", method: "POST")
assert_nil user_block.call(no_session_request)
end
test "api login and sso-link throttles match their own paths only" do
api_login_block = Rack::Attack.throttles["api_login/ip"].block
sso_link_block = Rack::Attack.throttles["api_sso_link/ip"].block
api_login_request = throttle_request("/api/v1/auth/login", method: "POST")
assert_equal "203.0.113.5", api_login_block.call(api_login_request)
assert_nil sso_link_block.call(api_login_request)
sso_link_request = throttle_request("/api/v1/auth/sso_link", method: "POST")
assert_equal "203.0.113.5", sso_link_block.call(sso_link_request)
assert_nil api_login_block.call(sso_link_request)
end
test "api login and sso-link email throttles discriminate JSON bodies, the documented mobile format" do
api_login_email_block = Rack::Attack.throttles["api_login/email"].block
api_sso_link_email_block = Rack::Attack.throttles["api_sso_link/email"].block
api_login_request = throttle_request("/api/v1/auth/login", method: "POST",
json_body: { email: " User@Example.com ", password: "secret" })
assert_equal "user@example.com", api_login_email_block.call(api_login_request)
sso_link_request = throttle_request("/api/v1/auth/sso_link", method: "POST",
json_body: { email: " User@Example.com ", password: "secret" })
assert_equal "user@example.com", api_sso_link_email_block.call(sso_link_request)
# The controller must still be able to read the body after Rack::Attack
# inspected it — this is what proves the peek rewinds rather than
# consuming the input stream.
assert_equal({ "email" => " User@Example.com ", "password" => "secret" }, JSON.parse(api_login_request.body.read))
malformed_request = throttle_request("/api/v1/auth/login", method: "POST", json_body_raw: "not json")
assert_nil api_login_email_block.call(malformed_request)
end
test "credential-guessing throttles still match when the path carries a format extension" do
# None of these routes are declared `format: false`, so Rails' default
# `(.:format)` segment means e.g. "/sessions.json" still reaches
# SessionsController#create even though request.path for that request is
# "/sessions.json", not "/sessions". A throttle keyed on exact string
# equality would silently let a scripted attacker brute-force every
# credential-guessing endpoint unthrottled just by appending an
# extension.
ip_block = Rack::Attack.throttles["logins/ip"].block
email_block = Rack::Attack.throttles["logins/email"].block
request = throttle_request("/sessions.json", method: "POST", params: { "email" => "user@example.com" })
assert_equal "203.0.113.5", ip_block.call(request)
assert_equal "user@example.com", email_block.call(request)
api_login_block = Rack::Attack.throttles["api_login/ip"].block
api_login_request = throttle_request("/api/v1/auth/login.json", method: "POST")
assert_equal "203.0.113.5", api_login_block.call(api_login_request)
# Rails' actual default segment matcher for `(.:format)` is `[^./?]+`,
# not `\w+` — it permits hyphens (and other punctuation), so a format
# value like "rate-limit" is a real route match, not just a hypothetical.
hyphenated_format_request = throttle_request("/api/v1/auth/login.rate-limit", method: "POST")
assert_equal "203.0.113.5", api_login_block.call(hyphenated_format_request)
# A path that merely starts with the throttled path, without being a
# format suffix, must still be ignored.
unrelated_request = throttle_request("/sessions_other", method: "POST", params: { "email" => "user@example.com" })
assert_nil ip_block.call(unrelated_request)
end
test "json email extraction tolerates non-object JSON payloads without raising" do
api_login_email_block = Rack::Attack.throttles["api_login/email"].block
null_request = throttle_request("/api/v1/auth/login", method: "POST", json_body_raw: "null")
assert_nil api_login_email_block.call(null_request)
array_request = throttle_request("/api/v1/auth/login", method: "POST", json_body_raw: "[1,2,3]")
assert_nil api_login_email_block.call(array_request)
end
test "json email extraction skips a non-rewindable rack.input instead of raising or consuming the body" do
# Rack 3 no longer requires rack.input to be rewindable (streaming
# servers may not buffer it) — simulate that by using an input object
# that only implements #read, not #rewind.
api_login_email_block = Rack::Attack.throttles["api_login/email"].block
request = throttle_request("/api/v1/auth/login", method: "POST", non_rewindable_json_body: { email: "user@example.com" })
assert_nil api_login_email_block.call(request)
# A block that read the body and then just returned nil (e.g. on a
# parse error) would also pass the assertion above while leaving the
# controller with an exhausted stream — assert the body was never
# touched at all.
assert_equal({ "email" => "user@example.com" }, JSON.parse(request.body.read))
end
private
NonRewindableInput = Struct.new(:io) do
def read(*args) = io.read(*args)
end
def throttle_request(path, method: "GET", params: {}, session: {}, json_body: nil, json_body_raw: nil, non_rewindable_json_body: nil)
# Rack::MockRequest.env_for doesn't set REMOTE_ADDR, so #ip is nil
# unless set explicitly — asserting against a real value here (rather
# than comparing to request.ip, which could trivially be nil on both
# sides) is what actually proves the ip-based discriminator extracts
# something.
opts = { method: method, params: params, "REMOTE_ADDR" => "203.0.113.5" }
if json_body || json_body_raw
opts[:input] = json_body_raw || json_body.to_json
opts["CONTENT_TYPE"] = "application/json"
elsif non_rewindable_json_body
opts[:input] = NonRewindableInput.new(StringIO.new(non_rewindable_json_body.to_json))
opts["CONTENT_TYPE"] = "application/json"
end
env = Rack::MockRequest.env_for(path, opts)
env["rack.session"] = session
Rack::Attack::Request.new(env)
end
end