From e7f83358aa86970c726db8545a606ad6753db187 Mon Sep 17 00:00:00 2001 From: Sam Date: Fri, 2 Mar 2018 12:13:04 +1100 Subject: [PATCH] SECURITY: ensure users have permission when moving categories --- app/controllers/posts_controller.rb | 9 ++++++ app/controllers/topics_controller.rb | 9 ++++++ config/locales/server.en.yml | 1 + lib/guardian/topic_guardian.rb | 6 +++- lib/post_revisor.rb | 6 ++-- spec/components/post_revisor_spec.rb | 26 ++++++++++++++++ spec/controllers/topics_controller_spec.rb | 24 +++------------ spec/requests/posts_controller_spec.rb | 36 ++++++++++++++++++++++ spec/requests/topics_controller_spec.rb | 19 ++++++++++++ 9 files changed, 113 insertions(+), 23 deletions(-) diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index a091e7e69d0..12771276852 100644 --- a/app/controllers/posts_controller.rb +++ b/app/controllers/posts_controller.rb @@ -178,6 +178,15 @@ class PostsController < ApplicationController if post.is_first_post? changes[:title] = params[:title] if params[:title] changes[:category_id] = params[:post][:category_id] if params[:post][:category_id] + + if changes[:category_id] && changes[:category_id].to_i != post.topic.category_id.to_i + category = Category.find_by(id: changes[:category_id]) + if category || (changes[:category_id].to_i == 0) + guardian.ensure_can_create_topic_on_category!(category) + else + return render_json_error(I18n.t('category.errors.not_found')) + end + end end # We don't need to validate edits to small action posts by staff diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index fa241cb5435..8e31c28ed30 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -225,6 +225,15 @@ class TopicsController < ApplicationController topic = Topic.find_by(id: params[:topic_id]) guardian.ensure_can_edit!(topic) + if params[:category_id] && (params[:category_id].to_i != topic.category_id.to_i) + category = Category.find_by(id: params[:category_id]) + if category || (params[:category_id].to_i == 0) + guardian.ensure_can_create_topic_on_category!(category) + else + return render_json_error(I18n.t('category.errors.not_found')) + end + end + changes = {} PostRevisor.tracked_topic_fields.each_key do |f| changes[f] = params[f] if params.has_key?(f) diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 0db6ff6c9a5..16df686b6cb 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -495,6 +495,7 @@ en: replace_paragraph: "(Replace this first paragraph with a brief description of your new category. This guidance will appear in the category selection area, so try to keep it below 200 characters. **Until you edit this description or create topics, this category won't appear on the categories page.**)" post_template: "%{replace_paragraph}\n\nUse the following paragraphs for a longer description, or to establish category guidelines or rules:\n\n- Why should people use this category? What is it for?\n\n- How exactly is this different than the other categories we already have?\n\n- What should topics in this category generally contain?\n\n- Do we need this category? Can we merge with another category, or subcategory?\n" errors: + not_found: "Category not found!" uncategorized_parent: "Uncategorized can't have a parent category" self_parent: "A subcategory's parent cannot be itself" depth: "You can't nest a subcategory under another" diff --git a/lib/guardian/topic_guardian.rb b/lib/guardian/topic_guardian.rb index 1bf07ef82ca..bf451ec2baf 100644 --- a/lib/guardian/topic_guardian.rb +++ b/lib/guardian/topic_guardian.rb @@ -19,8 +19,12 @@ module TopicGuardian end def can_create_topic_on_category?(category) + # allow for category to be a number as well + category_id = category + category_id = category.id if Category === category + can_create_topic?(nil) && - (!category || Category.topic_create_allowed(self).where(id: category.id).count == 1) + (!category || Category.topic_create_allowed(self).where(id: category_id).count == 1) end def can_create_post_on_topic?(topic) diff --git a/lib/post_revisor.rb b/lib/post_revisor.rb index a6b1623e8d1..6ae10376f66 100644 --- a/lib/post_revisor.rb +++ b/lib/post_revisor.rb @@ -68,8 +68,10 @@ class PostRevisor end track_topic_field(:category_id) do |tc, category_id| - tc.record_change('category_id', tc.topic.category_id, category_id) - tc.check_result(tc.topic.change_category_to_id(category_id)) + if tc.guardian.can_create_topic_on_category?(category_id) + tc.record_change('category_id', tc.topic.category_id, category_id) + tc.check_result(tc.topic.change_category_to_id(category_id)) + end end track_topic_field(:tags) do |tc, tags| diff --git a/spec/components/post_revisor_spec.rb b/spec/components/post_revisor_spec.rb index 7f0f950add2..8abeb897bee 100644 --- a/spec/components/post_revisor_spec.rb +++ b/spec/components/post_revisor_spec.rb @@ -38,6 +38,32 @@ describe PostRevisor do end end + context 'editing category' do + + it 'does not revise category when no permission to create a topic in category' do + category = Fabricate(:category) + category.set_permissions(staff: :full) + category.save! + + post = create_post + old_id = post.topic.category_id + + post.revise(post.user, category_id: category.id) + + post.reload + expect(post.topic.category_id).to eq(old_id) + + category.set_permissions(everyone: :full) + category.save! + + post.revise(post.user, category_id: category.id) + + post.reload + expect(post.topic.category_id).to eq(category.id) + end + + end + context 'revise wiki' do before do diff --git a/spec/controllers/topics_controller_spec.rb b/spec/controllers/topics_controller_spec.rb index f24e0811ede..12c7537cf07 100644 --- a/spec/controllers/topics_controller_spec.rb +++ b/spec/controllers/topics_controller_spec.rb @@ -1140,22 +1140,6 @@ describe TopicsController do expect(@topic.title).to eq('This is a new title for the topic') end - it 'triggers a change of category' do - Topic.any_instance.expects(:change_category_to_id).with(123).returns(true) - put :update, params: { - topic_id: @topic.id, slug: @topic.title, category_id: 123 - }, format: :json - - end - - it 'allows to change category to "uncategorized"' do - Topic.any_instance.expects(:change_category_to_id).with(0).returns(true) - put :update, params: { - topic_id: @topic.id, slug: @topic.title, category_id: "" - }, format: :json - - end - it "returns errors with invalid titles" do put :update, params: { topic_id: @topic.id, slug: @topic.title, title: 'asdf' @@ -1174,7 +1158,6 @@ describe TopicsController do end it "returns errors with invalid categories" do - Topic.any_instance.expects(:change_category_to_id).returns(false) put :update, params: { topic_id: @topic.id, slug: @topic.title, category_id: -1 }, format: :json @@ -1201,8 +1184,9 @@ describe TopicsController do context 'when there are no changes' do it 'does not call the PostRevisor' do PostRevisor.any_instance.expects(:revise!).never + put :update, params: { - topic_id: @topic.id, slug: @topic.title, title: @topic.title, category_id: nil + topic_id: @topic.id, slug: @topic.title, title: @topic.title, category_id: @topic.category_id }, format: :json expect(response).to be_success @@ -1216,10 +1200,10 @@ describe TopicsController do end it "can add a category to an uncategorized topic" do - Topic.any_instance.expects(:change_category_to_id).with(456).returns(true) + c = Fabricate(:category) put :update, params: { - topic_id: @topic.id, slug: @topic.title, category_id: 456 + topic_id: @topic.id, slug: @topic.title, category_id: c.id }, format: :json expect(response).to be_success diff --git a/spec/requests/posts_controller_spec.rb b/spec/requests/posts_controller_spec.rb index 62256403bf0..95496792285 100644 --- a/spec/requests/posts_controller_spec.rb +++ b/spec/requests/posts_controller_spec.rb @@ -12,11 +12,47 @@ RSpec.describe PostsController do let(:private_post) { Fabricate(:post, user: user, topic: private_topic) } + describe '#update' do + + it 'can not change category to a disallowed category' do + post = create_post + sign_in(post.user) + + category = Fabricate(:category) + category.set_permissions(staff: :full) + category.save! + + # strange API, why is topic id in here twice + put "/posts/#{post.id}.json", params: { + post: { category_id: category.id, raw: "this is a test edit to post" } + } + + expect(response.status).not_to eq(200) + expect(post.topic.category_id).not_to eq(category.id) + end + + end + describe '#create' do before do sign_in(user) end + it 'can not create a post in a disallowed category' do + + category.set_permissions(staff: :full) + category.save! + + post "/posts.json", params: { + raw: 'this is the test content', + title: 'this is the test title for the topic', + category: category.id, + meta_data: { xyz: 'abc' } + } + + expect(response.status).to eq(403) + end + it 'creates the post' do post "/posts.json", params: { raw: 'this is the test content', diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index 155fee81d9e..45ad9c2e2ef 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -4,6 +4,25 @@ RSpec.describe TopicsController do let(:topic) { Fabricate(:topic) } let(:user) { Fabricate(:user) } + describe '#update' do + + it 'can not change category to a disallowed category' do + post = create_post + sign_in(post.user) + + category = Fabricate(:category) + category.set_permissions(staff: :full) + category.save! + + # strange API, why is topic id in here twice + put "/t/#{post.topic_id}.json", params: { category_id: category.id, topic_id: post.topic_id } + expect(response.status).not_to eq(200) + + expect(post.topic.category_id).not_to eq(category.id) + end + + end + describe '#show' do let(:private_topic) { Fabricate(:private_message_topic) }