diff --git a/app/models/post.rb b/app/models/post.rb index 1ce6628a419..1b525a16413 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -951,9 +951,9 @@ class Post < ActiveRecord::Base end end - def update_uploads_secure_status + def update_uploads_secure_status(source:) if Discourse.store.external? - self.uploads.each { |upload| upload.update_secure_status } + self.uploads.each { |upload| upload.update_secure_status(source: source) } end end diff --git a/app/models/topic.rb b/app/models/topic.rb index a6cc367baf7..bed3e1b1003 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -865,6 +865,14 @@ class Topic < ActiveRecord::Base Jobs.enqueue(:notify_category_change, post_id: post.id, notified_user_ids: notified_user_ids) end end + + # when a topic changes category we may need to make uploads + # linked to posts secure/not secure depending on whether the + # category is private. this is only done if the category + # has actually changed to avoid noise. + DB.after_commit do + Jobs.enqueue(:update_topic_upload_security, topic_id: self.id) + end end Category.where(id: new_category.id).update_all("topic_count = topic_count + 1") @@ -873,13 +881,6 @@ class Topic < ActiveRecord::Base CategoryFeaturedTopic.feature_topics_for(old_category) unless @import_mode CategoryFeaturedTopic.feature_topics_for(new_category) unless @import_mode || old_category.try(:id) == new_category.id end - - # when a topic changes category we may need to make uploads - # linked to posts secure/not secure depending on whether the - # category is private - DB.after_commit do - Jobs.enqueue(:update_topic_upload_security, topic_id: self.id) - end end true diff --git a/app/models/upload.rb b/app/models/upload.rb index dac34616af3..be740f9e687 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -311,16 +311,30 @@ class Upload < ActiveRecord::Base self.posts.where("cooked LIKE '%/_optimized/%'").find_each(&:rebake!) end - def update_secure_status(secure_override_value: nil) - mark_secure = secure_override_value.nil? ? UploadSecurity.new(self).should_be_secure? : secure_override_value + def update_secure_status(source: "unknown", override: nil) + if override.nil? + mark_secure, reason = UploadSecurity.new(self).should_be_secure_with_reason + else + mark_secure = override + reason = "manually overridden" + end secure_status_did_change = self.secure? != mark_secure - self.update_column("secure", mark_secure) + self.update(secure_params(mark_secure, reason, source)) + Discourse.store.update_upload_ACL(self) if Discourse.store.external? secure_status_did_change end + def secure_params(secure, reason, source = "unknown") + { + secure: secure, + security_last_changed_reason: reason + " | source: #{source}", + security_last_changed_at: Time.zone.now + } + end + def self.migrate_to_new_scheme(limit: nil) problems = [] @@ -451,27 +465,29 @@ end # # Table name: uploads # -# id :integer not null, primary key -# user_id :integer not null -# original_filename :string not null -# filesize :integer not null -# width :integer -# height :integer -# url :string not null -# created_at :datetime not null -# updated_at :datetime not null -# sha1 :string(40) -# origin :string(1000) -# retain_hours :integer -# extension :string(10) -# thumbnail_width :integer -# thumbnail_height :integer -# etag :string -# secure :boolean default(FALSE), not null -# access_control_post_id :bigint -# original_sha1 :string -# verification_status :integer default(1), not null -# animated :boolean +# id :integer not null, primary key +# user_id :integer not null +# original_filename :string not null +# filesize :integer not null +# width :integer +# height :integer +# url :string not null +# created_at :datetime not null +# updated_at :datetime not null +# sha1 :string(40) +# origin :string(1000) +# retain_hours :integer +# extension :string(10) +# thumbnail_width :integer +# thumbnail_height :integer +# etag :string +# secure :boolean default(FALSE), not null +# access_control_post_id :bigint +# original_sha1 :string +# verification_status :integer default(1), not null +# animated :boolean +# security_last_changed_at :datetime +# security_last_changed_reason :string # # Indexes # diff --git a/db/migrate/20210127013637_add_upload_security_log_columns.rb b/db/migrate/20210127013637_add_upload_security_log_columns.rb new file mode 100644 index 00000000000..10818c8b2a6 --- /dev/null +++ b/db/migrate/20210127013637_add_upload_security_log_columns.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +class AddUploadSecurityLogColumns < ActiveRecord::Migration[6.0] + def change + add_column :uploads, :security_last_changed_at, :datetime, null: true + add_column :uploads, :security_last_changed_reason, :string, null: true + end +end diff --git a/lib/post_creator.rb b/lib/post_creator.rb index 7884c225817..5b921119905 100644 --- a/lib/post_creator.rb +++ b/lib/post_creator.rb @@ -419,8 +419,7 @@ class PostCreator end def update_uploads_secure_status - return if !SiteSetting.secure_media? - @post.update_uploads_secure_status + @post.update_uploads_secure_status(source: "post creator") if SiteSetting.secure_media? end def delete_owned_bookmarks diff --git a/lib/tasks/uploads.rake b/lib/tasks/uploads.rake index fd1955f7276..d2ab4e82c8d 100644 --- a/lib/tasks/uploads.rake +++ b/lib/tasks/uploads.rake @@ -650,7 +650,11 @@ end def mark_all_as_secure_login_required(uploads_to_update) puts "Marking #{uploads_to_update.count} upload(s) as secure because login_required is true.", "" - uploads_to_update.update_all(secure: true) + uploads_to_update.update_all( + secure: true, + security_last_changed_at: Time.zone.now, + security_last_changed_reason: "upload security rake task all secure login required" + ) puts "Finished marking upload(s) as secure." end @@ -665,11 +669,19 @@ end def update_specific_upload_security_no_login_required(upload_ids_to_mark_as_secure, upload_ids_to_mark_as_not_secure) if upload_ids_to_mark_as_secure.any? puts "Marking #{upload_ids_to_mark_as_secure.length} uploads as secure because UploadSecurity determined them to be secure." - Upload.where(id: upload_ids_to_mark_as_secure).update_all(secure: true) + Upload.where(id: upload_ids_to_mark_as_secure).update_all( + secure: true, + security_last_changed_at: Time.zone.now, + security_last_changed_reason: "upload security rake task mark as secure" + ) end if upload_ids_to_mark_as_not_secure.any? puts "Marking #{upload_ids_to_mark_as_not_secure.length} uploads as not secure because UploadSecurity determined them to be not secure." - Upload.where(id: upload_ids_to_mark_as_not_secure).update_all(secure: false) + Upload.where(id: upload_ids_to_mark_as_not_secure).update_all( + secure: false, + security_last_changed_at: Time.zone.now, + security_last_changed_reason: "upload security rake task mark as not secure" + ) end puts "Finished updating upload security." end diff --git a/lib/topic_upload_security_manager.rb b/lib/topic_upload_security_manager.rb index 124219a4cb9..896b813d822 100644 --- a/lib/topic_upload_security_manager.rb +++ b/lib/topic_upload_security_manager.rb @@ -31,7 +31,7 @@ class TopicUploadSecurityManager # we have already got the post preloaded so we may as well # attach it here to avoid another load in UploadSecurity upload.access_control_post = post - upload.update_secure_status + upload.update_secure_status(source: "topic upload security") end post.rebake! if secure_status_did_change Rails.logger.debug("Security updated & rebake complete in topic #{@topic.id} - post #{post.id}") @@ -62,7 +62,7 @@ class TopicUploadSecurityManager first_post_upload_appeared_in = upload.post_uploads.first.post if first_post_upload_appeared_in == post upload.update(access_control_post: post) - upload.update_secure_status + upload.update_secure_status(source: "topic upload security") else false end diff --git a/lib/upload_creator.rb b/lib/upload_creator.rb index 3d8b0e8383d..98df85db338 100644 --- a/lib/upload_creator.rb +++ b/lib/upload_creator.rb @@ -130,6 +130,13 @@ class UploadCreator end add_metadata! + + if SiteSetting.secure_media + secure, reason = UploadSecurity.new(@upload, @opts.merge(creating: true)).should_be_secure_with_reason + attrs = @upload.secure_params(secure, reason, "upload creator") + @upload.assign_attributes(attrs) + end + return @upload unless @upload.save DiscourseEvent.trigger(:before_upload_creation, @file, is_image, @opts[:for_export]) @@ -424,7 +431,6 @@ class UploadCreator @upload.for_export = true if @opts[:for_export] @upload.for_site_setting = true if @opts[:for_site_setting] @upload.for_gravatar = true if @opts[:for_gravatar] - @upload.secure = UploadSecurity.new(@upload, @opts).should_be_secure? end private diff --git a/lib/upload_security.rb b/lib/upload_security.rb index 45724735544..9af270de917 100644 --- a/lib/upload_security.rb +++ b/lib/upload_security.rb @@ -5,14 +5,16 @@ # # Some of these flags checked (e.g. all of the for_X flags and the opts[:type]) # are only set when _initially uploading_ via UploadCreator and are not present -# when an upload already exists. +# when an upload already exists, these will only be checked when the @creating +# option is present. # # If the upload already exists the best way to figure out whether it should be # secure alongside the site settings is the access_control_post_id, because the # original post the upload is linked to has far more bearing on its security context # post-upload. If the access_control_post_id does not exist then we just rely # on the current secure? status, otherwise there would be a lot of additional -# complex queries and joins to perform. +# complex queries and joins to perform. Over time more of these specific +# queries will be implemented. class UploadSecurity @@custom_public_types = [] @@ -38,32 +40,52 @@ class UploadSecurity @upload = upload @opts = opts @upload_type = @opts[:type] + @creating = @opts[:creating] end def should_be_secure? - return false if !SiteSetting.secure_media? - return false if uploading_in_public_context? - uploading_in_secure_context? + should_be_secure_with_reason.first end - 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? - end - - def uploading_in_secure_context? - return true if SiteSetting.login_required? - if @upload.access_control_post_id.present? - return access_control_post_has_secure_media? + def should_be_secure_with_reason + insecure_context_checks.each do |check, reason| + return [false, reason] if perform_check(check) end - uploading_in_composer? || @upload.for_private_message || @upload.for_group_message || @upload.secure? + secure_context_checks.each do |check, reason| + return [perform_check(check), reason] if priority_check?(check) + return [true, reason] if perform_check(check) + end + + [false, "no checks satisfied"] end - private + def secure_media_disabled_check + !SiteSetting.secure_media? + end + + def insecure_creation_for_modifiers_check + return false if !@creating + @upload.for_theme || @upload.for_site_setting || @upload.for_gravatar + end + + def public_type_check + PUBLIC_TYPES.include?(@upload_type) || @@custom_public_types.include?(@upload_type) + end + + def custom_emoji_check + @upload.id.present? && CustomEmoji.exists?(upload_id: @upload.id) + end + + def regular_emoji_check + return false if @upload.origin.blank? + uri = URI.parse(@upload.origin) + return true if Emoji.all.map(&:url).include?("#{uri.path}?#{uri.query}") + uri.path.include?("images/emoji") + end + + def login_required_check + SiteSetting.login_required? + end # whether the upload should remain secure or not after posting depends on its context, # which is based on the post it is linked to via access_control_post_id. @@ -71,28 +93,59 @@ class UploadSecurity # this may change to false if the upload was set to secure on upload e.g. in # a post composer then it turned out that the post itself was not in a secure context # - # if there is no access control post id and the upload is currently secure, we - # do not want to make it un-secure to avoid unintentionally exposing it - def access_control_post_has_secure_media? - @upload.access_control_post.with_secure_media? + # a post is with secure media if it is a private message or in a read restricted + # category + def access_control_post_has_secure_media_check + access_control_post&.with_secure_media? end - def public_type? - PUBLIC_TYPES.include?(@upload_type) || @@custom_public_types.include?(@upload_type) - end - - def uploading_in_composer? + def uploading_in_composer_check @upload_type == "composer" end - def used_for_custom_emoji? - @upload.id.present? && CustomEmoji.exists?(upload_id: @upload.id) + def secure_creation_for_modifiers_check + return false if !@creating + @upload.for_private_message || @upload.for_group_message end - def based_on_regular_emoji? - return false if @upload.origin.blank? - uri = URI.parse(@upload.origin) - return true if Emoji.all.map(&:url).include?("#{uri.path}?#{uri.query}") - uri.path.include?("images/emoji") + def already_secure_check + @upload.secure? + end + + private + + def access_control_post + @access_control_post ||= @upload.access_control_post_id.present? ? @upload.access_control_post : nil + end + + def insecure_context_checks + { + secure_media_disabled: "secure media is disabled", + insecure_creation_for_modifiers: "one or more creation for_modifiers was satisfied", + public_type: "upload is public type", + custom_emoji: "upload is used for custom emoji", + regular_emoji: "upload is used for regular emoji" + } + end + + def secure_context_checks + { + login_required: "login is required", + access_control_post_has_secure_media: "access control post dictates security", + secure_creation_for_modifiers: "one or more creation for_modifiers was satisfied", + uploading_in_composer: "uploading via the composer", + already_secure: "upload is already secure" + } + end + + # the access control check is important because that is the truest indicator + # of whether an upload should be secure or not, and thus should be returned + # immediately if there is an access control post + def priority_check?(check) + check == :access_control_post_has_secure_media && access_control_post + end + + def perform_check(check) + send("#{check}_check") end end diff --git a/spec/components/email/sender_spec.rb b/spec/components/email/sender_spec.rb index a5b48929914..3f80e23b2a2 100644 --- a/spec/components/email/sender_spec.rb +++ b/spec/components/email/sender_spec.rb @@ -439,7 +439,7 @@ describe Email::Sender do @secure_image_file = file_from_fixtures("logo.png", "images") @secure_image = UploadCreator.new(@secure_image_file, "logo.png") .create_for(Discourse.system_user.id) - @secure_image.update_secure_status(secure_override_value: true) + @secure_image.update_secure_status(override: true) @secure_image.update(access_control_post_id: reply.id) reply.update(raw: reply.raw + "\n" + "#{UploadMarkdown.new(@secure_image).image_markdown}") reply.rebake! diff --git a/spec/lib/upload_security_spec.rb b/spec/lib/upload_security_spec.rb index 698e5b12759..12fced51630 100644 --- a/spec/lib/upload_security_spec.rb +++ b/spec/lib/upload_security_spec.rb @@ -7,7 +7,7 @@ RSpec.describe UploadSecurity do let(:post_in_secure_context) { Fabricate(:post, topic: Fabricate(:topic, category: private_category)) } fab!(:upload) { Fabricate(:upload) } let(:type) { nil } - let(:opts) { { type: type } } + let(:opts) { { type: type, creating: true } } subject { described_class.new(upload, opts) } context "when secure media is enabled" do diff --git a/spec/models/post_spec.rb b/spec/models/post_spec.rb index 5fa6c13a049..856d46426f4 100644 --- a/spec/models/post_spec.rb +++ b/spec/models/post_spec.rb @@ -1495,7 +1495,7 @@ describe Post do SiteSetting.secure_media = true post = Fabricate(:post, raw: raw, user: user, topic: Fabricate(:private_message_topic, user: user)) post.link_post_uploads - post.update_uploads_secure_status + post.update_uploads_secure_status(source: "test") expect(PostUpload.where(post: post).joins(:upload).pluck(:upload_id, :secure)).to contain_exactly( [attachment_upload.id, true], @@ -1507,7 +1507,7 @@ describe Post do SiteSetting.secure_media = false post = Fabricate(:post, raw: raw, user: user, topic: Fabricate(:private_message_topic, user: user)) post.link_post_uploads - post.update_uploads_secure_status + post.update_uploads_secure_status(source: "test") expect(PostUpload.where(post: post).joins(:upload).pluck(:upload_id, :secure)).to contain_exactly( [attachment_upload.id, false], @@ -1520,7 +1520,7 @@ describe Post do private_category = Fabricate(:private_category, group: Fabricate(:group)) post = Fabricate(:post, raw: raw, user: user, topic: Fabricate(:topic, user: user, category: private_category)) post.link_post_uploads - post.update_uploads_secure_status + post.update_uploads_secure_status(source: "test") expect(PostUpload.where(post: post).joins(:upload).pluck(:upload_id, :secure)).to contain_exactly( [attachment_upload.id, true], @@ -1531,11 +1531,11 @@ describe Post do it "does not mark an upload as secure if it has already been used in a public topic" do post = Fabricate(:post, raw: raw, user: user, topic: Fabricate(:topic, user: user)) post.link_post_uploads - post.update_uploads_secure_status + post.update_uploads_secure_status(source: "test") pm = Fabricate(:post, raw: raw, user: user, topic: Fabricate(:private_message_topic, user: user)) pm.link_post_uploads - pm.update_uploads_secure_status + pm.update_uploads_secure_status(source: "test") expect(PostUpload.where(post: pm).joins(:upload).pluck(:upload_id, :secure)).to contain_exactly( [attachment_upload.id, false], diff --git a/spec/models/upload_spec.rb b/spec/models/upload_spec.rb index bf7024c04e4..6613a29cb0c 100644 --- a/spec/models/upload_spec.rb +++ b/spec/models/upload_spec.rb @@ -343,14 +343,14 @@ describe Upload do end describe '.update_secure_status' do - it "respects the secure_override_value parameter if provided" do + it "respects the override parameter if provided" do upload.update!(secure: true) - upload.update_secure_status(secure_override_value: true) + upload.update_secure_status(override: true) expect(upload.secure).to eq(true) - upload.update_secure_status(secure_override_value: false) + upload.update_secure_status(override: false) expect(upload.secure).to eq(false) end