Merge pull request from GHSA-9m38-prpc-m7w3

To prevent OTP brute-forcing
rubygems · Sep 7, 2022 · e870835 · e870835
1 parent fe32dca
commit e870835
Show file tree

Hide file tree

Showing 2 changed files with 31 additions and 6 deletions.
diff --git a/config/initializers/rack_attack.rb b/config/initializers/rack_attack.rb
@@ -46,12 +46,21 @@ class Rack::Attack
     { controller: "multifactor_auths",   action: "update" }
   ]
 
+  protected_api_key_actions = [
+    { controller: "api/v1/api_keys", action: "show" },
+    { controller: "api/v1/api_keys", action: "create" },
+    { controller: "api/v1/api_keys", action: "update" },
+
+    # not technically API key, but it's the only other action that uses authenticate_or_request_with_http_basic
+    # and we don't want to make it easy to guess user passwords (or figure out who has mfa enabled...)
+    { controller: "api/v1/profiles", action: "me" }
+  ]
+
   protected_api_mfa_actions = [
     { controller: "api/v1/deletions", action: "create" },
     { controller: "api/v1/owners",    action: "create" },
-    { controller: "api/v1/owners",    action: "destroy" },
-    { controller: "api/v1/api_keys",  action: "show" }
-  ]
+    { controller: "api/v1/owners",    action: "destroy" }
+  ] + protected_api_key_actions
 
   protected_ui_owners_actions = [
     { controller: "owners", action: "resend_confirmation" },
@@ -162,10 +171,8 @@ def self.protected_route?(protected_actions, path, method)
     User.normalize_email(req.params['session']['who']).presence if protected_route && req.params['session']
   end
 
-  protected_api_key_action = [{ controller: "api/v1/api_keys", action: "show" }]
-
   throttle("api_key/basic_auth", limit: REQUEST_LIMIT, period: LIMIT_PERIOD) do |req|
-    if protected_route?(protected_api_key_action, req.path, req.request_method)
+    if protected_route?(protected_api_key_actions, req.path, req.request_method)
       action_dispatch_req = ActionDispatch::Request.new(req.env)
       who = ActionController::HttpAuthentication::Basic.user_name_and_password(action_dispatch_req).first
       User.normalize_email(who).presence

diff --git a/test/integration/rack_attack_test.rb b/test/integration/rack_attack_test.rb
@@ -519,6 +519,24 @@ class RackAttackTest < ActionDispatch::IntegrationTest
           end
         end
 
+        should "throttle api key create at level #{level}" do
+          freeze_time do
+            exceed_exponential_limit_for("api/ip/#{level}", level)
+            get "/api/v1/api_key.json", headers: { REMOTE_ADDR: @ip_address }
+
+            assert_throttle_at(level)
+          end
+        end
+
+        should "throttle api key create by api key #{level}" do
+          freeze_time do
+            exceed_exponential_api_key_limit_for("api/key/#{level}", @user.display_id, level)
+            post "/api/v1/api_key.json", headers: { HTTP_AUTHORIZATION: @api_key }
+
+            assert_throttle_at(level)
+          end
+        end
+
         should "throttle mfa forgot password at level #{level}" do
           freeze_time do
             exceed_exponential_limit_for("clearance/ip/#{level}", level)