From a442658f8af02d8cae70b66ebbaeecbff5594e4b Mon Sep 17 00:00:00 2001 From: evazion Date: Thu, 22 Sep 2022 19:02:17 -0500 Subject: [PATCH] Fix #5237: Deleted comments can be viewed by other users * Fix it so non-moderators can't search deleted comments using the `updater`, `body`, `score`, `do_not_bump_post`, or `is_sticky` fields. Searching for these fields will exclude deleted comments. * Fix it so non-moderators can search for their own deleted comments using the `creator` field, but not for deleted comments belonging to other users. * Fix it so that if a regular user searches `commenter:`, they can only see posts with undeleted comments by that user. If a moderator or the commenter themselves searches `commenter:`, they can see all posts the user has commented on, including posts with deleted comments. * Fix it so the comment count on user profiles only counts visible comments. Regular users can only see the number of undeleted comments a user has, while moderators and the commenter themselves can see the total number of comments. Known issue: * It's still possible to order deleted comments by score, which can let you infer the score of deleted comments. --- app/logical/post_query.rb | 2 +- app/logical/post_query_builder.rb | 14 ++--- app/models/application_record.rb | 4 ++ app/models/post.rb | 4 +- app/models/user.rb | 2 +- app/policies/comment_policy.rb | 11 ++++ test/functional/comments_controller_test.rb | 57 ++++++++++++++++----- test/unit/post_query_builder_test.rb | 17 ++++++ 8 files changed, 88 insertions(+), 23 deletions(-) diff --git a/app/logical/post_query.rb b/app/logical/post_query.rb index f1dc0b237..457b40911 100644 --- a/app/logical/post_query.rb +++ b/app/logical/post_query.rb @@ -138,7 +138,7 @@ class PostQuery # True if the search depends on the current user because of permissions or privacy settings. def is_user_dependent_search? metatags.any? do |metatag| - metatag.name.in?(%w[upvoter upvote downvoter downvote search flagger fav ordfav favgroup ordfavgroup]) || + metatag.name.in?(%w[upvoter upvote downvoter downvote commenter comm search flagger fav ordfav favgroup ordfavgroup]) || metatag.name == "status" && metatag.value == "unmoderated" || metatag.name == "disapproved" && !metatag.value.downcase.in?(PostDisapproval::REASONS) end diff --git a/app/logical/post_query_builder.rb b/app/logical/post_query_builder.rb index 340c19b5c..7b181e16b 100644 --- a/app/logical/post_query_builder.rb +++ b/app/logical/post_query_builder.rb @@ -172,19 +172,19 @@ class PostQueryBuilder when "flagger" relation.flagger_matches(value, current_user) when "appealer" - relation.user_subquery_matches(PostAppeal.unscoped, value) + relation.user_subquery_matches(PostAppeal.unscoped, value, current_user) when "commenter", "comm" - relation.user_subquery_matches(Comment.unscoped, value) + relation.user_subquery_matches(Comment.unscoped, value, current_user) when "commentaryupdater", "artcomm" - relation.user_subquery_matches(ArtistCommentaryVersion.unscoped, value, field: :updater) + relation.user_subquery_matches(ArtistCommentaryVersion.unscoped, value, current_user, field: :updater) when "noter" - relation.user_subquery_matches(NoteVersion.unscoped.where(version: 1), value, field: :updater) + relation.user_subquery_matches(NoteVersion.unscoped.where(version: 1), value, current_user, field: :updater) when "noteupdater" - relation.user_subquery_matches(NoteVersion.unscoped, value, field: :updater) + relation.user_subquery_matches(NoteVersion.unscoped, value, current_user, field: :updater) when "upvoter", "upvote" - relation.user_subquery_matches(PostVote.active.positive.visible(current_user), value, field: :user) + relation.user_subquery_matches(PostVote.active.positive.visible(current_user), value, current_user, field: :user) when "downvoter", "downvote" - relation.user_subquery_matches(PostVote.active.negative.visible(current_user), value, field: :user) + relation.user_subquery_matches(PostVote.active.negative.visible(current_user), value, current_user, field: :user) when "random" relation # handled in the `build` method when *CATEGORY_COUNT_METATAGS diff --git a/app/models/application_record.rb b/app/models/application_record.rb index 56e739462..e1520bbd7 100644 --- a/app/models/application_record.rb +++ b/app/models/application_record.rb @@ -44,6 +44,10 @@ class ApplicationRecord < ActiveRecord::Base all end + def visible_for_search(attribute, current_user) + policy(current_user).visible_for_search(all, attribute) + end + def policy(current_user) Pundit.policy(current_user, self) end diff --git a/app/models/post.rb b/app/models/post.rb index acc104fbe..60c4c435d 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -1359,7 +1359,7 @@ class Post < ApplicationRecord end end - def user_subquery_matches(subquery, username, field: :creator, &block) + def user_subquery_matches(subquery, username, current_user, field: :creator, &block) subquery = subquery.where("post_id = posts.id").select(1) if username.downcase == "any" @@ -1369,7 +1369,7 @@ class Post < ApplicationRecord elsif block.nil? user = User.find_by_name(username) return none if user.nil? - subquery = subquery.where(field => user) + subquery = subquery.visible_for_search(field, current_user).where(field => user) where("EXISTS (#{subquery.to_sql})") else subquery = subquery.merge(block.call(username)) diff --git a/app/models/user.rb b/app/models/user.rb index e71eb1721..e81747f76 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -561,7 +561,7 @@ class User < ApplicationRecord end def comment_count - comments.count + comments.visible_for_search(:creator, CurrentUser.user).count end def favorite_group_count diff --git a/app/policies/comment_policy.rb b/app/policies/comment_policy.rb index 20df232c1..6557fc456 100644 --- a/app/policies/comment_policy.rb +++ b/app/policies/comment_policy.rb @@ -43,5 +43,16 @@ class CommentPolicy < ApplicationPolicy attributes end + def visible_for_search(comments, attribute) + case attribute + in :creator | :creator_id if !can_see_deleted? + comments.where(creator: user, is_deleted: true).or(comments.undeleted) + in :updater | :updater_id | :body | :score | :do_not_bump_post | :is_sticky if !can_see_deleted? + comments.undeleted + else + comments + end + end + alias_method :undelete?, :update? end diff --git a/test/functional/comments_controller_test.rb b/test/functional/comments_controller_test.rb index aa3a7a579..f25776309 100644 --- a/test/functional/comments_controller_test.rb +++ b/test/functional/comments_controller_test.rb @@ -52,23 +52,56 @@ class CommentsControllerTest < ActionDispatch::IntegrationTest end context "grouped by comment" do - setup do - @user_comment = create(:comment, post: @post, score: 10, do_not_bump_post: true, creator: @user) - @mod_comment = create(:comment, post: build(:post, tag_string: "touhou"), body: "blah", is_sticky: true, creator: @mod) - @deleted_comment = create(:comment, is_deleted: true) - end - should "render" do + create(:comment) + get comments_path(group_by: "comment") assert_response :success end + end - should respond_to_search(other_params: {group_by: "comment"}).with { [@deleted_comment, @mod_comment, @user_comment] } - should respond_to_search(body_matches: "blah").with { @mod_comment } - should respond_to_search(score: 10).with { @user_comment } - should respond_to_search(is_sticky: "true").with { @mod_comment } - should respond_to_search(do_not_bump_post: "true").with { @user_comment } - should respond_to_search(is_deleted: "true").with { @deleted_comment } + context "searching" do + setup do + @user_comment = create(:comment, post: @post, score: 10, do_not_bump_post: true, creator: @user) + @mod_comment = create(:comment, post: build(:post, tag_string: "touhou"), body: "blah", is_sticky: true, creator: @mod) + @deleted_comment = create(:comment, creator: create(:user, name: "deleted"), is_deleted: true, is_sticky: true, do_not_bump_post: true, score: 10, body: "blah") + end + + context "as a regular user" do + setup { CurrentUser.user = @user } + + should respond_to_search(other_params: {group_by: "comment"}).with { [@deleted_comment, @mod_comment, @user_comment] } + should respond_to_search(body_matches: "blah").with { @mod_comment } + should respond_to_search(score: 10).with { @user_comment } + should respond_to_search(is_sticky: "true").with { @mod_comment } + should respond_to_search(is_deleted: "true").with { @deleted_comment } + should respond_to_search(do_not_bump_post: "true").with { @user_comment } + should respond_to_search(creator_name: "deleted").with { [] } + end + + context "as the creator of a deleted comment" do + setup { CurrentUser.user = @deleted_comment.creator } + + should respond_to_search(other_params: {group_by: "comment"}).with { [@deleted_comment, @mod_comment, @user_comment] } + should respond_to_search(body_matches: "blah").with { @mod_comment } + should respond_to_search(score: 10).with { @user_comment } + should respond_to_search(is_sticky: "true").with { @mod_comment } + should respond_to_search(is_deleted: "true").with { @deleted_comment } + should respond_to_search(do_not_bump_post: "true").with { @user_comment } + should respond_to_search(creator_name: "deleted").with { @deleted_comment } + end + + context "as a moderator" do + setup { CurrentUser.user = @mod } + + should respond_to_search(other_params: {group_by: "comment"}).with { [@deleted_comment, @mod_comment, @user_comment] } + should respond_to_search(body_matches: "blah").with { [@deleted_comment, @mod_comment] } + should respond_to_search(score: 10).with { [@deleted_comment, @user_comment] } + should respond_to_search(is_sticky: "true").with { [@deleted_comment, @mod_comment] } + should respond_to_search(is_deleted: "true").with { @deleted_comment } + should respond_to_search(do_not_bump_post: "true").with { [@deleted_comment, @user_comment] } + should respond_to_search(creator_name: "deleted").with { @deleted_comment } + end context "using includes" do should respond_to_search(post_id: 100).with { @user_comment } diff --git a/test/unit/post_query_builder_test.rb b/test/unit/post_query_builder_test.rb index 803ef4bda..9e38d4a84 100644 --- a/test/unit/post_query_builder_test.rb +++ b/test/unit/post_query_builder_test.rb @@ -476,6 +476,16 @@ class PostQueryBuilderTest < ActiveSupport::TestCase assert_tag_match([posts[0]], "-commenter:#{users[1].name}") end + should "return posts with deleted comments correctly for the commenter: metatag" do + user = create(:user) + c1 = create(:comment, creator: user) + c2 = create(:comment, creator: user, is_deleted: true) + + assert_tag_match([c1.post], "commenter:#{user.name}", current_user: User.anonymous) + assert_tag_match([c2.post, c1.post], "commenter:#{user.name}", current_user: user) + assert_tag_match([c2.post, c1.post], "commenter:#{user.name}", current_user: create(:mod_user)) + end + should "return posts for the commenter: metatag" do posts = create_list(:post, 2) create(:comment, creator: create(:user, created_at: 2.weeks.ago), post: posts[0], is_deleted: false) @@ -1553,9 +1563,16 @@ class PostQueryBuilderTest < ActiveSupport::TestCase should "cache the count separately for different users" do @user = create(:user, enable_private_favorites: true) @post = as(@user) { create(:post, tag_string: "fav:#{@user.name}") } + @comment = create(:comment, post: @post, creator: @user, is_deleted: true) assert_equal(1, PostQuery.new("fav:#{@user.name}", current_user: @user).fast_count) assert_equal(0, PostQuery.new("fav:#{@user.name}").fast_count) + + assert_equal(1, PostQuery.new("commenter:#{@user.name}", current_user: @user).fast_count) + assert_equal(0, PostQuery.new("commenter:#{@user.name}").fast_count) + + assert_equal(1, PostQuery.new("comm:#{@user.name}", current_user: @user).fast_count) + assert_equal(0, PostQuery.new("comm:#{@user.name}").fast_count) end end end