SECURITY: User's read state for topic is leaked to unauthorized clients.

A user's read state for a topic such as the last read post number and the notification level is exposed.
This commit is contained in:
Alan Guo Xiang Tan 2021-08-11 11:01:13 +08:00
parent 9a60c83535
commit aed65ec16d
3 changed files with 62 additions and 9 deletions

View File

@ -145,6 +145,15 @@ class TopicTrackingState
return unless post.topic.regular? return unless post.topic.regular?
# TODO at high scale we are going to have to defer this, # TODO at high scale we are going to have to defer this,
# perhaps cut down to users that are around in the last 7 days as well # perhaps cut down to users that are around in the last 7 days as well
tags = nil
tag_ids = nil
if include_tags_in_report?
tag_ids, tags = post.topic.tags.pluck(:id, :name).transpose
end
scope = TopicUser
.tracking(post.topic_id)
.includes(user: :user_stat)
group_ids = group_ids =
if post.post_type == Post.types[:whisper] if post.post_type == Post.types[:whisper]
@ -153,15 +162,13 @@ class TopicTrackingState
post.topic.category && post.topic.category.secure_group_ids post.topic.category && post.topic.category.secure_group_ids
end end
tags = nil if group_ids.present?
tag_ids = nil scope = scope
if include_tags_in_report? .joins("INNER JOIN group_users gu ON gu.user_id = topic_users.user_id")
tag_ids, tags = post.topic.tags.pluck(:id, :name).transpose .where("gu.group_id IN (?)", group_ids)
end end
TopicUser scope
.tracking(post.topic_id)
.includes(user: :user_stat)
.select([:user_id, :last_read_post_number, :notification_level]) .select([:user_id, :last_read_post_number, :notification_level])
.each do |tu| .each do |tu|
@ -188,7 +195,9 @@ class TopicTrackingState
payload: payload payload: payload
} }
MessageBus.publish(self.unread_channel_key(tu.user_id), message.as_json, group_ids: group_ids) MessageBus.publish(self.unread_channel_key(tu.user_id), message.as_json,
user_ids: [tu.user_id]
)
end end
end end

View File

@ -137,7 +137,8 @@ describe PostCreator do
Jobs.run_immediately! Jobs.run_immediately!
UserActionManager.enable UserActionManager.enable
admin = Fabricate(:admin) admin = Fabricate(:user)
admin.grant_admin!
cat = Fabricate(:category) cat = Fabricate(:category)
cat.set_permissions(admins: :full) cat.set_permissions(admins: :full)

View File

@ -48,11 +48,54 @@ describe TopicTrackingState do
data = message.data data = message.data
expect(message.user_ids).to contain_exactly(post.user.id)
expect(message.group_ids).to eq(nil)
expect(data["topic_id"]).to eq(topic.id) expect(data["topic_id"]).to eq(topic.id)
expect(data["message_type"]).to eq(described_class::UNREAD_MESSAGE_TYPE) expect(data["message_type"]).to eq(described_class::UNREAD_MESSAGE_TYPE)
expect(data["payload"]["archetype"]).to eq(Archetype.default) expect(data["payload"]["archetype"]).to eq(Archetype.default)
end end
it "does not publish whisper post to non-staff users" do
post.update!(post_type: Post.types[:whisper])
messages = MessageBus.track_publish(described_class.unread_channel_key(post.user_id)) do
TopicTrackingState.publish_unread(post)
end
expect(messages).to eq([])
post.user.grant_admin!
message = MessageBus.track_publish(described_class.unread_channel_key(post.user_id)) do
TopicTrackingState.publish_unread(post)
end.first
expect(message.user_ids).to contain_exactly(post.user_id)
expect(message.group_ids).to eq(nil)
end
it "correctly publishes unread for a post in a restricted category" do
group = Fabricate(:group)
category = Fabricate(:private_category, group: group)
post.topic.update!(category: category)
messages = MessageBus.track_publish(described_class.unread_channel_key(post.user_id)) do
TopicTrackingState.publish_unread(post)
end
expect(messages).to eq([])
group.add(post.user)
message = MessageBus.track_publish(described_class.unread_channel_key(post.user_id)) do
TopicTrackingState.publish_unread(post)
end.first
expect(message.user_ids).to contain_exactly(post.user_id)
expect(message.group_ids).to eq(nil)
end
describe 'for a private message' do describe 'for a private message' do
before do before do
TopicUser.change( TopicUser.change(