From 131c5cff25cbb7937761375f40bc0a3aa2a0e03c Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Mon, 28 Nov 2022 10:42:05 +1000 Subject: [PATCH] SECURITY: Hide notifications for inaccessible topics (#19209) Filter notifications the user cannot see anymore via guardian.can_see_topic_ids --- app/controllers/notifications_controller.rb | 7 ++++ .../requests/notifications_controller_spec.rb | 32 +++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb index d89917b4f18..57a4e45c125 100644 --- a/app/controllers/notifications_controller.rb +++ b/app/controllers/notifications_controller.rb @@ -23,6 +23,7 @@ class NotificationsController < ApplicationController limit = 50 if limit > 50 notifications = Notification.recent_report(current_user, limit) + notifications = filter_inaccessible_notifications(notifications) changed = false if notifications.present? && !(params.has_key?(:silent) || @readonly_mode) @@ -50,6 +51,7 @@ class NotificationsController < ApplicationController 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, @@ -101,4 +103,9 @@ class NotificationsController < ApplicationController 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 diff --git a/spec/requests/notifications_controller_spec.rb b/spec/requests/notifications_controller_spec.rb index 3e903280f89..2fd3f075678 100644 --- a/spec/requests/notifications_controller_spec.rb +++ b/spec/requests/notifications_controller_spec.rb @@ -119,6 +119,38 @@ describe NotificationsController do 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 + 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 + get "/notifications.json" + expect(response.status).to eq(200) + expect_correct_notifications(response) + end + end + end end it 'should succeed' do