SECURITY: Show only visible tags in metadata

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

app/controllers/topics_controller.rb

@@ -1231,7 +1231,7 @@ class TopicsController < ApplicationController
 
     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

app/models/tag.rb

@@ -38,6 +38,7 @@ class Tag < ActiveRecord::Base
         ->(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
 

app/views/topics/show.html.erb

@@ -129,7 +129,7 @@
 
   <% 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 %>

lib/topic_view.rb

@@ -244,9 +244,9 @@ class TopicView
       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
@@ -713,10 +713,6 @@ class TopicView
       end
   end
 
-  def tags
-    @topic.tags.map(&:name)
-  end
-
   protected
 
   def read_posts_set
@@ -820,7 +816,7 @@ class TopicView
   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
@@ -990,4 +986,8 @@ class TopicView
       end
     end
   end
+
+  def visible_tags
+    @visible_tags ||= topic.tags.visible(guardian)
+  end
 end

spec/lib/topic_view_spec.rb

@@ -825,6 +825,25 @@ RSpec.describe TopicView do
             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
@@ -1072,17 +1091,4 @@ RSpec.describe TopicView do
       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

spec/requests/topics_controller_spec.rb

@@ -2507,6 +2507,28 @@ RSpec.describe TopicsController do
         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

spec/views/topics/show.html.erb_spec.rb

@@ -12,6 +12,8 @@ RSpec.describe "topics/show.html.erb" do
     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(