SECURITY: Hide private categories in user activity export (#16273)

In some of the user's own activity export data,
we sometimes showed a secure category's name or
exposed the existence of a secure category.
This commit is contained in:
Martin Brennan 2022-03-24 15:38:44 +10:00 committed by GitHub
parent 8dd6cb14ee
commit 9d5737fd28
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 51 additions and 22 deletions

View File

@ -261,15 +261,16 @@ module Jobs
CategoryUser
.where(user_id: @current_user.id)
.select(:category_id, :notification_level, :last_seen_at)
.includes(:category)
.merge(Category.secured(guardian))
.each do |cu|
yield [
cu.category_id,
piped_category_name(cu.category_id),
NotificationLevels.all[cu.notification_level],
cu.last_seen_at
]
end
yield [
cu.category_id,
piped_category_name(cu.category_id, cu.category),
NotificationLevels.all[cu.notification_level],
cu.last_seen_at
]
end
end
def flags_export
@ -408,10 +409,9 @@ module Jobs
@guardian ||= Guardian.new(@current_user)
end
def piped_category_name(category_id)
return "-" unless category_id
category = Category.find_by(id: category_id)
return "#{category_id}" unless category
def piped_category_name(category_id, category)
return "#{category_id}" if category_id && !category
return "-" if !guardian.can_see_category?(category)
categories = [category.name]
while category.parent_category_id && category = category.parent_category
categories << category.name
@ -433,10 +433,10 @@ module Jobs
user_archive_array = []
topic_data = user_archive.topic
user_archive = user_archive.as_json
topic_data = Topic.with_deleted.find_by(id: user_archive['topic_id']) if topic_data.nil?
topic_data = Topic.with_deleted.includes(:category).find_by(id: user_archive['topic_id']) if topic_data.nil?
return user_archive_array if topic_data.nil?
categories = piped_category_name(topic_data.category_id)
categories = piped_category_name(topic_data.category_id, topic_data.category)
is_pm = topic_data.archetype == "private_message" ? I18n.t("csv_export.boolean_yes") : I18n.t("csv_export.boolean_no")
url = "#{Discourse.base_url}/t/#{topic_data.slug}/#{topic_data.id}/#{user_archive['post_number']}"

View File

@ -15,7 +15,7 @@ describe Jobs::ExportUserArchive do
let(:component) { raise 'component not set' }
fab!(:admin) { Fabricate(:admin) }
fab!(:category) { Fabricate(:category_with_definition) }
fab!(:category) { Fabricate(:category_with_definition, name: "User Archive Category") }
fab!(:subcategory) { Fabricate(:category_with_definition, parent_category_id: category.id) }
fab!(:topic) { Fabricate(:topic, category: category) }
let(:post) { Fabricate(:post, user: user, topic: topic) }
@ -168,6 +168,17 @@ describe Jobs::ExportUserArchive do
_, csv_out = make_component_csv
expect(csv_out).to match cat2_id.to_s
end
it "can export a post from a secure category, obscuring the category name" do
cat2 = Fabricate(:private_category, group: Fabricate(:group), name: "Secret Cat")
topic2 = Fabricate(:topic, category: cat2, user: user, title: "This is a test secure topic")
_post2 = Fabricate(:post, topic: topic2, user: user)
data, csv_out = make_component_csv
expect(csv_out).not_to match "Secret Cat"
expect(data.length).to eq(1)
expect(data[0][:topic_title]).to eq("This is a test secure topic")
expect(data[0][:categories]).to eq("-")
end
end
context 'preferences' do
@ -314,9 +325,11 @@ describe Jobs::ExportUserArchive do
context 'category_preferences' do
let(:component) { 'category_preferences' }
let(:subsubcategory) { Fabricate(:category_with_definition, parent_category_id: subcategory.id) }
let(:announcements) { Fabricate(:category_with_definition) }
let(:deleted_category) { Fabricate(:category) }
let(:subsubcategory) { Fabricate(:category_with_definition, parent_category_id: subcategory.id, name: "User Archive Subcategory") }
let(:announcements) { Fabricate(:category_with_definition, name: "Announcements") }
let(:deleted_category) { Fabricate(:category, name: "Deleted Category") }
let(:secure_category_group) { Fabricate(:group) }
let(:secure_category) { Fabricate(:private_category, group: secure_category_group, name: "Super Secret Category") }
let(:reset_at) { DateTime.parse('2017-03-01 12:00') }
@ -341,11 +354,12 @@ describe Jobs::ExportUserArchive do
deleted_category.destroy!
end
it 'correctly exports the CategoryUser table' do
it 'correctly exports the CategoryUser table, excluding deleted categories' do
data, _csv_out = make_component_csv
expect(data.find { |r| r['category_id'] == category.id }).to be_nil
expect(data.length).to eq(4)
expect(data.find { |r| r['category_id'] == category.id.to_s }).to be_nil
expect(data.find { |r| r['category_id'] == deleted_category.id.to_s }).to be_nil
expect(data.length).to eq(3)
data.sort! { |a, b| a['category_id'].to_i <=> b['category_id'].to_i }
expect(data[0][:category_id]).to eq(subcategory.id.to_s)
@ -361,8 +375,23 @@ describe Jobs::ExportUserArchive do
expect(data[2][:category_names]).to eq(announcements.name)
expect(data[2][:notification_level]).to eq('watching_first_post')
expect(data[2][:dismiss_new_timestamp]).to eq('')
end
expect(data[3][:category_names]).to eq(data[3][:category_id])
it "does not include any secure categories the user does not have access to, even if the user has a CategoryUser record" do
CategoryUser.set_notification_level_for_category(user, NotificationLevels.all[:muted], secure_category.id)
data, _csv_out = make_component_csv
expect(data.any? { |r| r['category_id'] == secure_category.id.to_s }).to eq(false)
expect(data.length).to eq(3)
end
it "does include secure categories that the user has access to" do
CategoryUser.set_notification_level_for_category(user, NotificationLevels.all[:muted], secure_category.id)
GroupUser.create!(user: user, group: secure_category_group)
data, _csv_out = make_component_csv
expect(data.any? { |r| r['category_id'] == secure_category.id.to_s }).to eq(true)
expect(data.length).to eq(4)
end
end