From bab5e52e3881134d7c9c1e490eff290e5b81341d Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 11 Apr 2024 10:00:41 +1000 Subject: [PATCH] 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. --- .../shared_ai_conversations_controller.rb | 4 +- ...red_conversation_adjust_upload_security.rb | 51 +++++++++ app/models/shared_ai_conversation.rb | 26 ++++- plugin.rb | 4 + ...onversation_adjust_upload_security_spec.rb | 67 +++++++++++ .../ai_bot/shared_ai_conversations_spec.rb | 104 ++++++++++++++++++ 6 files changed, 249 insertions(+), 7 deletions(-) create mode 100644 app/jobs/regular/shared_conversation_adjust_upload_security.rb create mode 100644 spec/jobs/shared_conversation_adjust_upload_security_spec.rb diff --git a/app/controllers/discourse_ai/ai_bot/shared_ai_conversations_controller.rb b/app/controllers/discourse_ai/ai_bot/shared_ai_conversations_controller.rb index c197f9fc..aaed408e 100644 --- a/app/controllers/discourse_ai/ai_bot/shared_ai_conversations_controller.rb +++ b/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 diff --git a/app/jobs/regular/shared_conversation_adjust_upload_security.rb b/app/jobs/regular/shared_conversation_adjust_upload_security.rb new file mode 100644 index 00000000..350ca0e7 --- /dev/null +++ b/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 diff --git a/app/models/shared_ai_conversation.rb b/app/models/shared_ai_conversation.rb index 970deaa9..a3ba6781 100644 --- a/app/models/shared_ai_conversation.rb +++ b/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) - if conversation - conversation.update(**conversation_data) - conversation - else - create(user_id: user.id, target: target, **conversation_data) - end + 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 diff --git a/plugin.rb b/plugin.rb index 846e6b93..b90801d7 100644 --- a/plugin.rb +++ b/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 diff --git a/spec/jobs/shared_conversation_adjust_upload_security_spec.rb b/spec/jobs/shared_conversation_adjust_upload_security_spec.rb new file mode 100644 index 00000000..3a240495 --- /dev/null +++ b/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 diff --git a/spec/requests/ai_bot/shared_ai_conversations_spec.rb b/spec/requests/ai_bot/shared_ai_conversations_spec.rb index a46c317b..c45d153a 100644 --- a/spec/requests/ai_bot/shared_ai_conversations_spec.rb +++ b/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