From 322a20a9f4df4489124c496868da80dec97e4179 Mon Sep 17 00:00:00 2001
From: Sam <sam.saffron@gmail.com>
Date: Wed, 22 May 2024 15:36:29 +1000
Subject: [PATCH] FIX: paginating posts should allow for deletions and PMs
 (#27098)

Note this may have performance issues in some cases, will need to be monitored

Previous to this change we were bracketing on 50 id windows. They may end up
having zero posts we are searching for leading to posts.rss and .json returning
no results.

- avoids Post.last.id which is expensive
- order by id desc which is better cause we bracket on id
---
 app/controllers/posts_controller.rb    | 34 ++++++++++++++++-------
 spec/requests/posts_controller_spec.rb | 38 ++++++++++++++++++++++++--
 2 files changed, 60 insertions(+), 12 deletions(-)

diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb
index 83c1da0052a..39d84c88b3c 100644
--- a/app/controllers/posts_controller.rb
+++ b/app/controllers/posts_controller.rb
@@ -61,41 +61,55 @@ class PostsController < ApplicationController
   def latest
     params.permit(:before)
     last_post_id = params[:before].to_i
-    last_post_id = Post.last.id if last_post_id <= 0
+    last_post_id = nil if last_post_id <= 0
 
     if params[:id] == "private_posts"
       raise Discourse::NotFound if current_user.nil?
+
+      allowed_private_topics = TopicAllowedUser.where(user_id: current_user.id).select(:topic_id)
+
+      allowed_groups = GroupUser.where(user_id: current_user.id).select(:group_id)
+      allowed_private_topics_by_group =
+        TopicAllowedGroup.where(group_id: allowed_groups).select(:topic_id)
+
+      all_allowed =
+        Topic
+          .where(id: allowed_private_topics)
+          .or(Topic.where(id: allowed_private_topics_by_group))
+          .select(:id)
+
       posts =
         Post
           .private_posts
-          .order(created_at: :desc)
-          .where("posts.id <= ?", last_post_id)
-          .where("posts.id > ?", last_post_id - 50)
+          .order(id: :desc)
           .includes(topic: :category)
           .includes(user: %i[primary_group flair_group])
           .includes(:reply_to_user)
           .limit(50)
       rss_description = I18n.t("rss_description.private_posts")
+
+      posts = posts.where(topic_id: all_allowed) if !current_user.admin?
     else
       posts =
         Post
           .public_posts
           .visible
           .where(post_type: Post.types[:regular])
-          .order(created_at: :desc)
-          .where("posts.id <= ?", last_post_id)
-          .where("posts.id > ?", last_post_id - 50)
+          .order(id: :desc)
           .includes(topic: :category)
           .includes(user: %i[primary_group flair_group])
           .includes(:reply_to_user)
+          .where("categories.id" => Category.secured(guardian).select(:id))
           .limit(50)
+
       rss_description = I18n.t("rss_description.posts")
       @use_canonical = true
     end
 
-    # Remove posts the user doesn't have permission to see
-    # This isn't leaking any information we weren't already through the post ID numbers
-    posts = posts.reject { |post| !guardian.can_see?(post) || post.topic.blank? }
+    posts = posts.where("posts.id <= ?", last_post_id) if last_post_id
+
+    posts = posts.to_a
+
     counts = PostAction.counts_for(posts, current_user)
 
     respond_to do |format|
diff --git a/spec/requests/posts_controller_spec.rb b/spec/requests/posts_controller_spec.rb
index 22ed1fdb9dc..d948d225e52 100644
--- a/spec/requests/posts_controller_spec.rb
+++ b/spec/requests/posts_controller_spec.rb
@@ -92,7 +92,6 @@ RSpec.describe PostsController do
   let(:topicless_post) { Fabricate(:post, user: user, raw: "<p>Car 54, where are you?</p>") }
 
   let(:private_topic) { Fabricate(:topic, archetype: Archetype.private_message, category_id: nil) }
-
   let(:private_post) { Fabricate(:post, user: user, topic: private_topic) }
 
   describe "#show" do
@@ -2440,7 +2439,7 @@ RSpec.describe PostsController do
     it "can show whole topics" do
       topic = Fabricate(:topic)
       post = Fabricate(:post, topic: topic, post_number: 1, raw: "123456789")
-      post_2 = Fabricate(:post, topic: topic, post_number: 2, raw: "abcdefghij")
+      _post_2 = Fabricate(:post, topic: topic, post_number: 2, raw: "abcdefghij")
       post.save
       get "/raw/#{topic.id}"
       expect(response.status).to eq(200)
@@ -2580,6 +2579,23 @@ RSpec.describe PostsController do
         expect(body).to_not include(public_post.url)
       end
 
+      it "properly secures private posts" do
+        sign_in(user)
+
+        private_post
+
+        pm = Fabricate(:private_message_topic, recipient: user)
+        post_id = Fabricate(:post, topic: pm).id
+
+        get "/private-posts.json"
+        expect(response.status).to eq(200)
+
+        json = response.parsed_body
+        post_ids = json["private_posts"].map { |p| p["id"] }
+
+        expect(post_ids).to eq([post_id])
+      end
+
       it "returns private posts for json" do
         sign_in(admin)
 
@@ -2614,6 +2630,24 @@ RSpec.describe PostsController do
         expect(body).to_not include(private_post.url)
       end
 
+      it "doesn't include posts from secured categories you have no access to" do
+        public_post
+        private_post
+
+        category = Fabricate(:category, read_restricted: true)
+        topic = Fabricate(:topic, category: category)
+        secure_post = Fabricate(:post, topic: topic)
+
+        get "/posts.json"
+
+        expect(response.status).to eq(200)
+
+        body = response.parsed_body
+        ids = body["latest_posts"].map { |p| p["id"] }
+
+        expect(ids).not_to include secure_post.id
+      end
+
       it "doesn't include posts from hidden topics" do
         public_post.topic.update!(visible: false)