FIX: Only mark attachments as secure media if SiteSetting.secure_media? (#9009)

* Attachments (non media files) were being marked as secure if just
SiteSetting.prevent_anons_from_downloading_files was enabled. this
was not correct as nothing should be marked as actually "secure" in
the DB without that site setting enabled
* Also add a proper standalone spec file for the upload security class

lib/upload_security.rb

@@ -21,8 +21,9 @@ class UploadSecurity
   end
 
   def should_be_secure?
+    return false if !SiteSetting.secure_media?
     return false if uploading_in_public_context?
-    secure_attachment? || secure_media?
+    (secure_attachment? || supported_media?) && uploading_in_secure_context?
   end
 
   private
@@ -39,10 +40,6 @@ class UploadSecurity
     !supported_media? && SiteSetting.prevent_anons_from_downloading_files
   end
 
-  def secure_media?
-    SiteSetting.secure_media? && supported_media? && uploading_in_secure_context?
-  end
-
   def uploading_in_secure_context?
     return true if SiteSetting.login_required?
     if @upload.access_control_post_id.present?

spec/lib/upload_creator_spec.rb

@@ -173,14 +173,17 @@ RSpec.describe UploadCreator do
     describe 'secure attachments' do
       let(:filename) { "small.pdf" }
       let(:file) { file_from_fixtures(filename, "pdf") }
+      let(:opts) { { type: "composer" } }
 
       before do
+        enable_s3_uploads
+        SiteSetting.secure_media = true
         SiteSetting.prevent_anons_from_downloading_files = true
         SiteSetting.authorized_extensions = 'pdf|svg|jpg'
       end
 
       it 'should mark attachments as secure' do
-        upload = UploadCreator.new(file, filename).create_for(user.id)
+        upload = UploadCreator.new(file, filename, opts).create_for(user.id)
         stored_upload = Upload.last
 
         expect(stored_upload.secure?).to eq(true)
@@ -208,6 +211,7 @@ RSpec.describe UploadCreator do
       let(:file) { file_from_fixtures(filename) }
       let(:pdf_filename) { "small.pdf" }
       let(:pdf_file) { file_from_fixtures(pdf_filename, "pdf") }
+      let(:opts) { { type: "composer" } }
 
       before do
         enable_s3_uploads
@@ -226,8 +230,9 @@ RSpec.describe UploadCreator do
       it 'should return signed URL for secure attachments in S3' do
         SiteSetting.prevent_anons_from_downloading_files = true
         SiteSetting.authorized_extensions = 'pdf'
+        SiteSetting.secure_media = true
 
-        upload = UploadCreator.new(pdf_file, pdf_filename).create_for(user.id)
+        upload = UploadCreator.new(pdf_file, pdf_filename, opts).create_for(user.id)
         stored_upload = Upload.last
         signed_url = Discourse.store.url_for(stored_upload)
 

spec/lib/upload_security_spec.rb

@@ -0,0 +1,173 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe UploadSecurity do
+  let(:private_category) { Fabricate(:private_category, group: Fabricate(:group)) }
+  let(:post_in_secure_context) { Fabricate(:post, topic: Fabricate(:topic, category: private_category)) }
+  let(:upload) { Fabricate(:upload) }
+  let(:type) { nil }
+  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
+      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
+      before do
+        upload.update(access_control_post: post_in_secure_context)
+      end
+      it "returns true" do
+        expect(subject.should_be_secure?).to eq(true)
+      end
+    end
+
+    context "when uploading in the composer" do
+      let(:type) { "composer" }
+      it "returns true" do
+        expect(subject.should_be_secure?).to eq(true)
+      end
+    end
+    context "when uploading for a group message" do
+      before do
+        upload.stubs(:for_group_message).returns(true)
+      end
+      it "returns true" do
+        expect(subject.should_be_secure?).to eq(true)
+      end
+    end
+    context "when uploading for a PM" do
+      before do
+        upload.stubs(:for_private_message).returns(true)
+      end
+      it "returns true" do
+        expect(subject.should_be_secure?).to eq(true)
+      end
+    end
+    context "when upload is already secure" do
+      before do
+        upload.update(secure: true)
+      end
+      it "returns true" do
+        expect(subject.should_be_secure?).to eq(true)
+      end
+    end
+
+    context "when prevent_anons_from_downloading_files enabled for attachment" do
+      before do
+        SiteSetting.prevent_anons_from_downloading_files = true
+        upload.update(original_filename: 'test.pdf')
+      end
+
+      context "when the access control post has_secure_media?" do
+        before do
+          upload.update(access_control_post: post_in_secure_context)
+        end
+        it "returns true" do
+          expect(subject.should_be_secure?).to eq(true)
+        end
+      end
+    end
+  end
+
+  context "when secure media is disabled" do
+    before do
+      SiteSetting.secure_media = false
+    end
+    it "returns false" do
+      expect(subject.should_be_secure?).to eq(false)
+    end
+
+    context "when prevent_anons_from_downloading_files enabled for attachment" do
+      before do
+        SiteSetting.prevent_anons_from_downloading_files = true
+        upload.update(original_filename: 'test.pdf')
+      end
+      it "returns false" do
+        expect(subject.should_be_secure?).to eq(false)
+      end
+    end
+  end
+end

spec/models/upload_spec.rb

@@ -359,7 +359,8 @@ describe Upload do
     it 'marks a local attachment as secure if prevent_anons_from_downloading_files is enabled' do
       SiteSetting.prevent_anons_from_downloading_files = true
       SiteSetting.authorized_extensions = "pdf"
-      upload.update!(original_filename: "small.pdf", extension: "pdf")
+      upload.update!(original_filename: "small.pdf", extension: "pdf", secure: false, access_control_post: Fabricate(:private_message_post))
+      enable_secure_media
 
       expect { upload.update_secure_status }
         .to change { upload.secure }