Skip to content
Permalink
Browse files Browse the repository at this point in the history
SECURITY: leaking PMs and secure categories topics (#110)
Ensure that user has access to private messages and categories of he topics
  • Loading branch information
lis2 committed Oct 11, 2021
1 parent ba99f2d commit 213d90b
Show file tree
Hide file tree
Showing 2 changed files with 86 additions and 1 deletion.
Expand Up @@ -32,11 +32,14 @@ def reactions_given
raise Discourse::NotFound unless guardian.can_see_profile?(user)

reaction_users = DiscourseReactions::ReactionUser
.joins(:reaction, :post)
.joins(:reaction, post: :topic)
.joins("LEFT JOIN categories ON categories.id = topics.category_id")
.includes(:user, :post, :reaction)
.where(user_id: user.id)
.where('discourse_reactions_reactions.reaction_users_count IS NOT NULL')

reaction_users = secure_reaction_users!(reaction_users)

if params[:before_reaction_user_id]
reaction_users = reaction_users
.where('discourse_reactions_reaction_users.id < ?', params[:before_reaction_user_id].to_i)
Expand Down Expand Up @@ -187,5 +190,40 @@ def publish_change_to_clients!(post)

MessageBus.publish("/topic/#{post.topic.id}/reactions", message)
end

def secure_reaction_users!(reaction_users)
if !guardian.can_see_private_messages?(current_user.id) || !guardian.user
reaction_users = reaction_users.where("topics.archetype <> :private_message", private_message: archetype::private_message)
else
unless guardian.is_admin?
sql = <<~SQL
topics.archetype <> :private_message OR
EXISTS (
SELECT 1 FROM topic_allowed_users tu WHERE tu.topic_id = topics.id AND tu.user_id = :current_user_id
) OR
EXISTS (
SELECT 1 FROM topic_allowed_groups tg WHERE tg.topic_id = topics.id AND tg.group_id IN (
SELECT group_id FROM group_users gu WHERE gu.user_id = :current_user_id
)
)
SQL

reaction_users = reaction_users.where(sql, private_message: Archetype::private_message, current_user_id: guardian.user.id)
end
end

unless guardian.is_admin?
allowed = guardian.secure_category_ids
if allowed.present?
reaction_users = reaction_users.where("(categories.read_restricted IS NULL OR
NOT categories.read_restricted OR
(categories.read_restricted and categories.id in (:categories)) )", categories: guardian.secure_category_ids)
else
reaction_users = reaction_users.where("(categories.read_restricted IS NULL OR NOT categories.read_restricted)")
end
end

reaction_users
end
end
end
47 changes: 47 additions & 0 deletions spec/requests/custom_reactions_controller_spec.rb
@@ -1,6 +1,8 @@
# frozen_string_literal: true

require 'rails_helper'
require_relative '../fabricators/reaction_fabricator.rb'
require_relative '../fabricators/reaction_user_fabricator.rb'

describe DiscourseReactions::CustomReactionsController do
fab!(:post_1) { Fabricate(:post) }
Expand Down Expand Up @@ -109,6 +111,17 @@
end

context '#reactions_given' do
fab!(:private_topic) { Fabricate(:private_message_topic, user: user_2) }
fab!(:private_post) { Fabricate(:post, topic: private_topic) }
fab!(:secure_group) { Fabricate(:group) }
fab!(:secure_category) { Fabricate(:private_category, group: secure_group) }
fab!(:secure_topic) { Fabricate(:topic, category: secure_category) }
fab!(:secure_post) { Fabricate(:post, topic: secure_topic) }
fab!(:private_reaction) { Fabricate(:reaction, post: private_post, reaction_value: "hugs") }
fab!(:secure_reaction) { Fabricate(:reaction, post: secure_post, reaction_value: "hugs") }
fab!(:private_topic_reaction_user) { Fabricate(:reaction_user, reaction: private_reaction, user: user_2, post: private_post) }
fab!(:secure_topic_reaction_user) { Fabricate(:reaction_user, reaction: secure_reaction, user: user_2, post: secure_post) }

it 'returns reactions given by a user' do
sign_in(user_1)

Expand All @@ -121,6 +134,40 @@
expect(parsed[0]['reaction']['id']).to eq(reaction_1.id)
end

it 'does not return reactions for private messages' do
sign_in(user_1)

get "/discourse-reactions/posts/reactions.json", params: { username: user_2.username }
parsed = response.parsed_body
expect(response.parsed_body.map { |reaction| reaction["post_id"] }).not_to include(private_post.id)

sign_in(user_2)

get "/discourse-reactions/posts/reactions.json", params: { username: user_2.username }
parsed = response.parsed_body
expect(response.parsed_body.map { |reaction| reaction["post_id"] }).to include(private_post.id)
end

it 'does not return reactions for secure categories' do
secure_group.add(user_2)
sign_in(user_1)

get "/discourse-reactions/posts/reactions.json", params: { username: user_2.username }
parsed = response.parsed_body
expect(response.parsed_body.map { |reaction| reaction["post_id"] }).not_to include(secure_post.id)

secure_group.add(user_1)
get "/discourse-reactions/posts/reactions.json", params: { username: user_2.username }
parsed = response.parsed_body
expect(response.parsed_body.map { |reaction| reaction["post_id"] }).to include(secure_post.id)

sign_in(user_2)

get "/discourse-reactions/posts/reactions.json", params: { username: user_2.username }
parsed = response.parsed_body
expect(response.parsed_body.map { |reaction| reaction["post_id"] }).to include(secure_post.id)
end

context 'a post with one of your reactions has been deleted' do
fab!(:deleted_post) { Fabricate(:post) }
fab!(:kept_post) { Fabricate(:post) }
Expand Down

0 comments on commit 213d90b

Please sign in to comment.