From 01cdbd3a13af424dbb9dfe0202991ca63ea0bb4c Mon Sep 17 00:00:00 2001 From: Gerhard Schlager Date: Mon, 17 Dec 2018 00:09:13 +0100 Subject: [PATCH] FEATURE: Prohibit S3 bucket reusage This validation makes sure that the s3_upload_bucket and the s3_backup_bucket have different values. The backup bucket is allowed to be a subfolder of the upload bucket. The other way around is forbidden because the backup system searches by prefix and would return all files stored within the backup bucket and its subfolders. --- config/locales/server.en.yml | 3 +- lib/site_settings/validations.rb | 29 ++++++++ spec/lib/site_settings/validations_spec.rb | 80 ++++++++++++++++++++++ 3 files changed, 111 insertions(+), 1 deletion(-) create mode 100644 spec/lib/site_settings/validations_spec.rb diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 51fbcc0235a..ea615b65554 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -169,6 +169,7 @@ en: default_categories_already_selected: "You cannot select a category used in another list." s3_upload_bucket_is_required: "You cannot enable uploads to S3 unless you've provided the 's3_upload_bucket'." s3_backup_requires_s3_settings: "You cannot use S3 as backup location unless you've provided the '%{setting_name}'." + s3_bucket_reused: "You cannot use the same bucket for 's3_upload_bucket' and 's3_backup_bucket'. Choose a different bucket or use a different path for each bucket." conflicting_google_user_id: 'The Google Account ID for this account has changed; staff intervention is required for security reasons. Please contact staff and point them to
https://meta.discourse.org/t/76575' activemodel: @@ -190,7 +191,7 @@ en: error: "There was an error uploading that file. Please try again later." topic_invite: - failed_to_invite: "The user cannot be invited into this topic without a group membership in either one of the following groups: %{group_names}." + failed_to_invite: "The user cannot be invited into this topic without a group membership in either one of the following groups: %{group_names}." user_exists: "Sorry, that user has already been invited. You may only invite a user to a topic once." backup: diff --git a/lib/site_settings/validations.rb b/lib/site_settings/validations.rb index 0d50f488b3d..2d03fefc3cf 100644 --- a/lib/site_settings/validations.rb +++ b/lib/site_settings/validations.rb @@ -62,4 +62,33 @@ module SiteSettings::Validations validate_error(:s3_backup_requires_s3_settings, setting_name: "s3_secret_access_key") if SiteSetting.s3_secret_access_key.blank? end end + + def validate_s3_upload_bucket(new_val) + validate_bucket_setting("s3_upload_bucket", new_val, SiteSetting.s3_backup_bucket) + end + + def validate_s3_backup_bucket(new_val) + validate_bucket_setting("s3_backup_bucket", SiteSetting.s3_upload_bucket, new_val) + end + + private + + def validate_bucket_setting(setting_name, upload_bucket, backup_bucket) + return if upload_bucket.blank? || backup_bucket.blank? + + backup_bucket_name, backup_prefix = split_s3_bucket(backup_bucket) + upload_bucket_name, upload_prefix = split_s3_bucket(upload_bucket) + + return if backup_bucket_name != upload_bucket_name + + if backup_prefix == upload_prefix || backup_prefix.blank? || upload_prefix&.start_with?(backup_prefix) + validate_error(:s3_bucket_reused, setting_name: setting_name) + end + end + + def split_s3_bucket(s3_bucket) + bucket_name, prefix = s3_bucket.downcase.split("/", 2) + prefix&.chomp!("/") + [bucket_name, prefix] + end end diff --git a/spec/lib/site_settings/validations_spec.rb b/spec/lib/site_settings/validations_spec.rb new file mode 100644 index 00000000000..252099b4010 --- /dev/null +++ b/spec/lib/site_settings/validations_spec.rb @@ -0,0 +1,80 @@ +require 'rails_helper' +require 'site_settings/validations' + +describe SiteSettings::Validations do + subject { Class.new.include(described_class).new } + + context "s3 buckets reusage" do + let(:error_message) { I18n.t("errors.site_settings.s3_bucket_reused") } + + shared_examples "s3 bucket validation" do + def change_bucket_value(value) + SiteSetting.public_send("#{other_setting_name}=", value) + end + + it "shouldn't raise an error when both buckets are blank" do + change_bucket_value("") + expect { validate("") }.to_not raise_error + end + + it "shouldn't raise an error when only one bucket is set" do + change_bucket_value("") + expect { validate("my-awesome-bucket") }.to_not raise_error + end + + it "shouldn't raise an error when both buckets are equal, but use a different path" do + change_bucket_value("my-awesome-bucket/foo") + expect { validate("my-awesome-bucket/bar") }.to_not raise_error + end + + it "should raise an error when both buckets are equal" do + change_bucket_value("my-awesome-bucket") + expect { validate("my-awesome-bucket") }.to raise_error(Discourse::InvalidParameters, error_message) + end + + it "should raise an error when both buckets are equal except for a trailing slash" do + change_bucket_value("my-awesome-bucket/") + expect { validate("my-awesome-bucket") }.to raise_error(Discourse::InvalidParameters, error_message) + + change_bucket_value("my-awesome-bucket") + expect { validate("my-awesome-bucket/") }.to raise_error(Discourse::InvalidParameters, error_message) + end + end + + describe "#validate_s3_backup_bucket" do + let(:other_setting_name) { "s3_upload_bucket" } + + def validate(new_value) + subject.validate_s3_backup_bucket(new_value) + end + + it_behaves_like "s3 bucket validation" + + it "shouldn't raise an error when the 's3_backup_bucket' is a subdirectory of 's3_upload_bucket'" do + SiteSetting.s3_upload_bucket = "my-awesome-bucket" + expect { validate("my-awesome-bucket/backups") }.to_not raise_error + + SiteSetting.s3_upload_bucket = "my-awesome-bucket/foo" + expect { validate("my-awesome-bucket/foo/backups") }.to_not raise_error + end + end + + describe "#validate_s3_upload_bucket" do + let(:other_setting_name) { "s3_backup_bucket" } + + def validate(new_value) + subject.validate_s3_upload_bucket(new_value) + end + + it_behaves_like "s3 bucket validation" + + it "should raise an error when the 's3_upload_bucket' is a subdirectory of 's3_backup_bucket'" do + SiteSetting.s3_backup_bucket = "my-awesome-bucket" + expect { validate("my-awesome-bucket/uploads") }.to raise_error(Discourse::InvalidParameters, error_message) + + SiteSetting.s3_backup_bucket = "my-awesome-bucket/foo" + expect { validate("my-awesome-bucket/foo/uploads") }.to raise_error(Discourse::InvalidParameters, error_message) + end + end + end +end