SECURITY: Fix XSS on Shared AI Conversations local Onebox (#1069)

discourse · Jan 14, 2025 · 92f122c · 92f122c
1 parent cd03874
commit 92f122c
Show file tree

Hide file tree

Showing 3 changed files with 28 additions and 10 deletions.
diff --git a/app/models/shared_ai_conversation.rb b/app/models/shared_ai_conversation.rb
@@ -88,14 +88,7 @@ def url
   def html_excerpt
     html = +""
     populated_context.each do |post|
-      text =
-        PrettyText.excerpt(
-          post.cooked,
-          400,
-          text_entities: true,
-          strip_links: true,
-          strip_details: true,
-        )
+      text = PrettyText.excerpt(post.cooked, 400, strip_links: true, strip_details: true)
 
       html << "<p><b>#{post.user.username}</b>: #{text}</p>"
       if html.length > 1000

diff --git a/db/post_migrate/20241206115958_rebake_shared_ai_conversation_oneboxes.rb b/db/post_migrate/20241206115958_rebake_shared_ai_conversation_oneboxes.rb
@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+class RebakeSharedAiConversationOneboxes < ActiveRecord::Migration[7.2]
+  def up
+    # Safe marking for rebake using raw SQL
+    DB.exec(<<~SQL)
+      UPDATE posts
+      SET baked_version = NULL
+      WHERE raw LIKE '%/discourse-ai/ai-bot/shared-ai-conversations/%';
+    SQL
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end
diff --git a/spec/models/shared_ai_conversation_spec.rb b/spec/models/shared_ai_conversation_spec.rb
@@ -13,17 +13,20 @@
 
   fab!(:user)
 
+  let(:bad_user_input) { <<~HTML }
+    Just trying something `<marquee style="font-size: 200px; color: red;" scrollamount=20>h4cked</marquee>`
+  HTML
   let(:raw_with_details) { <<~HTML }
     <details>
     <summary>GitHub pull request diff</summary>
     <p><a href="https://github.com/discourse/discourse-ai/pull/521">discourse/discourse-ai 521</a></p>
     </details>
     <p>This is some other text</p>
-    HTML
+  HTML
 
   let(:bot_user) { claude_2.reload.user }
   let!(:topic) { Fabricate(:private_message_topic, recipient: bot_user) }
-  let!(:post1) { Fabricate(:post, topic: topic, post_number: 1) }
+  let!(:post1) { Fabricate(:post, topic: topic, post_number: 1, raw: bad_user_input) }
   let!(:post2) { Fabricate(:post, topic: topic, post_number: 2, raw: raw_with_details) }
 
   describe ".share_conversation" do
@@ -70,5 +73,12 @@
       expect(populated_context[1].id).to eq(post2.id)
       expect(populated_context[1].user.id).to eq(post2.user.id)
     end
+
+    it "escapes HTML" do
+      conversation = described_class.share_conversation(user, topic)
+      onebox = conversation.onebox
+      expect(onebox).not_to include("</marquee>")
+      expect(onebox).to include("AI Conversation with Claude-2")
+    end
   end
 end