Skip to content
Permalink
Browse files Browse the repository at this point in the history
SECURITY: Hide notifications for inaccessible topics (#19208)
Filter notifications the user cannot see anymore
via guardian.can_see_topic_ids
  • Loading branch information
martin-brennan committed Nov 28, 2022
1 parent 6335b2c commit c6ee28e
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 1 deletion.
10 changes: 9 additions & 1 deletion app/controllers/notifications_controller.rb
Expand Up @@ -40,7 +40,7 @@ def index
end

if notifications.present? && !(params.has_key?(:silent) || @readonly_mode)
if changed = current_user.bump_last_seen_notification!
if current_user.bump_last_seen_notification!
current_user.reload
current_user.publish_notifications_state
end
Expand All @@ -57,6 +57,8 @@ def index
end
end

notifications = filter_inaccessible_notifications(notifications)

json = {
notifications: serialize_data(notifications, NotificationSerializer),
seen_notification_id: current_user.seen_notification_id
Expand All @@ -82,6 +84,7 @@ def index

total_rows = notifications.dup.count
notifications = notifications.offset(offset).limit(60)
notifications = filter_inaccessible_notifications(notifications)
render_json_dump(notifications: serialize_data(notifications, NotificationSerializer),
total_rows_notifications: total_rows,
seen_notification_id: user.seen_notification_id,
Expand Down Expand Up @@ -145,4 +148,9 @@ def render_notification
render_json_dump(NotificationSerializer.new(@notification, scope: guardian, root: false))
end

def filter_inaccessible_notifications(notifications)
topic_ids = notifications.map { |n| n.topic_id }.compact.uniq
accessible_topic_ids = guardian.can_see_topic_ids(topic_ids: topic_ids)
notifications.select { |n| n.topic_id.blank? || accessible_topic_ids.include?(n.topic_id) }
end
end
44 changes: 44 additions & 0 deletions spec/requests/notifications_controller_spec.rb
Expand Up @@ -339,6 +339,50 @@ def delete_notification(resp_code, matcher)
expect(response.status).to eq(404)
end
end

context "with notifications for inaccessible topics" do
fab!(:sender) { Fabricate.build(:topic_allowed_user, user: Fabricate(:coding_horror)) }
fab!(:allowed_user) { Fabricate.build(:topic_allowed_user, user: user) }
fab!(:another_allowed_user) { Fabricate.build(:topic_allowed_user, user: Fabricate(:user)) }
fab!(:allowed_pm) { Fabricate(:private_message_topic, topic_allowed_users: [sender, allowed_user, another_allowed_user]) }
fab!(:forbidden_pm) { Fabricate(:private_message_topic, topic_allowed_users: [sender, another_allowed_user]) }
fab!(:allowed_pm_notification) { Fabricate(:private_message_notification, user: user, topic: allowed_pm) }
fab!(:forbidden_pm_notification) { Fabricate(:private_message_notification, user: user, topic: forbidden_pm) }

def expect_correct_notifications(response)
notification_ids = response.parsed_body["notifications"].map { |n| n["id"] }
expect(notification_ids).to include(allowed_pm_notification.id)
expect(notification_ids).to_not include(forbidden_pm_notification.id)
end

context "with 'recent' filter" do
it "doesn't include notifications from topics the user isn't allowed to see" do
SiteSetting.enable_experimental_sidebar_hamburger = true
get "/notifications.json", params: { recent: true }
expect(response.status).to eq(200)
expect_correct_notifications(response)

SiteSetting.enable_experimental_sidebar_hamburger = false
get "/notifications.json", params: { recent: true }
expect(response.status).to eq(200)
expect_correct_notifications(response)
end
end

context "without 'recent' filter" do
it "doesn't include notifications from topics the user isn't allowed to see" do
SiteSetting.enable_experimental_sidebar_hamburger = true
get "/notifications.json"
expect(response.status).to eq(200)
expect_correct_notifications(response)

SiteSetting.enable_experimental_sidebar_hamburger = false
get "/notifications.json"
expect(response.status).to eq(200)
expect_correct_notifications(response)
end
end
end
end

it 'should succeed' do
Expand Down

0 comments on commit c6ee28e

Please sign in to comment.