SECURITY: Filter unread bookmark reminders the user cannot see

There is an edge case where the following occurs:

1. The user sets a bookmark reminder on a post/topic
2. The post/topic is changed to a PM before or after the reminder
   fires, and the notification remains unread by the user
3. The user opens their bookmark reminder notification list
   and they can still see the notification even though they cannot
   access the topic anymore

There is a very low chance for information leaking here, since
the only thing that could be exposed is the topic title if it
changes to something sensitive.

This commit filters the bookmark unread notifications by using
the bookmarkable can_see? methods and also prevents sending
reminder notifications for bookmarks the user can no longer see.

app/controllers/notifications_controller.rb

@@ -55,7 +55,8 @@ class NotificationsController < ApplicationController
         end
       end
 
-      notifications = filter_inaccessible_notifications(notifications)
+      notifications =
+        Notification.filter_inaccessible_topic_notifications(current_user.guardian, notifications)
 
       json = {
         notifications: serialize_data(notifications, NotificationSerializer),
@@ -82,7 +83,8 @@ class NotificationsController < ApplicationController
 
       total_rows = notifications.dup.count
       notifications = notifications.offset(offset).limit(60)
-      notifications = filter_inaccessible_notifications(notifications)
+      notifications =
+        Notification.filter_inaccessible_topic_notifications(current_user.guardian, notifications)
       render_json_dump(
         notifications: serialize_data(notifications, NotificationSerializer),
         total_rows_notifications: total_rows,
@@ -155,10 +157,4 @@ class NotificationsController < ApplicationController
   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

app/controllers/users_controller.rb

@@ -1878,11 +1878,7 @@ class UsersController < ApplicationController
     end
 
     reminder_notifications =
-      Notification
+      BookmarkQuery.new(user: current_user).unread_notifications(limit: USER_MENU_LIST_LIMIT)
-        .for_user_menu(current_user.id, limit: USER_MENU_LIST_LIMIT)
-        .unread
-        .where(notification_type: Notification.types[:bookmark_reminder])
-
     if reminder_notifications.size < USER_MENU_LIST_LIMIT
       exclude_bookmark_ids =
         reminder_notifications.filter_map { |notification| notification.data_hash[:bookmark_id] }

app/models/notification.rb

@@ -227,6 +227,12 @@ class Notification < ActiveRecord::Base
     Notification.where(user_id: user_id, topic_id: topic_id).delete_all
   end
 
+  def self.filter_inaccessible_topic_notifications(guardian, 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
+
   # Be wary of calling this frequently. O(n) JSON parsing can suck.
   def data_hash
     @data_hash ||=

app/services/post_bookmarkable.rb

@@ -12,7 +12,7 @@ class PostBookmarkable < BaseBookmarkable
   end
 
   def self.preload_associations
-    [{ topic: [:tags] }, :user]
+    [{ topic: %i[tags category] }, :user]
   end
 
   def self.list_query(user, guardian)
@@ -54,7 +54,8 @@ class PostBookmarkable < BaseBookmarkable
   end
 
   def self.reminder_conditions(bookmark)
-    bookmark.bookmarkable.present? && bookmark.bookmarkable.topic.present?
+    bookmark.bookmarkable.present? && bookmark.bookmarkable.topic.present? &&
+      self.can_see?(bookmark.user.guardian, bookmark)
   end
 
   def self.can_see?(guardian, bookmark)

app/services/topic_bookmarkable.rb

@@ -12,7 +12,7 @@ class TopicBookmarkable < BaseBookmarkable
   end
 
   def self.preload_associations
-    [:tags, { first_post: :user }]
+    [:category, :tags, { first_post: :user }]
   end
 
   def self.perform_custom_preload!(topic_bookmarks, guardian)
@@ -58,7 +58,7 @@ class TopicBookmarkable < BaseBookmarkable
   end
 
   def self.reminder_conditions(bookmark)
-    bookmark.bookmarkable.present?
+    bookmark.bookmarkable.present? && self.can_see?(bookmark.user.guardian, bookmark)
   end
 
   def self.can_see?(guardian, bookmark)

lib/bookmark_query.rb

@@ -88,4 +88,28 @@ class BookmarkQuery
     BookmarkQuery.preload(results, self)
     results
   end
+
+  def unread_notifications(limit: 20)
+    reminder_notifications =
+      Notification
+        .for_user_menu(@user.id, limit: [limit, 100].min)
+        .unread
+        .where(notification_type: Notification.types[:bookmark_reminder])
+
+    # We preload associations like we do above for the list to avoid
+    # N1s in the can_see? guardian calls for each bookmark.
+    bookmarks =
+      Bookmark.where(
+        id: reminder_notifications.map { |n| n.data_hash[:bookmark_id] }.compact,
+        user: @user,
+      )
+    BookmarkQuery.preload(bookmarks, self)
+
+    reminder_notifications.select do |n|
+      bookmark = bookmarks.find { |bm| bm.id == n.data_hash[:bookmark_id] }
+      next if bookmark.blank?
+      bookmarkable = Bookmark.registered_bookmarkable_from_type(bookmark.bookmarkable_type)
+      bookmarkable.can_see?(@guardian, bookmark)
+    end
+  end
 end

plugins/chat/lib/chat/message_bookmarkable.rb

@@ -11,7 +11,7 @@ module Chat
     end
 
     def self.preload_associations
-      [:chat_channel]
+      [{ chat_channel: :chatable }]
     end
 
     def self.list_query(user, guardian)
@@ -58,7 +58,8 @@ module Chat
     end
 
     def self.reminder_conditions(bookmark)
-      bookmark.bookmarkable.present? && bookmark.bookmarkable.chat_channel.present?
+      bookmark.bookmarkable.present? && bookmark.bookmarkable.chat_channel.present? &&
+        self.can_see?(bookmark.user.guardian, bookmark)
     end
 
     def self.can_see?(guardian, bookmark)

plugins/chat/spec/lib/chat/message_bookmarkable_spec.rb

@@ -119,6 +119,13 @@ describe Chat::MessageBookmarkable do
       bookmark1.reload
       expect(registered_bookmarkable.can_send_reminder?(bookmark1)).to eq(false)
     end
+
+    it "cannot send reminder if the user cannot access the channel" do
+      expect(registered_bookmarkable.can_send_reminder?(bookmark1)).to eq(true)
+      bookmark1.bookmarkable.update!(chat_channel: Fabricate(:private_category_channel))
+      bookmark1.reload
+      expect(registered_bookmarkable.can_send_reminder?(bookmark1)).to eq(false)
+    end
   end
 
   describe "#reminder_handler" do

plugins/chat/spec/requests/core_ext/users_controller_spec.rb

@@ -22,4 +22,33 @@ describe UsersController do
       expect(membership.following).to eq(true)
     end
   end
+
+  describe "#user_menu_bookmarks" do
+    fab!(:chatters) { Fabricate(:group) }
+    let(:current_user) { Fabricate(:user, group_ids: [chatters.id]) }
+    let(:bookmark_message) { Fabricate(:chat_message) }
+    let(:bookmark_user) { current_user }
+
+    before do
+      register_test_bookmarkable(Chat::MessageBookmarkable)
+      SiteSetting.chat_allowed_groups = [chatters]
+      sign_in(current_user)
+    end
+
+    it "does not return any unread notifications for chat bookmarks that the user no longer has access to" do
+      bookmark_with_reminder =
+        Fabricate(:bookmark, user: current_user, bookmarkable: bookmark_message)
+      BookmarkReminderNotificationHandler.new(bookmark_with_reminder).send_notification
+
+      bookmark_with_reminder.bookmarkable.update!(
+        chat_channel: Fabricate(:private_category_channel),
+      )
+
+      get "/u/#{current_user.username}/user-menu-bookmarks"
+      expect(response.status).to eq(200)
+
+      notifications = response.parsed_body["notifications"]
+      expect(notifications.size).to eq(0)
+    end
+  end
 end

spec/lib/bookmark_query_spec.rb

@@ -105,6 +105,8 @@ RSpec.describe BookmarkQuery do
       context "with custom bookmarkable fitering" do
         before { register_test_bookmarkable }
 
+        after { DiscoursePluginRegistry.reset! }
+
         let!(:bookmark5) do
           Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:user, username: "bookmarkking"))
         end

spec/requests/users_controller_spec.rb

@@ -6922,8 +6922,7 @@ RSpec.describe UsersController do
         expect(response.status).to eq(200)
 
         notifications = response.parsed_body["notifications"]
-        expect(notifications.size).to eq(1)
+        expect(notifications.size).to eq(0)
-        expect(notifications.first["data"]["bookmark_id"]).to be_nil
 
         bookmarks = response.parsed_body["bookmarks"]
         expect(bookmarks.map { |bookmark| bookmark["id"] }).to contain_exactly(
@@ -6970,6 +6969,24 @@ RSpec.describe UsersController do
         bookmarks = response.parsed_body["bookmarks"]
         expect(bookmarks.size).to eq(1)
       end
+
+      it "does not return any unread notifications for bookmarks that the user no longer has access to" do
+        bookmark_with_reminder2 = Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:post))
+        TopicUser.change(user.id, bookmark_with_reminder2.bookmarkable.topic, total_msecs_viewed: 1)
+        BookmarkReminderNotificationHandler.new(bookmark_with_reminder2).send_notification
+
+        bookmark_with_reminder2.bookmarkable.topic.update!(
+          archetype: Archetype.private_message,
+          category: nil,
+        )
+
+        get "/u/#{user.username}/user-menu-bookmarks"
+        expect(response.status).to eq(200)
+
+        notifications = response.parsed_body["notifications"]
+        expect(notifications.size).to eq(1)
+        expect(notifications.first["data"]["bookmark_id"]).to eq(bookmark_with_reminder.id)
+      end
     end
   end
 

spec/services/post_bookmarkable_spec.rb

@@ -101,6 +101,13 @@ RSpec.describe PostBookmarkable do
       bookmark1.reload
       expect(registered_bookmarkable.can_send_reminder?(bookmark1)).to eq(false)
     end
+
+    it "cannot send reminder if the user cannot access the topic" do
+      expect(registered_bookmarkable.can_send_reminder?(bookmark1)).to eq(true)
+      bookmark1.bookmarkable.topic.update!(category: private_category)
+      bookmark1.reload
+      expect(registered_bookmarkable.can_send_reminder?(bookmark1)).to eq(false)
+    end
   end
 
   describe "#reminder_handler" do

spec/services/topic_bookmarkable_spec.rb

@@ -97,6 +97,13 @@ RSpec.describe TopicBookmarkable do
       bookmark1.reload
       expect(registered_bookmarkable.can_send_reminder?(bookmark1)).to eq(false)
     end
+
+    it "cannot send reminder if the user cannot access the topic" do
+      expect(registered_bookmarkable.can_send_reminder?(bookmark1)).to eq(true)
+      bookmark1.bookmarkable.update!(category: private_category)
+      bookmark1.reload
+      expect(registered_bookmarkable.can_send_reminder?(bookmark1)).to eq(false)
+    end
   end
 
   describe "#reminder_handler" do