Discource-C
/
discourse
mirror of https://github.com/discourse/discourse.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

SECURITY: add pagination to post replies 

When a post has some replies, and the user click on the button to show them, we would load ALL the replies. This could lead to DoS if there were a very large number of replies.

This adds support for pagination to these post replies.

Internal ref t/129773

FIX: Duplicated parent posts

DEV: Query refactor
This commit is contained in:
Régis Hanol 2024-08-01 19:51:27 +02:00 committed by Alan Guo Xiang Tan
parent e081cc14fb
commit 34d04e7507
No known key found for this signature in database
GPG Key ID: 286D2AB58F8C86B6
14 changed files with 328 additions and 182 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
app/assets/javascripts/discourse/app/adapters/post-reply-history.js
View File

@ -2,12 +2,9 @@ import RestAdapter from "discourse/adapters/rest";
import { ajax } from "discourse/lib/ajax";
export default class PostReplyHistoryAdapter extends RestAdapter {
find(store, type, findArgs) {
const maxReplies = this.siteSettings.max_reply_history;
return ajax(
`/posts/${findArgs.postId}/reply-history?max_replies=${maxReplies}`
).then((replies) => {
return { post_reply_histories: replies };
});
find(_store, _type, { postId }) {
return ajax(`/posts/${postId}/reply-history`).then(
(post_reply_histories) => ({ post_reply_histories })
);
}
}

8
app/assets/javascripts/discourse/app/adapters/post-reply.js
View File

@ -2,9 +2,9 @@ import RestAdapter from "discourse/adapters/rest";
import { ajax } from "discourse/lib/ajax";
export default class PostReplyAdapter extends RestAdapter {
find(store, type, findArgs) {
return ajax(`/posts/${findArgs.postId}/replies`).then((replies) => {
return { post_replies: replies };
});
find(_store, _type, { postId, after = 1 }) {
return ajax(`/posts/${postId}/replies?after=${after || 1}`).then(
(post_replies) => ({ post_replies })
);
}
}

52
app/assets/javascripts/discourse/app/widgets/post-menu.js
View File

@ -100,10 +100,7 @@ registerButton("read-count", (attrs, state) => {
if (attrs.showReadIndicator) {
const count = attrs.readCount;
if (count > 0) {
let ariaPressed = "false";
if (state?.readers && state.readers.length > 0) {
ariaPressed = "true";
}
let ariaPressed = (state?.readers?.length > 0).toString();
return {
action: "toggleWhoRead",
title: "post.controls.read_indicator",
@ -151,10 +148,8 @@ function likeCount(attrs, state) {
addContainer = true;
}
let ariaPressed = "false";
if (state?.likedUsers && state.likedUsers.length > 0) {
ariaPressed = "true";
}
let ariaPressed = (state?.likedUsers?.length > 0).toString();
return {
action: "toggleWhoLiked",
title,
@ -287,53 +282,52 @@ registerButton("wiki-edit", (attrs) => {
});
registerButton("replies", (attrs, state, siteSettings) => {
const replyCount = attrs.replyCount;
if (!replyCount) {
const count = attrs.replyCount;
if (!count) {
return;
}
let action = "toggleRepliesBelow",
icon = state.repliesShown ? "chevron-up" : "chevron-down";
let title;
let action = "toggleRepliesBelow";
let icon = state.repliesShown ? "chevron-up" : "chevron-down";
if (siteSettings.enable_filtered_replies_view) {
action = "toggleFilteredRepliesView";
icon = state.filteredRepliesShown ? "chevron-up" : "chevron-down";
title = state.filteredRepliesShown
? "post.view_all_posts"
: "post.filtered_replies_hint";
}
// Omit replies if the setting `suppress_reply_directly_below` is enabled
if (
replyCount === 1 &&
count === 1 &&
attrs.replyDirectlyBelow &&
siteSettings.suppress_reply_directly_below
) {
return;
}
let ariaPressed;
let ariaPressed, ariaExpanded;
if (!siteSettings.enable_filtered_replies_view) {
ariaPressed = state.repliesShown ? "true" : "false";
ariaPressed = state.repliesShown.toString();
ariaExpanded = state.repliesShown.toString();
}
return {
action,
icon,
className: "show-replies",
titleOptions: { count: replyCount },
title: siteSettings.enable_filtered_replies_view
? state.filteredRepliesShown
? "post.view_all_posts"
: "post.filtered_replies_hint"
: "",
labelOptions: { count: replyCount },
titleOptions: { count },
title,
labelOptions: { count },
label: attrs.mobileView ? "post.has_replies_count" : "post.has_replies",
iconRight: !siteSettings.enable_filtered_replies_view || attrs.mobileView,
disabled: !!attrs.deleted,
translatedAriaLabel: I18n.t("post.sr_expand_replies", {
count: replyCount,
}),
ariaExpanded:
!siteSettings.enable_filtered_replies_view && state.repliesShown
? "true"
: "false",
translatedAriaLabel: I18n.t("post.sr_expand_replies", { count }),
ariaExpanded,
ariaPressed,
ariaControls: `embedded-posts__bottom--${attrs.post_number}`,
};

111
app/assets/javascripts/discourse/app/widgets/post.js
View File

@ -31,9 +31,13 @@ import getURL, {
import { iconNode } from "discourse-common/lib/icon-library";
import I18n from "discourse-i18n";
function transformWithCallbacks(post) {
function transformWithCallbacks(post, topicUrl, store) {
let transformed = transformBasicPost(post);
postTransformCallbacks(transformed);
transformed.customShare = `${topicUrl}/${post.post_number}`;
transformed.asPost = store.createRecord("post", post);
return transformed;
}
@ -162,12 +166,10 @@ createWidget("reply-to-tab", {
tabindex: "0",
};
if (!this.attrs.mobileView) {
if (!attrs.mobileView) {
result["role"] = "button";
result["aria-controls"] = `embedded-posts__top--${attrs.post_number}`;
result["aria-expanded"] = this.attrs.repliesAbove.length
? "true"
: "false";
result["aria-expanded"] = (attrs.repliesAbove.length > 0).toString();
}
return result;
@ -526,7 +528,7 @@ createWidget("post-contents", {
const extraState = {
state: {
repliesShown: !!state.repliesBelow.length,
repliesShown: state.repliesBelow.length > 0,
filteredRepliesShown: state.filteredRepliesShown,
},
};
@ -534,12 +536,11 @@ createWidget("post-contents", {
const repliesBelow = state.repliesBelow;
if (repliesBelow.length) {
result.push(
h(
`section.embedded-posts.bottom#embedded-posts__bottom--${this.attrs.post_number}`,
[
repliesBelow.map((p) => {
return this.attach("embedded-post", p, {
let children = [];
repliesBelow.forEach((p) => {
children.push(
this.attach("embedded-post", p, {
model: p.asPost,
state: {
role: "region",
@ -548,17 +549,36 @@ createWidget("post-contents", {
username: p.username,
}),
},
})
);
});
}),
children.push(
this.attach("button", {
title: "post.collapse",
icon: "chevron-up",
action: "toggleRepliesBelow",
actionParam: "true",
actionParam: true,
className: "btn collapse-up",
translatedAriaLabel: I18n.t("post.sr_collapse_replies"),
}),
]
})
);
if (repliesBelow.length < this.attrs.replyCount) {
children.push(
this.attach("button", {
label: "post.load_more_replies",
action: "loadMoreReplies",
actionParam: repliesBelow[repliesBelow.length - 1]?.post_number,
className: "btn load-more-replies",
})
);
}
result.push(
h(
`section.embedded-posts.bottom#embedded-posts__bottom--${this.attrs.post_number}`,
children
)
);
}
@ -599,30 +619,28 @@ createWidget("post-contents", {
}
},
toggleRepliesBelow(goToPost = "false") {
loadMoreReplies(after = 1) {
return this.store
.find("post-reply", { postId: this.attrs.id, after })
.then((replies) => {
replies.forEach((reply) => {
this.state.repliesBelow.push(
transformWithCallbacks(reply, this.attrs.topicUrl, this.store)
);
});
});
},
toggleRepliesBelow(goToPost = false) {
if (this.state.repliesBelow.length) {
this.state.repliesBelow = [];
if (goToPost === "true") {
DiscourseURL.routeTo(
`${this.attrs.topicUrl}/${this.attrs.post_number}`
);
if (goToPost === true) {
const { topicUrl, post_number } = this.attrs;
DiscourseURL.routeTo(`${topicUrl}/${post_number}`);
}
return;
} else {
return this.loadMoreReplies();
}
const post = this.findAncestorModel();
const topicUrl = post ? post.get("topic.url") : null;
return this.store
.find("post-reply", { postId: this.attrs.id })
.then((posts) => {
this.state.repliesBelow = posts.map((p) => {
let result = transformWithCallbacks(p);
result.customShare = `${topicUrl}/${p.post_number}`;
result.asPost = this.store.createRecord("post", p);
return result;
});
});
},
expandFirstPost() {
@ -799,7 +817,7 @@ createWidget("post-article", {
title: "post.collapse",
icon: "chevron-down",
action: "toggleReplyAbove",
actionParam: "true",
actionParam: true,
className: "btn collapse-down",
}),
replies,
@ -835,7 +853,7 @@ createWidget("post-article", {
return post ? post.get("topic.url") : null;
},
toggleReplyAbove(goToPost = "false") {
toggleReplyAbove(goToPost = false) {
const replyPostNumber = this.attrs.reply_to_post_number;
if (this.siteSettings.enable_filtered_replies_view) {
@ -860,10 +878,9 @@ createWidget("post-article", {
if (this.state.repliesAbove.length) {
this.state.repliesAbove = [];
if (goToPost === "true") {
DiscourseURL.routeTo(
`${this.attrs.topicUrl}/${this.attrs.post_number}`
);
if (goToPost === true) {
const { topicUrl, post_number } = this.attrs;
DiscourseURL.routeTo(`${topicUrl}/${post_number}`);
}
return Promise.resolve();
} else {
@ -871,12 +888,10 @@ createWidget("post-article", {
return this.store
.find("post-reply-history", { postId: this.attrs.id })
.then((posts) => {
this.state.repliesAbove = posts.map((p) => {
let result = transformWithCallbacks(p);
result.customShare = `${topicUrl}/${p.post_number}`;
result.asPost = this.store.createRecord("post", p);
return result;
posts.forEach((post) => {
this.state.repliesAbove.push(
transformWithCallbacks(post, topicUrl, this.store)
);
});
});
}

8
app/assets/stylesheets/desktop/topic-post.scss
View File

@ -205,7 +205,6 @@ pre.codeblock-buttons:hover {
border: none;
> div {
position: relative;
margin-bottom: 1.5em;
&:last-of-type {
margin-bottom: 0;
.row {
@ -296,6 +295,13 @@ pre.codeblock-buttons:hover {
}
}
}
.load-more-replies {
font-size: var(--font-down-1);
position: absolute;
left: 55%;
transform: translate(-50%, 50%);
padding: 0.35em 0.5em;
}
.topic-avatar {
padding-left: 1em;

7
app/assets/stylesheets/mobile/topic-post.scss
View File

@ -217,6 +217,13 @@ nav.post-controls button.reply .d-icon {
}
}
}
.load-more-replies {
font-size: var(--font-down-1);
position: absolute;
left: 50%;
transform: translate(-50%, 150%);
padding: 0.35em 0.5em;
}
}
.post-actions {

65
app/controllers/posts_controller.rb
View File

@ -310,13 +310,16 @@ class PostsController < ApplicationController
def reply_history
post = find_post_from_params
reply_history = post.reply_history(params[:max_replies].to_i, guardian)
user_custom_fields = {}
if (added_fields = User.allowed_user_custom_fields(guardian)).present?
user_custom_fields = User.custom_fields_for_ids(reply_history.pluck(:user_id), added_fields)
end
topic_view =
TopicView.new(
post.topic,
current_user,
include_suggested: false,
include_related: false,
reply_history_for: post.id,
)
render_serialized(reply_history, PostSerializer, user_custom_fields: user_custom_fields)
render_json_dump(TopicViewPostsSerializer.new(topic_view, scope: guardian).post_stream[:posts])
end
def reply_ids
@ -426,17 +429,38 @@ class PostsController < ApplicationController
render_json_error(e.message)
end
# Direct replies to this post
MAX_POST_REPLIES ||= 20
def replies
params.permit(:after)
after = [params[:after].to_i, 1].max
post = find_post_from_params
replies = post.replies.secured(guardian)
user_custom_fields = {}
if (added_fields = User.allowed_user_custom_fields(guardian)).present?
user_custom_fields = User.custom_fields_for_ids(replies.pluck(:user_id), added_fields)
post_ids =
post
.replies
.secured(guardian)
.where(post_number: after + 1..)
.limit(MAX_POST_REPLIES)
.pluck(:id)
if post_ids.blank?
render_json_dump []
else
topic_view =
TopicView.new(
post.topic,
current_user,
post_ids:,
include_related: false,
include_suggested: false,
)
render_json_dump(
TopicViewPostsSerializer.new(topic_view, scope: guardian).post_stream[:posts],
)
end
render_serialized(replies, PostSerializer, user_custom_fields: user_custom_fields)
end
def revisions
@ -995,20 +1019,15 @@ class PostsController < ApplicationController
# A deleted post can be seen by staff or a category group moderator for the topic.
# But we must find the deleted post to determine which category it belongs to, so
# we must find.with_deleted
post = finder.with_deleted.first
raise Discourse::NotFound unless post
raise Discourse::NotFound unless post = finder.with_deleted.first
raise Discourse::NotFound unless post.topic ||= Topic.with_deleted.find_by(id: post.topic_id)
post.topic = Topic.with_deleted.find_by(id: post.topic_id)
if !post.topic ||
(
(post.deleted_at.present? || post.topic.deleted_at.present?) &&
!guardian.can_moderate_topic?(post.topic)
)
raise Discourse::NotFound
if post.deleted_at.present? || post.topic.deleted_at.present?
raise Discourse::NotFound unless guardian.can_moderate_topic?(post.topic)
end
guardian.ensure_can_see!(post)
post
end
end

21
app/models/post.rb
View File

@ -975,27 +975,6 @@ class Post < ActiveRecord::Base
.count
end
def reply_history(max_replies = 100, guardian = nil)
post_ids = DB.query_single(<<~SQL, post_id: id, topic_id: topic_id)
WITH RECURSIVE breadcrumb(id, reply_to_post_number) AS (
SELECT p.id, p.reply_to_post_number FROM posts AS p
WHERE p.id = :post_id
UNION
SELECT p.id, p.reply_to_post_number FROM posts AS p, breadcrumb
WHERE breadcrumb.reply_to_post_number = p.post_number
AND p.topic_id = :topic_id
)
SELECT id from breadcrumb
WHERE id <> :post_id
ORDER by id
SQL
# [1,2,3][-10,-1] => nil
post_ids = (post_ids[(0 - max_replies)..-1] || post_ids)
Post.secured(guardian).where(id: post_ids).includes(:user, :topic).order(:id).to_a
end
MAX_REPLY_LEVEL ||= 1000
def reply_ids(guardian = nil, only_replies_to_single_post: true)

2
config/locales/client.en.yml
View File

@ -3728,7 +3728,9 @@ en:
show_hidden: "View ignored content."
deleted_by_author_simple: "(post deleted by author)"
collapse: "collapse"
load_more_replies: "load more replies"
sr_collapse_replies: "Collapse embedded replies"
sr_load_more_replies: "Load more embedded replies"
sr_date: "Post date"
sr_expand_replies:
one: "This post has %{count} reply"

57
lib/topic_view.rb
View File

@ -875,6 +875,29 @@ class TopicView
Topic.with_deleted.includes(:category).find_by(id: topic_or_topic_id)
end
def find_post_replies_ids(post_id)
DB.query_single(<<~SQL, post_id: post_id)
WITH RECURSIVE breadcrumb(id, reply_to_post_number, topic_id, level) AS (
SELECT id, reply_to_post_number, topic_id, 0
FROM posts
WHERE id = :post_id
UNION
SELECT p.id, p.reply_to_post_number, p.topic_id, b.level + 1
FROM posts AS p
, breadcrumb AS b
WHERE b.reply_to_post_number = p.post_number
AND b.topic_id = p.topic_id
AND b.level < #{SiteSetting.max_reply_history}
)
SELECT id
FROM breadcrumb
WHERE id <> :post_id
ORDER BY id
SQL
end
def unfiltered_posts
result = filter_post_types(@topic.posts)
result = result.with_deleted if @guardian.can_see_deleted_posts?(@topic.category)
@ -974,33 +997,23 @@ class TopicView
)
end
# Reply history
if @reply_history_for.present?
post_ids = find_post_replies_ids(@reply_history_for)
@filtered_posts = @filtered_posts.where("posts.id IN (:post_ids)", post_ids:)
@contains_gaps = true
end
# Filtering upwards
if @filter_upwards_post_id.present?
post = Post.find(@filter_upwards_post_id)
post_ids = DB.query_single(<<~SQL, post_id: post.id, topic_id: post.topic_id)
WITH RECURSIVE breadcrumb(id, reply_to_post_number) AS (
SELECT p.id, p.reply_to_post_number FROM posts AS p
WHERE p.id = :post_id
UNION
SELECT p.id, p.reply_to_post_number FROM posts AS p, breadcrumb
WHERE breadcrumb.reply_to_post_number = p.post_number
AND p.topic_id = :topic_id
)
SELECT id from breadcrumb
WHERE id <> :post_id
ORDER by id
SQL
post_ids = (post_ids[(0 - SiteSetting.max_reply_history)..-1] || post_ids)
post_ids.push(post.id)
post_ids = find_post_replies_ids(@filter_upwards_post_id) | [@filter_upwards_post_id.to_i]
@filtered_posts =
@filtered_posts.where(
"
posts.post_number = 1
OR posts.id IN (:post_ids)
OR posts.id > :max_post_id",
{ post_ids: post_ids, max_post_id: post_ids.max },
"posts.post_number = 1 OR posts.id IN (:post_ids) OR posts.id > :max_post_id",
post_ids:,
max_post_id: post_ids.max,
)
@contains_gaps = true

14
spec/models/post_spec.rb
View File

@ -1027,20 +1027,6 @@ RSpec.describe Post do
end
end
describe "reply_history" do
let!(:p1) { Fabricate(:post, post_args) }
let!(:p2) { Fabricate(:post, post_args.merge(reply_to_post_number: p1.post_number)) }
let!(:p3) { Fabricate(:post, post_args) }
let!(:p4) { Fabricate(:post, post_args.merge(reply_to_post_number: p2.post_number)) }
it "returns the posts in reply to this post" do
expect(p4.reply_history).to eq([p1, p2])
expect(p4.reply_history(1)).to eq([p2])
expect(p3.reply_history).to be_blank
expect(p2.reply_history).to eq([p1])
end
end
describe "reply_ids" do
fab!(:topic)
let!(:p1) { Fabricate(:post, topic: topic, post_number: 1) }

25
spec/requests/posts_controller_spec.rb
View File

@ -208,6 +208,31 @@ RSpec.describe PostsController do
expect(json[0]["user_custom_fields"]["hello"]).to eq("world")
expect(json[0]["user_custom_fields"]["hidden"]).to be_blank
end
it "supports pagination" do
parent = Fabricate(:post)
30.times do
reply = Fabricate(:post, topic: parent.topic, reply_to_post_number: parent.post_number)
PostReply.create!(post: parent, reply:)
end
get "/posts/#{parent.id}/replies.json", params: { after: parent.post_number }
expect(response.status).to eq(200)
replies = response.parsed_body
expect(replies.size).to eq(20)
after = replies.last["post_number"]
get "/posts/#{parent.id}/replies.json", params: { after: }
expect(response.status).to eq(200)
replies = response.parsed_body
expect(replies.size).to eq(10)
expect(replies[0][:post_number]).to eq(after + 1)
get "/posts/#{parent.id}/replies.json", params: { after: 999_999 }
expect(response.status).to eq(200)
expect(response.parsed_body.size).to eq(0)
end
end
describe "#destroy" do

43
spec/system/page_objects/components/post.rb Normal file
View File

@ -0,0 +1,43 @@
# frozen_string_literal: true
module PageObjects
module Components
class Post < PageObjects::Components::Base
def initialize(post_number)
@post_number = post_number
end
def show_replies
find("#post_#{@post_number} .show-replies").click
end
def load_more_replies
find("#embedded-posts__bottom--#{@post_number} .load-more-replies").click
end
def has_replies?(count: nil)
find("#embedded-posts__bottom--#{@post_number}").has_css?(".reply", count:)
end
def has_more_replies?
find("#embedded-posts__bottom--#{@post_number}").has_css?(".load-more-replies")
end
def has_loaded_all_replies?
find("#embedded-posts__bottom--#{@post_number}").has_no_css?(".load-more-replies")
end
def show_parent_posts
find("#post_#{@post_number} .reply-to-tab").click
end
def has_parent_posts?(count: nil)
find("#embedded-posts__top--#{@post_number}").has_css?(".reply", count:)
end
def has_no_parent_post_content?(content)
find("#embedded-posts__top--#{@post_number}").has_no_content?(content)
end
end
end
end

60
spec/system/post_replies_spec.rb Normal file
View File

@ -0,0 +1,60 @@
# frozen_string_literal: true
describe "Post replies", type: :system do
fab!(:user)
fab!(:topic)
fab!(:post) { Fabricate(:post, user:, topic:) }
let(:first_post) { PageObjects::Components::Post.new(1) }
let(:third_chained_reply) { PageObjects::Components::Post.new(7) }
context "when loading post replies" do
before do
25.times do
reply = Fabricate(:post, topic:, reply_to_post_number: 1)
PostReply.create!(post:, reply:)
end
post.update!(reply_count: post.replies.count)
end
it "supports pagination" do
sign_in(user)
visit(topic.url)
first_post.show_replies
expect(first_post).to have_replies(count: 20)
expect(first_post).to have_more_replies
first_post.load_more_replies
expect(first_post).to have_replies(count: post.replies.count)
expect(first_post).to have_loaded_all_replies
end
end
context "when loading parent posts" do
before do
SiteSetting.max_reply_history = 2
3.times do |i|
PostReply.create!(post:, reply: Fabricate(:post, topic:))
reply = Fabricate(:post, topic:, reply_to_post_number: (i * 2) + 1, raw: "reply #{i + 1}")
PostReply.create!(post:, reply:)
end
post.update!(reply_count: post.replies.count)
end
it "does not duplicate replies" do
sign_in(user)
visit(topic.url)
third_chained_reply.show_parent_posts
expect(third_chained_reply).to have_parent_posts(count: 2)
expect(third_chained_reply).to have_no_parent_post_content("reply 3")
end
end
end