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

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
0a05cc2a · Jean-Philippe Lang · ff77fb6a · 0a05cc2a · 0a05cc2a · 0a05cc2a
Commit 0a05cc2a authored Jan 12, 2010 by Jean-Philippe Lang
Showing with 35 additions and 2 deletions

issues_controller.rb app/controllers/issues_controller.rb +2 -2

issue.rb app/models/issue.rb +26 -0

issues_controller_test.rb test/functional/issues_controller_test.rb +7 -0

No files found.
--- 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?

--- 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

--- 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'},