SECURITY: Hide notifications for inaccessible topics (#19208)

Filter notifications the user cannot see anymore
via guardian.can_see_topic_ids

app/controllers/notifications_controller.rb

@@ -40,7 +40,7 @@ class NotificationsController < ApplicationController
       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
@@ -57,6 +57,8 @@ class NotificationsController < ApplicationController
         end
       end
 
+      notifications = filter_inaccessible_notifications(notifications)
+
       json = {
         notifications: serialize_data(notifications, NotificationSerializer),
         seen_notification_id: current_user.seen_notification_id
@@ -82,6 +84,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,
@@ -145,4 +148,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

spec/requests/notifications_controller_spec.rb

@@ -339,6 +339,50 @@ RSpec.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
+              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