From c532f6eb3d2b45ba11b7ad7523e8a8a888650c0f Mon Sep 17 00:00:00 2001
From: Martin Brennan <martin@discourse.org>
Date: Wed, 6 Sep 2023 09:39:09 +1000
Subject: [PATCH] FEATURE: Secure uploads in PMs only (#23398)

This adds a new secure_uploads_pm_only site setting. When secure_uploads
is true with this setting, only uploads created in PMs will be marked
secure; no uploads in secure categories will be marked as secure, and
the login_required site setting has no bearing on upload security
either.

This is meant to be a stopgap solution to prevent secure uploads
in a single place (private messages) for sensitive admin data exports.
Ideally we would want a more comprehensive way of saying that certain
upload types get secured which is a hybrid/mixed mode secure uploads,
but for now this will do the trick.
---
 app/models/post.rb                            |  11 +-
 config/site_settings.yml                      |   4 +
 lib/tasks/uploads.rake                        |  25 +++-
 lib/topic_upload_security_manager.rb          |   7 +-
 lib/upload_security.rb                        |   2 +-
 spec/lib/upload_security_spec.rb              |   8 ++
 spec/models/post_spec.rb                      |  25 ++++
 spec/models/upload_spec.rb                    |  38 ++++--
 spec/support/system_helpers.rb                |   5 +-
 .../page_objects/components/composer.rb       |  12 +-
 spec/system/page_objects/pages/topic.rb       |  15 ++-
 spec/system/s3_secure_uploads_spec.rb         | 118 ++++++++++++++++++
 spec/system/s3_uploads_spec.rb                |   5 +-
 spec/tasks/uploads_spec.rb                    |  51 ++++++--
 14 files changed, 283 insertions(+), 43 deletions(-)
 create mode 100644 spec/system/s3_secure_uploads_spec.rb

diff --git a/app/models/post.rb b/app/models/post.rb
index d0068ac25be144..0801a77f7316d8 100644
--- a/app/models/post.rb
+++ b/app/models/post.rb
@@ -567,8 +567,15 @@ def reviewable_flag
 
   def with_secure_uploads?
     return false if !SiteSetting.secure_uploads?
-    SiteSetting.login_required? ||
-      (topic.present? && (topic.private_message? || topic.category&.read_restricted))
+
+    # NOTE: This is meant to be a stopgap solution to prevent secure uploads
+    # in a single place (private messages) for sensitive admin data exports.
+    # Ideally we would want a more comprehensive way of saying that certain
+    # upload types get secured which is a hybrid/mixed mode secure uploads,
+    # but for now this will do the trick.
+    return topic&.private_message? if SiteSetting.secure_uploads_pm_only?
+
+    SiteSetting.login_required? || topic&.private_message? || topic&.category&.read_restricted?
   end
 
   def hide!(post_action_type_id, reason = nil, custom_message: nil)
diff --git a/config/site_settings.yml b/config/site_settings.yml
index 86c7ffeb57d7bb..f82c04e2c8d29f 100644
--- a/config/site_settings.yml
+++ b/config/site_settings.yml
@@ -1439,6 +1439,10 @@ files:
     default: 1024
     min: 1
     max: 10240
+  secure_uploads_pm_only:
+    default: false
+    hidden: true
+    client: true
   enable_s3_uploads:
     default: false
     client: true
diff --git a/lib/tasks/uploads.rake b/lib/tasks/uploads.rake
index 3b2328b8281338..b795bacc4b2106 100644
--- a/lib/tasks/uploads.rake
+++ b/lib/tasks/uploads.rake
@@ -609,11 +609,15 @@ task "uploads:secure_upload_analyse_and_update" => :environment do
       update_uploads_access_control_post if SiteSetting.secure_uploads?
 
       puts "", "Analysing which uploads need to be marked secure and be rebaked.", ""
-      if SiteSetting.login_required?
-        # Simply mark all uploads linked to posts secure if login_required because no anons will be able to access them.
+      if SiteSetting.login_required? && !SiteSetting.secure_uploads_pm_only?
+        # Simply mark all uploads linked to posts secure if login_required because
+        # no anons will be able to access them; however if secure_uploads_pm_only is
+        # true then login_required will not mark other uploads secure.
         post_ids_to_rebake, all_upload_ids_changed = mark_all_as_secure_login_required
       else
-        # Otherwise only mark uploads linked to posts in secure categories or PMs as secure.
+        # Otherwise only mark uploads linked to posts either:
+        #   * In secure categories or PMs if !SiteSetting.secure_uploads_pm_only
+        #   * In PMs if SiteSetting.secure_uploads_pm_only
         post_ids_to_rebake, all_upload_ids_changed =
           update_specific_upload_security_no_login_required
       end
@@ -693,15 +697,24 @@ end
 def update_specific_upload_security_no_login_required
   # A simplification of the rules found in UploadSecurity which is a lot faster than
   # having to loop through records and use that class to check security.
+  filter_clause =
+    if SiteSetting.secure_uploads_pm_only?
+      "WHERE topics.archetype = 'private_message'"
+    else
+      <<~SQL
+        LEFT JOIN categories ON categories.id = topics.category_id
+        WHERE (topics.category_id IS NOT NULL AND categories.read_restricted) OR
+          (topics.archetype = 'private_message')
+      SQL
+    end
+
   post_upload_ids_marked_secure = DB.query_single(<<~SQL)
     WITH upl AS (
       SELECT DISTINCT ON (upload_id) upload_id
       FROM upload_references
       INNER JOIN posts ON posts.id = upload_references.target_id AND upload_references.target_type = 'Post'
       INNER JOIN topics ON topics.id = posts.topic_id
-      LEFT JOIN categories ON categories.id = topics.category_id
-      WHERE (topics.category_id IS NOT NULL AND categories.read_restricted) OR
-        (topics.archetype = 'private_message')
+      #{filter_clause}
     )
     UPDATE uploads
     SET secure = true,
diff --git a/lib/topic_upload_security_manager.rb b/lib/topic_upload_security_manager.rb
index aec874804bf91c..e7e03633385321 100644
--- a/lib/topic_upload_security_manager.rb
+++ b/lib/topic_upload_security_manager.rb
@@ -29,8 +29,9 @@ def run
 
         secure_status_did_change =
           post.owned_uploads_via_access_control.any? do |upload|
-            # we have already got the post preloaded so we may as well
+            # We already have the post preloaded so we may as well
             # attach it here to avoid another load in UploadSecurity
+            # (which is called via update_secure_status)
             upload.access_control_post = post
             upload.update_secure_status(source: "topic upload security")
           end
@@ -43,14 +44,14 @@ def run
 
     return if !SiteSetting.secure_uploads
 
-    # we only want to do this if secure uploads is enabled. if
+    # We only want to do this if secure uploads is enabled. If
     # the setting is turned on after a site has been running
     # already, we want to make sure that any post moves after
     # this are handled and upload secure statuses and ACLs
     # are updated appropriately, as well as setting the access control
     # post for secure uploads missing it.
     #
-    # examples (all after secure uploads is enabled):
+    # Examples (all after secure uploads is enabled):
     #
     #  -> a public topic is moved to a private category after
     #  -> a PM is converted to a public topic
diff --git a/lib/upload_security.rb b/lib/upload_security.rb
index ecdc3c43b40c30..4f97bdd797d6bf 100644
--- a/lib/upload_security.rb
+++ b/lib/upload_security.rb
@@ -163,7 +163,7 @@ def regular_emoji_check
   #### START PRIVATE CHECKS ####
 
   def login_required_check
-    SiteSetting.login_required?
+    SiteSetting.login_required? && !SiteSetting.secure_uploads_pm_only?
   end
 
   # Whether the upload should remain secure or not after posting depends on its context,
diff --git a/spec/lib/upload_security_spec.rb b/spec/lib/upload_security_spec.rb
index 7ad9174a757086..d2036f60658a90 100644
--- a/spec/lib/upload_security_spec.rb
+++ b/spec/lib/upload_security_spec.rb
@@ -28,6 +28,14 @@
           expect(security.should_be_secure?).to eq(true)
         end
 
+        context "if secure_uploads_pm_only" do
+          before { SiteSetting.secure_uploads_pm_only = true }
+
+          it "returns false" do
+            expect(security.should_be_secure?).to eq(false)
+          end
+        end
+
         context "when uploading in public context" do
           describe "for a public type badge_image" do
             let(:type) { "badge_image" }
diff --git a/spec/models/post_spec.rb b/spec/models/post_spec.rb
index 0d4fe09d689860..564ba925a8da2c 100644
--- a/spec/models/post_spec.rb
+++ b/spec/models/post_spec.rb
@@ -150,6 +150,7 @@ def post_with_body(body, user = nil)
   describe "with_secure_uploads?" do
     let(:topic) { Fabricate(:topic) }
     let!(:post) { Fabricate(:post, topic: topic) }
+
     it "returns false if secure uploads is not enabled" do
       expect(post.with_secure_uploads?).to eq(false)
     end
@@ -167,6 +168,14 @@ def post_with_body(body, user = nil)
         it "returns true" do
           expect(post.with_secure_uploads?).to eq(true)
         end
+
+        context "if secure_uploads_pm_only" do
+          before { SiteSetting.secure_uploads_pm_only = true }
+
+          it "returns false" do
+            expect(post.with_secure_uploads?).to eq(false)
+          end
+        end
       end
 
       context "if the topic category is read_restricted" do
@@ -176,6 +185,14 @@ def post_with_body(body, user = nil)
         it "returns true" do
           expect(post.with_secure_uploads?).to eq(true)
         end
+
+        context "if secure_uploads_pm_only" do
+          before { SiteSetting.secure_uploads_pm_only = true }
+
+          it "returns false" do
+            expect(post.with_secure_uploads?).to eq(false)
+          end
+        end
       end
 
       context "if the post is in a PM topic" do
@@ -184,6 +201,14 @@ def post_with_body(body, user = nil)
         it "returns true" do
           expect(post.with_secure_uploads?).to eq(true)
         end
+
+        context "if secure_uploads_pm_only" do
+          before { SiteSetting.secure_uploads_pm_only = true }
+
+          it "returns true" do
+            expect(post.with_secure_uploads?).to eq(true)
+          end
+        end
       end
     end
   end
diff --git a/spec/models/upload_spec.rb b/spec/models/upload_spec.rb
index ac80783a36c520..6ac71da0f8c87a 100644
--- a/spec/models/upload_spec.rb
+++ b/spec/models/upload_spec.rb
@@ -491,16 +491,38 @@
         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,
-            )
+          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
+
+      context "when secure_uploads_pm_only is true" do
+        before { SiteSetting.secure_uploads_pm_only = true }
+
+        it "does not mark an image upload as secure if login_required is enabled" do
+          SiteSetting.login_required = true
+          upload.update!(secure: false)
+          expect { upload.update_secure_status }.not_to change { upload.secure }
+          expect(upload.reload.secure).to eq(false)
+        end
+
+        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
+          upload.update!(secure: true, access_control_post: Fabricate(:private_message_post))
+          upload.update_secure_status
+          expect(upload.secure).to eq(true)
+        end
+      end
     end
   end
 
diff --git a/spec/support/system_helpers.rb b/spec/support/system_helpers.rb
index 95e0a65ea382c4..4bd88635931c8f 100644
--- a/spec/support/system_helpers.rb
+++ b/spec/support/system_helpers.rb
@@ -141,7 +141,7 @@ def select_text_range(selector, start = 0, offset = 5)
     page.execute_script(js, selector, start, offset)
   end
 
-  def setup_s3_system_test
+  def setup_s3_system_test(enable_secure_uploads: false, enable_direct_s3_uploads: true)
     SiteSetting.enable_s3_uploads = true
 
     SiteSetting.s3_upload_bucket = "discoursetest"
@@ -151,6 +151,9 @@ def setup_s3_system_test
     SiteSetting.s3_secret_access_key = MinioRunner.config.minio_root_password
     SiteSetting.s3_endpoint = MinioRunner.config.minio_server_url
 
+    SiteSetting.enable_direct_s3_uploads = enable_direct_s3_uploads
+    SiteSetting.secure_uploads = enable_secure_uploads
+
     MinioRunner.start
   end
 
diff --git a/spec/system/page_objects/components/composer.rb b/spec/system/page_objects/components/composer.rb
index da6ebe59605d1c..580841f52729a1 100644
--- a/spec/system/page_objects/components/composer.rb
+++ b/spec/system/page_objects/components/composer.rb
@@ -82,8 +82,8 @@ def category_chooser
       end
 
       def switch_category(category_name)
-        find(".category-chooser").click
-        find(".category-row[data-name='#{category_name}']").click
+        category_chooser.expand
+        category_chooser.select_row_by_name(category_name)
       end
 
       def preview
@@ -216,6 +216,14 @@ def has_in_progress_uploads?
         find("#{COMPOSER_ID}").has_css?("#file-uploading")
       end
 
+      def select_pm_user(username)
+        select_kit = PageObjects::Components::SelectKit.new("#private-message-users")
+        select_kit.expand
+        select_kit.search(username)
+        select_kit.select_row_by_value(username)
+        select_kit.collapse
+      end
+
       private
 
       def emoji_preview_selector(emoji)
diff --git a/spec/system/page_objects/pages/topic.rb b/spec/system/page_objects/pages/topic.rb
index 006b3b673c6295..262d8f9933b7b4 100644
--- a/spec/system/page_objects/pages/topic.rb
+++ b/spec/system/page_objects/pages/topic.rb
@@ -16,8 +16,7 @@ def visit_topic(topic, post_number: nil)
       end
 
       def open_new_topic
-        page.visit "/"
-        find("button#create-topic").click
+        page.visit "/new-topic"
         self
       end
 
@@ -32,6 +31,14 @@ def visit_topic_and_open_composer(topic)
         self
       end
 
+      def current_topic_id
+        find("h1[data-topic-id]")["data-topic-id"]
+      end
+
+      def current_topic
+        ::Topic.find(current_topic_id)
+      end
+
       def has_topic_title?(text)
         has_css?("h1 .fancy-title", text: text)
       end
@@ -44,9 +51,9 @@ def has_post_number?(number)
         has_css?("#post_#{number}")
       end
 
-      def post_by_number(post_or_number)
+      def post_by_number(post_or_number, wait: Capybara.default_max_wait_time)
         post_or_number = post_or_number.is_a?(Post) ? post_or_number.post_number : post_or_number
-        find(".topic-post:not(.staged) #post_#{post_or_number}")
+        find(".topic-post:not(.staged) #post_#{post_or_number}", wait: wait)
       end
 
       def post_by_number_selector(post_number)
diff --git a/spec/system/s3_secure_uploads_spec.rb b/spec/system/s3_secure_uploads_spec.rb
new file mode 100644
index 00000000000000..1ab9bcaa2769e8
--- /dev/null
+++ b/spec/system/s3_secure_uploads_spec.rb
@@ -0,0 +1,118 @@
+# frozen_string_literal: true
+
+describe "Uploading files in the composer to S3", type: :system do
+  fab!(:current_user) { Fabricate(:admin) }
+  fab!(:other_user) { Fabricate(:user, username: "otherguy") }
+
+  let(:modal) { PageObjects::Modals::Base.new }
+  let(:composer) { PageObjects::Components::Composer.new }
+  let(:topic_page) { PageObjects::Pages::Topic.new }
+
+  describe "secure uploads" do
+    def first_post_img(wait: Capybara.default_max_wait_time)
+      first_post = topic_page.post_by_number(1, wait: wait)
+      expect(first_post).to have_css("img[data-base62-sha1]")
+      first_post.find(".cooked").first("img")
+    end
+
+    def expect_first_post_to_have_secure_upload
+      img = first_post_img
+      expect(img["src"]).to include("/secure-uploads")
+      topic = topic_page.current_topic
+      expect(topic.first_post.uploads.first.secure).to eq(true)
+    end
+
+    it "marks uploads inside of private message posts as secure" do
+      skip_unless_s3_system_specs_enabled!
+
+      setup_s3_system_test(enable_secure_uploads: true)
+      sign_in(current_user)
+
+      topic_page.open_new_message
+
+      composer.fill_title("This is a test PM for secure uploads")
+      composer.select_pm_user("otherguy")
+
+      file_path = file_from_fixtures("logo.png", "images").path
+      attach_file(file_path) { composer.click_toolbar_button("upload") }
+
+      expect(page).to have_no_css("#file-uploading")
+      expect(composer.preview).to have_css(".image-wrapper")
+
+      composer.submit
+
+      expect_first_post_to_have_secure_upload
+    end
+
+    it "marks uploads inside of private category posts as secure" do
+      skip_unless_s3_system_specs_enabled!
+
+      private_category = Fabricate(:private_category, group: Fabricate(:group))
+      setup_s3_system_test(enable_secure_uploads: true)
+      sign_in(current_user)
+
+      topic_page.open_new_topic
+
+      composer.fill_title("This is a test PM for secure uploads")
+      composer.switch_category(private_category.name)
+
+      file_path = file_from_fixtures("logo.png", "images").path
+      attach_file(file_path) { composer.click_toolbar_button("upload") }
+
+      expect(page).to have_no_css("#file-uploading")
+      expect(composer.preview).to have_css(".image-wrapper")
+
+      composer.submit
+
+      expect_first_post_to_have_secure_upload
+    end
+
+    it "marks uploads for all posts as secure when login_required" do
+      skip_unless_s3_system_specs_enabled!
+
+      SiteSetting.login_required = true
+      setup_s3_system_test(enable_secure_uploads: true)
+      sign_in(current_user)
+
+      topic_page.open_new_topic
+
+      composer.fill_title("This is a test PM for secure uploads")
+
+      file_path = file_from_fixtures("logo.png", "images").path
+      attach_file(file_path) { composer.click_toolbar_button("upload") }
+
+      expect(page).to have_no_css("#file-uploading")
+      expect(composer.preview).to have_css(".image-wrapper")
+
+      composer.submit
+
+      expect_first_post_to_have_secure_upload
+    end
+
+    it "doesn't mark uploads for public posts as secure" do
+      skip_unless_s3_system_specs_enabled!
+
+      setup_s3_system_test(enable_secure_uploads: true)
+      sign_in(current_user)
+
+      topic_page.open_new_topic
+
+      composer.fill_title("This is a test PM for secure uploads")
+
+      file_path = file_from_fixtures("logo.png", "images").path
+      attach_file(file_path) { composer.click_toolbar_button("upload") }
+
+      expect(page).to have_no_css("#file-uploading")
+      expect(composer.preview).to have_css(".image-wrapper")
+
+      Jobs.run_immediately!
+      composer.submit
+
+      # Extra wait time is added because the job can slow down the processing of the request.
+      img = first_post_img(wait: 10)
+      expect(img["src"]).not_to include("/secure-uploads")
+      topic = topic_page.current_topic
+      expect(topic.first_post.uploads.first.secure).to eq(false)
+    end
+  end
+end
diff --git a/spec/system/s3_uploads_spec.rb b/spec/system/s3_uploads_spec.rb
index daec0371684782..a1f9090be228a8 100644
--- a/spec/system/s3_uploads_spec.rb
+++ b/spec/system/s3_uploads_spec.rb
@@ -5,10 +5,9 @@
 
   let(:modal) { PageObjects::Modals::Base.new }
   let(:composer) { PageObjects::Components::Composer.new }
+  let(:topic) { PageObjects::Pages::Topic.new }
 
   describe "direct S3 uploads" do
-    before { SiteSetting.enable_direct_s3_uploads = true }
-
     describe "single part uploads" do
       it "uploads custom avatars to S3" do
         skip_unless_s3_system_specs_enabled!
@@ -43,7 +42,7 @@
         setup_s3_system_test
         sign_in(current_user)
 
-        visit "/new-topic"
+        topic.open_new_topic
 
         file_path = file_from_fixtures("logo.png", "images").path
         attach_file(file_path) { composer.click_toolbar_button("upload") }
diff --git a/spec/tasks/uploads_spec.rb b/spec/tasks/uploads_spec.rb
index 8cb5c0f99ab87c..0f1a3693e1e3b6 100644
--- a/spec/tasks/uploads_spec.rb
+++ b/spec/tasks/uploads_spec.rb
@@ -54,22 +54,47 @@ def invoke_task
           expect(upload3.reload.access_control_post).to eq(post3)
         end
 
-        it "sets everything attached to a post as secure and rebakes all those posts if login is required" do
-          SiteSetting.login_required = true
-          freeze_time
+        context "when login_required" do
+          before { SiteSetting.login_required = true }
 
-          post1.update_columns(baked_at: 1.week.ago)
-          post2.update_columns(baked_at: 1.week.ago)
-          post3.update_columns(baked_at: 1.week.ago)
+          it "sets everything attached to a post as secure and rebakes all those posts" do
+            freeze_time
 
-          invoke_task
+            post1.update_columns(baked_at: 1.week.ago)
+            post2.update_columns(baked_at: 1.week.ago)
+            post3.update_columns(baked_at: 1.week.ago)
 
-          expect(post1.reload.baked_at).not_to eq_time(1.week.ago)
-          expect(post2.reload.baked_at).not_to eq_time(1.week.ago)
-          expect(post3.reload.baked_at).not_to eq_time(1.week.ago)
-          expect(upload2.reload.secure).to eq(true)
-          expect(upload1.reload.secure).to eq(true)
-          expect(upload3.reload.secure).to eq(true)
+            invoke_task
+
+            expect(post1.reload.baked_at).not_to eq_time(1.week.ago)
+            expect(post2.reload.baked_at).not_to eq_time(1.week.ago)
+            expect(post3.reload.baked_at).not_to eq_time(1.week.ago)
+            expect(upload2.reload.secure).to eq(true)
+            expect(upload1.reload.secure).to eq(true)
+            expect(upload3.reload.secure).to eq(true)
+          end
+
+          context "when secure_uploads_pm_only" do
+            before { SiteSetting.secure_uploads_pm_only = true }
+
+            it "only sets everything attached to a private message post as secure and rebakes all those posts" do
+              freeze_time
+
+              post1.update_columns(baked_at: 1.week.ago)
+              post2.update_columns(baked_at: 1.week.ago)
+              post3.update_columns(baked_at: 1.week.ago)
+              post3.topic.update(archetype: "private_message", category: nil)
+
+              invoke_task
+
+              expect(post1.reload.baked_at).to eq_time(1.week.ago)
+              expect(post2.reload.baked_at).to eq_time(1.week.ago)
+              expect(post3.reload.baked_at).not_to eq_time(1.week.ago)
+              expect(upload1.reload.secure).to eq(false)
+              expect(upload2.reload.secure).to eq(true)
+              expect(upload3.reload.secure).to eq(true)
+            end
+          end
         end
 
         it "sets the uploads that are media and attachments in the read restricted topic category to secure" do