From 2a57f4851aae1bff10610321bd29bc6959bfe81e Mon Sep 17 00:00:00 2001 From: James Fleming Date: Thu, 27 Jun 2013 13:47:20 +0200 Subject: [PATCH 01/14] Install strong_parameters --- Gemfile | 4 ++++ Gemfile.lock | 5 +++++ 2 files changed, 9 insertions(+) diff --git a/Gemfile b/Gemfile index 29ce5faa2..8c9ffac94 100644 --- a/Gemfile +++ b/Gemfile @@ -92,6 +92,10 @@ gem 'haml', '4.0.3' gem 'mobile-fu', '1.2.1' gem 'will_paginate', '3.0.4' +# Strong parameters + +gem 'strong_parameters' + ### GROUPS #### diff --git a/Gemfile.lock b/Gemfile.lock index 8fc8b7010..2f89ad7a8 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -380,6 +380,10 @@ GEM multi_json (~> 1.0) rack (~> 1.0) tilt (~> 1.1, != 1.3.0) + strong_parameters (0.2.1) + actionpack (~> 3.0) + activemodel (~> 3.0) + railties (~> 3.0) subexec (0.2.3) temple (0.6.6) thor (0.18.1) @@ -486,6 +490,7 @@ DEPENDENCIES sinon-rails (= 1.7.3) slim (= 1.3.9) spork (= 1.0.0rc3) + strong_parameters timecop (= 0.6.1) twitter (= 4.8.1) typhoeus (= 0.6.3) From 938de466f8f59bc42da85c22b0f4fff91302c1b0 Mon Sep 17 00:00:00 2001 From: James Fleming Date: Thu, 27 Jun 2013 15:15:36 +0200 Subject: [PATCH 02/14] Strong parameters for Aspect --- app/controllers/aspects_controller.rb | 8 +++- app/models/aspect.rb | 4 +- spec/controllers/aspects_controller_spec.rb | 48 +++++++++++++++++++++ 3 files changed, 56 insertions(+), 4 deletions(-) diff --git a/app/controllers/aspects_controller.rb b/app/controllers/aspects_controller.rb index 3a988b3e2..9ba5cf702 100644 --- a/app/controllers/aspects_controller.rb +++ b/app/controllers/aspects_controller.rb @@ -10,7 +10,7 @@ class AspectsController < ApplicationController :json def create - @aspect = current_user.aspects.build(params[:aspect]) + @aspect = current_user.aspects.build(aspect_params) aspecting_person_id = params[:aspect][:person_id] if @aspect.save @@ -92,7 +92,7 @@ class AspectsController < ApplicationController def update @aspect = current_user.aspects.where(:id => params[:id]).first - if @aspect.update_attributes!(params[:aspect]) + if @aspect.update_attributes!(aspect_params) flash[:notice] = I18n.t 'aspects.update.success', :name => @aspect.name else flash[:error] = I18n.t 'aspects.update.failure', :name => @aspect.name @@ -121,4 +121,8 @@ class AspectsController < ApplicationController @contact = current_user.share_with(@person, @aspect) end end + + def aspect_params + params.require(:aspect).permit(:name, :contacts_visible, :order_id) + end end diff --git a/app/models/aspect.rb b/app/models/aspect.rb index a1241b5d6..ef7e49ae4 100644 --- a/app/models/aspect.rb +++ b/app/models/aspect.rb @@ -3,6 +3,8 @@ # the COPYRIGHT file. class Aspect < ActiveRecord::Base + include ActiveModel::ForbiddenAttributesProtection + belongs_to :user has_many :aspect_memberships, :dependent => :destroy @@ -16,8 +18,6 @@ class Aspect < ActiveRecord::Base validates_uniqueness_of :name, :scope => :user_id, :case_sensitive => false - attr_accessible :name, :contacts_visible, :order_id - before_validation do name.strip! end diff --git a/spec/controllers/aspects_controller_spec.rb b/spec/controllers/aspects_controller_spec.rb index 1e68047e3..8da2b2554 100644 --- a/spec/controllers/aspects_controller_spec.rb +++ b/spec/controllers/aspects_controller_spec.rb @@ -47,6 +47,30 @@ describe AspectsController do end describe "#create" do + context "strong parameters" do + it "permits 'name', 'contacts_visible' and 'order_id'" do + post :create, "aspect" => { + "name" => "new aspect", + "contacts_visible" => true, + "order_id" => 1 + } + aspect = alice.aspects.last + aspect.name.should eq("new aspect") + aspect.contacts_visible.should eq(true) + aspect.order_id.should eq(1) + end + + it "forbids other params" do + post :create, "aspect" => { + "name" => "new aspect", + "user_id" => 123 + } + aspect = Aspect.last + aspect.name.should eq("new aspect") + aspect.user_id.should_not eq(123) + end + end + context "with valid params" do it "creates an aspect" do alice.aspects.count.should == 2 @@ -97,6 +121,30 @@ describe AspectsController do @alices_aspect_1 = alice.aspects.create(:name => "Bruisers") end + context "strong parameters" do + it "permits 'name', 'contacts_visible' and 'order_id'" do + put 'update', :id => @alices_aspect_1.id, "aspect" => { + "name" => "new aspect", + "contacts_visible" => true, + "order_id" => 1 + } + aspect = Aspect.find(@alices_aspect_1.id) + aspect.name.should eq("new aspect") + aspect.contacts_visible.should eq(true) + aspect.order_id.should eq(1) + end + + it "forbids other params" do + put :update, :id => @alices_aspect_1.id, "aspect" => { + "name" => "new aspect", + "user_id" => 123 + } + aspect = Aspect.find(@alices_aspect_1.id) + aspect.name.should eq("new aspect") + aspect.user_id.should_not eq(123) + end + end + it "doesn't overwrite random attributes" do new_user = FactoryGirl.create :user params = {"name" => "Bruisers"} From a0a9f01be188b8f0fe136b9f7510e563a99b6c7c Mon Sep 17 00:00:00 2001 From: James Fleming Date: Thu, 27 Jun 2013 16:10:11 +0200 Subject: [PATCH 03/14] Remove tests for strong parameters Remove a duplicated test. --- spec/controllers/aspects_controller_spec.rb | 58 +-------------------- 1 file changed, 1 insertion(+), 57 deletions(-) diff --git a/spec/controllers/aspects_controller_spec.rb b/spec/controllers/aspects_controller_spec.rb index 8da2b2554..7f0a0d875 100644 --- a/spec/controllers/aspects_controller_spec.rb +++ b/spec/controllers/aspects_controller_spec.rb @@ -41,36 +41,12 @@ describe AspectsController do response.should be_redirect end it 'redirects on an invalid id' do - get :show, 'id' => 4341029835 + get :show, 'id' => 4341029834 response.should be_redirect end end describe "#create" do - context "strong parameters" do - it "permits 'name', 'contacts_visible' and 'order_id'" do - post :create, "aspect" => { - "name" => "new aspect", - "contacts_visible" => true, - "order_id" => 1 - } - aspect = alice.aspects.last - aspect.name.should eq("new aspect") - aspect.contacts_visible.should eq(true) - aspect.order_id.should eq(1) - end - - it "forbids other params" do - post :create, "aspect" => { - "name" => "new aspect", - "user_id" => 123 - } - aspect = Aspect.last - aspect.name.should eq("new aspect") - aspect.user_id.should_not eq(123) - end - end - context "with valid params" do it "creates an aspect" do alice.aspects.count.should == 2 @@ -121,38 +97,6 @@ describe AspectsController do @alices_aspect_1 = alice.aspects.create(:name => "Bruisers") end - context "strong parameters" do - it "permits 'name', 'contacts_visible' and 'order_id'" do - put 'update', :id => @alices_aspect_1.id, "aspect" => { - "name" => "new aspect", - "contacts_visible" => true, - "order_id" => 1 - } - aspect = Aspect.find(@alices_aspect_1.id) - aspect.name.should eq("new aspect") - aspect.contacts_visible.should eq(true) - aspect.order_id.should eq(1) - end - - it "forbids other params" do - put :update, :id => @alices_aspect_1.id, "aspect" => { - "name" => "new aspect", - "user_id" => 123 - } - aspect = Aspect.find(@alices_aspect_1.id) - aspect.name.should eq("new aspect") - aspect.user_id.should_not eq(123) - end - end - - it "doesn't overwrite random attributes" do - new_user = FactoryGirl.create :user - params = {"name" => "Bruisers"} - params[:user_id] = new_user.id - put('update', :id => @alices_aspect_1.id, "aspect" => params) - Aspect.find(@alices_aspect_1.id).user_id.should == alice.id - end - it "should return the name and id of the updated item" do params = {"name" => "Bruisers"} put('update', :id => @alices_aspect_1.id, "aspect" => params) From 509a407286e102a1fe59b3d18b4fc6088a72204e Mon Sep 17 00:00:00 2001 From: James Fleming Date: Thu, 27 Jun 2013 16:32:33 +0200 Subject: [PATCH 04/14] Strong parameters for Block --- app/controllers/blocks_controller.rb | 6 +++++- app/models/block.rb | 2 ++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/app/controllers/blocks_controller.rb b/app/controllers/blocks_controller.rb index 237ef4d97..f6f13ed7c 100644 --- a/app/controllers/blocks_controller.rb +++ b/app/controllers/blocks_controller.rb @@ -4,7 +4,7 @@ class BlocksController < ApplicationController respond_to :html, :json def create - block = current_user.blocks.new(params[:block]) + block = current_user.blocks.new(block_params) if block.save disconnect_if_contact(block.person) @@ -39,4 +39,8 @@ class BlocksController < ApplicationController current_user.disconnect(contact, :force => true) end end + + def block_params + params.require(:block).permit(:person_id) + end end diff --git a/app/models/block.rb b/app/models/block.rb index 1eba8f4c8..146227ad4 100644 --- a/app/models/block.rb +++ b/app/models/block.rb @@ -1,4 +1,6 @@ class Block < ActiveRecord::Base + include ActiveModel::ForbiddenAttributesProtection + belongs_to :person belongs_to :user From b86b409f7db1997a09fada05fa6a9879f3a266dc Mon Sep 17 00:00:00 2001 From: James Fleming Date: Thu, 27 Jun 2013 18:09:16 +0200 Subject: [PATCH 05/14] Strong parameters for Conversation --- app/controllers/conversations_controller.rb | 11 ++++++----- app/models/conversation.rb | 1 + 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/app/controllers/conversations_controller.rb b/app/controllers/conversations_controller.rb index 986bcc145..9e5102cf7 100644 --- a/app/controllers/conversations_controller.rb +++ b/app/controllers/conversations_controller.rb @@ -34,13 +34,14 @@ class ConversationsController < ApplicationController person_ids = Contact.where(:id => params[:contact_ids].split(',')).map(&:person_id) end - params[:conversation][:participant_ids] = [*person_ids] | [current_user.person_id] - params[:conversation][:author] = current_user.person - message_text = params[:conversation].delete(:text) - params[:conversation][:messages_attributes] = [ {:author => current_user.person, :text => message_text }] + @conversation = Conversation.new + @conversation.subject = params[:conversation][:subject] + @conversation.participant_ids = [*person_ids] | [current_user.person_id] + @conversation.author = current_user.person + message_text = params[:conversation][:text] + @conversation.messages_attributes = [ {:author => current_user.person, :text => message_text }] @response = {} - @conversation = Conversation.new(params[:conversation]) if person_ids.present? && @conversation.save Postzord::Dispatcher.build(current_user, @conversation).post @response[:success] = true diff --git a/app/models/conversation.rb b/app/models/conversation.rb index 8d2f25c25..531cd0b9f 100644 --- a/app/models/conversation.rb +++ b/app/models/conversation.rb @@ -1,6 +1,7 @@ class Conversation < ActiveRecord::Base include Diaspora::Federated::Base include Diaspora::Guid + include ActiveModel::ForbiddenAttributesProtection xml_attr :subject xml_attr :created_at From 686d3baaad0316f98fb71b2b981021efff9514d8 Mon Sep 17 00:00:00 2001 From: James Fleming Date: Thu, 27 Jun 2013 18:47:33 +0200 Subject: [PATCH 06/14] Strong parameters for InvitationsController. Make InvitationsController#create spec pass --- app/controllers/invitations_controller.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/controllers/invitations_controller.rb b/app/controllers/invitations_controller.rb index 538f025aa..3ab954a68 100644 --- a/app/controllers/invitations_controller.rb +++ b/app/controllers/invitations_controller.rb @@ -50,7 +50,8 @@ class InvitationsController < ApplicationController end def create - emails = params[:email_inviter][:emails].split(',').map(&:strip).uniq + inviter_params = params.require(:email_inviter).permit(:message, :locale, :emails) + emails = inviter_params[:emails].split(',').map(&:strip).uniq valid_emails, invalid_emails = emails.partition { |email| valid_email?(email) } @@ -60,8 +61,7 @@ class InvitationsController < ApplicationController unless valid_emails.empty? Workers::Mail::InviteEmail.perform_async(valid_emails.join(','), current_user.id, - params[:email_inviter]) - + inviter_params) end if emails.empty? From 0e26a496b8197d5c108638aec7ae691e3ef82d06 Mon Sep 17 00:00:00 2001 From: James Fleming Date: Thu, 27 Jun 2013 20:01:14 +0200 Subject: [PATCH 07/14] Strong parameters for User Fetch user params instead of require. --- app/controllers/registrations_controller.rb | 6 +++++- app/controllers/users_controller.rb | 11 +++++++++-- app/models/user.rb | 16 +--------------- 3 files changed, 15 insertions(+), 18 deletions(-) diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index b1fbd635b..c7377f4ed 100644 --- a/app/controllers/registrations_controller.rb +++ b/app/controllers/registrations_controller.rb @@ -9,7 +9,7 @@ class RegistrationsController < Devise::RegistrationsController before_filter -> { @css_framework = :bootstrap }, only: [:new] def create - @user = User.build(params[:user]) + @user = User.build(user_params) @user.process_invite_acceptence(invite) if invite.present? if @user.save @@ -54,4 +54,8 @@ class RegistrationsController < Devise::RegistrationsController end helper_method :invite + + def user_params + params.require(:user).permit(:username, :email, :getting_started, :password, :password_confirmation, :language, :disable_mail, :invitation_service, :invitation_identifier, :show_community_spotlight_in_stream, :auto_follow_back, :auto_follow_back_aspect_id, :remember_me) + end end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 528baa33f..8cb796fe2 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -24,7 +24,7 @@ class UsersController < ApplicationController password_changed = false @user = current_user - if u = params[:user] + if u = user_params u.delete(:password) if u[:password].blank? u.delete(:password_confirmation) if u[:password].blank? and u[:password_confirmation].blank? u.delete(:language) if u[:language].blank? @@ -125,7 +125,8 @@ class UsersController < ApplicationController def getting_started_completed user = current_user - user.update_attributes(:getting_started => false) + user.getting_started = false + user.save redirect_to stream_path end @@ -157,4 +158,10 @@ class UsersController < ApplicationController end redirect_to edit_user_path end + + private + + def user_params + params.fetch(:user).permit(:username, :email, :current_password, :password, :password_confirmation, :language, :disable_mail, :invitation_service, :invitation_identifier, :show_community_spotlight_in_stream, :auto_follow_back, :auto_follow_back_aspect_id, :remember_me, :email_preferences => [:also_commented, :mentioned, :comment_on_post, :private_message, :started_sharing, :liked, :reshared]) + end end diff --git a/app/models/user.rb b/app/models/user.rb index a53bfe488..4c8897c79 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -7,6 +7,7 @@ class User < ActiveRecord::Base include Connecting include Querying include SocialActions + include ActiveModel::ForbiddenAttributesProtection scope :logged_in_since, lambda { |time| where('last_sign_in_at > ?', time) } scope :monthly_actives, lambda { |time = Time.now| logged_in_since(time - 1.month) } @@ -67,21 +68,6 @@ class User < ActiveRecord::Base before_save :guard_unconfirmed_email, :save_person! - attr_accessible :username, - :email, - :getting_started, - :password, - :password_confirmation, - :language, - :disable_mail, - :invitation_service, - :invitation_identifier, - :show_community_spotlight_in_stream, - :auto_follow_back, - :auto_follow_back_aspect_id, - :remember_me - - def self.all_sharing_with_person(person) User.joins(:contacts).where(:contacts => {:person_id => person.id}) end From 143a970e616eceed89c9e9019f26adbafd104c71 Mon Sep 17 00:00:00 2001 From: carolina Date: Thu, 4 Jul 2013 13:01:03 +0200 Subject: [PATCH 08/14] Added strong_parameters gem to Gemfile and created an initializer in config --- Gemfile | 5 +++++ config/application.rb | 2 +- config/initializers/strong_parameters.rb | 1 + 3 files changed, 7 insertions(+), 1 deletion(-) create mode 100644 config/initializers/strong_parameters.rb diff --git a/Gemfile b/Gemfile index 8c9ffac94..6a1a0874c 100644 --- a/Gemfile +++ b/Gemfile @@ -65,6 +65,11 @@ gem 'redcarpet', '3.0.0' gem 'roxml', '3.1.6' gem 'ruby-oembed', '0.8.8' + +#Rails 4 integration +gem 'strong_parameters' + + # Services gem 'omniauth', '1.1.4' diff --git a/config/application.rb b/config/application.rb index d061da764..ee781069e 100644 --- a/config/application.rb +++ b/config/application.rb @@ -49,7 +49,7 @@ module Diaspora # This will create an empty whitelist of attributes available for mass-assignment for all models # in your app. As such, your models will need to explicitly whitelist or blacklist accessible # parameters by using an attr_accessible or attr_protected declaration. - #config.active_record.whitelist_attributes = true + #config.active_record.whitelist_attributes = false # Enable the asset pipeline config.assets.enabled = true diff --git a/config/initializers/strong_parameters.rb b/config/initializers/strong_parameters.rb new file mode 100644 index 000000000..394c1f5fd --- /dev/null +++ b/config/initializers/strong_parameters.rb @@ -0,0 +1 @@ +ActiveRecord::Base.send(:include, ActiveModel::ForbiddenAttributesProtection) From 3ba63197e8ff17e1202f733b56ceefd8d9bb3215 Mon Sep 17 00:00:00 2001 From: mokus Date: Fri, 5 Jul 2013 13:52:10 +0200 Subject: [PATCH 09/14] Fixes failing tests by changing user model --- app/models/user.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 4c8897c79..1170b0082 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -64,7 +64,6 @@ class User < ActiveRecord::Base has_many :notifications, :foreign_key => :recipient_id - before_save :guard_unconfirmed_email, :save_person! @@ -342,7 +341,7 @@ class User < ActiveRecord::Base ###Helpers############ def self.build(opts = {}) - u = User.new(opts) + u = User.new(opts.except(:person)) u.setup(opts) u end From d6ba6d1b02341ef3a158549fb443e58a8c2a4af0 Mon Sep 17 00:00:00 2001 From: James Fleming Date: Sat, 6 Jul 2013 23:12:41 +0200 Subject: [PATCH 10/14] Use strong params in photos_controller Add specs to check mass-assignment gotchas in PhotosController. --- app/controllers/photos_controller.rb | 8 ++++++-- spec/controllers/photos_controller_spec.rb | 23 +++++++++++++++++++++- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/app/controllers/photos_controller.rb b/app/controllers/photos_controller.rb index 2d7fb936f..c70a4a920 100644 --- a/app/controllers/photos_controller.rb +++ b/app/controllers/photos_controller.rb @@ -41,7 +41,7 @@ class PhotosController < ApplicationController def create rescuing_photo_errors do if remotipart_submitted? - @photo = current_user.build_post(:photo, params[:photo]) + @photo = current_user.build_post(:photo, photo_params) if @photo.save respond_to do |format| format.json { render :json => {"success" => true, "data" => @photo.as_api_response(:backbone)} } @@ -114,7 +114,7 @@ class PhotosController < ApplicationController def update photo = current_user.photos.where(:id => params[:id]).first if photo - if current_user.update_post( photo, params[:photo] ) + if current_user.update_post( photo, photo_params ) flash.now[:notice] = I18n.t 'photos.update.notice' respond_to do |format| format.js{ render :json => photo, :status => 200 } @@ -133,6 +133,10 @@ class PhotosController < ApplicationController private + def photo_params + params.require(:photo).permit(:public, :text, :pending, :user_file, :image_url, :aspect_ids, :set_profile_photo) + end + def file_handler(params) # For XHR file uploads, request.params[:qqfile] will be the path to the temporary file # For regular form uploads (such as those made by Opera), request.params[:qqfile] will be an UploadedFile which can be returned unaltered. diff --git a/spec/controllers/photos_controller_spec.rb b/spec/controllers/photos_controller_spec.rb index af7e076dd..99635571e 100644 --- a/spec/controllers/photos_controller_spec.rb +++ b/spec/controllers/photos_controller_spec.rb @@ -54,6 +54,20 @@ describe PhotosController do }.should change(Photo, :count).by(1) end + it "doesn't allow mass assignment of person" do + new_user = FactoryGirl.create(:user) + @params[:photo][:author] = new_user + post :create, @params + Photo.last.author.should == alice.person + end + + it "doesn't allow mass assignment of person_id" do + new_user = FactoryGirl.create(:user) + @params[:photo][:author_id] = new_user.id + post :create, @params + Photo.last.author.should == alice.person + end + it 'can set the photo as the profile photo' do old_url = alice.person.profile.image_url @params[:photo][:set_profile_photo] = true @@ -137,7 +151,14 @@ describe PhotosController do @alices_photo.reload.text.should == "now with lasers!" end - it "doesn't overwrite random attributes" do + it "doesn't allow mass assignment of person" do + new_user = FactoryGirl.create(:user) + params = { :text => "now with lasers!", :author => new_user } + put :update, :id => @alices_photo.id, :photo => params + @alices_photo.reload.author.should == alice.person + end + + it "doesn't allow mass assignment of person_id" do new_user = FactoryGirl.create(:user) params = { :text => "now with lasers!", :author_id => new_user.id } put :update, :id => @alices_photo.id, :photo => params From 94e9fc5ac4ede1b44e5c047cd88720b894f65a78 Mon Sep 17 00:00:00 2001 From: James Fleming Date: Sat, 6 Jul 2013 23:21:27 +0200 Subject: [PATCH 11/14] Use strong params in profiles_controller --- app/controllers/profiles_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/profiles_controller.rb b/app/controllers/profiles_controller.rb index ef1ec28e4..c82b1f43b 100644 --- a/app/controllers/profiles_controller.rb +++ b/app/controllers/profiles_controller.rb @@ -33,7 +33,7 @@ class ProfilesController < ApplicationController def update # upload and set new profile photo - @profile_attrs = params[:profile] || {} + @profile_attrs = params.require(:profile).permit(:first_name, :last_name, :gender, :bio, :location, :searchable, :tag_string, :nsfw, :date => [:year, :month, :day]) || {} munge_tag_string From e8db2804cb33db3de450942f4739fdef2f4c29ee Mon Sep 17 00:00:00 2001 From: James Fleming Date: Mon, 8 Jul 2013 10:43:19 +0200 Subject: [PATCH 12/14] Fix ProfilesController#update spec for tags. --- spec/controllers/profiles_controller_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/controllers/profiles_controller_spec.rb b/spec/controllers/profiles_controller_spec.rb index 2651ed03c..12abb6726 100644 --- a/spec/controllers/profiles_controller_spec.rb +++ b/spec/controllers/profiles_controller_spec.rb @@ -71,7 +71,8 @@ describe ProfilesController do it 'sets tags' do params = { :id => eve.person.id, - :tags => '#apples #oranges'} + :tags => '#apples #oranges', + :profile => {:tag_string => ''} } put :update, params eve.person(true).profile.tag_list.to_set.should == ['apples', 'oranges'].to_set From 66a07bd93825dac81782b663a18c6155d3932646 Mon Sep 17 00:00:00 2001 From: James Fleming Date: Mon, 8 Jul 2013 17:09:20 +0200 Subject: [PATCH 13/14] Remove attributes protection. Use a blacklist approach instead of a blacklist approach in Profile#receive. Remove attr_accessible from models and make specs pass. --- app/models/account_deletion.rb | 2 -- app/models/aspect.rb | 2 -- app/models/block.rb | 1 - app/models/conversation.rb | 1 - app/models/invitation.rb | 2 -- app/models/o_embed_cache.rb | 1 - app/models/photo.rb | 3 +-- app/models/post.rb | 2 +- app/models/profile.rb | 6 ++---- app/models/reshare.rb | 1 - app/models/status_message.rb | 1 - app/models/user.rb | 2 +- spec/models/photo_spec.rb | 18 ------------------ spec/models/user_spec.rb | 2 +- 14 files changed, 6 insertions(+), 38 deletions(-) diff --git a/app/models/account_deletion.rb b/app/models/account_deletion.rb index c3ca2e142..492929e5b 100644 --- a/app/models/account_deletion.rb +++ b/app/models/account_deletion.rb @@ -9,8 +9,6 @@ class AccountDeletion < ActiveRecord::Base belongs_to :person after_create :queue_delete_account - attr_accessible :person - xml_name :account_deletion xml_attr :diaspora_handle diff --git a/app/models/aspect.rb b/app/models/aspect.rb index ef7e49ae4..c3d9de4a1 100644 --- a/app/models/aspect.rb +++ b/app/models/aspect.rb @@ -3,8 +3,6 @@ # the COPYRIGHT file. class Aspect < ActiveRecord::Base - include ActiveModel::ForbiddenAttributesProtection - belongs_to :user has_many :aspect_memberships, :dependent => :destroy diff --git a/app/models/block.rb b/app/models/block.rb index 146227ad4..361e4d934 100644 --- a/app/models/block.rb +++ b/app/models/block.rb @@ -1,5 +1,4 @@ class Block < ActiveRecord::Base - include ActiveModel::ForbiddenAttributesProtection belongs_to :person belongs_to :user diff --git a/app/models/conversation.rb b/app/models/conversation.rb index 531cd0b9f..8d2f25c25 100644 --- a/app/models/conversation.rb +++ b/app/models/conversation.rb @@ -1,7 +1,6 @@ class Conversation < ActiveRecord::Base include Diaspora::Federated::Base include Diaspora::Guid - include ActiveModel::ForbiddenAttributesProtection xml_attr :subject xml_attr :created_at diff --git a/app/models/invitation.rb b/app/models/invitation.rb index 1f2aa9cee..40e5037c9 100644 --- a/app/models/invitation.rb +++ b/app/models/invitation.rb @@ -9,8 +9,6 @@ class Invitation < ActiveRecord::Base belongs_to :recipient, :class_name => 'User' belongs_to :aspect - attr_accessible :sender, :recipient, :aspect, :language, :service, :identifier, :admin, :message - before_validation :set_email_as_default_service # before_create :share_with_exsisting_user, :if => :recipient_id? diff --git a/app/models/o_embed_cache.rb b/app/models/o_embed_cache.rb index 1d0ec6fb9..9aee6d00b 100644 --- a/app/models/o_embed_cache.rb +++ b/app/models/o_embed_cache.rb @@ -1,6 +1,5 @@ class OEmbedCache < ActiveRecord::Base serialize :data - attr_accessible :url validates :data, :presence => true has_many :posts diff --git a/app/models/photo.rb b/app/models/photo.rb index 7df5ab9bb..f34d6f3fb 100644 --- a/app/models/photo.rb +++ b/app/models/photo.rb @@ -41,7 +41,6 @@ class Photo < ActiveRecord::Base validates_associated :status_message delegate :author_name, to: :status_message, prefix: true - attr_accessible :text, :pending validate :ownership_of_status_message before_destroy :ensure_user_picture @@ -69,7 +68,7 @@ class Photo < ActiveRecord::Base end def self.diaspora_initialize(params = {}) - photo = self.new params.to_hash + photo = self.new params.to_hash.slice(:text, :pending) photo.author = params[:author] photo.public = params[:public] if params[:public] photo.pending = params[:pending] if params[:pending] diff --git a/app/models/post.rb b/app/models/post.rb index d5e466cab..59968356f 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -116,7 +116,7 @@ class Post < ActiveRecord::Base ############# def self.diaspora_initialize(params) - new_post = self.new params.to_hash + new_post = self.new params.to_hash.stringify_keys.slice(*self.column_names) new_post.author = params[:author] new_post.public = params[:public] if params[:public] new_post.pending = params[:pending] if params[:pending] diff --git a/app/models/profile.rb b/app/models/profile.rb index 5a8073d3b..76a56d73e 100644 --- a/app/models/profile.rb +++ b/app/models/profile.rb @@ -38,9 +38,6 @@ class Profile < ActiveRecord::Base validate :max_tags validate :valid_birthday - attr_accessible :first_name, :last_name, :image_url, :image_url_medium, - :image_url_small, :birthday, :gender, :bio, :location, :searchable, :date, :tag_string, :nsfw - belongs_to :person before_validation do self.tag_string = self.tag_string.split[0..4].join(' ') @@ -57,7 +54,8 @@ class Profile < ActiveRecord::Base def receive(user, person) Rails.logger.info("event=receive payload_type=profile sender=#{person} to=#{user}") - person.profile.update_attributes self.attributes.merge(:tag_string => self.tag_string) + profiles_attr = self.attributes.merge('tag_string' => self.tag_string).slice('diaspora_handle', 'first_name', 'last_name', 'image_url', 'image_url_small', 'image_url_medium', 'birthday', 'gender', 'bio', 'location', 'searchable', 'nsfw', 'tag_string') + person.profile.update_attributes(profiles_attr) person.profile end diff --git a/app/models/reshare.rb b/app/models/reshare.rb index 5b3c5341b..e5728429b 100644 --- a/app/models/reshare.rb +++ b/app/models/reshare.rb @@ -6,7 +6,6 @@ class Reshare < Post belongs_to :root, :class_name => 'Post', :foreign_key => :root_guid, :primary_key => :guid validate :root_must_be_public - attr_accessible :root_guid, :public validates_presence_of :root, :on => :create validates_uniqueness_of :root_guid, :scope => :author_id delegate :author, to: :root, prefix: true diff --git a/app/models/status_message.rb b/app/models/status_message.rb index 13821c4bd..f580be28d 100644 --- a/app/models/status_message.rb +++ b/app/models/status_message.rb @@ -25,7 +25,6 @@ class StatusMessage < Post # therefore, we put the validation in a before_destory callback instead of a validation before_destroy :presence_of_content - attr_accessible :text, :provider_display_name, :frame_name attr_accessor :oembed_url before_create :filter_mentions diff --git a/app/models/user.rb b/app/models/user.rb index 1170b0082..92d9cb8e8 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -7,7 +7,6 @@ class User < ActiveRecord::Base include Connecting include Querying include SocialActions - include ActiveModel::ForbiddenAttributesProtection scope :logged_in_since, lambda { |time| where('last_sign_in_at > ?', time) } scope :monthly_actives, lambda { |time = Time.now| logged_in_since(time - 1.month) } @@ -327,6 +326,7 @@ class User < ActiveRecord::Base params[:image_url_small] = photo.url(:thumb_small) end + params.stringify_keys!.slice!(*(Profile.column_names+['tag_string', 'date'])) if self.profile.update_attributes(params) deliver_profile_update true diff --git a/spec/models/photo_spec.rb b/spec/models/photo_spec.rb index f9e8d2036..3b71e2b6e 100644 --- a/spec/models/photo_spec.rb +++ b/spec/models/photo_spec.rb @@ -26,24 +26,6 @@ describe Photo do @saved_photo.save end - describe "protected attributes" do - it "doesn't allow mass assignment of person" do - @photo.save! - @photo.update_attributes(:author => FactoryGirl.build(:person)) - @photo.reload.author.should == @user.person - end - it "doesn't allow mass assignment of person_id" do - @photo.save! - @photo.update_attributes(:author_id => FactoryGirl.build(:person).id) - @photo.reload.author.should == @user.person - end - it 'allows assignment of text' do - @photo.save! - @photo.update_attributes(:text => "this is awesome!!") - @photo.reload.text.should == "this is awesome!!" - end - end - describe 'after_create' do it 'calls #queue_processing_job' do @photo.should_receive(:queue_processing_job) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index d0ac01f5a..60c1e157c 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -466,7 +466,7 @@ describe User do end it 'dispatches the profile when tags are set' do - @params = {:tags => '#what #hey'} + @params = {:tag_string => '#what #hey'} mailman = Postzord::Dispatcher.build(alice, Profile.new) Postzord::Dispatcher.should_receive(:build).and_return(mailman) alice.update_profile(@params).should be_true From 9ca9a6f310b5a6d5d8a6d2e98a75db0a60a0c2fb Mon Sep 17 00:00:00 2001 From: James Fleming Date: Tue, 9 Jul 2013 14:19:55 +0200 Subject: [PATCH 14/14] Remove duplicates and bad code habits. Remove some blank line. Do not chain bang methods. --- Gemfile | 6 +----- app/controllers/invitations_controller.rb | 5 ++++- app/controllers/profiles_controller.rb | 6 +++++- app/models/block.rb | 1 - app/models/user.rb | 3 ++- config/initializers/strong_parameters.rb | 1 + 6 files changed, 13 insertions(+), 9 deletions(-) diff --git a/Gemfile b/Gemfile index 6a1a0874c..59087dbd4 100644 --- a/Gemfile +++ b/Gemfile @@ -66,7 +66,7 @@ gem 'roxml', '3.1.6' gem 'ruby-oembed', '0.8.8' -#Rails 4 integration +# Please remove when migrating to Rails 4 gem 'strong_parameters' @@ -97,10 +97,6 @@ gem 'haml', '4.0.3' gem 'mobile-fu', '1.2.1' gem 'will_paginate', '3.0.4' -# Strong parameters - -gem 'strong_parameters' - ### GROUPS #### diff --git a/app/controllers/invitations_controller.rb b/app/controllers/invitations_controller.rb index 3ab954a68..4bbd21c94 100644 --- a/app/controllers/invitations_controller.rb +++ b/app/controllers/invitations_controller.rb @@ -50,7 +50,6 @@ class InvitationsController < ApplicationController end def create - inviter_params = params.require(:email_inviter).permit(:message, :locale, :emails) emails = inviter_params[:emails].split(',').map(&:strip).uniq valid_emails, invalid_emails = emails.partition { |email| valid_email?(email) } @@ -99,4 +98,8 @@ class InvitationsController < ApplicationController session[key] = nil return value end + + def inviter_params + params.require(:email_inviter).permit(:message, :locale, :emails) + end end diff --git a/app/controllers/profiles_controller.rb b/app/controllers/profiles_controller.rb index c82b1f43b..86e50604a 100644 --- a/app/controllers/profiles_controller.rb +++ b/app/controllers/profiles_controller.rb @@ -33,7 +33,7 @@ class ProfilesController < ApplicationController def update # upload and set new profile photo - @profile_attrs = params.require(:profile).permit(:first_name, :last_name, :gender, :bio, :location, :searchable, :tag_string, :nsfw, :date => [:year, :month, :day]) || {} + @profile_attrs = profile_params munge_tag_string @@ -78,4 +78,8 @@ class ProfilesController < ApplicationController end @profile_attrs[:tag_string] = (params[:tags]) ? params[:tags].gsub(',',' ') : "" end + + def profile_params + params.require(:profile).permit(:first_name, :last_name, :gender, :bio, :location, :searchable, :tag_string, :nsfw, :date => [:year, :month, :day]) || {} + end end diff --git a/app/models/block.rb b/app/models/block.rb index 361e4d934..1eba8f4c8 100644 --- a/app/models/block.rb +++ b/app/models/block.rb @@ -1,5 +1,4 @@ class Block < ActiveRecord::Base - belongs_to :person belongs_to :user diff --git a/app/models/user.rb b/app/models/user.rb index 92d9cb8e8..0ee9f6ed7 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -326,7 +326,8 @@ class User < ActiveRecord::Base params[:image_url_small] = photo.url(:thumb_small) end - params.stringify_keys!.slice!(*(Profile.column_names+['tag_string', 'date'])) + params.stringify_keys! + params.slice!(*(Profile.column_names+['tag_string', 'date'])) if self.profile.update_attributes(params) deliver_profile_update true diff --git a/config/initializers/strong_parameters.rb b/config/initializers/strong_parameters.rb index 394c1f5fd..69fdcd47c 100644 --- a/config/initializers/strong_parameters.rb +++ b/config/initializers/strong_parameters.rb @@ -1 +1,2 @@ +# Please remove when migrating to Rails 4 ActiveRecord::Base.send(:include, ActiveModel::ForbiddenAttributesProtection)