Skip to content

Commit e870835

Browse files
authored
Merge pull request from GHSA-9m38-prpc-m7w3
To prevent OTP brute-forcing
1 parent fe32dca commit e870835

File tree

2 files changed

+31
-6
lines changed

2 files changed

+31
-6
lines changed

config/initializers/rack_attack.rb

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -46,12 +46,21 @@ class Rack::Attack
4646
{ controller: "multifactor_auths", action: "update" }
4747
]
4848

49+
protected_api_key_actions = [
50+
{ controller: "api/v1/api_keys", action: "show" },
51+
{ controller: "api/v1/api_keys", action: "create" },
52+
{ controller: "api/v1/api_keys", action: "update" },
53+
54+
# not technically API key, but it's the only other action that uses authenticate_or_request_with_http_basic
55+
# and we don't want to make it easy to guess user passwords (or figure out who has mfa enabled...)
56+
{ controller: "api/v1/profiles", action: "me" }
57+
]
58+
4959
protected_api_mfa_actions = [
5060
{ controller: "api/v1/deletions", action: "create" },
5161
{ controller: "api/v1/owners", action: "create" },
52-
{ controller: "api/v1/owners", action: "destroy" },
53-
{ controller: "api/v1/api_keys", action: "show" }
54-
]
62+
{ controller: "api/v1/owners", action: "destroy" }
63+
] + protected_api_key_actions
5564

5665
protected_ui_owners_actions = [
5766
{ controller: "owners", action: "resend_confirmation" },
@@ -162,10 +171,8 @@ def self.protected_route?(protected_actions, path, method)
162171
User.normalize_email(req.params['session']['who']).presence if protected_route && req.params['session']
163172
end
164173

165-
protected_api_key_action = [{ controller: "api/v1/api_keys", action: "show" }]
166-
167174
throttle("api_key/basic_auth", limit: REQUEST_LIMIT, period: LIMIT_PERIOD) do |req|
168-
if protected_route?(protected_api_key_action, req.path, req.request_method)
175+
if protected_route?(protected_api_key_actions, req.path, req.request_method)
169176
action_dispatch_req = ActionDispatch::Request.new(req.env)
170177
who = ActionController::HttpAuthentication::Basic.user_name_and_password(action_dispatch_req).first
171178
User.normalize_email(who).presence

test/integration/rack_attack_test.rb

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -519,6 +519,24 @@ class RackAttackTest < ActionDispatch::IntegrationTest
519519
end
520520
end
521521

522+
should "throttle api key create at level #{level}" do
523+
freeze_time do
524+
exceed_exponential_limit_for("api/ip/#{level}", level)
525+
get "/api/v1/api_key.json", headers: { REMOTE_ADDR: @ip_address }
526+
527+
assert_throttle_at(level)
528+
end
529+
end
530+
531+
should "throttle api key create by api key #{level}" do
532+
freeze_time do
533+
exceed_exponential_api_key_limit_for("api/key/#{level}", @user.display_id, level)
534+
post "/api/v1/api_key.json", headers: { HTTP_AUTHORIZATION: @api_key }
535+
536+
assert_throttle_at(level)
537+
end
538+
end
539+
522540
should "throttle mfa forgot password at level #{level}" do
523541
freeze_time do
524542
exceed_exponential_limit_for("clearance/ip/#{level}", level)

0 commit comments

Comments
 (0)