diff --git a/lib/tasks/uploads.rake b/lib/tasks/uploads.rake
index 8410987d8e..6314efff69 100644
--- a/lib/tasks/uploads.rake
+++ b/lib/tasks/uploads.rake
@@ -537,6 +537,10 @@ task "uploads:sync_s3_acls" => :environment do
end
end
+#
+# TODO (martin) Update this rake task to use the _first_ UploadReference
+# record for each upload to determine security, and do not mark things
+# as secure if the first record is something public e.g. a site setting.
task "uploads:disable_secure_uploads" => :environment do
RailsMultisite::ConnectionManagement.each_connection do |db|
unless Discourse.store.external?
@@ -584,6 +588,10 @@ end
# the upload secure flag and S3 upload ACLs. Any uploads that
# have their secure status changed will have all associated posts
# rebaked.
+#
+# TODO (martin) Update this rake task to use the _first_ UploadReference
+# record for each upload to determine security, and do not mark things
+# as secure if the first record is something public e.g. a site setting.
task "uploads:secure_upload_analyse_and_update" => :environment do
RailsMultisite::ConnectionManagement.each_connection do |db|
unless Discourse.store.external?
diff --git a/lib/upload_security.rb b/lib/upload_security.rb
index ccaf86f1ed..c5de6c17bf 100644
--- a/lib/upload_security.rb
+++ b/lib/upload_security.rb
@@ -13,8 +13,12 @@
# 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. Over time more of these specific
-# queries will be implemented.
+# complex queries and joins to perform.
+#
+# These queries will be performed only if the @creating option is false. So if
+# an upload is included in a post, and it's an upload from a different source
+# (e.g. a category logo, site setting upload) then we will determine secure
+# state _based on the first place the upload was referenced_.
#
# NOTE: When updating this to add more cases where uploads will be marked
# secure, consider uploads:secure_upload_analyse_and_update as well, which
@@ -35,6 +39,18 @@ class UploadSecurity
badge_image
]
+ PUBLIC_UPLOAD_REFERENCE_TYPES = %w[
+ Badge
+ Category
+ CustomEmoji
+ Group
+ SiteSetting
+ ThemeField
+ User
+ UserAvatar
+ UserProfile
+ ]
+
def self.register_custom_public_type(type)
@@custom_public_types << type if !@@custom_public_types.include?(type)
end
@@ -65,6 +81,46 @@ class UploadSecurity
[false, "no checks satisfied"]
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_uploads_disabled: "secure uploads is disabled",
+ insecure_creation_for_modifiers: "one or more creation for_modifiers was satisfied",
+ public_type: "upload is public type",
+ regular_emoji: "upload is used for regular emoji",
+ publicly_referenced_first: "upload was publicly referenced when it was first created",
+ }
+ end
+
+ def secure_context_checks
+ {
+ login_required: "login is required",
+ access_control_post_has_secure_uploads: "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_uploads && access_control_post
+ end
+
+ def perform_check(check)
+ send("#{check}_check")
+ end
+
+ #### START PUBLIC CHECKS ####
+
def secure_uploads_disabled_check
!SiteSetting.secure_uploads?
end
@@ -78,8 +134,11 @@ class UploadSecurity
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)
+ def publicly_referenced_first_check
+ return false if @creating
+ first_reference = @upload.upload_references.order(created_at: :asc).first
+ return false if first_reference.blank?
+ PUBLIC_UPLOAD_REFERENCE_TYPES.include?(first_reference.target_type)
end
def regular_emoji_check
@@ -89,18 +148,26 @@ class UploadSecurity
uri.path.include?("images/emoji")
end
+ #### END PUBLIC CHECKS ####
+
+ #--------------------------#
+
+ #### START PRIVATE CHECKS ####
+
def login_required_check
SiteSetting.login_required?
end
- # whether the upload should remain secure or not after posting depends on its context,
+ # 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.
- # if that post is with_secure_uploads? then the upload should also be secure.
- # 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
#
- # a post is with secure uploads if it is a private message or in a read restricted
- # category
+ # If that post is with_secure_uploads? then the upload should also be secure.
+ #
+ # 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.
+ #
+ # A post is with secure uploads if it is a private message or in a read restricted
+ # category. See `Post#with_secure_uploads?` for the full definition.
def access_control_post_has_secure_uploads_check
access_control_post&.with_secure_uploads?
end
@@ -118,41 +185,5 @@ class UploadSecurity
@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_uploads_disabled: "secure uploads 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_uploads: "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_uploads && access_control_post
- end
-
- def perform_check(check)
- send("#{check}_check")
- end
+ #### END PRIVATE CHECKS ####
end
diff --git a/spec/integration/secure_uploads_spec.rb b/spec/integration/secure_uploads_spec.rb
new file mode 100644
index 0000000000..b9c3c4d067
--- /dev/null
+++ b/spec/integration/secure_uploads_spec.rb
@@ -0,0 +1,124 @@
+# frozen_string_literal: true
+
+describe "Secure uploads" do
+ fab!(:user) { Fabricate(:user) }
+ fab!(:group) { Fabricate(:group) }
+ fab!(:secure_category) { Fabricate(:private_category, group: group) }
+
+ before do
+ Jobs.run_immediately!
+
+ # this is done so the after_save callbacks for site settings to make
+ # UploadReference records works
+ @original_provider = SiteSetting.provider
+ SiteSetting.provider = SiteSettings::DbProvider.new(SiteSetting)
+ setup_s3
+ stub_s3_store
+ SiteSetting.secure_uploads = true
+ group.add(user)
+ user.reload
+ end
+
+ after { SiteSetting.provider = @original_provider }
+
+ def create_upload
+ filename = "logo.png"
+ file = file_from_fixtures(filename)
+ UploadCreator.new(file, filename).create_for(user.id)
+ end
+
+ def stub_presign_upload_get(upload)
+ # this is necessary because by default any upload inside a secure post is considered "secure"
+ # for the purposes of fetching hotlinked images until proven otherwise, and this is easier
+ # than trying to stub the presigned URL for s3 in a different way
+ stub_request(:get, "https:#{upload.url}").to_return(
+ status: 200,
+ body: file_from_fixtures("logo.png"),
+ )
+ Upload.stubs(:signed_url_from_secure_uploads_url).returns("https:#{upload.url}")
+ end
+
+ it "does not convert an upload to secure when it was first used in a site setting then in a post" do
+ upload = create_upload
+ SiteSetting.favicon = upload
+ expect(upload.reload.upload_references.count).to eq(1)
+ create_post(
+ title: "Secure upload post",
+ raw: "This is a new post ![](\"#{upload.url}\")
",
+ category: secure_category,
+ user: user,
+ )
+ upload.reload
+ expect(upload.upload_references.count).to eq(2)
+ expect(upload.secure).to eq(false)
+ end
+
+ it "does not convert an upload to insecure when it was first used in a secure post then a site setting" do
+ upload = create_upload
+ create_post(
+ title: "Secure upload post",
+ raw: "This is a new post ",
+ category: secure_category,
+ user: user,
+ )
+ expect(upload.reload.upload_references.count).to eq(1)
+ SiteSetting.favicon = upload
+ upload.reload
+ expect(upload.upload_references.count).to eq(2)
+ expect(upload.secure).to eq(true)
+ end
+
+ it "does not convert an upload to secure when it was first used in a public post then in a secure post" do
+ upload = create_upload
+
+ post =
+ create_post(
+ title: "Public upload post",
+ raw: "This is a new post ",
+ user: user,
+ )
+ upload.reload
+ expect(upload.upload_references.count).to eq(1)
+ expect(upload.secure).to eq(false)
+ expect(upload.access_control_post).to eq(post)
+
+ stub_presign_upload_get(upload)
+ create_post(
+ title: "Secure upload post",
+ raw: "This is a new post ",
+ category: secure_category,
+ user: user,
+ )
+ upload.reload
+ expect(upload.upload_references.count).to eq(2)
+ expect(upload.secure).to eq(false)
+ expect(upload.access_control_post).to eq(post)
+ end
+
+ it "does not convert an upload to insecure when it was first used in a secure post then in a public post" do
+ upload = create_upload
+
+ stub_presign_upload_get(upload)
+ post =
+ create_post(
+ title: "Secure upload post",
+ raw: "This is a new post ",
+ category: secure_category,
+ user: user,
+ )
+ upload.reload
+ expect(upload.upload_references.count).to eq(1)
+ expect(upload.secure).to eq(true)
+ expect(upload.access_control_post).to eq(post)
+
+ create_post(
+ title: "Public upload post",
+ raw: "This is a new post ",
+ user: user,
+ )
+ upload.reload
+ expect(upload.upload_references.count).to eq(2)
+ expect(upload.secure).to eq(true)
+ expect(upload.access_control_post).to eq(post)
+ end
+end
diff --git a/spec/lib/upload_security_spec.rb b/spec/lib/upload_security_spec.rb
index b5a307997f..1a70bfecfa 100644
--- a/spec/lib/upload_security_spec.rb
+++ b/spec/lib/upload_security_spec.rb
@@ -1,13 +1,14 @@
# frozen_string_literal: true
RSpec.describe UploadSecurity do
- let(:private_category) { Fabricate(:private_category, group: Fabricate(:group)) }
- let(:post_in_secure_context) do
+ fab!(:private_category) { Fabricate(:private_category, group: Fabricate(:group)) }
+ fab!(:post_in_secure_context) do
Fabricate(:post, topic: Fabricate(:topic, category: private_category))
end
fab!(:upload) { Fabricate(:upload) }
let(:type) { nil }
- let(:opts) { { type: type, creating: true } }
+ let(:opts) { { type: type, creating: creating } }
+
subject { described_class.new(upload, opts) }
context "when secure uploads is enabled" do
@@ -16,172 +17,293 @@ RSpec.describe UploadSecurity do
SiteSetting.secure_uploads = true
end
- context "when login_required (everything should be secure except public context items)" do
- before { SiteSetting.login_required = true }
- it "returns true" do
- expect(subject.should_be_secure?).to eq(true)
- end
+ context "when creating the upload" do
+ let(:creating) { true }
- context "when uploading in public context" do
- describe "for a public type badge_image" do
- let(:type) { "badge_image" }
- it "returns false" do
- expect(subject.should_be_secure?).to eq(false)
- end
- end
- describe "for a public type group_flair" do
- let(:type) { "group_flair" }
- 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 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 a public type category_logo" do
- let(:type) { "category_logo" }
- it "returns false" do
- expect(subject.should_be_secure?).to eq(false)
- end
- end
- describe "for a public type category_background" do
- let(:type) { "category_background" }
- it "returns false" do
- expect(subject.should_be_secure?).to eq(false)
- end
- end
- describe "for a custom public type" do
- let(:type) { "my_custom_type" }
+ context "when login_required (everything should be secure except public context items)" do
+ before { SiteSetting.login_required = true }
- it "returns true if the custom type has not been added" do
- expect(subject.should_be_secure?).to eq(true)
- end
-
- it "returns false if the custom type has been added" do
- UploadSecurity.register_custom_public_type(type)
- expect(subject.should_be_secure?).to eq(false)
- UploadSecurity.reset_custom_public_types
- end
- end
- describe "for_theme" do
- before { upload.stubs(:for_theme).returns(true) }
- it "returns false" do
- expect(subject.should_be_secure?).to eq(false)
- end
- end
- describe "for_site_setting" do
- before { upload.stubs(:for_site_setting).returns(true) }
- it "returns false" do
- expect(subject.should_be_secure?).to eq(false)
- end
- end
- describe "for_gravatar" do
- before { upload.stubs(:for_gravatar).returns(true) }
- 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 do |e|
- e.url == "/images/emoji/twitter/falafel.png?v=#{Emoji::EMOJI_VERSION}"
- end
- upload.update!(origin: "http://localhost:3000#{falafel.url}")
- expect(subject.should_be_secure?).to eq(false)
- end
- end
- end
- end
-
- context "when the access control post has_secure_uploads?" do
- before { upload.update(access_control_post_id: post_in_secure_context.id) }
- it "returns true" do
- expect(subject.should_be_secure?).to eq(true)
- end
-
- context "when the post is deleted" do
- before { post_in_secure_context.trash! }
- it "still determines whether the post has secure uploads; returns true" do
- expect(subject.should_be_secure?).to eq(true)
- end
- 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 { upload.stubs(:for_group_message).returns(true) }
- it "returns true" do
- expect(subject.should_be_secure?).to eq(true)
- end
- end
- context "when uploading for a PM" do
- before { upload.stubs(:for_private_message).returns(true) }
- it "returns true" do
- expect(subject.should_be_secure?).to eq(true)
- end
- end
- context "when upload is already secure" do
- before { upload.update(secure: true) }
- it "returns true" do
- expect(subject.should_be_secure?).to eq(true)
- end
- end
-
- context "for attachments" do
- before { upload.update(original_filename: "test.pdf") }
-
- context "when the access control post has_secure_uploads?" do
- before { upload.update(access_control_post: post_in_secure_context) }
it "returns true" do
expect(subject.should_be_secure?).to eq(true)
end
+
+ context "when uploading in public context" do
+ describe "for a public type badge_image" do
+ let(:type) { "badge_image" }
+ it "returns false" do
+ expect(subject.should_be_secure?).to eq(false)
+ end
+ end
+
+ describe "for a public type group_flair" do
+ let(:type) { "group_flair" }
+ 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 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 a public type category_logo" do
+ let(:type) { "category_logo" }
+ it "returns false" do
+ expect(subject.should_be_secure?).to eq(false)
+ end
+ end
+
+ describe "for a public type category_background" do
+ let(:type) { "category_background" }
+ it "returns false" do
+ expect(subject.should_be_secure?).to eq(false)
+ end
+ end
+
+ describe "for a custom public type" do
+ let(:type) { "my_custom_type" }
+
+ it "returns true if the custom type has not been added" do
+ expect(subject.should_be_secure?).to eq(true)
+ end
+
+ it "returns false if the custom type has been added" do
+ UploadSecurity.register_custom_public_type(type)
+ expect(subject.should_be_secure?).to eq(false)
+ UploadSecurity.reset_custom_public_types
+ end
+ end
+
+ describe "for_theme" do
+ before { upload.stubs(:for_theme).returns(true) }
+
+ it "returns false" do
+ expect(subject.should_be_secure?).to eq(false)
+ end
+ end
+
+ describe "for_site_setting" do
+ before { upload.stubs(:for_site_setting).returns(true) }
+
+ it "returns false" do
+ expect(subject.should_be_secure?).to eq(false)
+ end
+ end
+
+ describe "for_gravatar" do
+ before { upload.stubs(:for_gravatar).returns(true) }
+
+ it "returns false" do
+ 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 do |e|
+ e.url == "/images/emoji/twitter/falafel.png?v=#{Emoji::EMOJI_VERSION}"
+ end
+ upload.update!(origin: "http://localhost:3000#{falafel.url}")
+ expect(subject.should_be_secure?).to eq(false)
+ end
+ end
+ end
+ end
+
+ context "when the access control post has_secure_uploads?" do
+ before { upload.update(access_control_post_id: post_in_secure_context.id) }
+
+ it "returns true" do
+ expect(subject.should_be_secure?).to eq(true)
+ end
+
+ context "when the post is deleted" do
+ before { post_in_secure_context.trash! }
+
+ it "still determines whether the post has secure uploads; returns true" do
+ expect(subject.should_be_secure?).to eq(true)
+ end
+ 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 { upload.stubs(:for_group_message).returns(true) }
+ it "returns true" do
+ expect(subject.should_be_secure?).to eq(true)
+ end
+ end
+
+ context "when uploading for a PM" do
+ before { upload.stubs(:for_private_message).returns(true) }
+ it "returns true" do
+ expect(subject.should_be_secure?).to eq(true)
+ end
+ end
+
+ context "when upload is already secure" do
+ before { upload.update(secure: true) }
+ it "returns true" do
+ expect(subject.should_be_secure?).to eq(true)
+ end
+ end
+
+ context "for attachments" do
+ before { upload.update(original_filename: "test.pdf") }
+
+ context "when the access control post has_secure_uploads?" do
+ before { upload.update(access_control_post: post_in_secure_context) }
+ it "returns true" do
+ expect(subject.should_be_secure?).to eq(true)
+ end
+ end
+ end
+ end
+
+ context "when checking an existing upload" do
+ let(:creating) { false }
+
+ before do
+ # this is done so the after_save callbacks for site settings to make
+ # UploadReference records works
+ @original_provider = SiteSetting.provider
+ SiteSetting.provider = SiteSettings::DbProvider.new(SiteSetting)
+ setup_s3
+ SiteSetting.secure_uploads = true
+ end
+
+ after { SiteSetting.provider = @original_provider }
+
+ def create_secure_post_reference
+ UploadReference.ensure_exist!(upload_ids: [upload.id], target: post_in_secure_context)
+ upload.update!(access_control_post: post_in_secure_context)
+ end
+
+ describe "when the upload is first used for a post in a secure context" do
+ it "returns true" do
+ create_secure_post_reference
+ expect(subject.should_be_secure?).to eq(true)
+ end
+ end
+
+ describe "when the upload is first used for a site setting" do
+ it "returns false" do
+ SiteSetting.favicon = upload
+ create_secure_post_reference
+ expect(subject.should_be_secure?).to eq(false)
+ end
+ end
+
+ describe "when the upload is first used for a theme field" do
+ it "returns false" do
+ Fabricate(:theme_field, type_id: ThemeField.types[:theme_upload_var], upload: upload)
+ create_secure_post_reference
+ expect(subject.should_be_secure?).to eq(false)
+ end
+ end
+
+ describe "when the upload is first used for a group flair image" do
+ it "returns false" do
+ Fabricate(:group, flair_upload: upload)
+ create_secure_post_reference
+ expect(subject.should_be_secure?).to eq(false)
+ end
+ end
+
+ describe "when the upload is first used for a custom emoji" do
+ it "returns false" do
+ CustomEmoji.create(name: "meme", upload: upload)
+ create_secure_post_reference
+ expect(subject.should_be_secure?).to eq(false)
+ end
+ end
+
+ describe "when the upload is first used for a badge" do
+ it "returns false" do
+ Fabricate(:badge, image_upload: upload)
+ create_secure_post_reference
+ expect(subject.should_be_secure?).to eq(false)
+ end
+ end
+
+ describe "when the upload is first used for a category image (logo, dark logo, background)" do
+ it "returns false" do
+ Fabricate(:category, uploaded_logo: upload)
+ create_secure_post_reference
+ expect(subject.should_be_secure?).to eq(false)
+ end
+ end
+
+ describe "when the upload is first used for a user profile (profile background, card background)" do
+ it "returns false" do
+ user = Fabricate(:user)
+ user.user_profile.update!(card_background_upload: upload)
+ create_secure_post_reference
+ expect(subject.should_be_secure?).to eq(false)
+ end
+ end
+
+ describe "when the upload is first used for a user uploaded avatar" do
+ it "returns false" do
+ Fabricate(:user, uploaded_avatar: upload)
+ create_secure_post_reference
+ expect(subject.should_be_secure?).to eq(false)
+ end
+ end
+
+ describe "when the upload is first used for a UserAvatar" do
+ it "returns false" do
+ Fabricate(:user_avatar, custom_upload: upload)
+ create_secure_post_reference
+ expect(subject.should_be_secure?).to eq(false)
+ end
end
end
end
context "when secure uploads is disabled" do
+ let(:creating) { true }
+
before { SiteSetting.secure_uploads = false }
+
it "returns false" do
expect(subject.should_be_secure?).to eq(false)
end
context "for attachments" do
before { upload.update(original_filename: "test.pdf") }
+
it "returns false" do
expect(subject.should_be_secure?).to eq(false)
end