FIX: Stop category logo + background being marked secure (#10513)

Meta topic: https://meta.discourse.org/t/secure-media-uploads-breaks-category-logos/161693

Category backgrounds and logos are public uploads and should not be marked as secure.

I also discovered that a lot of the UploadSecurity specs for public types were returning false positives; this has been fixed.

lib/upload_security.rb

@@ -14,7 +14,10 @@
 # on the current secure? status, otherwise there would be a lot of additional
 # complex queries and joins to perform.
 class UploadSecurity
-  PUBLIC_TYPES = %w[avatar custom_emoji profile_background card_background]
+  PUBLIC_TYPES = %w[
+    avatar custom_emoji profile_background card_background category_logo category_background
+  ]
+
   def initialize(upload, opts = {})
     @upload = upload
     @opts = opts
@@ -30,7 +33,12 @@ class UploadSecurity
   private
 
   def uploading_in_public_context?
-    @upload.for_theme || @upload.for_site_setting || @upload.for_gravatar || public_type? || used_for_custom_emoji? || based_on_regular_emoji?
+    @upload.for_theme ||
+      @upload.for_site_setting ||
+      @upload.for_gravatar ||
+      public_type? ||
+      used_for_custom_emoji? ||
+      based_on_regular_emoji?
   end
 
   def uploading_in_secure_context?

spec/lib/upload_security_spec.rb

@@ -10,6 +10,23 @@ RSpec.describe UploadSecurity do
   let(:opts) { { type: type } }
   subject { described_class.new(upload, opts) }
 
+  context "when secure media is enabled" do
+    before do
+      SiteSetting.enable_s3_uploads = true
+      SiteSetting.s3_upload_bucket = "s3-upload-bucket"
+      SiteSetting.s3_access_key_id = "some key"
+      SiteSetting.s3_secret_access_key = "some secrets3_region key"
+      SiteSetting.secure_media = true
+    end
+
+    context "when login_required (everything should be secure except public context items)" do
+      before do
+        SiteSetting.login_required = true
+      end
+      it "returns true" do
+        expect(subject.should_be_secure?).to eq(true)
+      end
+
       context "when uploading in public context" do
         describe "for a public type avatar" do
           let(:type) { 'avatar' }
@@ -35,7 +52,18 @@ RSpec.describe UploadSecurity do
             expect(subject.should_be_secure?).to eq(false)
           end
         end
-
+        describe "for a public type category_logo" do
+          let(:type) { 'category_logo' }
+          it "returns false" do
+            expect(subject.should_be_secure?).to eq(false)
+          end
+        end
+        describe "for a public type category_background" do
+          let(:type) { 'category_background' }
+          it "returns false" do
+            expect(subject.should_be_secure?).to eq(false)
+          end
+        end
         describe "for_theme" do
           before do
             upload.stubs(:for_theme).returns(true)
@@ -76,23 +104,6 @@ RSpec.describe UploadSecurity do
           end
         end
       end
-
-  context "when secure media is enabled" do
-    before do
-      SiteSetting.enable_s3_uploads = true
-      SiteSetting.s3_upload_bucket = "s3-upload-bucket"
-      SiteSetting.s3_access_key_id = "some key"
-      SiteSetting.s3_secret_access_key = "some secrets3_region key"
-      SiteSetting.secure_media = true
-    end
-
-    context "when login_required" do
-      before do
-        SiteSetting.login_required = true
-      end
-      it "returns true" do
-        expect(subject.should_be_secure?).to eq(true)
-      end
     end
 
     context "when the access control post has_secure_media?" do