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