From d8d54a92f15782bb906d2f0b2c7d5e966b29f819 Mon Sep 17 00:00:00 2001
From: Sam Saffron <sam.saffron@gmail.com>
Date: Fri, 8 May 2020 13:30:16 +1000
Subject: [PATCH] FEATURE: tighten rate limiting rules for forgot password

1. Total 6 attempts per day per user
2. Total of 5 per unique email/login that is not found per hour
3. If an admin blocks an IP that IP can not request a reset
---
 app/controllers/session_controller.rb    | 14 +++++--
 config/locales/server.en.yml             |  1 +
 spec/requests/session_controller_spec.rb | 47 ++++++++++++++++++++++++
 3 files changed, 59 insertions(+), 3 deletions(-)

diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb
index aa46f2dba2f..024d746aeb5 100644
--- a/app/controllers/session_controller.rb
+++ b/app/controllers/session_controller.rb
@@ -401,13 +401,21 @@ class SessionController < ApplicationController
   def forgot_password
     params.require(:login)
 
+    if ScreenedIpAddress.should_block?(request.remote_ip)
+      return render_json_error(I18n.t("login.reset_not_allowed_from_ip_address"))
+    end
+
     RateLimiter.new(nil, "forgot-password-hr-#{request.remote_ip}", 6, 1.hour).performed!
     RateLimiter.new(nil, "forgot-password-min-#{request.remote_ip}", 3, 1.minute).performed!
 
-    RateLimiter.new(nil, "forgot-password-login-hour-#{params[:login].to_s[0..100]}", 12, 1.hour).performed!
-    RateLimiter.new(nil, "forgot-password-login-min-#{params[:login].to_s[0..100]}", 3, 1.minute).performed!
-
     user = User.find_by_username_or_email(params[:login])
+
+    if user
+      RateLimiter.new(nil, "forgot-password-login-day-#{user.username}", 6, 1.day).performed!
+    else
+      RateLimiter.new(nil, "forgot-password-login-hour-#{params[:login].to_s[0..100]}", 5, 1.hour).performed!
+    end
+
     user_presence = user.present? && user.human? && !user.staged
     if user_presence
       email_token = user.email_tokens.create(email: user.email)
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index f49bb379796..c948a2ef62d 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -2385,6 +2385,7 @@ en:
     not_activated: "You can't log in yet. We sent an activation email to you. Please follow the instructions in the email to activate your account."
     not_allowed_from_ip_address: "You can't log in as %{username} from that IP address."
     admin_not_allowed_from_ip_address: "You can't log in as admin from that IP address."
+    reset_not_allowed_from_ip_address: "You can't request a password reset from that IP address."
     suspended: "You can't log in until %{date}."
     suspended_with_reason: "Account suspended until %{date}: %{reason}"
     errors: "%{errors}"
diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb
index 51b023df684..49b4d0943ff 100644
--- a/spec/requests/session_controller_spec.rb
+++ b/spec/requests/session_controller_spec.rb
@@ -1825,6 +1825,53 @@ RSpec.describe SessionController do
       expect(response.status).to eq(400)
     end
 
+    it 'should correctly screen ips' do
+      ScreenedIpAddress.create!(
+        ip_address: '100.0.0.1',
+        action_type: ScreenedIpAddress.actions[:block]
+      )
+
+      post "/session/forgot_password.json",
+        params: { login: 'made_up' },
+        headers: { 'REMOTE_ADDR' => '100.0.0.1'  }
+
+      expect(response.parsed_body).to eq({
+        "errors" => [I18n.t("login.reset_not_allowed_from_ip_address")]
+      })
+
+    end
+
+    it 'should correctly rate limits' do
+      RateLimiter.enable
+      RateLimiter.clear_all!
+
+      user = Fabricate(:user)
+
+      3.times do
+        post "/session/forgot_password.json", params: { login: user.username }
+        expect(response.status).to eq(200)
+      end
+
+      post "/session/forgot_password.json", params: { login: user.username }
+      expect(response.status).to eq(422)
+
+      3.times do
+        post "/session/forgot_password.json",
+          params: { login: user.username },
+          headers: { 'REMOTE_ADDR' => '10.1.1.1'  }
+
+        expect(response.status).to eq(200)
+      end
+
+      post "/session/forgot_password.json",
+        params: { login: user.username },
+        headers: { 'REMOTE_ADDR' => '100.1.1.1'  }
+
+      # not allowed, max 6 a day
+      expect(response.status).to eq(422)
+
+    end
+
     context 'for a non existant username' do
       it "doesn't generate a new token for a made up username" do
         expect do