Merge pull request #2706 from evazion/fix-2704

Partial fixes for #2704

app/controllers/comments_controller.rb

@@ -23,7 +23,7 @@ class CommentsController < ApplicationController
   def update
     @comment = Comment.find(params[:id])
     check_privilege(@comment)
-    @comment.update_attributes(params[:comment])
+    @comment.update_attributes(params[:comment].permit(:body))
     respond_with(@comment, :location => post_path(@comment.post_id))
   end
 

app/models/comment.rb

@@ -1,6 +1,7 @@
 class Comment < ActiveRecord::Base
   include Mentionable
 
+  validate :validate_post_exists, :on => :create
   validate :validate_creator_is_not_limited, :on => :create
   validates_format_of :body, :with => /\S/, :message => 'has no content'
   belongs_to :post
@@ -148,6 +149,10 @@ class Comment < ActiveRecord::Base
     User.id_to_name(updater_id)
   end
 
+  def validate_post_exists
+    errors.add(:post, "must exist") unless Post.exists?(post_id)
+  end
+
   def validate_creator_is_not_limited
     if creator.is_comment_limited? && !do_not_bump_post?
       errors.add(:base, "You can only post #{Danbooru.config.member_comment_limit} comments per hour")

app/models/post.rb

@@ -45,7 +45,7 @@ class Post < ActiveRecord::Base
   has_many :favorites, :dependent => :destroy
   validates_uniqueness_of :md5
   validate :post_is_not_its_own_parent
-  attr_accessible :source, :rating, :tag_string, :old_tag_string, :old_parent_id, :old_source, :old_rating, :last_noted_at, :parent_id, :has_embedded_notes, :as => [:member, :builder, :gold, :platinum, :janitor, :moderator, :admin, :default]
+  attr_accessible :source, :rating, :tag_string, :old_tag_string, :old_parent_id, :old_source, :old_rating, :parent_id, :has_embedded_notes, :as => [:member, :builder, :gold, :platinum, :janitor, :moderator, :admin, :default]
   attr_accessible :is_rating_locked, :is_note_locked, :as => [:builder, :janitor, :moderator, :admin]
   attr_accessible :is_status_locked, :as => [:admin]
 

test/functional/comments_controller_test.rb

@@ -33,6 +33,28 @@ class CommentsControllerTest < ActionController::TestCase
         post :update, {:id => @comment.id, :comment => {:body => "abc"}}, {:user_id => @comment.creator_id}
         assert_redirected_to post_path(@comment.post)
       end
+
+      should "only allow changing the body" do
+        params = {
+          id: @comment.id,
+          comment: {
+            body: "herp derp",
+            do_not_bump_post: true,
+            is_deleted: true,
+            post_id: FactoryGirl.create(:post).id,
+          }
+        }
+
+        post :update, params, { :user_id => @comment.creator_id }
+        @comment.reload
+
+        assert_equal("herp derp", @comment.body)
+        assert_equal(false, @comment.do_not_bump_post)
+        assert_equal(false, @comment.is_deleted)
+        assert_equal(@post.id, @comment.post_id)
+
+        assert_redirected_to post_path(@post)
+      end
     end
 
     context "create action"do
@@ -43,6 +65,11 @@ class CommentsControllerTest < ActionController::TestCase
         comment = Comment.last
         assert_redirected_to post_path(comment.post)
       end
+
+      should "not allow commenting on nonexistent posts" do
+        post :create, {:comment => FactoryGirl.attributes_for(:comment, :post_id => -1)}, {:user_id => @user.id}
+        assert_response :error
+      end
     end
   end
 end

test/functional/posts_controller_test.rb

@@ -103,6 +103,14 @@ class PostsControllerTest < ActionController::TestCase
         @post.reload
         assert_equal("bbb", @post.tag_string)
       end
+
+      should "ignore restricted params" do
+        post :update, {:id => @post.id, :post => {:last_noted_at => 1.minute.ago}}, {:user_id => @user.id}
+        assert_redirected_to post_path(@post)
+
+        @post.reload
+        assert_nil(@post.last_noted_at)
+      end
     end
 
     context "revert action" do

test/unit/comment_test.rb

@@ -101,6 +101,13 @@ class CommentTest < ActiveSupport::TestCase
         assert(comment.errors.empty?, comment.errors.full_messages.join(", "))
       end
 
+      should "not validate if the post does not exist" do
+        comment = FactoryGirl.build(:comment, :post_id => -1)
+
+        assert_not(comment.valid?)
+        assert_equal(["must exist"], comment.errors[:post])
+      end
+
       should "not bump the parent post" do
         post = FactoryGirl.create(:post)
         comment = FactoryGirl.create(:comment, :do_not_bump_post => true, :post => post)

test/unit/post_test.rb

@@ -1723,5 +1723,9 @@ class PostTest < ActiveSupport::TestCase
       end
     end
   end
+
+  context "Mass assignment: " do
+    should_not allow_mass_assignment_of(:last_noted_at).as(:member)
+  end
 end