diff --git a/lib/validators/upload_validator.rb b/lib/validators/upload_validator.rb index 99b9078626f..450c8dafc39 100644 --- a/lib/validators/upload_validator.rb +++ b/lib/validators/upload_validator.rb @@ -33,6 +33,8 @@ class UploadValidator < ActiveModel::Validator return true end + return true if changing_upload_security?(upload) + if is_authorized?(upload, extension) if FileHelper.is_supported_image?(upload.original_filename) authorized_image_extension(upload, extension) @@ -44,6 +46,16 @@ class UploadValidator < ActiveModel::Validator end end + # this should only be run on existing records, and covers cases of + # upload.update_secure_status being run outside of the creation flow, + # where some cases e.g. have exemptions on the extension enforcement + def changing_upload_security?(upload) + !upload.new_record? && \ + upload.changed_attributes.keys.all? do |attribute| + %w(secure security_last_changed_at security_last_changed_reason).include?(attribute) + end + end + def is_authorized?(upload, extension) extension_authorized?(upload, extension, authorized_extensions(upload)) end diff --git a/spec/jobs/regular/update_post_uploads_secure_status_spec.rb b/spec/jobs/regular/update_post_uploads_secure_status_spec.rb new file mode 100644 index 00000000000..768726e0ce8 --- /dev/null +++ b/spec/jobs/regular/update_post_uploads_secure_status_spec.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe Jobs::UpdatePostUploadsSecureStatus do + fab!(:post) { Fabricate(:post) } + + before do + PostUpload.create!(post: post, upload: Fabricate(:upload)) + PostUpload.create!(post: post, upload: Fabricate(:upload)) + end + + context "when secure uploads is enabled" do + before do + setup_s3 + stub_s3_store + SiteSetting.secure_media = true + end + + context "when login_required" do + before { SiteSetting.login_required = true } + + it "updates all the uploads to secure" do + described_class.new.execute(post_id: post.id) + post.reload + expect(post.post_uploads.map(&:upload).map(&:secure).all?(true)).to eq(true) + end + + it "updates all the uploads to secure even if their extension is not authorized" do + SiteSetting.authorized_extensions = "" + described_class.new.execute(post_id: post.id) + post.reload + expect(post.post_uploads.map(&:upload).map(&:secure).all?(true)).to eq(true) + end + end + end +end diff --git a/spec/lib/upload_creator_spec.rb b/spec/lib/upload_creator_spec.rb index 62904eb8c20..bd240a5de0b 100644 --- a/spec/lib/upload_creator_spec.rb +++ b/spec/lib/upload_creator_spec.rb @@ -322,6 +322,15 @@ RSpec.describe UploadCreator do expect(upload.secure?).to eq(false) end + + it "sets a reason for the security" do + upload = UploadCreator.new(file, filename, opts).create_for(user.id) + stored_upload = Upload.last + + expect(stored_upload.secure?).to eq(true) + expect(stored_upload.security_last_changed_at).not_to eq(nil) + expect(stored_upload.security_last_changed_reason).to eq("uploading via the composer | source: upload creator") + end end context 'uploading to s3' do diff --git a/spec/models/upload_spec.rb b/spec/models/upload_spec.rb index 69c2e54177f..10c32040a50 100644 --- a/spec/models/upload_spec.rb +++ b/spec/models/upload_spec.rb @@ -433,25 +433,25 @@ describe Upload do enable_secure_media end - it 'does not mark an image upload as not secure when there is no access control post id, to avoid unintentional exposure' do + it "does not mark an image upload as not secure when there is no access control post id, to avoid unintentional exposure" do upload.update!(secure: true) upload.update_secure_status expect(upload.secure).to eq(true) end - it 'marks the upload as not secure if its access control post is a public post' do + it "marks the upload as not secure if its access control post is a public post" do upload.update!(secure: true, access_control_post: Fabricate(:post)) upload.update_secure_status expect(upload.secure).to eq(false) end - it 'leaves the upload as secure if its access control post is a PM post' do + it "leaves the upload as secure if its access control post is a PM post" do upload.update!(secure: true, access_control_post: Fabricate(:private_message_post)) upload.update_secure_status expect(upload.secure).to eq(true) end - it 'marks an image upload as secure if login_required is enabled' do + it "marks an image upload as secure if login_required is enabled" do SiteSetting.login_required = true upload.update!(secure: false) @@ -461,15 +461,15 @@ describe Upload do expect(upload.reload.secure).to eq(true) end - it 'does not mark an upload used for a custom emoji as secure' do + it "does not mark an upload used for a custom emoji as secure" do SiteSetting.login_required = true upload.update!(secure: false) - CustomEmoji.create(name: 'meme', upload: upload) + CustomEmoji.create(name: "meme", upload: upload) upload.update_secure_status expect(upload.reload.secure).to eq(false) end - it 'does not mark an upload whose origin matches a regular emoji as secure (sometimes emojis are downloaded in pull_hotlinked_images)' do + it "does not mark an upload whose origin matches a regular emoji as secure (sometimes emojis are downloaded in pull_hotlinked_images)" do SiteSetting.login_required = true falafel = Emoji.all.find { |e| e.url == "/images/emoji/twitter/falafel.png?v=#{Emoji::EMOJI_VERSION}" } upload.update!(secure: false, origin: "http://localhost:3000#{falafel.url}") @@ -477,7 +477,7 @@ describe Upload do expect(upload.reload.secure).to eq(false) end - it 'does not mark any upload with origin containing images/emoji in the URL' do + it "does not mark any upload with origin containing images/emoji in the URL" do SiteSetting.login_required = true upload.update!(secure: false, origin: "http://localhost:3000/images/emoji/test.png") upload.update_secure_status @@ -491,6 +491,29 @@ describe Upload do upload.update!(secure: true, access_control_post: Fabricate(:private_message_post)) expect { upload.update_secure_status }.not_to raise_error end + + it "succeeds even if the extension of the upload is not authorized" do + upload.update!(secure: false, access_control_post: Fabricate(:private_message_post)) + SiteSetting.login_required = true + SiteSetting.authorized_extensions = "" + upload.update_secure_status + upload.reload + expect(upload.secure).to eq(true) + end + + it "respects the authorized extensions when creating a new upload, no matter its secure status" do + SiteSetting.login_required = true + SiteSetting.authorized_extensions = "" + expect do + upl = Fabricate( + :upload, + access_control_post: Fabricate(:private_message_post), + security_last_changed_at: Time.zone.now, + security_last_changed_reason: "test", + secure: true + ) + end.to raise_error(ActiveRecord::RecordInvalid) + end end end