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

@@ -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 <br><a href="https://meta.discourse.org/t/76575">https://meta.discourse.org/t/76575</a>'
 
   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:

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

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