FIX: Secure/unsecure uploads when sharing AI conversations (#554)

This commit uses a new plugin modifier introduced in https://github.com/discourse/discourse/pull/26508
to mark all uploads as _not_ secure in shared PM AI conversations.
This is so images created by the AI bot (or uploaded by the user)
do not end up as broken URLs because of the security requirements
around them.

This relies on the UpdateTopicUploadSecurity job in core as well,
which is fired when an AI conversation is shared or deleted.

app/controllers/discourse_ai/ai_bot/shared_ai_conversations_controller.rb

@@ -26,7 +26,9 @@ module DiscourseAi
 
       def destroy
         ensure_allowed_destroy!
-        @shared_conversation.destroy
+
+        SharedAiConversation.destroy_conversation(@shared_conversation)
+
         render json:
                  success_json.merge(message: I18n.t("discourse_ai.share_ai.conversation_deleted"))
       end

app/jobs/regular/shared_conversation_adjust_upload_security.rb

@@ -0,0 +1,51 @@
+# frozen_string_literal: true
+
+module ::Jobs
+  class SharedConversationAdjustUploadSecurity < ::Jobs::Base
+    def execute(args)
+      if args[:conversation_id].present?
+        # The conversation context includes post cooked content so this
+        # must be updated when target uploads security changes.
+        update_conversation(args[:conversation_id])
+      elsif args[:target_id].present? && args[:target_type].present?
+        # If we deleted the conversation then we just need to update the target's
+        # uploads security, no need to update the conversation.
+        update_target(args[:target_id], args[:target_type])
+      end
+    end
+
+    private
+
+    def update_conversation(conversation_id)
+      conversation = SharedAiConversation.find_by(id: conversation_id)
+      return if conversation.blank?
+
+      # NOTE: Only Topics are supported for now, in future we will need a more flexible
+      # way of doing this.
+      if conversation.target_type == "Topic"
+        rebaked_posts = TopicUploadSecurityManager.new(conversation.target).run
+
+        if rebaked_posts.any?
+          new_context =
+            conversation.context.map do |context_post|
+              rebaked_post = rebaked_posts.find { |p| p.id == context_post["id"] }
+              context_post["cooked"] = rebaked_post.cooked if rebaked_post
+              context_post
+            end
+
+          conversation.update(context: new_context)
+        end
+      end
+    end
+
+    def update_target(target_id, target_type)
+      # NOTE: Only Topics are supported for now, in future we will need a more flexible
+      # way of doing this.
+      if target_type == "Topic"
+        topic = target_type.constantize.find_by(id: target_id)
+        return if topic.blank?
+        TopicUploadSecurityManager.new(topic).run
+      end
+    end
+  end
+end

app/models/shared_ai_conversation.rb

@@ -19,12 +19,26 @@ class SharedAiConversation < ActiveRecord::Base
     conversation = find_by(user: user, target: target)
     conversation_data = build_conversation_data(target, max_posts: max_posts)
 
+    conversation =
       if conversation
         conversation.update(**conversation_data)
         conversation
       else
         create(user_id: user.id, target: target, **conversation_data)
       end
+
+    ::Jobs.enqueue(:shared_conversation_adjust_upload_security, conversation_id: conversation.id)
+
+    conversation
+  end
+
+  def self.destroy_conversation(conversation)
+    conversation.destroy
+    ::Jobs.enqueue(
+      :shared_conversation_adjust_upload_security,
+      target_id: conversation.target_id,
+      target_type: conversation.target_type,
+    )
   end
 
   # Technically this may end up being a chat message

plugin.rb

@@ -64,4 +64,8 @@ after_initialize do
   end
 
   reloadable_patch { |plugin| Guardian.prepend DiscourseAi::GuardianExtensions }
+
+  register_modifier(:post_should_secure_uploads?) do |_, _, topic|
+    false if topic.private_message? && SharedAiConversation.exists?(target: topic)
+  end
 end

spec/jobs/shared_conversation_adjust_upload_security_spec.rb

@@ -0,0 +1,67 @@
+# frozen_string_literal: true
+
+RSpec.describe Jobs::SharedConversationAdjustUploadSecurity do
+  let(:params) { {} }
+
+  fab!(:bot_user) do
+    SiteSetting.discourse_ai_enabled = true
+    SiteSetting.ai_bot_enabled_chat_bots = "claude-2"
+    SiteSetting.ai_bot_enabled = true
+    SiteSetting.ai_bot_allowed_groups = "10"
+    SiteSetting.ai_bot_public_sharing_allowed_groups = "10"
+    User.find(DiscourseAi::AiBot::EntryPoint::CLAUDE_V2_ID)
+  end
+  fab!(:user)
+  fab!(:topic) { Fabricate(:private_message_topic, user: user, recipient: bot_user) }
+  fab!(:post_1) { Fabricate(:post, topic: topic, user: bot_user) }
+  fab!(:post_2) { Fabricate(:post, topic: topic, user: user) }
+  fab!(:conversation) { SharedAiConversation.share_conversation(user, topic) }
+
+  def run_job
+    described_class.new.execute(params)
+  end
+
+  context "when conversation is created" do
+    let(:params) { { conversation_id: conversation.id } }
+
+    it "does nothing for a conversation that has been deleted before the job ran" do
+      conversation.destroy
+      SharedAiConversation.any_instance.expects(:update).never
+      run_job
+    end
+
+    it "does nothing if there weren't any posts with secure uploads in the topic" do
+      original_context = conversation.context
+      run_job
+      expect(conversation.reload.context).to eq(original_context)
+    end
+
+    context "when topic posts were rebaked because they had secure uploads" do
+      it "updates the conversation cooked post content after rebaking" do
+        post_2.update!(raw: "some new rebaked content")
+        TopicUploadSecurityManager.any_instance.expects(:run).returns([post_2])
+        original_context = conversation.context
+        run_job
+        expect(conversation.reload.context).not_to eq(original_context)
+      end
+    end
+  end
+
+  context "when conversation has been deleted" do
+    let(:params) { { target_id: topic.id, target_type: "Topic" } }
+
+    before { conversation.destroy! }
+
+    it "runs the topic upload security manager but doesn't attempt to update a conversation" do
+      SharedAiConversation.any_instance.expects(:update).never
+      TopicUploadSecurityManager.any_instance.expects(:run).once
+      run_job
+    end
+
+    it "doesn't attempt to run the topic upload security manager if the topic has been deleted" do
+      TopicUploadSecurityManager.any_instance.expects(:run).never
+      topic.trash!
+      run_job
+    end
+  end
+end

spec/requests/ai_bot/shared_ai_conversations_spec.rb

@@ -83,6 +83,59 @@ RSpec.describe DiscourseAi::AiBot::SharedAiConversationsController do
         post "#{path}.json", params: { topic_id: user_pm_share.id }
         expect(response).to have_http_status(:success)
       end
+
+      context "when secure uploads are enabled" do
+        let(:upload_1) { Fabricate(:s3_image_upload, user: bot_user, secure: true) }
+        let(:upload_2) { Fabricate(:s3_image_upload, user: bot_user, secure: true) }
+        let(:post_with_upload_1) { Fabricate(:post, topic: user_pm_share, user: bot_user) }
+        let(:post_with_upload_2) { Fabricate(:post, topic: user_pm_share, user: bot_user) }
+
+        before do
+          enable_secure_uploads
+          stub_s3_store
+          SiteSetting.secure_uploads_pm_only = true
+          FileStore::S3Store.any_instance.stubs(:update_upload_ACL).returns(true)
+          Jobs.run_immediately!
+
+          upload_1.update!(
+            access_control_post: post_with_upload_1,
+            sha1: SecureRandom.hex(20),
+            original_sha1: upload_1.sha1,
+          )
+          upload_2.update!(
+            access_control_post: post_with_upload_2,
+            sha1: SecureRandom.hex(20),
+            original_sha1: upload_2.sha1,
+          )
+          post_with_upload_1.update!(
+            raw: "This is a post with a cool AI generated picture ![wow](#{upload_1.short_url})",
+          )
+          post_with_upload_2.update!(
+            raw:
+              "Another post that has been birthed by AI with a picture ![meow](#{upload_2.short_url})",
+          )
+        end
+
+        it "marks all of those uploads as not secure when sharing the topic" do
+          post "#{path}.json", params: { topic_id: user_pm_share.id }
+          expect(response).to have_http_status(:success)
+          expect(upload_1.reload.secure).to eq(false)
+          expect(upload_2.reload.secure).to eq(false)
+        end
+
+        it "rebakes any posts in the topic with uploads attached when sharing the topic so image urls become non-secure" do
+          post_1_cooked = post_with_upload_1.cooked
+          post_2_cooked = post_with_upload_2.cooked
+
+          post "#{path}.json", params: { topic_id: user_pm_share.id }
+          expect(response).to have_http_status(:success)
+
+          expect(post_with_upload_1.reload.cooked).not_to eq(post_1_cooked)
+          expect(post_with_upload_1.reload.cooked).not_to include("secure-uploads")
+          expect(post_with_upload_2.reload.cooked).not_to eq(post_2_cooked)
+          expect(post_with_upload_2.reload.cooked).not_to include("secure-uploads")
+        end
+      end
     end
 
     context "when not logged in" do
@@ -107,6 +160,57 @@ RSpec.describe DiscourseAi::AiBot::SharedAiConversationsController do
         delete "#{path}/123.json"
         expect(response).not_to have_http_status(:success)
       end
+
+      context "when secure uploads are enabled" do
+        let(:upload_1) { Fabricate(:s3_image_upload, user: bot_user, secure: false) }
+        let(:upload_2) { Fabricate(:s3_image_upload, user: bot_user, secure: false) }
+
+        before do
+          enable_secure_uploads
+          stub_s3_store
+          SiteSetting.secure_uploads_pm_only = true
+          FileStore::S3Store.any_instance.stubs(:update_upload_ACL).returns(true)
+          Jobs.run_immediately!
+
+          upload_1.update!(
+            access_control_post: shared_conversation.target.posts.first,
+            sha1: SecureRandom.hex(20),
+            original_sha1: upload_1.sha1,
+          )
+          upload_2.update!(
+            access_control_post: shared_conversation.target.posts.second,
+            sha1: SecureRandom.hex(20),
+            original_sha1: upload_2.sha1,
+          )
+          shared_conversation.target.posts.first.update!(
+            raw: "This is a post with a cool AI generated picture ![wow](#{upload_1.short_url})",
+          )
+          shared_conversation.target.posts.second.update!(
+            raw:
+              "Another post that has been birthed by AI with a picture ![meow](#{upload_2.short_url})",
+          )
+        end
+
+        it "marks all uploads in the PM back as secure when unsharing the conversation" do
+          delete "#{path}/#{shared_conversation.share_key}.json"
+          expect(response).to have_http_status(:success)
+          expect(upload_1.reload.secure).to eq(true)
+          expect(upload_2.reload.secure).to eq(true)
+        end
+
+        it "rebakes any posts in the topic with uploads attached when sharing the topic so image urls become secure" do
+          post_1_cooked = shared_conversation.target.posts.first.cooked
+          post_2_cooked = shared_conversation.target.posts.second.cooked
+
+          delete "#{path}/#{shared_conversation.share_key}.json"
+          expect(response).to have_http_status(:success)
+
+          expect(shared_conversation.target.posts.first.reload.cooked).not_to eq(post_1_cooked)
+          expect(shared_conversation.target.posts.first.reload.cooked).to include("secure-uploads")
+          expect(shared_conversation.target.posts.second.reload.cooked).not_to eq(post_2_cooked)
+          expect(shared_conversation.target.posts.second.reload.cooked).to include("secure-uploads")
+        end
+      end
     end
 
     context "when not logged in" do