From 154afa60eb074e2a07f570547686bbe3dcfe644e Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Wed, 20 Apr 2022 14:11:39 +1000 Subject: [PATCH] FIX: Skip upload extension validation when changing security (#16498) When changing upload security using `Upload#update_secure_status`, we may not have the context of how an upload is being created, because this code path can be run through scheduled jobs. When calling update_secure_status, the normal ActiveRecord validations are run, and ours include validating extensions. In some cases the upload is created in an automated way, such as user export zips, and the security is applied later, with the extension prohibited from use when normally uploading. This caused the upload to fail validation on `update_secure_status`, causing the security change to silently fail. This fixes the issue by skipping the file extension validation when the upload security is being changed. --- lib/validators/upload_validator.rb | 12 ++++++ .../update_post_uploads_secure_status_spec.rb | 37 ++++++++++++++++++ spec/lib/upload_creator_spec.rb | 9 +++++ spec/models/upload_spec.rb | 39 +++++++++++++++---- 4 files changed, 89 insertions(+), 8 deletions(-) create mode 100644 spec/jobs/regular/update_post_uploads_secure_status_spec.rb 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