From c6f5df4caaca266ecf920b35966857df8d7b5687 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Mon, 18 Dec 2017 11:25:22 +0800 Subject: [PATCH] SECURITY: Don't pass email backup token to sidekiq as a parameter. * This exposes the token in the Sidekiq dashboard which can be viewed by an admin and defeats the purpose of using a token in the download backup email ink. --- app/controllers/admin/backups_controller.rb | 11 ++-- app/jobs/regular/download_backup_email.rb | 13 ++-- .../admin/backups_controller_spec.rb | 6 +- spec/jobs/download_backup_email_spec.rb | 22 +++++++ .../requests/admin/backups_controller_spec.rb | 64 +++++++++++++++++++ 5 files changed, 106 insertions(+), 10 deletions(-) create mode 100644 spec/jobs/download_backup_email_spec.rb create mode 100644 spec/requests/admin/backups_controller_spec.rb diff --git a/app/controllers/admin/backups_controller.rb b/app/controllers/admin/backups_controller.rb index d0922f886384bc..199f3c4f0a7422 100644 --- a/app/controllers/admin/backups_controller.rb +++ b/app/controllers/admin/backups_controller.rb @@ -1,5 +1,4 @@ require "backup_restore/backup_restore" -require "email_backup_token" class Admin::BackupsController < Admin::AdminController @@ -47,10 +46,12 @@ def cancel def email if backup = Backup[params.fetch(:id)] - token = EmailBackupToken.set(current_user.id) - download_url = "#{url_for(controller: 'backups', action: 'show')}?token=#{token}" - Jobs.enqueue(:download_backup_email, to_address: current_user.email, backup_file_path: download_url) - render nothing: true + Jobs.enqueue(:download_backup_email, + user_id: current_user.id, + backup_file_path: url_for(controller: 'backups', action: 'show') + ) + + render body: nil else render nothing: true, status: 404 end diff --git a/app/jobs/regular/download_backup_email.rb b/app/jobs/regular/download_backup_email.rb index dbf12cdd1300fa..43b2d9cd8f7bf4 100644 --- a/app/jobs/regular/download_backup_email.rb +++ b/app/jobs/regular/download_backup_email.rb @@ -1,4 +1,5 @@ require_dependency 'email/sender' +require_dependency "email_backup_token" module Jobs @@ -7,13 +8,17 @@ class DownloadBackupEmail < Jobs::Base sidekiq_options queue: 'critical' def execute(args) - to_address = args[:to_address] - backup_file_path = args[:backup_file_path] + user_id = args[:user_id] + user = User.find_by(id: user_id) + raise Discourse::InvalidParameters.new(:user_id) unless user - raise Discourse::InvalidParameters.new(:to_address) if to_address.blank? + backup_file_path = args[:backup_file_path] raise Discourse::InvalidParameters.new(:backup_file_path) if backup_file_path.blank? - message = DownloadBackupMailer.send_email(to_address, backup_file_path) + backup_file_path = URI(backup_file_path) + backup_file_path.query = URI.encode_www_form(token: EmailBackupToken.set(user.id)) + + message = DownloadBackupMailer.send_email(user.email, backup_file_path.to_s) Email::Sender.new(message, :download_backup_message).send end diff --git a/spec/controllers/admin/backups_controller_spec.rb b/spec/controllers/admin/backups_controller_spec.rb index 5da9c9023a9a06..f9c78426bb3298 100644 --- a/spec/controllers/admin/backups_controller_spec.rb +++ b/spec/controllers/admin/backups_controller_spec.rb @@ -132,7 +132,11 @@ it "enqueues email job" do Backup.expects(:[]).with(backup_filename).returns(b) - Jobs.expects(:enqueue).with(:download_backup_email, has_entries(to_address: @admin.email)) + + Jobs.expects(:enqueue).with(:download_backup_email, + user_id: @admin.id, + backup_file_path: "http://test.host/admin/backups/2014-02-10-065935.tar.gz" + ) xhr :put, :email, id: backup_filename diff --git a/spec/jobs/download_backup_email_spec.rb b/spec/jobs/download_backup_email_spec.rb new file mode 100644 index 00000000000000..cda261c675d6d3 --- /dev/null +++ b/spec/jobs/download_backup_email_spec.rb @@ -0,0 +1,22 @@ +require 'rails_helper' + +RSpec.describe Jobs::DownloadBackupEmail do + let(:user) { Fabricate(:admin) } + + it "should work" do + described_class.new.execute( + user_id: user.id, + backup_file_path: "http://some.example.test/" + ) + + email = ActionMailer::Base.deliveries.last + + expect(email.subject).to eq(I18n.t('download_backup_mailer.subject_template', + email_prefix: SiteSetting.title + )) + + expect(email.body.raw_source).to eq(I18n.t('download_backup_mailer.text_body_template', + backup_file_path: "http://some.example.test/?token=#{EmailBackupToken.get(user.id)}" + )) + end +end diff --git a/spec/requests/admin/backups_controller_spec.rb b/spec/requests/admin/backups_controller_spec.rb new file mode 100644 index 00000000000000..1fc67d76015b01 --- /dev/null +++ b/spec/requests/admin/backups_controller_spec.rb @@ -0,0 +1,64 @@ +require 'rails_helper' + +RSpec.describe Admin::BackupsController do + let(:admin) { Fabricate(:admin) } + + before do + sign_in(admin) + end + + describe '#rollback' do + it 'should rollback the restore' do + BackupRestore.expects(:rollback!) + + post "/admin/backups/rollback.json" + + expect(response).to be_success + end + + it 'should not allow rollback via a GET request' do + expect { get "/admin/backups/rollback.json" } + .to raise_error(ActionController::RoutingError) + end + end + + describe '#cancel' do + it "should cancel an backup" do + BackupRestore.expects(:cancel!) + + delete "/admin/backups/cancel.json" + + expect(response).to be_success + end + + it 'should not allow cancel via a GET request' do + expect { get "/admin/backups/cancel.json" } + .to raise_error(ActionController::RoutingError) + end + end + + describe "#email" do + let(:backup_filename) { "test.tar.gz" } + let(:backup) { Backup.new(backup_filename) } + + it "enqueues email job" do + Backup.expects(:[]).with(backup_filename).returns(backup) + + Jobs.expects(:enqueue).with(:download_backup_email, + user_id: admin.id, + backup_file_path: 'http://www.example.com/admin/backups/test.tar.gz' + ) + + put "/admin/backups/#{backup_filename}.json" + + expect(response).to be_success + end + + it "returns 404 when the backup does not exist" do + put "/admin/backups/#{backup_filename}.json" + + expect(response).to be_not_found + end + + end +end