CVE-2023-25819: Exposure of Sensitive Information to an Unauthorized Actor (CWE-200)
value: Discourse is an open source platform for community discussion. Tags that are normally private are showing in metadata. This affects any site running the `tests-passed` or `beta` branches >= 3.1.0.beta2. The issue is patched in the latest `beta` and `tests-passed` version of Discourse.
CWE description: The product exposes sensitive information to an actor that is not explicitly authorized to have access to that information.
CVSS v3 base score: 5.3
Published: 2023-03-04T01:15Z
Repository: https://github.com/discourse/discourse
Language: Ruby
Files changed: app/controllers/topics_controller.rb, app/models/tag.rb, app/views/topics/show.html.erb, lib/topic_view.rb, spec/lib/topic_view_spec.rb, spec/requests/topics_controller_spec.rb, spec/views/topics/show.html.erb_spec.rb
Commit date: 2023-02-23 17:22:20 +0100
Commit message: SECURITY: Show only visible tags in metadata

Currently, the topic metadata show both public and private
tags whereas only visible ones should be exposed.

Diff: diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb
index 0f8e7444c40f..d85a74dbb8b7 100644
--- a/app/controllers/topics_controller.rb
+++ b/app/controllers/topics_controller.rb
@@ -1229,11 +1229,11 @@ class TopicsController < ApplicationController
             !!params[:filter_top_level_replies],
       )
 
     respond_to do |format|
       format.html do
-        @tags = SiteSetting.tagging_enabled ? @topic_view.topic.tags : []
+        @tags = SiteSetting.tagging_enabled ? @topic_view.topic.tags.visible(guardian) : []
         @breadcrumbs = helpers.categories_breadcrumb(@topic_view.topic) || []
         @description_meta =
           @topic_view.topic.excerpt.present? ? @topic_view.topic.excerpt : @topic_view.summary
         store_preloaded("topic_#{@topic_view.topic.id}", MultiJson.dump(topic_view_serializer))
         render :show
diff --git a/app/models/tag.rb b/app/models/tag.rb
index 71435ef046fd..f3bdedc41a68 100644
--- a/app/models/tag.rb
+++ b/app/models/tag.rb
@@ -36,10 +36,11 @@ class Tag < ActiveRecord::Base
 
   scope :used_tags_in_regular_topics,
         ->(guardian) { where("tags.#{Tag.topic_count_column(guardian)} > 0") }
 
   scope :base_tags, -> { where(target_tag_id: nil) }
+  scope :visible, ->(guardian = nil) { merge(DiscourseTagging.visible_tags(guardian)) }
 
   has_many :tag_users, dependent: :destroy # notification settings
 
   has_many :topic_tags, dependent: :destroy
   has_many :topics, through: :topic_tags
diff --git a/app/views/topics/show.html.erb b/app/views/topics/show.html.erb
index f9bb80008f24..60670a3cd527 100644
--- a/app/views/topics/show.html.erb
+++ b/app/views/topics/show.html.erb
@@ -127,11 +127,11 @@
 
   <% end %>
 
   <% content_for :head do %>
     <%= auto_discovery_link_tag(@topic_view, {action: :feed, slug: @topic_view.topic.slug, topic_id: @topic_view.topic.id}, rel: 'alternate nofollow', title: t('rss_posts_in_topic', topic: @topic_view.title), type: 'application/rss+xml') %>
-    <%= raw crawlable_meta_data(title: @topic_view.title, description: @topic_view.summary(strip_images: true), image: @topic_view.image_url, read_time: @topic_view.read_time, like_count: @topic_view.like_count, ignore_canonical: true, published_time: @topic_view.published_time, breadcrumbs: @breadcrumbs, tags: @topic_view.tags) %>
+    <%= raw crawlable_meta_data(title: @topic_view.title, description: @topic_view.summary(strip_images: true), image: @topic_view.image_url, read_time: @topic_view.read_time, like_count: @topic_view.like_count, ignore_canonical: true, published_time: @topic_view.published_time, breadcrumbs: @breadcrumbs, tags: @tags.map(&:name)) %>
 
     <% if @topic_view.prev_page || @topic_view.next_page %>
       <% if @topic_view.prev_page %>
         <link rel="prev" href="<%= @topic_view.prev_page_path -%>">
       <% end %>
diff --git a/lib/topic_view.rb b/lib/topic_view.rb
index b73e93a25888..854306235d30 100644
--- a/lib/topic_view.rb
+++ b/lib/topic_view.rb
@@ -242,13 +242,13 @@ class TopicView
     end
     if SiteSetting.topic_page_title_includes_category
       if @topic.category_id != SiteSetting.uncategorized_category_id && @topic.category_id &&
            @topic.category
         title += " - #{@topic.category.name}"
-      elsif SiteSetting.tagging_enabled && @topic.tags.exists?
+      elsif SiteSetting.tagging_enabled && visible_tags.exists?
         title +=
-          " - #{@topic.tags.order("tags.#{Tag.topic_count_column(@guardian)} DESC").first.name}"
+          " - #{visible_tags.order("tags.#{Tag.topic_count_column(@guardian)} DESC").first.name}"
       end
     end
     title
   end
 
@@ -711,14 +711,10 @@ class TopicView
           hash
         end
       end
   end
 
-  def tags
-    @topic.tags.map(&:name)
-  end
-
   protected
 
   def read_posts_set
     @read_posts_set ||=
       begin
@@ -818,11 +814,11 @@ class TopicView
   end
 
   def find_topic(topic_or_topic_id)
     return topic_or_topic_id if topic_or_topic_id.is_a?(Topic)
     # with_deleted covered in #check_and_raise_exceptions
-    Topic.with_deleted.includes(:category, :tags).find_by(id: topic_or_topic_id)
+    Topic.with_deleted.includes(:category).find_by(id: topic_or_topic_id)
   end
 
   def unfiltered_posts
     result = filter_post_types(@topic.posts)
     result = result.with_deleted if @guardian.can_see_deleted_posts?(@topic.category)
@@ -988,6 +984,10 @@ class TopicView
                .exists?
         StaffActionLogger.new(@user).log_check_personal_message(@topic)
       end
     end
   end
+
+  def visible_tags
+    @visible_tags ||= topic.tags.visible(guardian)
+  end
 end
diff --git a/spec/lib/topic_view_spec.rb b/spec/lib/topic_view_spec.rb
index 503284648897..79b6bc9563f4 100644
--- a/spec/lib/topic_view_spec.rb
+++ b/spec/lib/topic_view_spec.rb
@@ -823,10 +823,29 @@ RSpec.describe TopicView do
 
             it { is_expected.to start_with(topic.title) }
             it { is_expected.not_to include(tag1.name) }
             it { is_expected.not_to include(tag2.name) }
           end
+
+          context "with restricted tags" do
+            let(:tag_group) { Fabricate.build(:tag_group) }
+            let(:tag_group_permission) do
+              Fabricate.build(:tag_group_permission, tag_group: tag_group)
+            end
+
+            before do
+              SiteSetting.tagging_enabled = true
+              # avoid triggering a `before_create` callback in `TagGroup` which
+              # messes with permissions
+              tag_group.tag_group_permissions << tag_group_permission
+              tag_group.save!
+              tag_group_permission.tag_group.tags << tag2
+            end
+
+            it { is_expected.not_to include(tag2.name) }
+            it { is_expected.to include(tag1.name) }
+          end
         end
       end
     end
 
     context "with categorized topic" do
@@ -1070,19 +1089,6 @@ RSpec.describe TopicView do
       context "when category is not moderated" do
         it { expect(topic_view.queued_posts_enabled?).to be(nil) }
       end
     end
   end
-
-  describe "#tags" do
-    subject(:topic_view_tags) { topic_view.tags }
-
-    let(:topic_view) { described_class.new(topic, user) }
-    let(:topic) { Fabricate.build(:topic, tags: tags) }
-    let(:tags) { Fabricate.build_times(2, :tag) }
-    let(:user) { Fabricate(:user) }
-
-    it "returns the tags names" do
-      expect(topic_view_tags).to match tags.map(&:name)
-    end
-  end
 end
diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb
index 5f9c62c03b9c..6ddfff1af7be 100644
--- a/spec/requests/topics_controller_spec.rb
+++ b/spec/requests/topics_controller_spec.rb
@@ -2505,10 +2505,32 @@ RSpec.describe TopicsController do
         body = response.body
 
         expect(body).to have_tag(:script, src: "/assets/discourse.js")
         expect(body).to have_tag(:meta, with: { name: "fragment" })
       end
+
+      context "with restricted tags" do
+        let(:tag_group) { Fabricate.build(:tag_group) }
+        let(:tag_group_permission) { Fabricate.build(:tag_group_permission, tag_group: tag_group) }
+        let(:restricted_tag) { Fabricate(:tag) }
+        let(:public_tag) { Fabricate(:tag) }
+
+        before do
+          # avoid triggering a `before_create` callback in `TagGroup` which
+          # messes with permissions
+          tag_group.tag_group_permissions << tag_group_permission
+          tag_group.save!
+          tag_group_permission.tag_group.tags << restricted_tag
+          topic.tags << [public_tag, restricted_tag]
+        end
+
+        it "doesn’t expose restricted tags" do
+          get "/t/#{topic.slug}/#{topic.id}/print", headers: { HTTP_USER_AGENT: "Rails Testing" }
+          expect(response.body).to match(public_tag.name)
+          expect(response.body).not_to match(restricted_tag.name)
+        end
+      end
     end
 
     it "records redirects" do
       get "/t/#{topic.id}", headers: { HTTP_REFERER: "http://twitter.com" }
       get "/t/#{topic.slug}/#{topic.id}", headers: { HTTP_REFERER: nil }
diff --git a/spec/views/topics/show.html.erb_spec.rb b/spec/views/topics/show.html.erb_spec.rb
index dda660961d4c..e2293544417b 100644
--- a/spec/views/topics/show.html.erb_spec.rb
+++ b/spec/views/topics/show.html.erb_spec.rb
@@ -10,10 +10,12 @@ RSpec.describe "topics/show.html.erb" do
     topic_view = OpenStruct.new(topic: topic, posts: [])
     topic_view.stubs(:summary).returns("")
     view.stubs(:crawler_layout?).returns(false)
     view.stubs(:url_for).returns("https://www.example.com/test.rss")
     view.instance_variable_set("@topic_view", topic_view)
+    assign(:tags, [])
+
     render template: "topics/show", formats: [:html]
 
     expect(view.content_for(:head)).to match(
       %r{<link rel="alternate nofollow" type="application/rss\+xml" title="[^"]+" href="https://www.example.com/test\.rss" />},
     )

Vulnerable code:         @tags = SiteSetting.tagging_enabled ? @topic_view.topic.tags : []
    <%= raw crawlable_meta_data(title: @topic_view.title, description: @topic_view.summary(strip_images: true), image: @topic_view.image_url, read_time: @topic_view.read_time, like_count: @topic_view.like_count, ignore_canonical: true, published_time: @topic_view.published_time, breadcrumbs: @breadcrumbs, tags: @topic_view.tags) %>
      elsif SiteSetting.tagging_enabled && @topic.tags.exists?
          " - #{@topic.tags.order("tags.#{Tag.topic_count_column(@guardian)} DESC").first.name}"
  def tags
    @topic.tags.map(&:name)
  end

    Topic.with_deleted.includes(:category, :tags).find_by(id: topic_or_topic_id)

  describe "#tags" do
    subject(:topic_view_tags) { topic_view.tags }

    let(:topic_view) { described_class.new(topic, user) }
    let(:topic) { Fabricate.build(:topic, tags: tags) }
    let(:tags) { Fabricate.build_times(2, :tag) }
    let(:user) { Fabricate(:user) }

    it "returns the tags names" do
      expect(topic_view_tags).to match tags.map(&:name)
    end
  end
Fixed code:         @tags = SiteSetting.tagging_enabled ? @topic_view.topic.tags.visible(guardian) : []
  scope :visible, ->(guardian = nil) { merge(DiscourseTagging.visible_tags(guardian)) }
    <%= raw crawlable_meta_data(title: @topic_view.title, description: @topic_view.summary(strip_images: true), image: @topic_view.image_url, read_time: @topic_view.read_time, like_count: @topic_view.like_count, ignore_canonical: true, published_time: @topic_view.published_time, breadcrumbs: @breadcrumbs, tags: @tags.map(&:name)) %>
      elsif SiteSetting.tagging_enabled && visible_tags.exists?
          " - #{visible_tags.order("tags.#{Tag.topic_count_column(@guardian)} DESC").first.name}"
    Topic.with_deleted.includes(:category).find_by(id: topic_or_topic_id)

  def visible_tags
    @visible_tags ||= topic.tags.visible(guardian)
  end

          context "with restricted tags" do
            let(:tag_group) { Fabricate.build(:tag_group) }
            let(:tag_group_permission) do
              Fabricate.build(:tag_group_permission, tag_group: tag_group)
            end

            before do
              SiteSetting.tagging_enabled = true
              # avoid triggering a `before_create` callback in `TagGroup` which
              # messes with permissions
              tag_group.tag_group_permissions << tag_group_permission
              tag_group.save!
              tag_group_permission.tag_group.tags << tag2
            end

            it { is_expected.not_to include(tag2.name) }
            it { is_expected.to include(tag1.name) }
          end

      context "with restricted tags" do
        let(:tag_group) { Fabricate.build(:tag_group) }
        let(:tag_group_permission) { Fabricate.build(:tag_group_permission, tag_group: tag_group) }
        let(:restricted_tag) { Fabricate(:tag) }
        let(:public_tag) { Fabricate(:tag) }

        before do
          # avoid triggering a `before_create` callback in `TagGroup` which
          # messes with permissions
          tag_group.tag_group_permissions << tag_group_permission
          tag_group.save!
          tag_group_permission.tag_group.tags << restricted_tag
          topic.tags << [public_tag, restricted_tag]
        end

        it "doesn’t expose restricted tags" do
          get "/t/#{topic.slug}/#{topic.id}/print", headers: { HTTP_USER_AGENT: "Rails Testing" }
          expect(response.body).to match(public_tag.name)
          expect(response.body).not_to match(restricted_tag.name)
        end
      end
    assign(:tags, [])