CVE-2022-41944: Incorrect Authorization (CWE-863)
value: Discourse is an open-source discussion platform. In stable versions prior to 2.8.12 and beta or tests-passed versions prior to 2.9.0.beta.13, under certain conditions, a user can see notifications for topics they no longer have access to. If there is sensitive information in the topic title, it will therefore have been exposed. This issue is patched in stable version 2.8.12, beta version 2.9.0.beta13, and tests-passed version 2.9.0.beta13. There are no workarounds available.
CWE description: The product performs an authorization check when an actor attempts to access a resource or perform an action, but it does not correctly perform the check. This allows attackers to bypass intended access restrictions.
CVSS v3 base score: 4.3
Published: 2022-11-28T15:15Z
Repository: https://github.com/discourse/discourse
Language: Ruby
Files changed: app/controllers/notifications_controller.rb, spec/requests/notifications_controller_spec.rb
Commit date: 2022-11-28 10:41:57 +1000
Commit message: SECURITY: Hide notifications for inaccessible topics (#19208)

Filter notifications the user cannot see anymore
via guardian.can_see_topic_ids
Diff: diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb
index 5566dd488e3d..ca7e548a70d3 100644
--- a/app/controllers/notifications_controller.rb
+++ b/app/controllers/notifications_controller.rb
@@ -38,11 +38,11 @@ class NotificationsController < ApplicationController
       else
         notifications = Notification.recent_report(current_user, limit, notification_types)
       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
       end
 
@@ -55,10 +55,12 @@ class NotificationsController < ApplicationController
           # block is executed
           User.find_by(id: current_user_id)&.bump_last_seen_reviewable!
         end
       end
 
+      notifications = filter_inaccessible_notifications(notifications)
+
       json = {
         notifications: serialize_data(notifications, NotificationSerializer),
         seen_notification_id: current_user.seen_notification_id
       }
       if include_reviewables
@@ -80,10 +82,11 @@ class NotificationsController < ApplicationController
 
       notifications = notifications.where(read: false) if params[:filter] == "unread"
 
       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,
                        load_more_notifications: notifications_path(username: user.username, offset: offset + 60, filter: params[:filter]))
     end
@@ -143,6 +146,11 @@ 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
diff --git a/spec/requests/notifications_controller_spec.rb b/spec/requests/notifications_controller_spec.rb
index f263fef37da7..a087bb2f60bf 100644
--- a/spec/requests/notifications_controller_spec.rb
+++ b/spec/requests/notifications_controller_spec.rb
@@ -337,10 +337,54 @@ RSpec.describe NotificationsController do
           it 'should raise the right error' do
             get "/notifications.json", params: { username: 'somedude' }
             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
         put "/notifications/mark-read.json"
         expect(response.status).to eq(200)

Vulnerable code:         if changed = current_user.bump_last_seen_notification!
Fixed code:         if current_user.bump_last_seen_notification!
      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

        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