Enabled strong_parameters across all models/controllers.

All models are now using ActiveModel::ForbiddenAttributesProtection, which shifts the responsibility for parameter whitelisting for mass-assignments from the model to the controller. attr_accessible has been disabled and removed as this functionality replaces that.

The require_parameters method in the ApplicationController has been removed in favor of strong_parameters' #require method.

It is important to note that there is still some refactoring required to get all parameters to pass through #require and #permit so that we can guarantee that parameter values are scalar. Currently strong_parameters, in most cases, is only being utilized to require parameters and to whitelist the few places that do mass-assignments.

app/controllers/admin/impersonate_controller.rb

@@ -1,7 +1,7 @@
 class Admin::ImpersonateController < Admin::AdminController
 
   def create
-    requires_parameters(:username_or_email)
+    params.require(:username_or_email)
 
     user = User.find_by_username_or_email(params[:username_or_email]).first
 

app/controllers/admin/site_customizations_controller.rb

@@ -9,7 +9,7 @@ class Admin::SiteCustomizationsController < Admin::AdminController
   end
 
   def create
-    @site_customization = SiteCustomization.new(params[:site_customization])
+    @site_customization = SiteCustomization.new(site_customization_params)
     @site_customization.user_id = current_user.id
 
     respond_to do |format|
@@ -25,7 +25,7 @@ class Admin::SiteCustomizationsController < Admin::AdminController
     @site_customization = SiteCustomization.find(params[:id])
 
     respond_to do |format|
-      if @site_customization.update_attributes(params[:site_customization])
+      if @site_customization.update_attributes(site_customization_params)
         format.json { head :no_content }
       else
         format.json { render json: @site_customization.errors, status: :unprocessable_entity }
@@ -42,4 +42,10 @@ class Admin::SiteCustomizationsController < Admin::AdminController
     end
   end
 
+  private
+
+    def site_customization_params
+      params.require(:site_customization).permit(:name, :stylesheet, :header, :position, :enabled, :key, :override_default_style, :stylesheet_baked)
+    end
+
 end

app/controllers/admin/site_settings_controller.rb

@@ -7,7 +7,7 @@ class Admin::SiteSettingsController < Admin::AdminController
   end
 
   def update
-    requires_parameter(:value)
+    params.require(:value)
     SiteSetting.send("#{params[:id]}=", params[:value])
     render nothing: true
   end

app/controllers/application_controller.rb

@@ -257,14 +257,6 @@ class ApplicationController < ActionController::Base
       Rack::MiniProfiler.authorize_request
     end
 
-    def requires_parameters(*required)
-      required.each do |p|
-        raise Discourse::InvalidParameters.new(p) unless params.has_key?(p)
-      end
-    end
-
-    alias :requires_parameter :requires_parameters
-
     def store_incoming_links
       IncomingLink.add(request,current_user) unless request.xhr?
     end

app/controllers/clicks_controller.rb

@@ -3,14 +3,11 @@ class ClicksController < ApplicationController
   skip_before_filter :check_xhr
 
   def track
-    requires_parameter(:url)
-    if params[:topic_id].present? || params[:post_id].present?
-      args = {url: params[:url], ip: request.remote_ip}
-      args[:user_id] = current_user.id if current_user.present?
-      args[:post_id] = params[:post_id].to_i if params[:post_id].present?
-      args[:topic_id] = params[:topic_id].to_i if params[:topic_id].present?
+    params = track_params.merge(ip: request.remote_ip)
 
-      TopicLinkClick.create_from(args)
+    if params[:topic_id].present? || params[:post_id].present?
+      params.merge!({ user_id: current_user.id }) if current_user.present?
+      TopicLinkClick.create_from(params)
     end
 
     # Sometimes we want to record a link without a 302. Since XHR has to load the redirected
@@ -22,4 +19,11 @@ class ClicksController < ApplicationController
     end
   end
 
+  private
+
+    def track_params
+      params.require(:url)
+      params.permit(:url, :post_id, :topic_id, :redirect)
+    end
+
 end

app/controllers/posts_controller.rb

@@ -25,7 +25,7 @@ class PostsController < ApplicationController
   end
 
   def create
-    requires_parameter(:post)
+    params.require(:post)
 
     post_creator = PostCreator.new(current_user,
                                    raw: params[:post][:raw],
@@ -55,7 +55,7 @@ class PostsController < ApplicationController
   end
 
   def update
-    requires_parameter(:post)
+    params.require(:post)
 
     post = Post.where(id: params[:id]).first
     post.image_sizes = params[:image_sizes] if params[:image_sizes].present?
@@ -138,7 +138,7 @@ class PostsController < ApplicationController
 
   def destroy_many
 
-    requires_parameters(:post_ids)
+    params.require(:post_ids)
 
     posts = Post.where(id: params[:post_ids])
     raise Discourse::InvalidParameters.new(:post_ids) if posts.blank?

app/controllers/search_controller.rb

@@ -7,7 +7,7 @@ class SearchController < ApplicationController
   end
 
   def query
-    requires_parameter(:term)
+    params.require(:term)
 
     search_args = {guardian: guardian}
     search_args[:type_filter] = params[:type_filter] if params[:type_filter].present?
@@ -35,6 +35,4 @@ class SearchController < ApplicationController
     render_json_dump(search.execute.as_json)
   end
 
-
-
 end

app/controllers/session_controller.rb

@@ -6,7 +6,8 @@ class SessionController < ApplicationController
   skip_before_filter :redirect_to_login_if_required
 
   def create
-    requires_parameter(:login, :password)
+    params.require(:login)
+    params.require(:password)
 
     login = params[:login]
     login = login[1..-1] if login[0] == "@"
@@ -47,7 +48,7 @@ class SessionController < ApplicationController
   end
 
   def forgot_password
-    requires_parameter(:login)
+    params.require(:login)
 
     user = User.where('username_lower = :username or email = :email', username: params[:login].downcase, email: Email.downcase(params[:login])).first
     if user.present?

app/controllers/topics_controller.rb

@@ -78,7 +78,8 @@ class TopicsController < ApplicationController
   end
 
   def similar_to
-    requires_parameters(:title, :raw)
+    params.require(:title)
+    params.require(:raw)
     title, raw = params[:title], params[:raw]
 
     raise Discourse::InvalidParameters.new(:title) if title.length < SiteSetting.min_title_similar_length
@@ -89,7 +90,8 @@ class TopicsController < ApplicationController
   end
 
   def status
-    requires_parameters(:status, :enabled)
+    params.require(:status)
+    params.require(:enabled)
 
     raise Discourse::InvalidParameters.new(:status) unless %w(visible closed pinned archived).include?(params[:status])
     @topic = Topic.where(id: params[:topic_id].to_i).first
@@ -115,7 +117,7 @@ class TopicsController < ApplicationController
   end
 
   def autoclose
-    requires_parameter(:auto_close_days)
+    raise Discourse::InvalidParameters.new(:auto_close_days) unless params.has_key?(:auto_close_days)
     @topic = Topic.where(id: params[:topic_id].to_i).first
     guardian.ensure_can_moderate!(@topic)
     @topic.auto_close_days = params[:auto_close_days]
@@ -136,7 +138,7 @@ class TopicsController < ApplicationController
   end
 
   def invite
-    requires_parameter(:user)
+    params.require(:user)
     topic = Topic.where(id: params[:topic_id]).first
     guardian.ensure_can_invite_to!(topic)
 
@@ -154,7 +156,7 @@ class TopicsController < ApplicationController
   end
 
   def merge_topic
-    requires_parameters(:destination_topic_id)
+    params.require(:destination_topic_id)
 
     topic = Topic.where(id: params[:topic_id]).first
     guardian.ensure_can_move_posts!(topic)
@@ -168,7 +170,7 @@ class TopicsController < ApplicationController
   end
 
   def move_posts
-    requires_parameters(:post_ids)
+    params.require(:post_ids)
 
     topic = Topic.where(id: params[:topic_id]).first
     guardian.ensure_can_move_posts!(topic)

app/controllers/users_controller.rb

@@ -65,7 +65,7 @@ class UsersController < ApplicationController
   end
 
   def username
-    requires_parameter(:new_username)
+    params.require(:new_username)
 
     user = fetch_user_from_params
     guardian.ensure_can_edit!(user)
@@ -86,14 +86,14 @@ class UsersController < ApplicationController
   end
 
   def is_local_username
-    requires_parameter(:username)
+    params.require(:username)
     u = params[:username].downcase
     r = User.exec_sql('select 1 from users where username_lower = ?', u).values
     render json: {valid: r.length == 1}
   end
 
   def check_username
-    requires_parameter(:username)
+    params.require(:username)
 
     validator = UsernameValidator.new(params[:username])
     if !validator.valid_format?
@@ -259,7 +259,7 @@ class UsersController < ApplicationController
   end
 
   def change_email
-    requires_parameter(:email)
+    params.require(:email)
     user = fetch_user_from_params
     guardian.ensure_can_edit!(user)
     lower_email = Email.downcase(params[:email]).strip

app/models/admin_log.rb

@@ -2,8 +2,6 @@
 # like deleting users, changing site settings, etc.
 # Use the AdminLogger class to log records to this table.
 class AdminLog < ActiveRecord::Base
-  attr_accessible :action, :admin_id, :target_user_id, :details
-
   belongs_to :admin,        class_name: 'User'
   belongs_to :target_user,  class_name: 'User' # can be nil
 

app/models/cas_user_info.rb

@@ -1,5 +1,3 @@
 class CasUserInfo < ActiveRecord::Base
-  attr_accessible :email, :cas_user_id, :first_name, :gender, :last_name, :name, :user_id, :username, :link
   belongs_to :user
-
 end

app/models/category.rb

@@ -1,6 +1,4 @@
 class Category < ActiveRecord::Base
-  include ActiveModel::ForbiddenAttributesProtection
-
   belongs_to :topic, dependent: :destroy
   belongs_to :topic_only_relative_url,
     select: "id, title, slug",

app/models/facebook_user_info.rb

@@ -1,5 +1,4 @@
 class FacebookUserInfo < ActiveRecord::Base
-  attr_accessible :email, :facebook_user_id, :first_name, :gender, :last_name, :name, :user_id, :username, :link
   belongs_to :user
 end
 

app/models/invite.rb

@@ -1,7 +1,6 @@
 require_dependency 'trashable'
 
 class Invite < ActiveRecord::Base
-  include ActiveModel::ForbiddenAttributesProtection
   include Trashable
 
   belongs_to :user

app/models/post_action.rb

@@ -5,12 +5,9 @@ require_dependency 'trashable'
 class PostAction < ActiveRecord::Base
   class AlreadyActed < StandardError; end
 
-  include ActiveModel::ForbiddenAttributesProtection
   include RateLimiter::OnCreateRecord
   include Trashable
 
-  attr_accessible :post_action_type_id, :post_id, :user_id, :post, :user, :post_action_type, :message, :related_post_id, :staff_took_action
-
   belongs_to :post
   belongs_to :user
   belongs_to :post_action_type

app/models/post_action_type.rb

@@ -1,8 +1,6 @@
 require_dependency 'enum'
 
 class PostActionType < ActiveRecord::Base
-  attr_accessible :id, :is_flag, :name_key, :icon
-
   class << self
     def ordered
       order('position asc').all

app/models/site_setting.rb

@@ -6,8 +6,6 @@ class SiteSetting < ActiveRecord::Base
   validates_presence_of :name
   validates_presence_of :data_type
 
-  attr_accessible :description, :name, :value, :data_type
-
   # settings available in javascript under Discourse.SiteSettings
   client_setting(:title, "Discourse")
   client_setting(:logo_url, '/assets/d-logo-sketch.png')

app/models/topic_allowed_group.rb

@@ -1,7 +1,6 @@
 class TopicAllowedGroup < ActiveRecord::Base
   belongs_to :topic
   belongs_to :group
-  attr_accessible :group_id, :user_id
 
   validates_uniqueness_of :topic_id, scope: :group_id
 end

app/models/topic_allowed_user.rb

@@ -1,7 +1,6 @@
 class TopicAllowedUser < ActiveRecord::Base
   belongs_to :topic
   belongs_to :user
-  attr_accessible :topic_id, :user_id
 
   validates_uniqueness_of :topic_id, scope: :user_id
 end

app/models/upload.rb

@@ -5,8 +5,6 @@ require 's3'
 require 'local_store'
 
 class Upload < ActiveRecord::Base
-  include ActiveModel::ForbiddenAttributesProtection
-
   belongs_to :user
   belongs_to :topic
 

app/models/user.rb

@@ -7,8 +7,6 @@ require_dependency 'discourse'
 require_dependency 'post_destroyer'
 
 class User < ActiveRecord::Base
-  attr_accessible :name, :username, :password, :email, :bio_raw, :website
-
   has_many :posts
   has_many :notifications
   has_many :topic_users

app/models/user_action.rb

@@ -2,7 +2,6 @@ class UserAction < ActiveRecord::Base
   belongs_to :user
   belongs_to :target_post, class_name: "Post"
   belongs_to :target_topic, class_name: "Topic"
-  attr_accessible :acting_user_id, :action_type, :target_topic_id, :target_post_id, :target_user_id, :user_id
 
   validates_presence_of :action_type
   validates_presence_of :user_id

app/models/user_open_id.rb

@@ -1,6 +1,5 @@
 class UserOpenId < ActiveRecord::Base
   belongs_to :user
-  attr_accessible :email, :url, :user_id, :active
 
   validates_presence_of :email
   validates_presence_of :url

app/models/user_visit.rb

@@ -1,5 +1,4 @@
 class UserVisit < ActiveRecord::Base
-  attr_accessible :visited_at, :user_id
 
   # A list of visits in the last month by day
   def self.by_day(sinceDaysAgo=30)

config/application.rb

@@ -114,6 +114,10 @@ module Discourse
     config.ember.ember_location = "#{Rails.root}/app/assets/javascripts/external_production/ember.js"
     config.ember.handlebars_location = "#{Rails.root}/app/assets/javascripts/external/handlebars-1.0.rc.3.js"
 
+    # Since we are using strong_parameters, we can disable and remove
+    # attr_accessible.
+    config.active_record.whitelist_attributes = false
+
     # So open id logs somewhere sane
     config.after_initialize do
       OpenID::Util.logger = Rails.logger

config/initializers/strong_parameters.rb

@@ -0,0 +1 @@
+ActiveRecord::Base.send(:include, ActiveModel::ForbiddenAttributesProtection)

spec/controllers/admin/impersonate_controller_spec.rb

@@ -21,7 +21,7 @@ describe Admin::ImpersonateController do
     context 'create' do
 
       it 'requires a username_or_email parameter' do
-        lambda { xhr :put, :create }.should raise_error(Discourse::InvalidParameters)
+	lambda { xhr :put, :create }.should raise_error(ActionController::ParameterMissing)
       end
 
       it 'returns 404 when that user does not exist' do

spec/controllers/admin/site_settings_controller_spec.rb

@@ -26,7 +26,7 @@ describe Admin::SiteSettingsController do
     context 'update' do
 
       it 'requires a value parameter' do
-        lambda { xhr :put, :update, id: 'test_setting' }.should raise_error(Discourse::InvalidParameters)
+	lambda { xhr :put, :update, id: 'test_setting' }.should raise_error(ActionController::ParameterMissing)
       end
 
       it 'sets the value when the param is present' do

spec/controllers/clicks_controller_spec.rb

@@ -6,7 +6,7 @@ describe ClicksController do
 
     context 'missing params' do
       it 'raises an error without the url param' do
-        lambda { xhr :get, :track, post_id: 123 }.should raise_error(Discourse::InvalidParameters)
+	lambda { xhr :get, :track, post_id: 123 }.should raise_error(ActionController::ParameterMissing)
       end
 
       it "redirects to the url even without the topic_id or post_id params" do
@@ -24,7 +24,7 @@ describe ClicksController do
 
       context 'with a post_id' do
         it 'calls create_from' do
-          TopicLinkClick.expects(:create_from).with(url: 'http://discourse.org', post_id: 123, ip: '192.168.0.1')
+	  TopicLinkClick.expects(:create_from).with('url' => 'http://discourse.org', 'post_id' => '123', 'ip' => '192.168.0.1')
           xhr :get, :track, url: 'http://discourse.org', post_id: 123
           response.should redirect_to("http://discourse.org")
         end
@@ -36,13 +36,13 @@ describe ClicksController do
         end
 
         it 'will pass the user_id to create_from' do
-          TopicLinkClick.expects(:create_from).with(url: 'http://discourse.org', post_id: 123, ip: '192.168.0.1')
+	  TopicLinkClick.expects(:create_from).with('url' => 'http://discourse.org', 'post_id' => '123', 'ip' => '192.168.0.1')
           xhr :get, :track, url: 'http://discourse.org', post_id: 123
           response.should redirect_to("http://discourse.org")
         end
 
         it "doesn't redirect with the redirect=false param" do
-          TopicLinkClick.expects(:create_from).with(url: 'http://discourse.org', post_id: 123, ip: '192.168.0.1')
+	  TopicLinkClick.expects(:create_from).with('url' => 'http://discourse.org', 'post_id' => '123', 'ip' => '192.168.0.1', 'redirect' => 'false')
           xhr :get, :track, url: 'http://discourse.org', post_id: 123, redirect: 'false'
           response.should_not be_redirect
         end
@@ -51,7 +51,7 @@ describe ClicksController do
 
       context 'with a topic_id' do
         it 'calls create_from' do
-          TopicLinkClick.expects(:create_from).with(url: 'http://discourse.org', topic_id: 789, ip: '192.168.0.1')
+	  TopicLinkClick.expects(:create_from).with('url' => 'http://discourse.org', 'topic_id' => '789', 'ip' => '192.168.0.1')
           xhr :get, :track, url: 'http://discourse.org', topic_id: 789
           response.should redirect_to("http://discourse.org")
         end

spec/controllers/posts_controller_spec.rb

@@ -141,7 +141,7 @@ describe PostsController do
       let!(:post2) { Fabricate(:post, topic_id: post1.topic_id, user: poster, post_number: 3) }
 
       it "raises invalid parameters no post_ids" do
-        lambda { xhr :delete, :destroy_many }.should raise_error(Discourse::InvalidParameters)
+	lambda { xhr :delete, :destroy_many }.should raise_error(ActionController::ParameterMissing)
       end
 
       it "raises invalid parameters with missing ids" do
@@ -193,7 +193,7 @@ describe PostsController do
         update_params.delete(:post)
         lambda {
           xhr :put, :update, update_params
-        }.should raise_error(Discourse::InvalidParameters)
+	}.should raise_error(ActionController::ParameterMissing)
       end
 
       it "raises an error when the user doesn't have permission to see the post" do
@@ -258,7 +258,7 @@ describe PostsController do
       let(:new_post) { Fabricate.build(:post, user: user) }
 
       it "raises an exception without a post parameter" do
-        lambda { xhr :post, :create }.should raise_error(Discourse::InvalidParameters)
+	lambda { xhr :post, :create }.should raise_error(ActionController::ParameterMissing)
       end
 
       it 'calls the post creator' do

spec/controllers/session_controller_spec.rb

@@ -13,7 +13,7 @@ describe SessionController do
       end
 
       it "raises an error when the login isn't present" do
-        lambda { xhr :post, :create }.should raise_error(Discourse::InvalidParameters)
+	lambda { xhr :post, :create }.should raise_error(ActionController::ParameterMissing)
       end
 
       describe 'invalid password' do
@@ -114,7 +114,7 @@ describe SessionController do
   describe '.forgot_password' do
 
     it 'raises an error without a username parameter' do
-      lambda { xhr :post, :forgot_password }.should raise_error(Discourse::InvalidParameters)
+      lambda { xhr :post, :forgot_password }.should raise_error(ActionController::ParameterMissing)
     end
 
     context 'for a non existant username' do

spec/controllers/topics_controller_spec.rb

@@ -13,7 +13,7 @@ describe TopicsController do
       let(:topic) { p1.topic }
 
       it "raises an error without postIds" do
-        lambda { xhr :post, :move_posts, topic_id: topic.id, title: 'blah' }.should raise_error(Discourse::InvalidParameters)
+	lambda { xhr :post, :move_posts, topic_id: topic.id, title: 'blah' }.should raise_error(ActionController::ParameterMissing)
       end
 
       it "raises an error when the user doesn't have permission to move the posts" do
@@ -106,7 +106,7 @@ describe TopicsController do
       let(:topic) { p1.topic }
 
       it "raises an error without destination_topic_id" do
-        lambda { xhr :post, :merge_topic, topic_id: topic.id }.should raise_error(Discourse::InvalidParameters)
+	lambda { xhr :post, :merge_topic, topic_id: topic.id }.should raise_error(ActionController::ParameterMissing)
       end
 
       it "raises an error when the user doesn't have permission to merge" do
@@ -144,11 +144,11 @@ describe TopicsController do
     let(:raw) { 'this body is long enough to search for' }
 
     it "requires a title" do
-      -> { xhr :get, :similar_to, raw: raw }.should raise_error(Discourse::InvalidParameters)
+      -> { xhr :get, :similar_to, raw: raw }.should raise_error(ActionController::ParameterMissing)
     end
 
     it "requires a raw body" do
-      -> { xhr :get, :similar_to, title: title }.should raise_error(Discourse::InvalidParameters)
+      -> { xhr :get, :similar_to, title: title }.should raise_error(ActionController::ParameterMissing)
     end
 
     it "raises an error if the title length is below the minimum" do
@@ -218,11 +218,11 @@ describe TopicsController do
       end
 
       it 'requires the status parameter' do
-        lambda { xhr :put, :status, topic_id: @topic.id, enabled: true }.should raise_error(Discourse::InvalidParameters)
+	lambda { xhr :put, :status, topic_id: @topic.id, enabled: true }.should raise_error(ActionController::ParameterMissing)
       end
 
       it 'requires the enabled parameter' do
-        lambda { xhr :put, :status, topic_id: @topic.id, status: 'visible' }.should raise_error(Discourse::InvalidParameters)
+	lambda { xhr :put, :status, topic_id: @topic.id, status: 'visible' }.should raise_error(ActionController::ParameterMissing)
       end
 
       it 'raises an error with a status not in the whitelist' do
@@ -526,7 +526,7 @@ describe TopicsController do
       end
 
       it 'requires an email parameter' do
-        lambda { xhr :post, :invite, topic_id: @topic.id }.should raise_error(Discourse::InvalidParameters)
+	lambda { xhr :post, :invite, topic_id: @topic.id }.should raise_error(ActionController::ParameterMissing)
       end
 
       describe 'without permission' do

spec/controllers/users_controller_spec.rb

@@ -178,7 +178,7 @@ describe UsersController do
       let!(:user) { log_in }
 
       it 'raises an error without an email parameter' do
-        lambda { xhr :put, :change_email, username: user.username }.should raise_error(Discourse::InvalidParameters)
+	lambda { xhr :put, :change_email, username: user.username }.should raise_error(ActionController::ParameterMissing)
       end
 
       it "raises an error if you can't edit the user" do
@@ -489,7 +489,7 @@ describe UsersController do
       let(:new_username) { "#{user.username}1234" }
 
       it 'raises an error without a new_username param' do
-        lambda { xhr :put, :username, username: user.username }.should raise_error(Discourse::InvalidParameters)
+	lambda { xhr :put, :username, username: user.username }.should raise_error(ActionController::ParameterMissing)
       end
 
       it 'raises an error when you don\'t have permission to change the user' do
@@ -518,7 +518,7 @@ describe UsersController do
     end
 
     it 'raises an error without a username parameter' do
-      lambda { xhr :get, :check_username }.should raise_error(Discourse::InvalidParameters)
+      lambda { xhr :get, :check_username }.should raise_error(ActionController::ParameterMissing)
     end
 
     shared_examples_for 'when username is unavailable locally' do