Skip to content
Permalink
Browse files Browse the repository at this point in the history
SECURITY: Restrict unlisted topic creation (#19259)
  • Loading branch information
s3lase committed Dec 1, 2022
1 parent 9513e7b commit 0ce38bd
Show file tree
Hide file tree
Showing 6 changed files with 128 additions and 1 deletion.
1 change: 1 addition & 0 deletions config/locales/server.en.yml
Expand Up @@ -589,6 +589,7 @@ en:
target_user_not_found: "One of the users you are sending this message to could not be found."
unable_to_update: "There was an error updating that topic."
unable_to_tag: "There was an error tagging the topic."
unable_to_unlist: "Sorry, you cannot create an unlisted topic."
featured_link:
invalid: "is invalid. URL should include http:// or https://."
user:
Expand Down
2 changes: 2 additions & 0 deletions lib/guardian/topic_guardian.rb
Expand Up @@ -181,6 +181,8 @@ def can_toggle_topic_visibility?(topic)
can_moderate?(topic) || can_perform_action_available_to_group_moderators?(topic)
end

alias :can_create_unlisted_topic? :can_toggle_topic_visibility?

def can_convert_topic?(topic)
return false unless @user.in_any_groups?(SiteSetting.personal_message_enabled_groups_map)
return false if topic.blank?
Expand Down
17 changes: 16 additions & 1 deletion lib/topic_creator.rb
Expand Up @@ -24,6 +24,8 @@ def valid?
# this allows us to add errors
valid = topic.valid?

validate_visibility(topic)

category = find_category
if category.present? && guardian.can_tag?(topic)
tags = @opts[:tags].presence || []
Expand All @@ -46,6 +48,8 @@ def valid?

def create
topic = Topic.new(setup_topic_params)

validate_visibility!(topic)
setup_tags(topic)

if fields = @opts[:custom_fields]
Expand All @@ -67,6 +71,18 @@ def create

private

def validate_visibility(topic)
if !@opts[:skip_validations] && !topic.visible && !guardian.can_create_unlisted_topic?(topic)
topic.errors.add(:base, :unable_to_unlist)
end
end

def validate_visibility!(topic)
validate_visibility(topic)

rollback_from_errors!(topic) if topic.errors.full_messages.present?
end

def create_shared_draft(topic)
return if @opts[:shared_draft].blank? || @opts[:shared_draft] == 'false'

Expand Down Expand Up @@ -302,5 +318,4 @@ def find_or_create_user(email, display_name)

user
end

end
16 changes: 16 additions & 0 deletions spec/lib/guardian/topic_guardian_spec.rb
Expand Up @@ -132,6 +132,22 @@
end
end

describe "#can_create_unlisted_topic?" do
it "returns true for moderators" do
expect(Guardian.new(moderator).can_create_unlisted_topic?(topic)).to eq(true)
end

it "returns true for TL4 users" do
tl4_user = Fabricate(:user, trust_level: TrustLevel[4])

expect(Guardian.new(tl4_user).can_create_unlisted_topic?(topic)).to eq(true)
end

it "returns false for regular users" do
expect(Guardian.new(user).can_create_unlisted_topic?(topic)).to eq(false)
end
end

# The test cases here are intentionally kept brief because majority of the cases are already handled by
# `TopicGuardianCanSeeConsistencyCheck` which we run to ensure that the implementation between `TopicGuardian#can_see_topic_ids`
# and `TopicGuardian#can_see_topic?` is consistent.
Expand Down
22 changes: 22 additions & 0 deletions spec/lib/topic_creator_spec.rb
Expand Up @@ -518,5 +518,27 @@
expect(topic.external_id).to eq('external_id')
end
end

context "when invisible/unlisted" do
let(:unlisted_attrs) { valid_attrs.merge(visible: false) }

it "throws an exception for a non-staff user" do
expect do
TopicCreator.create(user, Guardian.new(user), unlisted_attrs)
end.to raise_error(ActiveRecord::Rollback)
end

it "is invalid for a non-staff user" do
expect(TopicCreator.new(user, Guardian.new(user), unlisted_attrs).valid?).to eq(false)
end

it "creates unlisted topic for an admin" do
expect(TopicCreator.create(admin, Guardian.new(admin), unlisted_attrs)).to be_valid
end

it "is valid for an admin" do
expect(TopicCreator.new(admin, Guardian.new(admin), unlisted_attrs).valid?).to eq(true)
end
end
end
end
71 changes: 71 additions & 0 deletions spec/requests/posts_controller_spec.rb
Expand Up @@ -923,6 +923,34 @@

expect(response.status).to eq(422)
end

it "creates unlisted topic with admin master key" do
master_key = Fabricate(:api_key).key

expect do
post "/posts.json",
params: { raw: "this is a test title", title: "this is test body", unlist_topic: true },
headers: { HTTP_API_USERNAME: admin.username, HTTP_API_KEY: master_key }
end.to change { Topic.count }.by(1)

expect(response.status).to eq(200)
expect(Topic.find(response.parsed_body["topic_id"]).visible).to eq(false)
end

it "prevents creation of unlisted topic with non-admin key" do
user_key = ApiKey.create!(user: user).key

expect do
post "/posts.json",
params: { raw: "this is a test title", title: "this is test body", unlist_topic: true },
headers: { HTTP_API_USERNAME: user.username, HTTP_API_KEY: user_key }
end.not_to change { Topic.count }

expect(response.status).to eq(422)
expect(response.parsed_body["errors"]).to include(
I18n.t("activerecord.errors.models.topic.attributes.base.unable_to_unlist")
)
end
end

describe "when logged in" do
Expand Down Expand Up @@ -1250,6 +1278,7 @@
expect(topic.title).to eq('This is the test title for the topic')
expect(topic.category).to eq(category)
expect(topic.meta_data).to eq("xyz" => 'abc')
expect(topic.visible).to eq(true)
end

it 'can create an uncategorized topic' do
Expand Down Expand Up @@ -1411,6 +1440,48 @@
end
end

context "with topic unlisting" do
context "when logged in as staff" do
before do
sign_in(admin)
end

it "creates an unlisted topic" do
expect do
post "/posts.json", params: {
raw: "this is the test content",
title: "this is the test title for the topic",
unlist_topic: true
}
end.to change { Topic.count }.by(1)

expect(response.status).to eq(200)
expect(Topic.find(response.parsed_body["topic_id"]).visible).to eq(false)
end
end

context "when logged in as a non-staff user" do
before do
sign_in(user)
end

it "prevents creation of an unlisted topic" do
expect do
post "/posts.json", params: {
raw: "this is the test content",
title: "this is the test title for the topic",
unlist_topic: true
}
end.not_to change { Topic.count }

expect(response.status).to eq(422)
expect(response.parsed_body["errors"]).to include(
I18n.t("activerecord.errors.models.topic.attributes.base.unable_to_unlist")
)
end
end
end

describe 'shared draft' do
fab!(:destination_category) { Fabricate(:category) }

Expand Down

0 comments on commit 0ce38bd

Please sign in to comment.