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

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.

diff --git a/lib/upload_security.rb b/lib/upload_security.rb
index 8876446..52da01b 100644
--- a/lib/upload_security.rb
+++ b/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?
diff --git a/spec/lib/upload_security_spec.rb b/spec/lib/upload_security_spec.rb
index 8f99025..faba025 100644
--- a/spec/lib/upload_security_spec.rb
+++ b/spec/lib/upload_security_spec.rb
@@ -10,73 +10,6 @@ RSpec.describe UploadSecurity do
   let(:opts) { { type: type } }
   subject { described_class.new(upload, opts) }
 
-  context "when uploading in public context" do
-    describe "for a public type avatar" do
-      let(:type) { 'avatar' }
-      it "returns false" do
-        expect(subject.should_be_secure?).to eq(false)
-      end
-    end
-    describe "for a public type custom_emoji" do
-      let(:type) { 'custom_emoji' }
-      it "returns false" do
-        expect(subject.should_be_secure?).to eq(false)
-      end
-    end
-    describe "for a public type profile_background" do
-      let(:type) { 'profile_background' }
-      it "returns false" do
-        expect(subject.should_be_secure?).to eq(false)
-      end
-    end
-    describe "for a public type avatar" do
-      let(:type) { 'avatar' }
-      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)
-      end
-      it "returns false" do
-        expect(subject.should_be_secure?).to eq(false)
-      end
-    end
-    describe "for_site_setting" do
-      before do
-        upload.stubs(:for_site_setting).returns(true)
-      end
-      it "returns false" do
-        expect(subject.should_be_secure?).to eq(false)
-      end
-    end
-    describe "for_gravatar" do
-      before do
-        upload.stubs(:for_gravatar).returns(true)
-      end
-      it "returns false" do
-        expect(subject.should_be_secure?).to eq(false)
-      end
-    end
-
-    describe "when the upload is used for a custom emoji" do
-      it "returns false" do
-        CustomEmoji.create(name: 'meme', upload: upload)
-        expect(subject.should_be_secure?).to eq(false)
-      end
-    end
-
-    describe "when it is based on a regular emoji" do
-      it "returns false" do
-        falafel = Emoji.all.find { |e| e.url == '/images/emoji/twitter/falafel.png?v=9' }
-        upload.update!(origin: "http://localhost:3000#{falafel.url}")
-        expect(subject.should_be_secure?).to eq(false)
-      end
-    end
-  end
-
   context "when secure media is enabled" do
     before do
       SiteSetting.enable_s3_uploads = true
@@ -86,13 +19,91 @@ RSpec.describe UploadSecurity do
       SiteSetting.secure_media = true
     end
 
-    context "when login_required" do
+    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' }
+          it "returns false" do
+            expect(subject.should_be_secure?).to eq(false)
+          end
+        end
+        describe "for a public type custom_emoji" do
+          let(:type) { 'custom_emoji' }
+          it "returns false" do
+            expect(subject.should_be_secure?).to eq(false)
+          end
+        end
+        describe "for a public type profile_background" do
+          let(:type) { 'profile_background' }
+          it "returns false" do
+            expect(subject.should_be_secure?).to eq(false)
+          end
+        end
+        describe "for a public type avatar" do
+          let(:type) { 'avatar' }
+          it "returns false" 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)
+          end
+          it "returns false" do
+            expect(subject.should_be_secure?).to eq(false)
+          end
+        end
+        describe "for_site_setting" do
+          before do
+            upload.stubs(:for_site_setting).returns(true)
+          end
+          it "returns false" do
+            expect(subject.should_be_secure?).to eq(false)
+          end
+        end
+        describe "for_gravatar" do
+          before do
+            upload.stubs(:for_gravatar).returns(true)
+          end
+          it "returns false" do
+            expect(subject.should_be_secure?).to eq(false)
+          end
+        end
+
+        describe "when the upload is used for a custom emoji" do
+          it "returns false" do
+            CustomEmoji.create(name: 'meme', upload: upload)
+            expect(subject.should_be_secure?).to eq(false)
+          end
+        end
+
+        describe "when it is based on a regular emoji" do
+          it "returns false" do
+            falafel = Emoji.all.find { |e| e.url == '/images/emoji/twitter/falafel.png?v=9' }
+            upload.update!(origin: "http://localhost:3000#{falafel.url}")
+            expect(subject.should_be_secure?).to eq(false)
+          end
+        end
+      end
     end
 
     context "when the access control post has_secure_media?" do

GitHub sha: e8a842ab

1 Like

This commit appears in #10513 which was merged by martin.