CVE-2023-45816 (Ruby, published 2023-11-10T15:15Z)
Description: value: Discourse is an open source platform for community discussion. Prior to version 3.1.3 of the `stable` branch and version 3.2.0.beta3 of the `beta` and `tests-passed` branches, there is an edge case where a bookmark reminder is sent and an unread notification is generated, but the underlying bookmarkable (e.g. post, topic, chat message) security has changed, making it so the user can no longer access the underlying resource. As of version 3.1.3 of the `stable` branch and version 3.2.0.beta3 of the `beta` and `tests-passed` branches, bookmark reminders are now no longer sent if the user does not have access to the underlying bookmarkable, and also the unread bookmark notifications are always filtered by access. There are no known workarounds.
Severity: nan (CVSS v3 base score: 3.3)
Weakness: CWE-200 \u2014 Exposure of Sensitive Information to an Unauthorized Actor
The product exposes sensitive information to an actor that is not explicitly authorized to have access to that information.
Repository: https://github.com/discourse/discourse
Files changed:
app/controllers/notifications_controller.rb
app/controllers/users_controller.rb
app/models/notification.rb
app/services/post_bookmarkable.rb
app/services/topic_bookmarkable.rb
lib/bookmark_query.rb
plugins/chat/lib/chat/message_bookmarkable.rb
plugins/chat/spec/lib/chat/message_bookmarkable_spec.rb
plugins/chat/spec/requests/core_ext/users_controller_spec.rb
spec/lib/bookmark_query_spec.rb
spec/requests/users_controller_spec.rb
spec/services/post_bookmarkable_spec.rb
spec/services/topic_bookmarkable_spec.rb
Commit message: 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.


Vulnerable code:
      notifications = filter_inaccessible_notifications(notifications)
      notifications = filter_inaccessible_notifications(notifications)

  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
      Notification
        .for_user_menu(current_user.id, limit: USER_MENU_LIST_LIMIT)
        .unread
        .where(notification_type: Notification.types[:bookmark_reminder])

    [{ topic: [:tags] }, :user]
    bookmark.bookmarkable.present? && bookmark.bookmarkable.topic.present?
    [:tags, { first_post: :user }]
    bookmark.bookmarkable.present?
      [:chat_channel]
      bookmark.bookmarkable.present? && bookmark.bookmarkable.chat_channel.present?
        expect(notifications.size).to eq(1)
        expect(notifications.first["data"]["bookmark_id"]).to be_nil

Fixed code:
      notifications =
        Notification.filter_inaccessible_topic_notifications(current_user.guardian, notifications)
      notifications =
        Notification.filter_inaccessible_topic_notifications(current_user.guardian, notifications)
      BookmarkQuery.new(user: current_user).unread_notifications(limit: USER_MENU_LIST_LIMIT)
  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

    [{ topic: %i[tags category] }, :user]
    bookmark.bookmarkable.present? && bookmark.bookmarkable.topic.present? &&
      self.can_see?(bookmark.user.guardian, bookmark)
    [:category, :tags, { first_post: :user }]
    bookmark.bookmarkable.present? && self.can_see?(bookmark.user.guardian, bookmark)

  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
      [{ chat_channel: :chatable }]
      bookmark.bookmarkable.present? && bookmark.bookmarkable.chat_channel.present? &&
        self.can_see?(bookmark.user.guardian, bookmark)

    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

  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
        after { DiscoursePluginRegistry.reset! }

        expect(notifications.size).to eq(0)

      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

    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

    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