Skip to content
Permalink
Browse files Browse the repository at this point in the history
SECURITY: Rate limit the creation of backups
  • Loading branch information
Flink committed Mar 16, 2023
1 parent b5bee9d commit 78a3efa
Show file tree
Hide file tree
Showing 4 changed files with 78 additions and 22 deletions.
8 changes: 8 additions & 0 deletions app/controllers/admin/backups_controller.rb
Expand Up @@ -34,6 +34,14 @@ def status
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",
Expand Down
23 changes: 11 additions & 12 deletions lib/backup_restore/backuper.rb
Expand Up @@ -5,7 +5,7 @@

module BackupRestore
class Backuper
attr_reader :success
attr_reader :success, :store

def initialize(user_id, opts = {})
@user_id = user_id
Expand Down Expand Up @@ -46,7 +46,6 @@ def run
rescue Exception => ex
log "EXCEPTION: " + ex.message
log ex.backtrace.join("\n")
@success = false
else
@success = true
@backup_filename
Expand All @@ -55,7 +54,7 @@ def run
clean_up
notify_user
log "Finished!"
publish_completion(@success)
publish_completion
end

protected
Expand Down Expand Up @@ -337,12 +336,12 @@ def add_remote_uploads_to_archive(tar_filename)
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
Expand All @@ -354,16 +353,16 @@ def delete_old
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)
Expand All @@ -378,11 +377,11 @@ def clean_up
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)

Expand All @@ -396,7 +395,7 @@ def delete_uploaded_archive

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
Expand Down Expand Up @@ -450,7 +449,7 @@ def save_log(message, timestamp)
@logs << "[#{timestamp}] #{message}"
end

def publish_completion(success)
def publish_completion
if success
log("[SUCCESS]")
DiscourseEvent.trigger(:backup_complete, logs: @logs, ticket: @ticket)
Expand Down
48 changes: 39 additions & 9 deletions spec/lib/backup_restore/backuper_spec.rb
@@ -1,18 +1,20 @@
# frozen_string_literal: true

RSpec.describe BackupRestore::Backuper do
it "returns a non-empty parameterized title when site title contains unicode" do
SiteSetting.title = "Ɣ"
backuper = BackupRestore::Backuper.new(Discourse.system_user.id)
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)

expect(backuper.send(:get_parameterized_title)).to eq("discourse")
end
expect(backuper.send(:get_parameterized_title)).to eq("discourse")
end

it "returns a valid parameterized site title" do
SiteSetting.title = "Coding Horror"
backuper = BackupRestore::Backuper.new(Discourse.system_user.id)
it "returns a valid parameterized site title" do
SiteSetting.title = "Coding Horror"
backuper = BackupRestore::Backuper.new(Discourse.system_user.id)

expect(backuper.send(:get_parameterized_title)).to eq("coding-horror")
expect(backuper.send(:get_parameterized_title)).to eq("coding-horror")
end
end

describe "#notify_user" do
Expand Down Expand Up @@ -69,4 +71,32 @@
)
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
21 changes: 20 additions & 1 deletion spec/requests/admin/backups_controller_spec.rb
Expand Up @@ -137,7 +137,10 @@ def map_preloaded

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(
Expand All @@ -149,6 +152,22 @@ def map_preloaded

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
Expand Down

0 comments on commit 78a3efa

Please sign in to comment.