Set a white list of issue attributes that can be mass-assigned from controllers.

git-svn-id: svn+ssh://rubyforge.org/var/svn/redmine/trunk@3308 e93f8b46-1217-0410-a6f0-8f06a7374b81

3 files changed, 35 insertions, 2 deletions

diff --git a/app/controllers/issues_controller.rb b/app/controllers/issues_controller.rb
index ded4e1a52..bb2e55fd3 100644
--- a/app/controllers/issues_controller.rb
+++ b/app/controllers/issues_controller.rb
@@ -131,7 +131,7 @@ class IssuesController < ApplicationController
       return
     end
     if params[:issue].is_a?(Hash)
-      @issue.attributes = params[:issue]
+      @issue.safe_attributes = params[:issue]
       @issue.watcher_user_ids = params[:issue]['watcher_user_ids'] if User.current.allowed_to?(:add_issue_watchers, @project)
     end
     @issue.author = User.current
@@ -181,7 +181,7 @@ class IssuesController < ApplicationController
       attrs = params[:issue].dup
       attrs.delete_if {|k,v| !UPDATABLE_ATTRS_ON_TRANSITION.include?(k) } unless @edit_allowed
       attrs.delete(:status_id) unless @allowed_statuses.detect {|s| s.id.to_s == attrs[:status_id].to_s}
-      @issue.attributes = attrs
+      @issue.safe_attributes = attrs
     end
 
     if request.post?
diff --git a/app/models/issue.rb b/app/models/issue.rb
index f4ebb2936..2780fd4c5 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -165,6 +165,32 @@ class Issue < ActiveRecord::Base
     write_attribute :estimated_hours, (h.is_a?(String) ? h.to_hours : h)
   end
   
+  SAFE_ATTRIBUTES = %w(
+    tracker_id
+    status_id
+    category_id
+    assigned_to_id
+    priority_id
+    fixed_version_id
+    subject
+    description
+    start_date
+    due_date
+    done_ratio
+    estimated_hours
+    custom_field_values
+  ) unless const_defined?(:SAFE_ATTRIBUTES)
+  
+  # Safely sets attributes
+  # Should be called from controllers instead of #attributes=
+  # attr_accessible is too rough because we still want things like
+  # Issue.new(:project => foo) to work
+  # TODO: move workflow/permission checks from controllers to here
+  def safe_attributes=(attrs, user=User.current)
+    return if attrs.nil?
+    self.attributes = attrs.reject {|k,v| !SAFE_ATTRIBUTES.include?(k)}
+  end
+  
   def done_ratio
     if Issue.use_status_for_done_ratio? && status && status.default_done_ratio?
       status.default_done_ratio
diff --git a/test/functional/issues_controller_test.rb b/test/functional/issues_controller_test.rb
index 4b806de89..2ea91d5e2 100644
--- a/test/functional/issues_controller_test.rb
+++ b/test/functional/issues_controller_test.rb
@@ -641,6 +641,13 @@ class IssuesControllerTest < ActionController::TestCase
                                         :value => 'Value for field 2'}
   end
   
+  def test_post_new_should_ignore_non_safe_attributes
+    @request.session[:user_id] = 2
+    assert_nothing_raised do
+      post :new, :project_id => 1, :issue => { :tracker => "A param can not be a Tracker" }
+    end
+  end
+  
   def test_copy_routing
     assert_routing(
       {:method => :get, :path => '/projects/world_domination/issues/567/copy'},