SECURITY: Rate limit the creation of backups

app/controllers/admin/backups_controller.rb

@@ -34,6 +34,14 @@ class Admin::BackupsController < Admin::AdminController
   end
 
   def create
+    RateLimiter.new(
+      current_user,
+      "max-backups-per-minute",
+      1,
+      1.minute,
+      apply_limit_to_staff: true,
+    ).performed!
+
     opts = {
       publish_to_message_bus: true,
       with_uploads: params.fetch(:with_uploads) == "true",

lib/backup_restore/backuper.rb

@@ -5,7 +5,7 @@ require "file_store/s3_store"
 
 module BackupRestore
   class Backuper
-    attr_reader :success
+    attr_reader :success, :store
 
     def initialize(user_id, opts = {})
       @user_id = user_id
@@ -46,7 +46,6 @@ module BackupRestore
     rescue Exception => ex
       log "EXCEPTION: " + ex.message
       log ex.backtrace.join("\n")
-      @success = false
     else
       @success = true
       @backup_filename
@@ -55,7 +54,7 @@ module BackupRestore
       clean_up
       notify_user
       log "Finished!"
-      publish_completion(@success)
+      publish_completion
     end
 
     protected
@@ -337,12 +336,12 @@ module BackupRestore
     end
 
     def upload_archive
-      return unless @store.remote?
+      return unless store.remote?
 
       log "Uploading archive..."
       content_type = MiniMime.lookup_by_filename(@backup_filename).content_type
       archive_path = File.join(@archive_directory, @backup_filename)
-      @store.upload_file(@backup_filename, archive_path, content_type)
+      store.upload_file(@backup_filename, archive_path, content_type)
     end
 
     def after_create_hook
@@ -354,16 +353,16 @@ module BackupRestore
       return if Rails.env.development?
 
       log "Deleting old backups..."
-      @store.delete_old
+      store.delete_old
     rescue => ex
       log "Something went wrong while deleting old backups.", ex
     end
 
     def notify_user
-      return if @success && @user.id == Discourse::SYSTEM_USER_ID
+      return if success && @user.id == Discourse::SYSTEM_USER_ID
 
       log "Notifying '#{@user.username}' of the end of the backup..."
-      status = @success ? :backup_succeeded : :backup_failed
+      status = success ? :backup_succeeded : :backup_failed
 
       logs = Discourse::Utils.logs_markdown(@logs, user: @user)
       post = SystemMessage.create_from_system_user(@user, status, logs: logs)
@@ -378,11 +377,11 @@ module BackupRestore
       delete_uploaded_archive
       remove_tar_leftovers
       mark_backup_as_not_running
-      refresh_disk_space
+      refresh_disk_space if success
     end
 
     def delete_uploaded_archive
-      return unless @store.remote?
+      return unless store.remote?
 
       archive_path = File.join(@archive_directory, @backup_filename)
 
@@ -396,7 +395,7 @@ module BackupRestore
 
     def refresh_disk_space
       log "Refreshing disk stats..."
-      @store.reset_cache
+      store.reset_cache
     rescue => ex
       log "Something went wrong while refreshing disk stats.", ex
     end
@@ -450,7 +449,7 @@ module BackupRestore
       @logs << "[#{timestamp}] #{message}"
     end
 
-    def publish_completion(success)
+    def publish_completion
       if success
         log("[SUCCESS]")
         DiscourseEvent.trigger(:backup_complete, logs: @logs, ticket: @ticket)

spec/lib/backup_restore/backuper_spec.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 RSpec.describe BackupRestore::Backuper do
+  describe "#get_parameterized_title" do
     it "returns a non-empty parameterized title when site title contains unicode" do
       SiteSetting.title = "Ɣ"
       backuper = BackupRestore::Backuper.new(Discourse.system_user.id)
@@ -14,6 +15,7 @@ RSpec.describe BackupRestore::Backuper do
 
       expect(backuper.send(:get_parameterized_title)).to eq("coding-horror")
     end
+  end
 
   describe "#notify_user" do
     before { freeze_time Time.zone.parse("2010-01-01 12:00") }
@@ -69,4 +71,32 @@ RSpec.describe BackupRestore::Backuper do
       )
     end
   end
+
+  describe "#run" do
+    subject(:run) { backup.run }
+
+    let(:backup) { described_class.new(user.id) }
+    let(:user) { Discourse.system_user }
+    let(:store) { backup.store }
+
+    before { backup.stubs(:success).returns(success) }
+
+    context "when the result isn't successful" do
+      let(:success) { false }
+
+      it "doesn't refresh disk stats" do
+        store.expects(:reset_cache).never
+        run
+      end
+    end
+
+    context "when the result is successful" do
+      let(:success) { true }
+
+      it "refreshes disk stats" do
+        store.expects(:reset_cache)
+        run
+      end
+    end
+  end
 end

spec/requests/admin/backups_controller_spec.rb

@@ -137,7 +137,10 @@ RSpec.describe Admin::BackupsController do
 
   describe "#create" do
     context "when logged in as an admin" do
-      before { sign_in(admin) }
+      before do
+        sign_in(admin)
+        BackupRestore.stubs(:backup!)
+      end
 
       it "starts a backup" do
         BackupRestore.expects(:backup!).with(
@@ -149,6 +152,22 @@ RSpec.describe Admin::BackupsController do
 
         expect(response.status).to eq(200)
       end
+
+      context "with rate limiting enabled" do
+        before do
+          RateLimiter.clear_all!
+          RateLimiter.enable
+        end
+
+        after { RateLimiter.disable }
+
+        it "is rate limited" do
+          post "/admin/backups.json", params: { with_uploads: false, client_id: "foo" }
+          post "/admin/backups.json", params: { with_uploads: false, client_id: "foo" }
+
+          expect(response).to have_http_status :too_many_requests
+        end
+      end
     end
 
     shared_examples "backups creation not allowed" do