SECURITY: Hide PM count for tags by default (#20061)

Currently `Topic#pm_topic_count` is a count of all personal messages tagged for a given tag. As a result, any user with access to PM tags can poll a sensitive tag to determine if a new personal message has been created using that tag even if the user does not have access to the personal message. We classify this as a minor leak in sensitive information.

With this commit, `Topic#pm_topic_count` is hidden from users by default unless the `display_personal_messages_tag_counts` site setting is enabled.

app/assets/javascripts/discourse/app/models/tag.js

@@ -1,15 +1,13 @@
 import RestModel from "discourse/models/rest";
 import discourseComputed from "discourse-common/utils/decorators";
+import { readOnly } from "@ember/object/computed";
 
 export default RestModel.extend({
-  @discourseComputed("count", "pm_count")
+  pmOnly: readOnly("pm_only"),
-  totalCount(count, pmCount) {
-    return count + pmCount;
-  },
 
   @discourseComputed("count", "pm_count")
-  pmOnly(count, pmCount) {
+  totalCount(count, pmCount) {
-    return count === 0 && pmCount > 0;
+    return pmCount ? count + pmCount : count;
   },
 
   @discourseComputed("id")

app/assets/javascripts/discourse/tests/helpers/create-pretender.js

@@ -100,8 +100,8 @@ export function applyDefaultHandlers(pretender) {
     return response({
       tags: [
         { id: "eviltrout", count: 1 },
-        { id: "planned", text: "planned", count: 7, pm_count: 0 },
+        { id: "planned", text: "planned", count: 7, pm_only: false },
-        { id: "private", text: "private", count: 0, pm_count: 7 },
+        { id: "private", text: "private", count: 0, pm_only: true },
       ],
       extras: {
         tag_groups: [
@@ -109,24 +109,24 @@ export function applyDefaultHandlers(pretender) {
             id: 2,
             name: "Ford Cars",
             tags: [
-              { id: "Escort", text: "Escort", count: 1, pm_count: 0 },
+              { id: "Escort", text: "Escort", count: 1, pm_only: false },
-              { id: "focus", text: "focus", count: 3, pm_count: 0 },
+              { id: "focus", text: "focus", count: 3, pm_only: false },
             ],
           },
           {
             id: 1,
             name: "Honda Cars",
             tags: [
-              { id: "civic", text: "civic", count: 4, pm_count: 0 },
+              { id: "civic", text: "civic", count: 4, pm_only: false },
-              { id: "accord", text: "accord", count: 2, pm_count: 0 },
+              { id: "accord", text: "accord", count: 2, pm_only: false },
             ],
           },
           {
             id: 1,
             name: "Makes",
             tags: [
-              { id: "ford", text: "ford", count: 5, pm_count: 0 },
+              { id: "ford", text: "ford", count: 5, pm_only: false },
-              { id: "honda", text: "honda", count: 6, pm_count: 0 },
+              { id: "honda", text: "honda", count: 6, pm_only: false },
             ],
           },
         ],

app/assets/javascripts/discourse/tests/integration/components/select-kit/tag-drop-test.js

@@ -28,7 +28,7 @@ module("Integration | Component | select-kit/tag-drop", function (hooks) {
     pretender.get("/tags/filter/search", (params) => {
       if (params.queryParams.q === "dav") {
         return response({
-          results: [{ id: "David", name: "David", count: 2, pm_count: 0 }],
+          results: [{ id: "David", name: "David", count: 2, pm_only: false }],
         });
       }
     });

app/assets/javascripts/discourse/tests/unit/models/tag-test.js

@@ -0,0 +1,31 @@
+import { module, test } from "qunit";
+import { getOwner } from "discourse-common/lib/get-owner";
+import { setupTest } from "ember-qunit";
+
+module("Unit | Model | tag", function (hooks) {
+  setupTest(hooks);
+
+  hooks.beforeEach(function () {
+    this.store = getOwner(this).lookup("service:store");
+  });
+
+  test("totalCount when pm_count is not present", function (assert) {
+    const tag = this.store.createRecord("tag", { count: 5 });
+    assert.strictEqual(tag.totalCount, 5);
+  });
+
+  test("totalCount when pm_count is present", function (assert) {
+    const tag = this.store.createRecord("tag", { count: 5, pm_count: 8 });
+    assert.strictEqual(tag.totalCount, 13);
+  });
+
+  test("pmOnly", function (assert) {
+    const tag = this.store.createRecord("tag", { pm_only: false });
+
+    assert.notOk(tag.pmOnly);
+
+    tag.set("pm_only", true);
+
+    assert.ok(tag.pmOnly);
+  });
+});

app/controllers/tags_controller.rb

@@ -401,16 +401,22 @@ class TagsController < ::ApplicationController
 
         next if topic_count == 0 && t.pm_topic_count > 0 && !show_pm_tags
 
-        {
+        attrs = {
           id: t.name,
           text: t.name,
           name: t.name,
           description: t.description,
           count: topic_count,
-          pm_count: show_pm_tags ? t.pm_topic_count : 0,
+          pm_only: topic_count == 0 && t.pm_topic_count > 0,
           target_tag:
             t.target_tag_id ? target_tags.find { |x| x.id == t.target_tag_id }&.name : nil,
         }
+
+        if show_pm_tags && SiteSetting.display_personal_messages_tag_counts
+          attrs[:pm_count] = t.pm_topic_count
+        end
+
+        attrs
       end
       .compact
   end

config/locales/server.en.yml

@@ -1652,6 +1652,7 @@ en:
     content_security_policy_script_src: "Additional allowlisted script sources. The current host and CDN are included by default. See <a href='https://meta.discourse.org/t/mitigate-xss-attacks-with-content-security-policy/104243' target='_blank'>Mitigate XSS Attacks with Content Security Policy.</a>"
     invalidate_inactive_admin_email_after_days: "Admin accounts that have not visited the site in this number of days will need to re-validate their email address before logging in. Set to 0 to disable."
     include_secure_categories_in_tag_counts: "When enabled, count of topics for a tag will include topics that are in read restricted categories for all users. When disabled, normal users are only shown a count of topics for a tag where all the topics are in public categories."
+    display_personal_messages_tag_counts: "When enabled, count of personal messages tagged with a given tag will be displayed."
     top_menu: "Determine which items appear in the homepage navigation, and in what order. Example latest|new|unread|categories|top|read|posted|bookmarks"
     post_menu: "Determine which items appear on the post menu, and in what order. Example like|edit|flag|delete|share|bookmark|reply"
     post_menu_hidden_items: "The menu items to hide by default in the post menu unless an expansion ellipsis is clicked on."

config/site_settings.yml

@@ -1806,6 +1806,8 @@ security:
     hidden: true
   include_secure_categories_in_tag_counts:
     default: false
+  display_personal_messages_tag_counts:
+    default: false
 
 onebox:
   post_onebox_maxlength:

spec/requests/tags_controller_spec.rb

@@ -12,9 +12,21 @@ RSpec.describe TagsController do
 
   describe "#index" do
     fab!(:test_tag) { Fabricate(:tag, name: "test", description: "some description") }
+
     fab!(:topic_tag) do
-      Fabricate(:tag, name: "topic-test", public_topic_count: 1, staff_topic_count: 1)
+      Fabricate(
+        :tag,
+        name: "topic-test",
+        public_topic_count: 1,
+        staff_topic_count: 1,
+        pm_topic_count: 5,
+      )
     end
+
+    fab!(:pm_only_tag) do
+      Fabricate(:tag, public_topic_count: 0, staff_topic_count: 0, pm_topic_count: 1)
+    end
+
     fab!(:synonym) { Fabricate(:tag, name: "synonym", target_tag: topic_tag) }
 
     shared_examples "retrieves the right tags" do
@@ -27,13 +39,63 @@ RSpec.describe TagsController do
 
         tags = response.parsed_body["tags"]
 
+        serialized_tag = tags.find { |t| t["id"] == test_tag.name }
+
+        expect(serialized_tag["count"]).to eq(0)
+        expect(serialized_tag["pm_count"]).to eq(nil)
+        expect(serialized_tag["pm_only"]).to eq(false)
+
+        serialized_tag = tags.find { |t| t["id"] == topic_tag.name }
+
+        expect(serialized_tag["count"]).to eq(1)
+        expect(serialized_tag["pm_count"]).to eq(nil)
+        expect(serialized_tag["pm_only"]).to eq(false)
+      end
+
+      it "does not include pm_count attribute when user cannot tag PM topics even if display_personal_messages_tag_counts site setting has been enabled" do
+        SiteSetting.display_personal_messages_tag_counts = true
+
+        sign_in(admin)
+
+        get "/tags.json"
+
+        expect(response.status).to eq(200)
+
+        tags = response.parsed_body["tags"]
+
         expect(tags[0]["name"]).to eq(test_tag.name)
-        expect(tags[0]["count"]).to eq(0)
+        expect(tags[0]["pm_count"]).to eq(nil)
-        expect(tags[0]["pm_count"]).to eq(0)
 
         expect(tags[1]["name"]).to eq(topic_tag.name)
-        expect(tags[1]["count"]).to eq(1)
+        expect(tags[1]["pm_count"]).to eq(nil)
-        expect(tags[1]["pm_count"]).to eq(0)
+      end
+
+      it "includes pm_count attribute when user can tag PM topics and display_personal_messages_tag_counts site setting has been enabled" do
+        SiteSetting.display_personal_messages_tag_counts = true
+        SiteSetting.pm_tags_allowed_for_groups = Group::AUTO_GROUPS[:admins]
+
+        sign_in(admin)
+
+        get "/tags.json"
+
+        expect(response.status).to eq(200)
+
+        tags = response.parsed_body["tags"]
+
+        serialized_tag = tags.find { |t| t["id"] == test_tag.name }
+
+        expect(serialized_tag["pm_count"]).to eq(0)
+        expect(serialized_tag["pm_only"]).to eq(false)
+
+        serialized_tag = tags.find { |t| t["id"] == topic_tag.name }
+
+        expect(serialized_tag["pm_count"]).to eq(5)
+        expect(serialized_tag["pm_only"]).to eq(false)
+
+        serialized_tag = tags.find { |t| t["id"] == pm_only_tag.name }
+
+        expect(serialized_tag["pm_count"]).to eq(1)
+        expect(serialized_tag["pm_only"]).to eq(true)
       end
 
       it "only retrieve tags that have been used in public topics for non-staff user" do
@@ -48,7 +110,7 @@ RSpec.describe TagsController do
 
         expect(tags[0]["name"]).to eq(topic_tag.name)
         expect(tags[0]["count"]).to eq(1)
-        expect(tags[0]["pm_count"]).to eq(0)
+        expect(tags[0]["pm_count"]).to eq(nil)
       end
     end
 
@@ -66,17 +128,17 @@ RSpec.describe TagsController do
       context "when enabled" do
         before do
           SiteSetting.pm_tags_allowed_for_groups = "1|2|3"
+          SiteSetting.display_personal_messages_tag_counts = true
           sign_in(admin)
         end
 
         it "shows topic tags and pm tags" do
           get "/tags.json"
           tags = response.parsed_body["tags"]
-          expect(tags.length).to eq(2)
 
           serialized_tag = tags.find { |t| t["id"] == topic_tag.name }
           expect(serialized_tag["count"]).to eq(2)
-          expect(serialized_tag["pm_count"]).to eq(0)
+          expect(serialized_tag["pm_count"]).to eq(5)
 
           serialized_tag = tags.find { |t| t["id"] == test_tag.name }
           expect(serialized_tag["count"]).to eq(0)
@@ -180,7 +242,7 @@ RSpec.describe TagsController do
         expect(tags[0]["text"]).to eq(test_tag.name)
         expect(tags[0]["description"]).to eq(test_tag.description)
         expect(tags[0]["count"]).to eq(0)
-        expect(tags[0]["pm_count"]).to eq(0)
+        expect(tags[0]["pm_count"]).to eq(nil)
         expect(tags[0]["target_tag"]).to eq(nil)
 
         expect(tags[1]["name"]).to eq(topic_tag.name)
@@ -193,7 +255,7 @@ RSpec.describe TagsController do
         expect(categories[0]["tags"][0]["text"]).to eq(test_tag.name)
         expect(categories[0]["tags"][0]["description"]).to eq(test_tag.description)
         expect(categories[0]["tags"][0]["count"]).to eq(0)
-        expect(categories[0]["tags"][0]["pm_count"]).to eq(0)
+        expect(categories[0]["tags"][0]["pm_count"]).to eq(nil)
         expect(categories[0]["tags"][0]["target_tag"]).to eq(nil)
       end