Merge pull request #2714 from evazion/fix-2704

Fix mass assignment vuln to tag alias/implication status (partial fix for #2704).

app/controllers/tag_aliases_controller.rb

@@ -15,7 +15,7 @@ class TagAliasesController < ApplicationController
     @tag_alias = TagAlias.find(params[:id])
 
     if @tag_alias.is_pending? && @tag_alias.editable_by?(CurrentUser.user)
-      @tag_alias.update_attributes(params[:tag_alias])
+      @tag_alias.update_attributes(update_params)
     end
 
     respond_with(@tag_alias)
@@ -46,4 +46,10 @@ class TagAliasesController < ApplicationController
     @tag_alias.approve!(CurrentUser.user.id)
     respond_with(@tag_alias, :location => tag_alias_path(@tag_alias))
   end
+
+private
+
+  def update_params
+    params.require(:tag_alias).permit(:antecedent_name, :consequent_name, :forum_topic_id)
+  end
 end

app/controllers/tag_implications_controller.rb

@@ -15,7 +15,7 @@ class TagImplicationsController < ApplicationController
     @tag_implication = TagImplication.find(params[:id])
 
     if @tag_implication.is_pending? && @tag_implication.editable_by?(CurrentUser.user)
-      @tag_implication.update_attributes(params[:tag_implication])
+      @tag_implication.update_attributes(update_params)
     end
 
     respond_with(@tag_implication)
@@ -51,4 +51,10 @@ class TagImplicationsController < ApplicationController
     @tag_implication.approve!(CurrentUser.user.id)
     respond_with(@tag_implication, :location => tag_implication_path(@tag_implication))
   end
+
+private
+
+  def update_params
+    params.require(:tag_implication).permit(:antecedent_name, :consequent_name, :forum_topic_id)
+  end
 end

app/models/tag_alias.rb

@@ -6,7 +6,11 @@ class TagAlias < ActiveRecord::Base
   after_destroy :clear_all_cache
   before_validation :initialize_creator, :on => :create
   before_validation :normalize_names
+  validates_format_of :status, :with => /\A(active|deleted|pending|processing|queued|error: .*)\Z/
   validates_presence_of :creator_id, :antecedent_name, :consequent_name
+  validates :creator, presence: { message: "must exist" }, if: lambda { creator_id.present? }
+  validates :approver, presence: { message: "must exist" }, if: lambda { approver_id.present? }
+  validates :forum_topic, presence: { message: "must exist" }, if: lambda { forum_topic_id.present? }
   validates_uniqueness_of :antecedent_name
   validate :absence_of_transitive_relation
   validate :antecedent_and_consequent_are_different
@@ -15,7 +19,8 @@ class TagAlias < ActiveRecord::Base
   belongs_to :creator, :class_name => "User"
   belongs_to :approver, :class_name => "User"
   belongs_to :forum_topic
-  attr_accessible :antecedent_name, :consequent_name, :forum_topic_id, :status, :skip_secondary_validations
+  attr_accessible :antecedent_name, :consequent_name, :forum_topic_id, :skip_secondary_validations
+  attr_accessible :status, :as => [:admin]
 
   module SearchMethods
     def name_matches(name)

app/models/tag_implication.rb

@@ -9,14 +9,19 @@ class TagImplication < ActiveRecord::Base
   belongs_to :forum_topic
   before_validation :initialize_creator, :on => :create
   before_validation :normalize_names
+  validates_format_of :status, :with => /\A(active|deleted|pending|processing|queued|error: .*)\Z/
   validates_presence_of :creator_id, :antecedent_name, :consequent_name
+  validates :creator, presence: { message: "must exist" }, if: lambda { creator_id.present? }
+  validates :approver, presence: { message: "must exist" }, if: lambda { approver_id.present? }
+  validates :forum_topic, presence: { message: "must exist" }, if: lambda { forum_topic_id.present? }
   validates_uniqueness_of :antecedent_name, :scope => :consequent_name
   validate :absence_of_circular_relation
   validate :antecedent_is_not_aliased
   validate :consequent_is_not_aliased
   validate :antecedent_and_consequent_are_different
   validate :wiki_pages_present, :on => :create
-  attr_accessible :antecedent_name, :consequent_name, :descendant_names, :forum_topic_id, :status, :forum_topic, :skip_secondary_validations
+  attr_accessible :antecedent_name, :consequent_name, :forum_topic_id, :skip_secondary_validations
+  attr_accessible :status, :as => [:admin]
 
   module DescendantMethods
     extend ActiveSupport::Concern

test/functional/tag_aliases_controller_test.rb

@@ -41,6 +41,15 @@ class TagAliasesControllerTest < ActionController::TestCase
           @tag_alias.reload
           assert_equal("xxx", @tag_alias.antecedent_name)
         end
+
+        should "not allow changing the status" do
+          post :update, {:id => @tag_alias.id, :tag_alias => {:status => "active"}}, {:user_id => @user.id}
+          @tag_alias.reload
+          assert_equal("pending", @tag_alias.status)
+        end
+
+        # TODO: Broken in shoulda-matchers 2.8.0. Need to upgrade to 3.1.1.
+        should_eventually permit(:antecedent_name, :consequent_name, :forum_topic_id).for(:update)
       end
 
       context "for an approved alias" do

test/functional/tag_implications_controller_test.rb

@@ -42,6 +42,15 @@ class TagImplicationsControllerTest < ActionController::TestCase
           @tag_implication.reload
           assert_equal("xxx", @tag_implication.antecedent_name)
         end
+
+        should "not allow changing the status" do
+          post :update, {:id => @tag_implication.id, :tag_implication => {:status => "active"}}, {:user_id => @user.id}
+          @tag_implication.reload
+          assert_equal("pending", @tag_implication.status)
+        end
+
+        # TODO: Broken in shoulda-matchers 2.8.0. Need to upgrade to 3.1.1.
+        should_eventually permit(:antecedent_name, :consequent_name, :forum_topic_id).for(:update)
       end
 
       context "for an approved implication" do

test/unit/tag_alias_test.rb

@@ -17,6 +17,36 @@ class TagAliasTest < ActiveSupport::TestCase
       CurrentUser.ip_addr = nil
     end
 
+    context "on validation" do
+      subject do
+        FactoryGirl.create(:tag, :name => "aaa")
+        FactoryGirl.create(:tag, :name => "bbb")
+        FactoryGirl.create(:tag_alias, :antecedent_name => "aaa", :consequent_name => "bbb")
+      end
+
+      should allow_value('active').for(:status)
+      should allow_value('deleted').for(:status)
+      should allow_value('pending').for(:status)
+      should allow_value('processing').for(:status)
+      should allow_value('queued').for(:status)
+      should allow_value('error: derp').for(:status)
+
+      should_not allow_value('ACTIVE').for(:status)
+      should_not allow_value('error').for(:status)
+      should_not allow_value('derp').for(:status)
+
+      should allow_value(nil).for(:forum_topic_id)
+      should_not allow_value(-1).for(:forum_topic_id).with_message("must exist", against: :forum_topic)
+
+      should allow_value(nil).for(:approver_id)
+      should_not allow_value(-1).for(:approver_id).with_message("must exist", against: :approver)
+
+      should_not allow_value(nil).for(:creator_id)
+      should_not allow_value(-1).for(:creator_id).with_message("must exist", against: :creator)
+
+      should_not allow_mass_assignment_of(:status).as(:member)
+    end
+
     should "populate the creator information" do
       ta = FactoryGirl.create(:tag_alias, :antecedent_name => "aaa", :consequent_name => "bbb")
       assert_equal(CurrentUser.user.id, ta.creator_id)

test/unit/tag_implication_test.rb

@@ -16,6 +16,38 @@ class TagImplicationTest < ActiveSupport::TestCase
       CurrentUser.ip_addr = nil
     end
 
+    context "on validation" do
+      subject do
+        FactoryGirl.create(:tag, :name => "aaa")
+        FactoryGirl.create(:tag, :name => "bbb")
+        FactoryGirl.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "bbb")
+      end
+
+      should allow_value('active').for(:status)
+      should allow_value('deleted').for(:status)
+      should allow_value('pending').for(:status)
+      should allow_value('processing').for(:status)
+      should allow_value('queued').for(:status)
+      should allow_value('error: derp').for(:status)
+
+      should_not allow_value('ACTIVE').for(:status)
+      should_not allow_value('error').for(:status)
+      should_not allow_value('derp').for(:status)
+
+      should allow_value(nil).for(:forum_topic_id)
+      should_not allow_value(-1).for(:forum_topic_id).with_message("must exist", against: :forum_topic)
+
+      should allow_value(nil).for(:approver_id)
+      should_not allow_value(-1).for(:approver_id).with_message("must exist", against: :approver)
+
+      should_not allow_value(nil).for(:creator_id)
+      should_not allow_value(-1).for(:creator_id).with_message("must exist", against: :creator)
+
+      should_not allow_mass_assignment_of(:status).as(:member)
+      should_not allow_mass_assignment_of(:forum_topic).as(:member)
+      should_not allow_mass_assignment_of(:descendant_names).as(:member)
+    end
+
     should "ignore pending implications when building descendant names" do
       ti2 = FactoryGirl.build(:tag_implication, :antecedent_name => "b", :consequent_name => "c", :status => "pending")
       ti2.save