Download as
Browse Code »

Commit 0439387be00bfb862b4454000f805f11fb8cc389

Authored by Dmitriy Zaporozhets
2 parents dddb5b5d 83efcabc
Exists in master and in 4 other branches 6-8-stable, 7-0-stable, 7-0-stable-spb, spb-stable

Merge pull request #1567 from NARKOZ/mass-assignment 

set activerecord whitelist_attributes to true [2]
Showing 29 changed files with 102 additions and 46 deletions   Show diff stats
app/models/event.rb
  Diff comments   View file @0439387
1 1 class Event < ActiveRecord::Base
2 2 include PushEvent
3 3  
  4 + attr_accessible :project, :action, :data, :author_id, :project_id,
  5 + :target_id, :target_type
  6 +
4 7 default_scope where("author_id IS NOT NULL")
5 8  
6 9 Created = 1
... ...
app/models/issue.rb
  Diff comments   View file @0439387
... ... @@ -2,6 +2,9 @@ class Issue &lt; ActiveRecord::Base
2 2 include IssueCommonality
3 3 include Votes
4 4  
  5 + attr_accessible :title, :assignee_id, :closed, :position, :description,
  6 + :milestone_id, :label_list, :author_id_of_changes
  7 +
5 8 acts_as_taggable_on :labels
6 9  
7 10 belongs_to :milestone
... ...
app/models/key.rb
  Diff comments   View file @0439387
... ... @@ -4,7 +4,7 @@ class Key &lt; ActiveRecord::Base
4 4 belongs_to :user
5 5 belongs_to :project
6 6  
7   - attr_protected :user_id
  7 + attr_accessible :key, :title
8 8  
9 9 validates :title,
10 10 presence: true,
... ...
app/models/merge_request.rb
  Diff comments   View file @0439387
... ... @@ -4,6 +4,9 @@ class MergeRequest &lt; ActiveRecord::Base
4 4 include IssueCommonality
5 5 include Votes
6 6  
  7 + attr_accessible :title, :assignee_id, :closed, :target_branch, :source_branch,
  8 + :author_id_of_changes
  9 +
7 10 BROKEN_DIFF = "--broken-diff"
8 11  
9 12 UNCHECKED = 1
... ... @@ -48,7 +51,8 @@ class MergeRequest &lt; ActiveRecord::Base
48 51 end
49 52  
50 53 def mark_as_unchecked
51   - self.update_attributes(state: UNCHECKED)
  54 + self.state = UNCHECKED
  55 + self.save
52 56 end
53 57  
54 58 def can_be_merged?
... ...
app/models/milestone.rb
  Diff comments   View file @0439387
... ... @@ -13,6 +13,8 @@
13 13 #
14 14  
15 15 class Milestone < ActiveRecord::Base
  16 + attr_accessible :title, :description, :due_date, :closed
  17 +
16 18 belongs_to :project
17 19 has_many :issues
18 20  
... ...
app/models/note.rb
  Diff comments   View file @0439387
... ... @@ -2,6 +2,9 @@ require &#39;carrierwave/orm/activerecord&#39;
2 2 require 'file_size_validator'
3 3  
4 4 class Note < ActiveRecord::Base
  5 + attr_accessible :note, :noteable, :noteable_id, :noteable_type, :project_id,
  6 + :attachment, :line_code
  7 +
5 8 belongs_to :project
6 9 belongs_to :noteable, polymorphic: true
7 10 belongs_to :author,
... ... @@ -16,7 +19,6 @@ class Note &lt; ActiveRecord::Base
16 19 to: :author,
17 20 prefix: true
18 21  
19   - attr_protected :author, :author_id
20 22 attr_accessor :notify
21 23 attr_accessor :notify_author
22 24  
... ...
app/models/project.rb
  Diff comments   View file @0439387
... ... @@ -6,6 +6,9 @@ class Project &lt; ActiveRecord::Base
6 6 include Authority
7 7 include Team
8 8  
  9 + attr_accessible :name, :path, :description, :code, :default_branch, :issues_enabled,
  10 + :wall_enabled, :merge_requests_enabled, :wiki_enabled
  11 +
9 12 #
10 13 # Relations
11 14 #
... ... @@ -26,11 +29,6 @@ class Project &lt; ActiveRecord::Base
26 29 attr_accessor :error_code
27 30  
28 31 #
29   - # Protected attributes
30   - #
31   - attr_protected :private_flag, :owner_id
32   -
33   - #
34 32 # Scopes
35 33 #
36 34 scope :public_only, where(private_flag: false)
... ...
app/models/protected_branch.rb
  Diff comments   View file @0439387
1 1 class ProtectedBranch < ActiveRecord::Base
2 2 include GitHost
3 3  
  4 + attr_accessible :name
  5 +
4 6 belongs_to :project
5 7 validates_presence_of :project_id
6 8 validates_presence_of :name
... ...
app/models/snippet.rb
  Diff comments   View file @0439387
1 1 class Snippet < ActiveRecord::Base
2 2 include Linguist::BlobHelper
3 3  
  4 + attr_accessible :title, :content, :file_name, :expires_at
  5 +
4 6 belongs_to :project
5 7 belongs_to :author, class_name: "User"
6 8 has_many :notes, as: :noteable, dependent: :destroy
... ... @@ -9,7 +11,6 @@ class Snippet &lt; ActiveRecord::Base
9 11 :email,
10 12 to: :author,
11 13 prefix: true
12   - attr_protected :author, :author_id, :project, :project_id
13 14  
14 15 validates_presence_of :project_id
15 16 validates_presence_of :author_id
... ... @@ -46,11 +47,11 @@ class Snippet &lt; ActiveRecord::Base
46 47 0
47 48 end
48 49  
49   - def name
  50 + def name
50 51 file_name
51 52 end
52 53  
53   - def mode
  54 + def mode
54 55 nil
55 56 end
56 57  
... ...
app/models/users_project.rb
  Diff comments   View file @0439387
... ... @@ -6,11 +6,11 @@ class UsersProject &lt; ActiveRecord::Base
6 6 DEVELOPER = 30
7 7 MASTER = 40
8 8  
  9 + attr_accessible :user, :user_id, :project_access
  10 +
9 11 belongs_to :user
10 12 belongs_to :project
11 13  
12   - attr_protected :project_id, :project
13   -
14 14 after_save :update_repository
15 15 after_destroy :update_repository
16 16  
... ...
app/models/web_hook.rb
  Diff comments   View file @0439387
1 1 class WebHook < ActiveRecord::Base
2 2 include HTTParty
3 3  
  4 + attr_accessible :url
  5 +
4 6 # HTTParty timeout
5 7 default_timeout 10
6 8  
... ... @@ -18,11 +20,11 @@ class WebHook &lt; ActiveRecord::Base
18 20 post_url = url.gsub(parsed_url.userinfo+"@", "")
19 21 WebHook.post(post_url,
20 22 body: data.to_json,
21   - headers: { "Content-Type" => "application/json" },
  23 + headers: { "Content-Type" => "application/json" },
22 24 basic_auth: {username: parsed_url.user, password: parsed_url.password})
23 25 end
24 26 end
25   -
  27 +
26 28 end
27 29 # == Schema Information
28 30 #
... ...
app/models/wiki.rb
  Diff comments   View file @0439387
1 1 class Wiki < ActiveRecord::Base
  2 + attr_accessible :title, :content, :slug
  3 +
2 4 belongs_to :project
3 5 belongs_to :user
4 6 has_many :notes, as: :noteable, dependent: :destroy
... ...
app/roles/issue_commonality.rb
  Diff comments   View file @0439387
... ... @@ -3,8 +3,6 @@ module IssueCommonality
3 3 extend ActiveSupport::Concern
4 4  
5 5 included do
6   - attr_protected :author, :author_id, :project, :project_id
7   -
8 6 belongs_to :project
9 7 belongs_to :author, class_name: "User"
10 8 belongs_to :assignee, class_name: "User"
... ...
config/application.rb
  Diff comments   View file @0439387
... ... @@ -39,6 +39,12 @@ module Gitlab
39 39 # Configure sensitive parameters which will be filtered from the log file.
40 40 config.filter_parameters += [:password]
41 41  
  42 + # Enforce whitelist mode for mass assignment.
  43 + # This will create an empty whitelist of attributes available for mass-assignment for all models
  44 + # in your app. As such, your models will need to explicitly whitelist or blacklist accessible
  45 + # parameters by using an attr_accessible or attr_protected declaration.
  46 + config.active_record.whitelist_attributes = true
  47 +
42 48 # Enable the asset pipeline
43 49 config.assets.enabled = true
44 50  
... ...
config/environments/development.rb
  Diff comments   View file @0439387
... ... @@ -33,7 +33,7 @@ Gitlab::Application.configure do
33 33  
34 34 # Raise exception on mass assignment protection for Active Record models
35 35 config.active_record.mass_assignment_sanitizer = :strict
36   -
  36 +
37 37 # Log the query plan for queries taking more than this (works
38 38 # with SQLite, MySQL, and PostgreSQL)
39 39 config.active_record.auto_explain_threshold_in_seconds = 0.5
... ...
config/environments/test.rb
  Diff comments   View file @0439387
... ... @@ -34,6 +34,9 @@ Gitlab::Application.configure do
34 34 # like if you have constraints or database-specific column types
35 35 # config.active_record.schema_format = :sql
36 36  
  37 + # Raise exception on mass assignment protection for Active Record models
  38 + # config.active_record.mass_assignment_sanitizer = :strict
  39 +
37 40 # Print deprecation notices to the stderr
38 41 config.active_support.deprecation = :stderr
39 42  
... ...
lib/gitlab/auth.rb
  Diff comments   View file @0439387
... ... @@ -30,7 +30,7 @@ module Gitlab
30 30 log.info "#{ldap_prefix}Creating user from #{provider} login"\
31 31 " {uid => #{uid}, name => #{name}, email => #{email}}"
32 32 password = Devise.friendly_token[0, 8].downcase
33   - @user = User.new(
  33 + @user = User.new({
34 34 extern_uid: uid,
35 35 provider: provider,
36 36 name: name,
... ... @@ -38,7 +38,7 @@ module Gitlab
38 38 password: password,
39 39 password_confirmation: password,
40 40 projects_limit: Gitlab.config.default_projects_limit,
41   - )
  41 + }, as: :admin)
42 42 if Gitlab.config.omniauth['block_auto_created_users'] && !ldap
43 43 @user.blocked = true
44 44 end
... ...
spec/models/issue_spec.rb
  Diff comments   View file @0439387
... ... @@ -5,6 +5,11 @@ describe Issue do
5 5 it { should belong_to(:milestone) }
6 6 end
7 7  
  8 + describe "Mass assignment" do
  9 + it { should_not allow_mass_assignment_of(:author_id) }
  10 + it { should_not allow_mass_assignment_of(:project_id) }
  11 + end
  12 +
8 13 describe "Validation" do
9 14 it { should ensure_length_of(:description).is_within(0..2000) }
10 15 it { should ensure_inclusion_of(:closed).in_array([true, false]) }
... ...
spec/models/key_spec.rb
  Diff comments   View file @0439387
... ... @@ -6,6 +6,11 @@ describe Key do
6 6 it { should belong_to(:project) }
7 7 end
8 8  
  9 + describe "Mass assignment" do
  10 + it { should_not allow_mass_assignment_of(:project_id) }
  11 + it { should_not allow_mass_assignment_of(:user_id) }
  12 + end
  13 +
9 14 describe "Validation" do
10 15 it { should validate_presence_of(:title) }
11 16 it { should validate_presence_of(:key) }
... ...
spec/models/merge_request_spec.rb
  Diff comments   View file @0439387
... ... @@ -6,6 +6,11 @@ describe MergeRequest do
6 6 it { should validate_presence_of(:source_branch) }
7 7 end
8 8  
  9 + describe "Mass assignment" do
  10 + it { should_not allow_mass_assignment_of(:author_id) }
  11 + it { should_not allow_mass_assignment_of(:project_id) }
  12 + end
  13 +
9 14 describe 'modules' do
10 15 it { should include_module(IssueCommonality) }
11 16 it { should include_module(Votes) }
... ...
spec/models/milestone_spec.rb
  Diff comments   View file @0439387
... ... @@ -6,6 +6,10 @@ describe Milestone do
6 6 it { should have_many(:issues) }
7 7 end
8 8  
  9 + describe "Mass assignment" do
  10 + it { should_not allow_mass_assignment_of(:project_id) }
  11 + end
  12 +
9 13 describe "Validation" do
10 14 it { should validate_presence_of(:title) }
11 15 it { should validate_presence_of(:project_id) }
... ...
spec/models/note_spec.rb
  Diff comments   View file @0439387
... ... @@ -7,6 +7,11 @@ describe Note do
7 7 it { should belong_to(:author).class_name('User') }
8 8 end
9 9  
  10 + describe "Mass assignment" do
  11 + it { should_not allow_mass_assignment_of(:author) }
  12 + it { should_not allow_mass_assignment_of(:author_id) }
  13 + end
  14 +
10 15 describe "Validation" do
11 16 it { should validate_presence_of(:note) }
12 17 it { should validate_presence_of(:project) }
... ...
spec/models/project_spec.rb
  Diff comments   View file @0439387
... ... @@ -17,6 +17,11 @@ describe Project do
17 17 it { should have_many(:protected_branches).dependent(:destroy) }
18 18 end
19 19  
  20 + describe "Mass assignment" do
  21 + it { should_not allow_mass_assignment_of(:owner_id) }
  22 + it { should_not allow_mass_assignment_of(:private_flag) }
  23 + end
  24 +
20 25 describe "Validation" do
21 26 let!(:project) { create(:project) }
22 27  
... ...
spec/models/protected_branch_spec.rb
  Diff comments   View file @0439387
... ... @@ -5,6 +5,10 @@ describe ProtectedBranch do
5 5 it { should belong_to(:project) }
6 6 end
7 7  
  8 + describe "Mass assignment" do
  9 + it { should_not allow_mass_assignment_of(:project_id) }
  10 + end
  11 +
8 12 describe 'Validation' do
9 13 it { should validate_presence_of(:project_id) }
10 14 it { should validate_presence_of(:name) }
... ...
spec/models/snippet_spec.rb
  Diff comments   View file @0439387
... ... @@ -7,6 +7,11 @@ describe Snippet do
7 7 it { should have_many(:notes).dependent(:destroy) }
8 8 end
9 9  
  10 + describe "Mass assignment" do
  11 + it { should_not allow_mass_assignment_of(:author_id) }
  12 + it { should_not allow_mass_assignment_of(:project_id) }
  13 + end
  14 +
10 15 describe "Validation" do
11 16 it { should validate_presence_of(:author_id) }
12 17 it { should validate_presence_of(:project_id) }
... ...
spec/models/user_spec.rb
  Diff comments   View file @0439387
... ... @@ -15,6 +15,11 @@ describe User do
15 15 it { should have_many(:assigned_merge_requests).dependent(:destroy) }
16 16 end
17 17  
  18 + describe "Mass assignment" do
  19 + it { should_not allow_mass_assignment_of(:projects_limit) }
  20 + it { should allow_mass_assignment_of(:projects_limit).as(:admin) }
  21 + end
  22 +
18 23 describe 'validations' do
19 24 it { should validate_presence_of(:projects_limit) }
20 25 it { should validate_numericality_of(:projects_limit) }
... ... @@ -73,30 +78,4 @@ describe User do
73 78 user.authentication_token.should_not be_blank
74 79 end
75 80 end
76   -
77   - describe "attributes can be changed by a regular user" do
78   - before do
79   - @user = Factory :user
80   - @user.update_attributes(skype: "testskype", linkedin: "testlinkedin")
81   - end
82   - it { @user.skype.should == 'testskype' }
83   - it { @user.linkedin.should == 'testlinkedin' }
84   - end
85   -
86   - describe "attributes that shouldn't be changed by a regular user" do
87   - before do
88   - @user = Factory :user
89   - @user.update_attributes(projects_limit: 50)
90   - end
91   - it { @user.projects_limit.should_not == 50 }
92   - end
93   -
94   - describe "attributes can be changed by an admin user" do
95   - before do
96   - @admin_user = Factory :admin
97   - @admin_user.update_attributes({ skype: "testskype", projects_limit: 50 }, as: :admin)
98   - end
99   - it { @admin_user.skype.should == 'testskype' }
100   - it { @admin_user.projects_limit.should == 50 }
101   - end
102 81 end
... ...
spec/models/users_project_spec.rb
  Diff comments   View file @0439387
... ... @@ -6,6 +6,10 @@ describe UsersProject do
6 6 it { should belong_to(:user) }
7 7 end
8 8  
  9 + describe "Mass assignment" do
  10 + it { should_not allow_mass_assignment_of(:project_id) }
  11 + end
  12 +
9 13 describe "Validation" do
10 14 let!(:users_project) { create(:users_project) }
11 15  
... ...
spec/models/web_hook_spec.rb
  Diff comments   View file @0439387
... ... @@ -5,6 +5,10 @@ describe ProjectHook do
5 5 it { should belong_to :project }
6 6 end
7 7  
  8 + describe "Mass assignment" do
  9 + it { should_not allow_mass_assignment_of(:project_id) }
  10 + end
  11 +
8 12 describe "Validations" do
9 13 it { should validate_presence_of(:url) }
10 14  
... ...
spec/models/wiki_spec.rb
  Diff comments   View file @0439387
... ... @@ -7,6 +7,11 @@ describe Wiki do
7 7 it { should have_many(:notes).dependent(:destroy) }
8 8 end
9 9  
  10 + describe "Mass assignment" do
  11 + it { should_not allow_mass_assignment_of(:project_id) }
  12 + it { should_not allow_mass_assignment_of(:user_id) }
  13 + end
  14 +
10 15 describe "Validation" do
11 16 it { should validate_presence_of(:title) }
12 17 it { should ensure_length_of(:title).is_within(1..250) }
... ...