From 1340fca8f17e4e3cce211e6eafed18cde7f57386 Mon Sep 17 00:00:00 2001 From: Tom Hughes Date: Mon, 5 Mar 2012 16:31:11 +0000 Subject: [PATCH] Turn on mass assignment protection Require any attribute that is going to be mass assigned to be whitelisted, and whitelist those attributes which need it --- app/controllers/trace_controller.rb | 4 ++-- app/controllers/user_roles_controller.rb | 4 +++- app/models/client_application.rb | 9 +++++++-- app/models/diary_comment.rb | 2 ++ app/models/diary_entry.rb | 2 ++ app/models/message.rb | 2 ++ app/models/oauth_nonce.rb | 4 +++- app/models/request_token.rb | 2 +- app/models/tracetag.rb | 2 ++ app/models/user.rb | 3 +++ app/models/user_block.rb | 8 +++++--- app/models/user_token.rb | 2 ++ config/application.rb | 2 +- script/deliver-message | 11 +++++++---- test/integration/user_blocks_test.rb | 20 ++++++++++++-------- test/unit/diary_entry_test.rb | 4 ++-- test/unit/node_test.rb | 12 +++++++----- test/unit/oauth_token_test.rb | 4 +++- test/unit/user_test.rb | 12 ++++++++---- 19 files changed, 74 insertions(+), 35 deletions(-) diff --git a/app/controllers/trace_controller.rb b/app/controllers/trace_controller.rb index d709b5cf1..e3f41f0ac 100644 --- a/app/controllers/trace_controller.rb +++ b/app/controllers/trace_controller.rb @@ -153,7 +153,7 @@ class TraceController < ApplicationController @trace.errors.add(:gpx_file, "can't be blank") end else - @trace = Trace.new(:visibility => default_visibility) + @trace = Trace.new({:visibility => default_visibility}, :without_protection => true) end @title = t 'trace.create.upload_trace' @@ -386,7 +386,7 @@ private :inserted => true, :user => @user, :timestamp => Time.now.getutc - }) + }, :without_protection => true) Trace.transaction do begin diff --git a/app/controllers/user_roles_controller.rb b/app/controllers/user_roles_controller.rb index 2d867d2d8..54dc90dee 100644 --- a/app/controllers/user_roles_controller.rb +++ b/app/controllers/user_roles_controller.rb @@ -11,7 +11,9 @@ class UserRolesController < ApplicationController around_filter :setup_nonce def grant - @this_user.roles.create(:role => @role, :granter_id => @user.id) + @this_user.roles.create({ + :role => @role, :granter_id => @user.id + }, :without_protection => true) redirect_to :controller => 'user', :action => 'view', :display_name => @this_user.display_name end diff --git a/app/models/client_application.rb b/app/models/client_application.rb index 55f34e6e2..b1f402292 100644 --- a/app/models/client_application.rb +++ b/app/models/client_application.rb @@ -13,6 +13,11 @@ class ClientApplication < ActiveRecord::Base validates_format_of :support_url, :with => /\Ahttp(s?):\/\/(\w+:{0,1}\w*@)?(\S+)(:[0-9]+)?(\/|\/([\w#!:.?+=&%@!\-\/]))?/i, :allow_blank=>true validates_format_of :callback_url, :with => /\A[a-z][a-z0-9.+-]*:\/\/(\w+:{0,1}\w*@)?(\S+)(:[0-9]+)?(\/|\/([\w#!:.?+=&%@!\-\/]))?/i, :allow_blank=>true + attr_accessible :name, :url, :support_url, :callback_url, + :allow_read_prefs, :allow_write_prefs, + :allow_write_diary, :allow_write_api, + :allow_read_gpx, :allow_write_gpx + before_validation :generate_keys, :on => :create attr_accessor :token_callback_url @@ -54,7 +59,7 @@ class ClientApplication < ActiveRecord::Base permissions.each do |p| params[p] = true end - RequestToken.create(params) + RequestToken.create(params, :without_protection => true) end def access_token_for_user(user) @@ -65,7 +70,7 @@ class ClientApplication < ActiveRecord::Base params[p] = true end - token = access_tokens.create(params) + token = access_tokens.create(params, :without_protection => true) end token diff --git a/app/models/diary_comment.rb b/app/models/diary_comment.rb index 0013606d0..b915e027a 100644 --- a/app/models/diary_comment.rb +++ b/app/models/diary_comment.rb @@ -5,6 +5,8 @@ class DiaryComment < ActiveRecord::Base validates_presence_of :body validates_associated :diary_entry + attr_accessible :body + def digest md5 = Digest::MD5.new md5 << diary_entry_id.to_s diff --git a/app/models/diary_entry.rb b/app/models/diary_entry.rb index 318672c03..1d836353f 100644 --- a/app/models/diary_entry.rb +++ b/app/models/diary_entry.rb @@ -23,4 +23,6 @@ class DiaryEntry < ActiveRecord::Base validates_numericality_of :longitude, :allow_nil => true, :greater_than_or_equal_to => -180, :less_than_or_equal_to => 180 validates_associated :language + + attr_accessible :title, :body, :language_code, :latitude, :longitude end diff --git a/app/models/message.rb b/app/models/message.rb index 053c7a4c0..0b3416003 100644 --- a/app/models/message.rb +++ b/app/models/message.rb @@ -9,6 +9,8 @@ class Message < ActiveRecord::Base validates_inclusion_of :message_read, :in => [ true, false ] validates_as_utf8 :title + attr_accessible :title, :body + def digest md5 = Digest::MD5.new md5 << from_user_id.to_s diff --git a/app/models/oauth_nonce.rb b/app/models/oauth_nonce.rb index 075351b91..3ae50d3a4 100644 --- a/app/models/oauth_nonce.rb +++ b/app/models/oauth_nonce.rb @@ -3,7 +3,9 @@ class OauthNonce < ActiveRecord::Base validates_presence_of :nonce, :timestamp validates_uniqueness_of :nonce, :scope => :timestamp - + + attr_accessible :nonce, :timestamp + # Remembers a nonce and it's associated timestamp. It returns false if it has already been used def self.remember(nonce, timestamp) oauth_nonce = OauthNonce.create(:nonce => nonce, :timestamp => timestamp) diff --git a/app/models/request_token.rb b/app/models/request_token.rb index 1ac502bc7..6e4ec40c3 100644 --- a/app/models/request_token.rb +++ b/app/models/request_token.rb @@ -21,7 +21,7 @@ class RequestToken < OauthToken params[p] = read_attribute(p) } - access_token = AccessToken.create(params) + access_token = AccessToken.create(params, :without_protection => true) invalidate! access_token end diff --git a/app/models/tracetag.rb b/app/models/tracetag.rb index d30d9d5ab..1b8ba2309 100644 --- a/app/models/tracetag.rb +++ b/app/models/tracetag.rb @@ -4,5 +4,7 @@ class Tracetag < ActiveRecord::Base validates_format_of :tag, :with => /^[^\/;.,?]*$/ validates_length_of :tag, :within => 1..255 + attr_accessible :tag + belongs_to :trace, :foreign_key => 'gpx_id' end diff --git a/app/models/user.rb b/app/models/user.rb index 9a230898a..0c9e76d54 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -41,6 +41,9 @@ class User < ActiveRecord::Base validates_numericality_of :home_zoom, :only_integer => true, :allow_nil => true validates_inclusion_of :preferred_editor, :in => Editors::ALL_EDITORS, :allow_nil => true + attr_accessible :display_name, :email, :email_confirmation, :openid_url, + :pass_crypt, :pass_crypt_confirmation, :consider_pd + after_initialize :set_creation_time before_save :encrypt_password diff --git a/app/models/user_block.rb b/app/models/user_block.rb index f8c05c92c..7bf8f86b5 100644 --- a/app/models/user_block.rb +++ b/app/models/user_block.rb @@ -18,9 +18,11 @@ class UserBlock < ActiveRecord::Base # revokes the block, allowing the user to use the API again. the argument # is the user object who is revoking the ban. def revoke!(revoker) - update_attributes({ :ends_at => Time.now.getutc(), - :revoker_id => revoker.id, - :needs_view => false }) + update_attributes({ + :ends_at => Time.now.getutc(), + :revoker_id => revoker.id, + :needs_view => false + }, :without_protection => true) end private diff --git a/app/models/user_token.rb b/app/models/user_token.rb index dda191187..9a754d344 100644 --- a/app/models/user_token.rb +++ b/app/models/user_token.rb @@ -1,6 +1,8 @@ class UserToken < ActiveRecord::Base belongs_to :user + attr_accessible :referer + after_initialize :set_defaults private diff --git a/config/application.rb b/config/application.rb index c85f47884..25df04b44 100644 --- a/config/application.rb +++ b/config/application.rb @@ -62,7 +62,7 @@ module OpenStreetMap # This will create an empty whitelist of attributes available for mass-assignment for all models # in your app. As such, your models will need to explicitly whitelist or blacklist accessible # parameters by using an attr_accessible or attr_protected declaration. - # config.active_record.whitelist_attributes = true + config.active_record.whitelist_attributes = true # Enable the asset pipeline config.assets.enabled = true diff --git a/script/deliver-message b/script/deliver-message index 9cce21f30..669e54a66 100755 --- a/script/deliver-message +++ b/script/deliver-message @@ -26,10 +26,13 @@ else body = mail end -message = Message.new(:sender => from, :recipient => to, - :sent_on => mail.date.new_offset(0), - :title => mail.subject.sub(/\[OpenStreetMap\] */, ""), - :body => body.decoded) +message = Message.new({ + :sender => from, + :recipient => to, + :sent_on => mail.date.new_offset(0), + :title => mail.subject.sub(/\[OpenStreetMap\] */, ""), + :body => body.decoded +}, :without_protection => true) message.save! Notifier.message_notification(message).deliver diff --git a/test/integration/user_blocks_test.rb b/test/integration/user_blocks_test.rb index 7003d7692..11fcae91a 100644 --- a/test/integration/user_blocks_test.rb +++ b/test/integration/user_blocks_test.rb @@ -17,10 +17,12 @@ class UserBlocksTest < ActionController::IntegrationTest assert_response :success # now block the user - UserBlock.create(:user_id => blocked_user.id, - :creator_id => users(:moderator_user).id, - :reason => "testing", - :ends_at => Time.now.getutc + 5.minutes) + UserBlock.create({ + :user_id => blocked_user.id, + :creator_id => users(:moderator_user).id, + :reason => "testing", + :ends_at => Time.now.getutc + 5.minutes + }, :without_protection => true) get "/api/#{API_VERSION}/user/details", nil, auth_header(blocked_user.display_name, "test") assert_response :forbidden end @@ -29,10 +31,12 @@ class UserBlocksTest < ActionController::IntegrationTest blocked_user = users(:public_user) moderator = users(:moderator_user) - block = UserBlock.create(:user_id => blocked_user.id, - :creator_id => moderator.id, - :reason => "testing", - :ends_at => Time.now.getutc + 5.minutes) + block = UserBlock.create({ + :user_id => blocked_user.id, + :creator_id => moderator.id, + :reason => "testing", + :ends_at => Time.now.getutc + 5.minutes + }, :without_protection => true) get "/api/#{API_VERSION}/user/details", nil, auth_header(blocked_user.display_name, "test") assert_response :forbidden diff --git a/test/unit/diary_entry_test.rb b/test/unit/diary_entry_test.rb index d645aa860..0e080210b 100644 --- a/test/unit/diary_entry_test.rb +++ b/test/unit/diary_entry_test.rb @@ -25,8 +25,8 @@ class DiaryEntryTest < ActiveSupport::TestCase end def diary_entry_valid(attrs, result = true) - entry = DiaryEntry.new(diary_entries(:normal_user_entry_1).attributes) - entry.attributes = attrs + entry = DiaryEntry.new(diary_entries(:normal_user_entry_1).attributes, :without_protection => true) + entry.assign_attributes(attrs, :without_protection => true) assert_equal result, entry.valid?, "Expected #{attrs.inspect} to be #{result}" end end diff --git a/test/unit/node_test.rb b/test/unit/node_test.rb index 6bfcf7926..78f060678 100644 --- a/test/unit/node_test.rb +++ b/test/unit/node_test.rb @@ -77,11 +77,13 @@ class NodeTest < ActiveSupport::TestCase # Check that you can create a node and store it def test_create - node_template = Node.new(:latitude => 12.3456, - :longitude => 65.4321, - :changeset_id => changesets(:normal_user_first_change).id, - :visible => 1, - :version => 1) + node_template = Node.new({ + :latitude => 12.3456, + :longitude => 65.4321, + :changeset_id => changesets(:normal_user_first_change).id, + :visible => 1, + :version => 1 + }, :without_protection => true) assert node_template.create_with_history(users(:normal_user)) node = Node.find(node_template.id) diff --git a/test/unit/oauth_token_test.rb b/test/unit/oauth_token_test.rb index eb8219c57..ff0353791 100644 --- a/test/unit/oauth_token_test.rb +++ b/test/unit/oauth_token_test.rb @@ -15,7 +15,9 @@ class OauthTokenTest < ActiveSupport::TestCase ## # check that an authorized token is authorised and can be invalidated def test_token_authorisation - tok = RequestToken.create :client_application => client_applications(:oauth_web_app) + tok = RequestToken.create({ + :client_application => client_applications(:oauth_web_app) + }, :without_protection => true) assert_equal false, tok.authorized?, "Token should be created unauthorised." tok.authorize!(users(:public_user)) assert_equal true, tok.authorized?, "Token should now be authorised." diff --git a/test/unit/user_test.rb b/test/unit/user_test.rb index 843c31905..976a543cf 100644 --- a/test/unit/user_test.rb +++ b/test/unit/user_test.rb @@ -18,23 +18,27 @@ class UserTest < ActiveSupport::TestCase end def test_unique_email - new_user = User.new(:email => users(:normal_user).email, + new_user = User.new({ + :email => users(:normal_user).email, :status => "active", :pass_crypt => Digest::MD5.hexdigest('test'), :display_name => "new user", :data_public => 1, - :description => "desc") + :description => "desc" + }, :without_protection => true) assert !new_user.save assert new_user.errors[:email].include?("has already been taken") end def test_unique_display_name - new_user = User.new(:email => "tester@openstreetmap.org", + new_user = User.new({ + :email => "tester@openstreetmap.org", :status => "pending", :pass_crypt => Digest::MD5.hexdigest('test'), :display_name => users(:normal_user).display_name, :data_public => 1, - :description => "desc") + :description => "desc" + }, :without_protection => true) assert !new_user.save assert new_user.errors[:display_name].include?("has already been taken") end -- 2.39.5