From 631f4d6f96472152da835a074f44188c6e2433ad Mon Sep 17 00:00:00 2001 From: Mark VanLandingham Date: Mon, 23 Mar 2026 10:38:38 -0500 Subject: [PATCH 1/3] FIX: Don't leak the existance of whispers --- app/models/nested_view_post_stat.rb | 14 ++- .../discourse/components/nested-post.gjs | 1 + assets/stylesheets/common/nested-view.scss | 7 ++ ...hisper_counts_to_nested_view_post_stats.rb | 16 +++ lib/discourse_nested_replies/tree_loader.rb | 26 ++++- plugin.rb | 43 +++++++- spec/lib/nested_replies_n_plus_one_spec.rb | 88 +++++++++++++++ .../requests/nested_topics_controller_spec.rb | 104 ++++++++++++++++++ 8 files changed, 285 insertions(+), 14 deletions(-) create mode 100644 db/migrate/20260323000000_add_whisper_counts_to_nested_view_post_stats.rb diff --git a/app/models/nested_view_post_stat.rb b/app/models/nested_view_post_stat.rb index 063371b..de69a09 100644 --- a/app/models/nested_view_post_stat.rb +++ b/app/models/nested_view_post_stat.rb @@ -8,12 +8,14 @@ class NestedViewPostStat < ActiveRecord::Base # # Table name: nested_view_post_stats # -# id :bigint not null, primary key -# direct_reply_count :integer default(0), not null -# total_descendant_count :integer default(0), not null -# created_at :datetime not null -# updated_at :datetime not null -# post_id :bigint not null +# id :bigint not null, primary key +# direct_reply_count :integer default(0), not null +# total_descendant_count :integer default(0), not null +# whisper_direct_reply_count :integer default(0), not null +# whisper_total_descendant_count :integer default(0), not null +# created_at :datetime not null +# updated_at :datetime not null +# post_id :bigint not null # # Indexes # diff --git a/assets/javascripts/discourse/components/nested-post.gjs b/assets/javascripts/discourse/components/nested-post.gjs index 75a526a..8e762d5 100644 --- a/assets/javascripts/discourse/components/nested-post.gjs +++ b/assets/javascripts/discourse/components/nested-post.gjs @@ -245,6 +245,7 @@ export default class NestedPost extends Component { (if @parentLineHighlighted "--parent-line-highlighted") (if this.collapsed "nested-post--collapsed") (if @isPinned "nested-post--pinned") + (if @post.isWhisper "nested-post--whisper") (if @post.deleted "nested-post--deleted") }} > diff --git a/assets/stylesheets/common/nested-view.scss b/assets/stylesheets/common/nested-view.scss index 7c61197..637bc73 100644 --- a/assets/stylesheets/common/nested-view.scss +++ b/assets/stylesheets/common/nested-view.scss @@ -608,6 +608,13 @@ font-style: italic; } + &--whisper { + .nested-post__content .cooked { + font-style: italic; + color: var(--primary-medium); + } + } + &__deleted-avatar-placeholder { width: var(--nested-avatar-size); height: var(--nested-avatar-size); diff --git a/db/migrate/20260323000000_add_whisper_counts_to_nested_view_post_stats.rb b/db/migrate/20260323000000_add_whisper_counts_to_nested_view_post_stats.rb new file mode 100644 index 0000000..3fb40c0 --- /dev/null +++ b/db/migrate/20260323000000_add_whisper_counts_to_nested_view_post_stats.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +class AddWhisperCountsToNestedViewPostStats < ActiveRecord::Migration[7.2] + def change + add_column :nested_view_post_stats, + :whisper_direct_reply_count, + :integer, + default: 0, + null: false + add_column :nested_view_post_stats, + :whisper_total_descendant_count, + :integer, + default: 0, + null: false + end +end diff --git a/lib/discourse_nested_replies/tree_loader.rb b/lib/discourse_nested_replies/tree_loader.rb index 3c0c355..62ac42e 100644 --- a/lib/discourse_nested_replies/tree_loader.rb +++ b/lib/discourse_nested_replies/tree_loader.rb @@ -23,6 +23,12 @@ def initialize(topic:, guardian:) @guardian = guardian end + def visible_post_types + types = [Post.types[:regular], Post.types[:moderator_action]] + types << Post.types[:whisper] if guardian.is_staff? + types + end + def op_post @op_post ||= load_posts_for_tree(topic.posts.where(post_number: 1)).first end @@ -46,7 +52,7 @@ def load_posts_for_tree(scope) def apply_visibility(scope) scope = scope.unscope(where: :deleted_at) - scope = scope.where(post_type: [Post.types[:regular], Post.types[:moderator_action]]) + scope = scope.where(post_type: visible_post_types) scope end @@ -104,7 +110,7 @@ def batch_load_siblings(ancestors, sort) topic_id: topic.id, parent_numbers: parent_numbers, limit: SIBLINGS_PER_ANCESTOR, - post_types: [Post.types[:regular], Post.types[:moderator_action]], + post_types: visible_post_types, } sibling_ids = DB.query_single(<<~SQL, **sql_params) @@ -151,7 +157,7 @@ def batch_load_siblings(ancestors, sort) # grandchildren, etc.) and returns them as a flat scope. Used when # cap_nesting_depth is ON to flatten deep legacy threads at the last level. def flat_descendants_scope(parent_post_number, sort:, offset: 0, limit: CHILDREN_PER_PAGE) - post_types = [Post.types[:regular], Post.types[:moderator_action]] + post_types = visible_post_types order_expr = DiscourseNestedReplies::Sort.sql_order_expression(sort) descendant_post_numbers = @@ -201,16 +207,28 @@ def direct_reply_counts(post_numbers) .with_deleted .where(topic_id: topic.id) .where(reply_to_post_number: post_numbers) + .where(post_type: visible_post_types) .group(:reply_to_post_number) .count end # Batch-load total descendant counts from the stats table. # Returns { post_id => count } keyed by post ID (not post_number). + # For non-staff, subtracts whisper counts to avoid leaking whisper existence. def total_descendant_counts(post_ids) return {} if post_ids.empty? - NestedViewPostStat.where(post_id: post_ids.uniq).pluck(:post_id, :total_descendant_count).to_h + if guardian.is_staff? + NestedViewPostStat + .where(post_id: post_ids.uniq) + .pluck(:post_id, :total_descendant_count) + .to_h + else + NestedViewPostStat + .where(post_id: post_ids.uniq) + .pluck(:post_id, Arel.sql("total_descendant_count - whisper_total_descendant_count")) + .to_h + end end end end diff --git a/plugin.rb b/plugin.rb index 0c471d4..dcd590e 100644 --- a/plugin.rb +++ b/plugin.rb @@ -51,10 +51,14 @@ module ::DiscourseNestedReplies post_numbers = topic_view.posts.map(&:post_number) next if post_numbers.empty? + visible_types = [Post.types[:regular], Post.types[:moderator_action]] + visible_types << Post.types[:whisper] if topic_view.guardian.is_staff? + counts = Post .where(topic_id: topic_view.topic.id, deleted_at: nil) .where(reply_to_post_number: post_numbers) + .where(post_type: visible_types) .group(:reply_to_post_number) .count @@ -199,6 +203,8 @@ def reactions # Keep nested_view_post_stats counts in sync when posts are created or deleted. # direct_reply_count: incremented on the immediate parent only. # total_descendant_count: incremented on ALL ancestors up the reply chain. + # whisper_* columns track the whisper subset so non-staff users see + # counts that exclude whispers (prevents leaking whisper existence). add_model_callback(:post, :after_create) do next if reply_to_post_number.blank? @@ -215,28 +221,45 @@ def reactions ancestor_ids = ancestors.map(&:id) direct_parent_id = ancestors.find { |a| a.depth == 1 }&.id + is_whisper = post_type == Post.types[:whisper] ? 1 : 0 # Single upsert: increment total_descendant_count for all ancestors, # direct_reply_count only for the immediate parent. - DB.exec(<<~SQL, ids: ancestor_ids, parent_id: direct_parent_id) - INSERT INTO nested_view_post_stats (post_id, direct_reply_count, total_descendant_count, created_at, updated_at) + # whisper_* columns are incremented only when the new post is a whisper. + DB.exec(<<~SQL, ids: ancestor_ids, parent_id: direct_parent_id, whisper: is_whisper) + INSERT INTO nested_view_post_stats (post_id, direct_reply_count, total_descendant_count, + whisper_direct_reply_count, whisper_total_descendant_count, + created_at, updated_at) SELECT aid, CASE WHEN aid = :parent_id THEN 1 ELSE 0 END, 1, + CASE WHEN aid = :parent_id THEN :whisper ELSE 0 END, + :whisper, NOW(), NOW() FROM unnest(ARRAY[:ids]::int[]) AS aid ON CONFLICT (post_id) DO UPDATE SET total_descendant_count = nested_view_post_stats.total_descendant_count + 1, direct_reply_count = nested_view_post_stats.direct_reply_count + CASE WHEN nested_view_post_stats.post_id = :parent_id THEN 1 ELSE 0 END, + whisper_total_descendant_count = nested_view_post_stats.whisper_total_descendant_count + :whisper, + whisper_direct_reply_count = nested_view_post_stats.whisper_direct_reply_count + + CASE WHEN nested_view_post_stats.post_id = :parent_id THEN :whisper ELSE 0 END, updated_at = NOW() SQL end add_model_callback(:post, :after_destroy) do if reply_to_post_number.present? - my_descendants = NestedViewPostStat.where(post_id: id).pick(:total_descendant_count) || 0 + stat = + NestedViewPostStat.where(post_id: id).pick( + :total_descendant_count, + :whisper_total_descendant_count, + ) + my_descendants = stat&.first || 0 + my_whisper_descendants = stat&.second || 0 removed = 1 + my_descendants + is_whisper = post_type == Post.types[:whisper] ? 1 : 0 + whisper_removed = is_whisper + my_whisper_descendants # Walk ancestors (including deleted — post may already be soft-deleted) for stats decrement ancestors = @@ -251,16 +274,28 @@ def reactions direct_parent_id = ancestors.find { |a| a.depth == 1 }&.id # Single UPDATE: decrement stats for all ancestors, clamped at 0 - DB.exec(<<~SQL, ids: ancestor_ids, parent_id: direct_parent_id, removed: removed) + DB.exec( + <<~SQL, UPDATE nested_view_post_stats SET total_descendant_count = GREATEST(total_descendant_count - :removed, 0), direct_reply_count = GREATEST( direct_reply_count - CASE WHEN post_id = :parent_id THEN 1 ELSE 0 END, 0 ), + whisper_total_descendant_count = GREATEST(whisper_total_descendant_count - :whisper_removed, 0), + whisper_direct_reply_count = GREATEST( + whisper_direct_reply_count - CASE WHEN post_id = :parent_id THEN :is_whisper ELSE 0 END, + 0 + ), updated_at = NOW() WHERE post_id = ANY(ARRAY[:ids]::int[]) SQL + ids: ancestor_ids, + parent_id: direct_parent_id, + removed: removed, + whisper_removed: whisper_removed, + is_whisper: is_whisper, + ) end end diff --git a/spec/lib/nested_replies_n_plus_one_spec.rb b/spec/lib/nested_replies_n_plus_one_spec.rb index 6a96b12..121de55 100644 --- a/spec/lib/nested_replies_n_plus_one_spec.rb +++ b/spec/lib/nested_replies_n_plus_one_spec.rb @@ -87,6 +87,51 @@ def build_chain(depth:, topic:) expect(stat.total_descendant_count).to be >= 1 end end + + it "tracks whisper counts separately from regular counts" do + chain = build_chain(depth: 2, topic: topic) + Fabricate( + :post, + topic: topic, + user: user, + reply_to_post_number: chain.last.post_number, + post_type: Post.types[:whisper], + ) + + parent_stat = NestedViewPostStat.find_by(post_id: chain.last.id) + expect(parent_stat.direct_reply_count).to eq(1) + expect(parent_stat.whisper_direct_reply_count).to eq(1) + expect(parent_stat.total_descendant_count).to eq(1) + expect(parent_stat.whisper_total_descendant_count).to eq(1) + end + + it "does not increment whisper counts for regular posts" do + chain = build_chain(depth: 2, topic: topic) + Fabricate(:post, topic: topic, user: user, reply_to_post_number: chain.last.post_number) + + parent_stat = NestedViewPostStat.find_by(post_id: chain.last.id) + expect(parent_stat.direct_reply_count).to eq(1) + expect(parent_stat.whisper_direct_reply_count).to eq(0) + expect(parent_stat.total_descendant_count).to eq(1) + expect(parent_stat.whisper_total_descendant_count).to eq(0) + end + + it "increments whisper counts on all ancestors" do + chain = build_chain(depth: 3, topic: topic) + Fabricate( + :post, + topic: topic, + user: user, + reply_to_post_number: chain.last.post_number, + post_type: Post.types[:whisper], + ) + + chain.each do |ancestor| + stat = NestedViewPostStat.find_by(post_id: ancestor.id) + next unless stat + expect(stat.whisper_total_descendant_count).to be >= 1 + end + end end describe "after_destroy stats decrement" do @@ -130,6 +175,49 @@ def build_chain(depth:, topic:) leaf.destroy! expect(NestedViewPostStat.find_by(post_id: leaf_id)).to be_nil end + + it "decrements whisper counts when a whisper is destroyed" do + chain = build_chain(depth: 2, topic: topic) + whisper = + Fabricate( + :post, + topic: topic, + user: user, + reply_to_post_number: chain.last.post_number, + post_type: Post.types[:whisper], + ) + + parent_stat = NestedViewPostStat.find_by(post_id: chain.last.id) + expect(parent_stat.whisper_direct_reply_count).to eq(1) + expect(parent_stat.whisper_total_descendant_count).to eq(1) + + whisper.destroy! + + parent_stat.reload + expect(parent_stat.direct_reply_count).to eq(0) + expect(parent_stat.whisper_direct_reply_count).to eq(0) + expect(parent_stat.total_descendant_count).to eq(0) + expect(parent_stat.whisper_total_descendant_count).to eq(0) + end + + it "does not decrement whisper counts when a regular post is destroyed" do + chain = build_chain(depth: 2, topic: topic) + Fabricate( + :post, + topic: topic, + user: user, + reply_to_post_number: chain.last.post_number, + post_type: Post.types[:whisper], + ) + regular = + Fabricate(:post, topic: topic, user: user, reply_to_post_number: chain.last.post_number) + + regular.destroy! + + parent_stat = NestedViewPostStat.find_by(post_id: chain.last.id) + expect(parent_stat.direct_reply_count).to eq(1) + expect(parent_stat.whisper_direct_reply_count).to eq(1) + end end describe "context endpoint" do diff --git a/spec/requests/nested_topics_controller_spec.rb b/spec/requests/nested_topics_controller_spec.rb index 9780a6b..d628ab9 100644 --- a/spec/requests/nested_topics_controller_spec.rb +++ b/spec/requests/nested_topics_controller_spec.rb @@ -415,6 +415,110 @@ def pin_url(topic) end end + describe "whisper visibility" do + fab!(:whisper) do + Fabricate( + :post, + topic: topic, + user: admin, + reply_to_post_number: nil, + post_type: Post.types[:whisper], + ) + end + + it "excludes whispers for regular users" do + sign_in(user) + get roots_url(topic) + json = response.parsed_body + root_ids = json["roots"].map { |r| r["id"] } + expect(root_ids).not_to include(whisper.id) + end + + it "includes whispers for staff" do + sign_in(admin) + get roots_url(topic) + json = response.parsed_body + root_ids = json["roots"].map { |r| r["id"] } + expect(root_ids).to include(whisper.id) + end + + it "excludes whisper children for regular users" do + root = Fabricate(:post, topic: topic, user: user, reply_to_post_number: nil) + whisper_child = + Fabricate( + :post, + topic: topic, + user: admin, + reply_to_post_number: root.post_number, + post_type: Post.types[:whisper], + ) + sign_in(user) + + get children_url(topic, root.post_number) + json = response.parsed_body + child_ids = json["children"].map { |c| c["id"] } + expect(child_ids).not_to include(whisper_child.id) + end + + it "includes whisper children for staff" do + root = Fabricate(:post, topic: topic, user: user, reply_to_post_number: nil) + whisper_child = + Fabricate( + :post, + topic: topic, + user: admin, + reply_to_post_number: root.post_number, + post_type: Post.types[:whisper], + ) + sign_in(admin) + + get children_url(topic, root.post_number) + json = response.parsed_body + child_ids = json["children"].map { |c| c["id"] } + expect(child_ids).to include(whisper_child.id) + end + end + + describe "whisper reply count visibility" do + it "excludes whisper from reply counts for regular users" do + root = Fabricate(:post, topic: topic, user: user, reply_to_post_number: nil) + Fabricate(:post, topic: topic, user: user, reply_to_post_number: root.post_number) + Fabricate( + :post, + topic: topic, + user: admin, + reply_to_post_number: root.post_number, + post_type: Post.types[:whisper], + ) + sign_in(user) + + get roots_url(topic) + json = response.parsed_body + root_json = json["roots"].find { |r| r["id"] == root.id } + expect(root_json["direct_reply_count"]).to eq(1) + expect(root_json["total_descendant_count"]).to eq(1) + end + + it "includes whisper in reply counts for staff" do + root = Fabricate(:post, topic: topic, user: user, reply_to_post_number: nil) + Fabricate(:post, topic: topic, user: user, reply_to_post_number: root.post_number) + Fabricate( + :post, + topic: topic, + user: admin, + reply_to_post_number: root.post_number, + post_type: Post.types[:whisper], + ) + sign_in(admin) + + get roots_url(topic) + json = response.parsed_body + root_json = json["roots"].find { |r| r["id"] == root.id } + expect(root_json["direct_reply_count"]).to eq(2) + expect(root_json["total_descendant_count"]).to eq(2) + end + end + it "skips the on_preload reply-count query for nested endpoints" do root_post = Fabricate(:post, topic: topic, user: user, reply_to_post_number: nil) Fabricate(:post, topic: topic, user: user, reply_to_post_number: root_post.post_number) From c179c137f1a8baedb094e58ce2d18fd6e054087a Mon Sep 17 00:00:00 2001 From: Mark VanLandingham Date: Mon, 23 Mar 2026 10:51:23 -0500 Subject: [PATCH 2/3] Use site setting for allowed group --- lib/discourse_nested_replies/tree_loader.rb | 4 ++-- plugin.rb | 2 +- spec/requests/nested_topics_controller_spec.rb | 4 ++++ 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/discourse_nested_replies/tree_loader.rb b/lib/discourse_nested_replies/tree_loader.rb index 62ac42e..a582877 100644 --- a/lib/discourse_nested_replies/tree_loader.rb +++ b/lib/discourse_nested_replies/tree_loader.rb @@ -25,7 +25,7 @@ def initialize(topic:, guardian:) def visible_post_types types = [Post.types[:regular], Post.types[:moderator_action]] - types << Post.types[:whisper] if guardian.is_staff? + types << Post.types[:whisper] if guardian.user&.whisperer? types end @@ -218,7 +218,7 @@ def direct_reply_counts(post_numbers) def total_descendant_counts(post_ids) return {} if post_ids.empty? - if guardian.is_staff? + if guardian.user&.whisperer? NestedViewPostStat .where(post_id: post_ids.uniq) .pluck(:post_id, :total_descendant_count) diff --git a/plugin.rb b/plugin.rb index dcd590e..6c3651b 100644 --- a/plugin.rb +++ b/plugin.rb @@ -52,7 +52,7 @@ module ::DiscourseNestedReplies next if post_numbers.empty? visible_types = [Post.types[:regular], Post.types[:moderator_action]] - visible_types << Post.types[:whisper] if topic_view.guardian.is_staff? + visible_types << Post.types[:whisper] if topic_view.guardian.user&.whisperer? counts = Post diff --git a/spec/requests/nested_topics_controller_spec.rb b/spec/requests/nested_topics_controller_spec.rb index d628ab9..c51fabd 100644 --- a/spec/requests/nested_topics_controller_spec.rb +++ b/spec/requests/nested_topics_controller_spec.rb @@ -416,6 +416,8 @@ def pin_url(topic) end describe "whisper visibility" do + before { SiteSetting.whispers_allowed_groups = "#{Group::AUTO_GROUPS[:staff]}" } + fab!(:whisper) do Fabricate( :post, @@ -480,6 +482,8 @@ def pin_url(topic) end describe "whisper reply count visibility" do + before { SiteSetting.whispers_allowed_groups = "#{Group::AUTO_GROUPS[:staff]}" } + it "excludes whisper from reply counts for regular users" do root = Fabricate(:post, topic: topic, user: user, reply_to_post_number: nil) Fabricate(:post, topic: topic, user: user, reply_to_post_number: root.post_number) From 246cb36b1884279af0c5d7d51965d39cc0afd01b Mon Sep 17 00:00:00 2001 From: Mark VanLandingham Date: Mon, 23 Mar 2026 11:15:30 -0500 Subject: [PATCH 3/3] lint --- app/models/nested_view_post_stat.rb | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/app/models/nested_view_post_stat.rb b/app/models/nested_view_post_stat.rb index de69a09..ae2c325 100644 --- a/app/models/nested_view_post_stat.rb +++ b/app/models/nested_view_post_stat.rb @@ -8,14 +8,14 @@ class NestedViewPostStat < ActiveRecord::Base # # Table name: nested_view_post_stats # -# id :bigint not null, primary key -# direct_reply_count :integer default(0), not null -# total_descendant_count :integer default(0), not null -# whisper_direct_reply_count :integer default(0), not null -# whisper_total_descendant_count :integer default(0), not null -# created_at :datetime not null -# updated_at :datetime not null -# post_id :bigint not null +# id :bigint not null, primary key +# direct_reply_count :integer default(0), not null +# total_descendant_count :integer default(0), not null +# whisper_direct_reply_count :integer default(0), not null +# whisper_total_descendant_count :integer default(0), not null +# created_at :datetime not null +# updated_at :datetime not null +# post_id :bigint not null # # Indexes #