diff --git a/lib/upload_security.rb b/lib/upload_security.rb index 86647892372..d2e11d9eef5 100644 --- a/lib/upload_security.rb +++ b/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? diff --git a/spec/lib/upload_creator_spec.rb b/spec/lib/upload_creator_spec.rb index c627f9d115d..b2df27603e3 100644 --- a/spec/lib/upload_creator_spec.rb +++ b/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) diff --git a/spec/lib/upload_security_spec.rb b/spec/lib/upload_security_spec.rb new file mode 100644 index 00000000000..3dc812bbdc2 --- /dev/null +++ b/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 diff --git a/spec/models/upload_spec.rb b/spec/models/upload_spec.rb index 78c4ee9dad8..5ae6b3ced0e 100644 --- a/spec/models/upload_spec.rb +++ b/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 }