From 3c5fb871c0f54af47679ae71ad449666b01d8216 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Fri, 13 Oct 2023 11:41:10 +1000 Subject: [PATCH] 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 | 12 +++----- app/controllers/users_controller.rb | 6 +--- app/models/notification.rb | 6 ++++ app/services/post_bookmarkable.rb | 5 ++-- app/services/topic_bookmarkable.rb | 4 +-- lib/bookmark_query.rb | 24 +++++++++++++++ plugins/chat/lib/chat/message_bookmarkable.rb | 5 ++-- .../lib/chat/message_bookmarkable_spec.rb | 7 +++++ .../core_ext/users_controller_spec.rb | 29 +++++++++++++++++++ spec/lib/bookmark_query_spec.rb | 2 ++ spec/requests/users_controller_spec.rb | 21 ++++++++++++-- spec/services/post_bookmarkable_spec.rb | 7 +++++ spec/services/topic_bookmarkable_spec.rb | 7 +++++ 13 files changed, 114 insertions(+), 21 deletions(-) diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb index 75cd9c60b79..ecf965cf256 100644 --- a/app/controllers/notifications_controller.rb +++ b/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 diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 968911f2a01..238ec7d9639 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1878,11 +1878,7 @@ class UsersController < ApplicationController end reminder_notifications = - Notification - .for_user_menu(current_user.id, limit: USER_MENU_LIST_LIMIT) - .unread - .where(notification_type: Notification.types[:bookmark_reminder]) - + BookmarkQuery.new(user: current_user).unread_notifications(limit: USER_MENU_LIST_LIMIT) if reminder_notifications.size < USER_MENU_LIST_LIMIT exclude_bookmark_ids = reminder_notifications.filter_map { |notification| notification.data_hash[:bookmark_id] } diff --git a/app/models/notification.rb b/app/models/notification.rb index 3673a3798f8..91c506a28dd 100644 --- a/app/models/notification.rb +++ b/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 ||= diff --git a/app/services/post_bookmarkable.rb b/app/services/post_bookmarkable.rb index 97086d9ba40..4769b4224f2 100644 --- a/app/services/post_bookmarkable.rb +++ b/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) diff --git a/app/services/topic_bookmarkable.rb b/app/services/topic_bookmarkable.rb index f79af4c07c4..02e092584a9 100644 --- a/app/services/topic_bookmarkable.rb +++ b/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) diff --git a/lib/bookmark_query.rb b/lib/bookmark_query.rb index 7624f011369..dd066f6bd2a 100644 --- a/lib/bookmark_query.rb +++ b/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 diff --git a/plugins/chat/lib/chat/message_bookmarkable.rb b/plugins/chat/lib/chat/message_bookmarkable.rb index a759aef1209..4a9b2025679 100644 --- a/plugins/chat/lib/chat/message_bookmarkable.rb +++ b/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) diff --git a/plugins/chat/spec/lib/chat/message_bookmarkable_spec.rb b/plugins/chat/spec/lib/chat/message_bookmarkable_spec.rb index f5d050dcf77..d2030004322 100644 --- a/plugins/chat/spec/lib/chat/message_bookmarkable_spec.rb +++ b/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 diff --git a/plugins/chat/spec/requests/core_ext/users_controller_spec.rb b/plugins/chat/spec/requests/core_ext/users_controller_spec.rb index 8e9d060589d..b7ff5445fd2 100644 --- a/plugins/chat/spec/requests/core_ext/users_controller_spec.rb +++ b/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 diff --git a/spec/lib/bookmark_query_spec.rb b/spec/lib/bookmark_query_spec.rb index d54c10d07d2..3bac9c55ca3 100644 --- a/spec/lib/bookmark_query_spec.rb +++ b/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 diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index caa11464358..60a40f49fed 100644 --- a/spec/requests/users_controller_spec.rb +++ b/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.first["data"]["bookmark_id"]).to be_nil + expect(notifications.size).to eq(0) 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 diff --git a/spec/services/post_bookmarkable_spec.rb b/spec/services/post_bookmarkable_spec.rb index dbd1d8855ca..234a80eb193 100644 --- a/spec/services/post_bookmarkable_spec.rb +++ b/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 diff --git a/spec/services/topic_bookmarkable_spec.rb b/spec/services/topic_bookmarkable_spec.rb index b5aa7685974..594bc6a1182 100644 --- a/spec/services/topic_bookmarkable_spec.rb +++ b/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