Skip to content

Commit 9d5737f

Browse files
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.
1 parent 8dd6cb1 commit 9d5737f

File tree

2 files changed

+51
-22
lines changed

2 files changed

+51
-22
lines changed

Diff for: app/jobs/regular/export_user_archive.rb

+14-14
Original file line numberDiff line numberDiff line change
@@ -261,15 +261,16 @@ def category_preferences_export
261261

262262
CategoryUser
263263
.where(user_id: @current_user.id)
264-
.select(:category_id, :notification_level, :last_seen_at)
264+
.includes(:category)
265+
.merge(Category.secured(guardian))
265266
.each do |cu|
266-
yield [
267-
cu.category_id,
268-
piped_category_name(cu.category_id),
269-
NotificationLevels.all[cu.notification_level],
270-
cu.last_seen_at
271-
]
272-
end
267+
yield [
268+
cu.category_id,
269+
piped_category_name(cu.category_id, cu.category),
270+
NotificationLevels.all[cu.notification_level],
271+
cu.last_seen_at
272+
]
273+
end
273274
end
274275

275276
def flags_export
@@ -408,10 +409,9 @@ def guardian
408409
@guardian ||= Guardian.new(@current_user)
409410
end
410411

411-
def piped_category_name(category_id)
412-
return "-" unless category_id
413-
category = Category.find_by(id: category_id)
414-
return "#{category_id}" unless category
412+
def piped_category_name(category_id, category)
413+
return "#{category_id}" if category_id && !category
414+
return "-" if !guardian.can_see_category?(category)
415415
categories = [category.name]
416416
while category.parent_category_id && category = category.parent_category
417417
categories << category.name
@@ -433,10 +433,10 @@ def get_user_archive_fields(user_archive)
433433
user_archive_array = []
434434
topic_data = user_archive.topic
435435
user_archive = user_archive.as_json
436-
topic_data = Topic.with_deleted.find_by(id: user_archive['topic_id']) if topic_data.nil?
436+
topic_data = Topic.with_deleted.includes(:category).find_by(id: user_archive['topic_id']) if topic_data.nil?
437437
return user_archive_array if topic_data.nil?
438438

439-
categories = piped_category_name(topic_data.category_id)
439+
categories = piped_category_name(topic_data.category_id, topic_data.category)
440440
is_pm = topic_data.archetype == "private_message" ? I18n.t("csv_export.boolean_yes") : I18n.t("csv_export.boolean_no")
441441
url = "#{Discourse.base_url}/t/#{topic_data.slug}/#{topic_data.id}/#{user_archive['post_number']}"
442442

Diff for: spec/jobs/export_user_archive_spec.rb

+37-8
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
let(:component) { raise 'component not set' }
1616

1717
fab!(:admin) { Fabricate(:admin) }
18-
fab!(:category) { Fabricate(:category_with_definition) }
18+
fab!(:category) { Fabricate(:category_with_definition, name: "User Archive Category") }
1919
fab!(:subcategory) { Fabricate(:category_with_definition, parent_category_id: category.id) }
2020
fab!(:topic) { Fabricate(:topic, category: category) }
2121
let(:post) { Fabricate(:post, user: user, topic: topic) }
@@ -168,6 +168,17 @@ def make_component_json
168168
_, csv_out = make_component_csv
169169
expect(csv_out).to match cat2_id.to_s
170170
end
171+
172+
it "can export a post from a secure category, obscuring the category name" do
173+
cat2 = Fabricate(:private_category, group: Fabricate(:group), name: "Secret Cat")
174+
topic2 = Fabricate(:topic, category: cat2, user: user, title: "This is a test secure topic")
175+
_post2 = Fabricate(:post, topic: topic2, user: user)
176+
data, csv_out = make_component_csv
177+
expect(csv_out).not_to match "Secret Cat"
178+
expect(data.length).to eq(1)
179+
expect(data[0][:topic_title]).to eq("This is a test secure topic")
180+
expect(data[0][:categories]).to eq("-")
181+
end
171182
end
172183

173184
context 'preferences' do
@@ -314,9 +325,11 @@ def make_component_json
314325
context 'category_preferences' do
315326
let(:component) { 'category_preferences' }
316327

317-
let(:subsubcategory) { Fabricate(:category_with_definition, parent_category_id: subcategory.id) }
318-
let(:announcements) { Fabricate(:category_with_definition) }
319-
let(:deleted_category) { Fabricate(:category) }
328+
let(:subsubcategory) { Fabricate(:category_with_definition, parent_category_id: subcategory.id, name: "User Archive Subcategory") }
329+
let(:announcements) { Fabricate(:category_with_definition, name: "Announcements") }
330+
let(:deleted_category) { Fabricate(:category, name: "Deleted Category") }
331+
let(:secure_category_group) { Fabricate(:group) }
332+
let(:secure_category) { Fabricate(:private_category, group: secure_category_group, name: "Super Secret Category") }
320333

321334
let(:reset_at) { DateTime.parse('2017-03-01 12:00') }
322335

@@ -341,11 +354,12 @@ def make_component_json
341354
deleted_category.destroy!
342355
end
343356

344-
it 'correctly exports the CategoryUser table' do
357+
it 'correctly exports the CategoryUser table, excluding deleted categories' do
345358
data, _csv_out = make_component_csv
346359

347-
expect(data.find { |r| r['category_id'] == category.id }).to be_nil
348-
expect(data.length).to eq(4)
360+
expect(data.find { |r| r['category_id'] == category.id.to_s }).to be_nil
361+
expect(data.find { |r| r['category_id'] == deleted_category.id.to_s }).to be_nil
362+
expect(data.length).to eq(3)
349363
data.sort! { |a, b| a['category_id'].to_i <=> b['category_id'].to_i }
350364

351365
expect(data[0][:category_id]).to eq(subcategory.id.to_s)
@@ -361,8 +375,23 @@ def make_component_json
361375
expect(data[2][:category_names]).to eq(announcements.name)
362376
expect(data[2][:notification_level]).to eq('watching_first_post')
363377
expect(data[2][:dismiss_new_timestamp]).to eq('')
378+
end
364379

365-
expect(data[3][:category_names]).to eq(data[3][:category_id])
380+
it "does not include any secure categories the user does not have access to, even if the user has a CategoryUser record" do
381+
CategoryUser.set_notification_level_for_category(user, NotificationLevels.all[:muted], secure_category.id)
382+
data, _csv_out = make_component_csv
383+
384+
expect(data.any? { |r| r['category_id'] == secure_category.id.to_s }).to eq(false)
385+
expect(data.length).to eq(3)
386+
end
387+
388+
it "does include secure categories that the user has access to" do
389+
CategoryUser.set_notification_level_for_category(user, NotificationLevels.all[:muted], secure_category.id)
390+
GroupUser.create!(user: user, group: secure_category_group)
391+
data, _csv_out = make_component_csv
392+
393+
expect(data.any? { |r| r['category_id'] == secure_category.id.to_s }).to eq(true)
394+
expect(data.length).to eq(4)
366395
end
367396
end
368397

0 commit comments

Comments
 (0)