From b61e10f9ade8233dd3740cef7e765f3c182b7130 Mon Sep 17 00:00:00 2001 From: Ian Christian Myers Date: Fri, 7 Jun 2013 00:52:03 -0700 Subject: [PATCH] All parameters for #create in PostsController pass through strong_parameters. We are now explicitly whitelisting all parameters for Post creation. A nice side-effect is that it cleans up the #create action in PostsController. We can now trust that all parameters entering PostCreator are of a safe scalar type. --- .../javascripts/discourse/models/post.js | 5 +- app/controllers/posts_controller.rb | 33 ++++++++----- lib/post_creator.rb | 2 - spec/components/post_creator_spec.rb | 4 -- spec/controllers/posts_controller_spec.rb | 48 +++++++++---------- 5 files changed, 48 insertions(+), 44 deletions(-) diff --git a/app/assets/javascripts/discourse/models/post.js b/app/assets/javascripts/discourse/models/post.js index ffbc2ce9cfc..60a34fe1a09 100644 --- a/app/assets/javascripts/discourse/models/post.js +++ b/app/assets/javascripts/discourse/models/post.js @@ -162,7 +162,10 @@ Discourse.Post = Discourse.Model.extend({ // We're saving a post data = { - post: this.getProperties('raw', 'topic_id', 'reply_to_post_number', 'category'), + raw: this.get('raw'), + topic_id: this.get('topic_id'), + reply_to_post_number: this.get('reply_to_post_number'), + category: this.get('category'), archetype: this.get('archetype'), title: this.get('title'), image_sizes: this.get('imageSizes'), diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index 4616a76cdbe..9a9d5a086b9 100644 --- a/app/controllers/posts_controller.rb +++ b/app/controllers/posts_controller.rb @@ -25,19 +25,7 @@ class PostsController < ApplicationController end def create - params.require(:post) - - post_creator = PostCreator.new(current_user, - raw: params[:post][:raw], - topic_id: params[:post][:topic_id], - title: params[:title], - archetype: params[:archetype], - category: params[:post][:category], - target_usernames: params[:target_usernames], - reply_to_post_number: params[:post][:reply_to_post_number], - image_sizes: params[:image_sizes], - meta_data: params[:meta_data], - auto_close_days: params[:auto_close_days]) + post_creator = PostCreator.new(current_user, create_params) post = post_creator.create if post_creator.errors.present? @@ -197,4 +185,23 @@ class PostsController < ApplicationController guardian.ensure_can_see!(post) post end + + private + + def create_params + params.require(:raw) + params.permit( + :raw, + :topic_id, + :title, + :archetype, + :category, + :target_usernames, + :reply_to_post_number, + :image_sizes, + :auto_close_days + ).tap do |whitelisted| + whitelisted[:meta_data] = params[:meta_data] + end + end end diff --git a/lib/post_creator.rb b/lib/post_creator.rb index b4da9d4701d..6db17d5fb05 100644 --- a/lib/post_creator.rb +++ b/lib/post_creator.rb @@ -38,8 +38,6 @@ class PostCreator @user = user @opts = opts @spam = false - - raise Discourse::InvalidParameters.new(:raw) if @opts[:raw].blank? end # True if the post was considered spam diff --git a/spec/components/post_creator_spec.rb b/spec/components/post_creator_spec.rb index 9b7fd06e2cb..d57a5259fe4 100644 --- a/spec/components/post_creator_spec.rb +++ b/spec/components/post_creator_spec.rb @@ -10,10 +10,6 @@ describe PostCreator do let(:user) { Fabricate(:user) } - it 'raises an error without a raw value' do - lambda { PostCreator.new(user, {}) }.should raise_error(Discourse::InvalidParameters) - end - context 'new topic' do let(:category) { Fabricate(:category, user: user) } let(:topic) { Fabricate(:topic, user: user) } diff --git a/spec/controllers/posts_controller_spec.rb b/spec/controllers/posts_controller_spec.rb index 587f5b5a5b2..0936b7a9879 100644 --- a/spec/controllers/posts_controller_spec.rb +++ b/spec/controllers/posts_controller_spec.rb @@ -257,19 +257,19 @@ describe PostsController do let!(:user) { log_in } let(:new_post) { Fabricate.build(:post, user: user) } - it "raises an exception without a post parameter" do - lambda { xhr :post, :create }.should raise_error(ActionController::ParameterMissing) + it "raises an exception without a raw parameter" do + lambda { xhr :post, :create }.should raise_error(ActionController::ParameterMissing) end it 'calls the post creator' do PostCreator.any_instance.expects(:create).returns(new_post) - xhr :post, :create, post: {raw: 'test'} + xhr :post, :create, {raw: 'test'} response.should be_success end it 'returns JSON of the post' do PostCreator.any_instance.expects(:create).returns(new_post) - xhr :post, :create, post: {raw: 'test'} + xhr :post, :create, {raw: 'test'} ::JSON.parse(response.body).should be_present end @@ -284,7 +284,7 @@ describe PostsController do end it "does not succeed" do - xhr :post, :create, post: {raw: 'test'} + xhr :post, :create, {raw: 'test'} User.any_instance.expects(:flag_linked_posts_as_spam).never response.should_not be_success end @@ -292,7 +292,7 @@ describe PostsController do it "it triggers flag_linked_posts_as_spam when the post creator returns spam" do PostCreator.any_instance.expects(:spam?).returns(true) User.any_instance.expects(:flag_linked_posts_as_spam) - xhr :post, :create, post: {raw: 'test'} + xhr :post, :create, {raw: 'test'} end end @@ -308,48 +308,48 @@ describe PostsController do end it "passes raw through" do - PostCreator.expects(:new).with(user, has_entries(raw: 'hello')).returns(post_creator) - xhr :post, :create, post: {raw: 'hello'} + PostCreator.expects(:new).with(user, has_entries('raw' => 'hello')).returns(post_creator) + xhr :post, :create, {raw: 'hello'} end it "passes title through" do - PostCreator.expects(:new).with(user, has_entries(title: 'new topic title')).returns(post_creator) - xhr :post, :create, post: {raw: 'hello'}, title: 'new topic title' + PostCreator.expects(:new).with(user, has_entries('title' => 'new topic title')).returns(post_creator) + xhr :post, :create, {raw: 'hello', title: 'new topic title'} end it "passes topic_id through" do - PostCreator.expects(:new).with(user, has_entries(topic_id: '1234')).returns(post_creator) - xhr :post, :create, post: {raw: 'hello', topic_id: 1234} + PostCreator.expects(:new).with(user, has_entries('topic_id' => '1234')).returns(post_creator) + xhr :post, :create, {raw: 'hello', topic_id: 1234} end it "passes archetype through" do - PostCreator.expects(:new).with(user, has_entries(archetype: 'private_message')).returns(post_creator) - xhr :post, :create, post: {raw: 'hello'}, archetype: 'private_message' + PostCreator.expects(:new).with(user, has_entries('archetype' => 'private_message')).returns(post_creator) + xhr :post, :create, {raw: 'hello', archetype: 'private_message'} end it "passes category through" do - PostCreator.expects(:new).with(user, has_entries(category: 'cool')).returns(post_creator) - xhr :post, :create, post: {raw: 'hello', category: 'cool'} + PostCreator.expects(:new).with(user, has_entries('category' => 'cool')).returns(post_creator) + xhr :post, :create, {raw: 'hello', category: 'cool'} end it "passes target_usernames through" do - PostCreator.expects(:new).with(user, has_entries(target_usernames: 'evil,trout')).returns(post_creator) - xhr :post, :create, post: {raw: 'hello'}, target_usernames: 'evil,trout' + PostCreator.expects(:new).with(user, has_entries('target_usernames' => 'evil,trout')).returns(post_creator) + xhr :post, :create, {raw: 'hello', target_usernames: 'evil,trout'} end it "passes reply_to_post_number through" do - PostCreator.expects(:new).with(user, has_entries(reply_to_post_number: '6789')).returns(post_creator) - xhr :post, :create, post: {raw: 'hello', reply_to_post_number: 6789} + PostCreator.expects(:new).with(user, has_entries('reply_to_post_number' => '6789')).returns(post_creator) + xhr :post, :create, {raw: 'hello', reply_to_post_number: 6789} end it "passes image_sizes through" do - PostCreator.expects(:new).with(user, has_entries(image_sizes: 'test')).returns(post_creator) - xhr :post, :create, post: {raw: 'hello'}, image_sizes: 'test' + PostCreator.expects(:new).with(user, has_entries('image_sizes' => 'test')).returns(post_creator) + xhr :post, :create, {raw: 'hello', image_sizes: 'test'} end it "passes meta_data through" do - PostCreator.expects(:new).with(user, has_entries(meta_data: {'xyz' => 'abc'})).returns(post_creator) - xhr :post, :create, post: {raw: 'hello'}, meta_data: {xyz: 'abc'} + PostCreator.expects(:new).with(user, has_entries('meta_data' => {'xyz' => 'abc'})).returns(post_creator) + xhr :post, :create, {raw: 'hello', meta_data: {xyz: 'abc'}} end end