Commit aefe2e952f33267ce38fb9270400f4f6f194d37b
1 parent
a8eb525e
Exists in
master
and in
4 other branches
Fixing unsafe use of Thread.current variable :current_user
Showing
14 changed files
with
196 additions
and
120 deletions
Show diff stats
app/controllers/application_controller.rb
| ... | ... | @@ -2,7 +2,7 @@ class ApplicationController < ActionController::Base |
| 2 | 2 | before_filter :authenticate_user! |
| 3 | 3 | before_filter :reject_blocked! |
| 4 | 4 | before_filter :check_password_expiration |
| 5 | - before_filter :set_current_user_for_thread | |
| 5 | + around_filter :set_current_user_for_thread | |
| 6 | 6 | before_filter :add_abilities |
| 7 | 7 | before_filter :dev_tools if Rails.env == 'development' |
| 8 | 8 | before_filter :default_headers |
| ... | ... | @@ -50,6 +50,11 @@ class ApplicationController < ActionController::Base |
| 50 | 50 | |
| 51 | 51 | def set_current_user_for_thread |
| 52 | 52 | Thread.current[:current_user] = current_user |
| 53 | + begin | |
| 54 | + yield | |
| 55 | + ensure | |
| 56 | + Thread.current[:current_user] = nil | |
| 57 | + end | |
| 53 | 58 | end |
| 54 | 59 | |
| 55 | 60 | def abilities | ... | ... |
db/fixtures/development/09_issues.rb
| ... | ... | @@ -11,17 +11,22 @@ Gitlab::Seeder.quiet do |
| 11 | 11 | next unless user |
| 12 | 12 | |
| 13 | 13 | user_id = user.id |
| 14 | - Thread.current[:current_user] = user | |
| 15 | 14 | |
| 16 | - Issue.seed(:id, [{ | |
| 17 | - id: i, | |
| 18 | - project_id: project.id, | |
| 19 | - author_id: user_id, | |
| 20 | - assignee_id: user_id, | |
| 21 | - state: ['opened', 'closed'].sample, | |
| 22 | - milestone: project.milestones.sample, | |
| 23 | - title: Faker::Lorem.sentence(6) | |
| 24 | - }]) | |
| 15 | + begin | |
| 16 | + Thread.current[:current_user] = user | |
| 17 | + | |
| 18 | + Issue.seed(:id, [{ | |
| 19 | + id: i, | |
| 20 | + project_id: project.id, | |
| 21 | + author_id: user_id, | |
| 22 | + assignee_id: user_id, | |
| 23 | + state: ['opened', 'closed'].sample, | |
| 24 | + milestone: project.milestones.sample, | |
| 25 | + title: Faker::Lorem.sentence(6) | |
| 26 | + }]) | |
| 27 | + ensure | |
| 28 | + Thread.current[:current_user] = nil | |
| 29 | + end | |
| 25 | 30 | print('.') |
| 26 | 31 | end |
| 27 | 32 | ... | ... |
db/fixtures/development/10_merge_requests.rb
| ... | ... | @@ -17,19 +17,23 @@ Gitlab::Seeder.quiet do |
| 17 | 17 | next if branches.uniq.size < 2 |
| 18 | 18 | |
| 19 | 19 | user_id = user.id |
| 20 | - Thread.current[:current_user] = user | |
| 21 | - | |
| 22 | - MergeRequest.seed(:id, [{ | |
| 23 | - id: i, | |
| 24 | - source_branch: branches.first, | |
| 25 | - target_branch: branches.last, | |
| 26 | - source_project_id: project.id, | |
| 27 | - target_project_id: project.id, | |
| 28 | - author_id: user_id, | |
| 29 | - assignee_id: user_id, | |
| 30 | - milestone: project.milestones.sample, | |
| 31 | - title: Faker::Lorem.sentence(6) | |
| 32 | - }]) | |
| 20 | + begin | |
| 21 | + Thread.current[:current_user] = user | |
| 22 | + | |
| 23 | + MergeRequest.seed(:id, [{ | |
| 24 | + id: i, | |
| 25 | + source_branch: branches.first, | |
| 26 | + target_branch: branches.last, | |
| 27 | + source_project_id: project.id, | |
| 28 | + target_project_id: project.id, | |
| 29 | + author_id: user_id, | |
| 30 | + assignee_id: user_id, | |
| 31 | + milestone: project.milestones.sample, | |
| 32 | + title: Faker::Lorem.sentence(6) | |
| 33 | + }]) | |
| 34 | + ensure | |
| 35 | + Thread.current[:current_user] = nil | |
| 36 | + end | |
| 33 | 37 | print('.') |
| 34 | 38 | end |
| 35 | 39 | end | ... | ... |
features/support/env.rb
lib/api/helpers.rb
| ... | ... | @@ -31,6 +31,15 @@ module API |
| 31 | 31 | end |
| 32 | 32 | end |
| 33 | 33 | |
| 34 | + def set_current_user_for_thread | |
| 35 | + Thread.current[:current_user] = current_user | |
| 36 | + begin | |
| 37 | + yield | |
| 38 | + ensure | |
| 39 | + Thread.current[:current_user] = nil | |
| 40 | + end | |
| 41 | + end | |
| 42 | + | |
| 34 | 43 | def user_project |
| 35 | 44 | @project ||= find_project(params[:id]) |
| 36 | 45 | @project || not_found! | ... | ... |
lib/api/issues.rb
| ... | ... | @@ -2,7 +2,6 @@ module API |
| 2 | 2 | # Issues API |
| 3 | 3 | class Issues < Grape::API |
| 4 | 4 | before { authenticate! } |
| 5 | - before { Thread.current[:current_user] = current_user } | |
| 6 | 5 | |
| 7 | 6 | resource :issues do |
| 8 | 7 | # Get currently authenticated user's issues |
| ... | ... | @@ -49,15 +48,17 @@ module API |
| 49 | 48 | # Example Request: |
| 50 | 49 | # POST /projects/:id/issues |
| 51 | 50 | post ":id/issues" do |
| 52 | - required_attributes! [:title] | |
| 53 | - attrs = attributes_for_keys [:title, :description, :assignee_id, :milestone_id] | |
| 54 | - attrs[:label_list] = params[:labels] if params[:labels].present? | |
| 55 | - @issue = user_project.issues.new attrs | |
| 56 | - @issue.author = current_user | |
| 57 | - if @issue.save | |
| 58 | - present @issue, with: Entities::Issue | |
| 59 | - else | |
| 60 | - not_found! | |
| 51 | + set_current_user_for_thread do | |
| 52 | + required_attributes! [:title] | |
| 53 | + attrs = attributes_for_keys [:title, :description, :assignee_id, :milestone_id] | |
| 54 | + attrs[:label_list] = params[:labels] if params[:labels].present? | |
| 55 | + @issue = user_project.issues.new attrs | |
| 56 | + @issue.author = current_user | |
| 57 | + if @issue.save | |
| 58 | + present @issue, with: Entities::Issue | |
| 59 | + else | |
| 60 | + not_found! | |
| 61 | + end | |
| 61 | 62 | end |
| 62 | 63 | end |
| 63 | 64 | |
| ... | ... | @@ -75,16 +76,18 @@ module API |
| 75 | 76 | # Example Request: |
| 76 | 77 | # PUT /projects/:id/issues/:issue_id |
| 77 | 78 | put ":id/issues/:issue_id" do |
| 78 | - @issue = user_project.issues.find(params[:issue_id]) | |
| 79 | - authorize! :modify_issue, @issue | |
| 79 | + set_current_user_for_thread do | |
| 80 | + @issue = user_project.issues.find(params[:issue_id]) | |
| 81 | + authorize! :modify_issue, @issue | |
| 80 | 82 | |
| 81 | - attrs = attributes_for_keys [:title, :description, :assignee_id, :milestone_id, :state_event] | |
| 82 | - attrs[:label_list] = params[:labels] if params[:labels].present? | |
| 83 | + attrs = attributes_for_keys [:title, :description, :assignee_id, :milestone_id, :state_event] | |
| 84 | + attrs[:label_list] = params[:labels] if params[:labels].present? | |
| 83 | 85 | |
| 84 | - if @issue.update_attributes attrs | |
| 85 | - present @issue, with: Entities::Issue | |
| 86 | - else | |
| 87 | - not_found! | |
| 86 | + if @issue.update_attributes attrs | |
| 87 | + present @issue, with: Entities::Issue | |
| 88 | + else | |
| 89 | + not_found! | |
| 90 | + end | |
| 88 | 91 | end |
| 89 | 92 | end |
| 90 | 93 | ... | ... |
lib/api/merge_requests.rb
| ... | ... | @@ -2,7 +2,6 @@ module API |
| 2 | 2 | # MergeRequest API |
| 3 | 3 | class MergeRequests < Grape::API |
| 4 | 4 | before { authenticate! } |
| 5 | - before { Thread.current[:current_user] = current_user } | |
| 6 | 5 | |
| 7 | 6 | resource :projects do |
| 8 | 7 | helpers do |
| ... | ... | @@ -70,28 +69,30 @@ module API |
| 70 | 69 | # POST /projects/:id/merge_requests |
| 71 | 70 | # |
| 72 | 71 | post ":id/merge_requests" do |
| 73 | - authorize! :write_merge_request, user_project | |
| 74 | - required_attributes! [:source_branch, :target_branch, :title] | |
| 75 | - attrs = attributes_for_keys [:source_branch, :target_branch, :assignee_id, :title, :target_project_id] | |
| 76 | - merge_request = user_project.merge_requests.new(attrs) | |
| 77 | - merge_request.author = current_user | |
| 78 | - merge_request.source_project = user_project | |
| 79 | - target_project_id = attrs[:target_project_id] | |
| 80 | - if not_fork?(target_project_id, user_project) | |
| 81 | - merge_request.target_project = user_project | |
| 82 | - else | |
| 83 | - if target_matches_fork(target_project_id,user_project) | |
| 84 | - merge_request.target_project = Project.find_by_id(attrs[:target_project_id]) | |
| 72 | + set_current_user_for_thread do | |
| 73 | + authorize! :write_merge_request, user_project | |
| 74 | + required_attributes! [:source_branch, :target_branch, :title] | |
| 75 | + attrs = attributes_for_keys [:source_branch, :target_branch, :assignee_id, :title, :target_project_id] | |
| 76 | + merge_request = user_project.merge_requests.new(attrs) | |
| 77 | + merge_request.author = current_user | |
| 78 | + merge_request.source_project = user_project | |
| 79 | + target_project_id = attrs[:target_project_id] | |
| 80 | + if not_fork?(target_project_id, user_project) | |
| 81 | + merge_request.target_project = user_project | |
| 85 | 82 | else |
| 86 | - render_api_error!('(Bad Request) Specified target project that is not the source project, or the source fork of the project.', 400) | |
| 83 | + if target_matches_fork(target_project_id,user_project) | |
| 84 | + merge_request.target_project = Project.find_by_id(attrs[:target_project_id]) | |
| 85 | + else | |
| 86 | + render_api_error!('(Bad Request) Specified target project that is not the source project, or the source fork of the project.', 400) | |
| 87 | + end | |
| 87 | 88 | end |
| 88 | - end | |
| 89 | 89 | |
| 90 | - if merge_request.save | |
| 91 | - merge_request.reload_code | |
| 92 | - present merge_request, with: Entities::MergeRequest | |
| 93 | - else | |
| 94 | - handle_merge_request_errors! merge_request.errors | |
| 90 | + if merge_request.save | |
| 91 | + merge_request.reload_code | |
| 92 | + present merge_request, with: Entities::MergeRequest | |
| 93 | + else | |
| 94 | + handle_merge_request_errors! merge_request.errors | |
| 95 | + end | |
| 95 | 96 | end |
| 96 | 97 | end |
| 97 | 98 | |
| ... | ... | @@ -109,17 +110,19 @@ module API |
| 109 | 110 | # PUT /projects/:id/merge_request/:merge_request_id |
| 110 | 111 | # |
| 111 | 112 | put ":id/merge_request/:merge_request_id" do |
| 112 | - attrs = attributes_for_keys [:source_branch, :target_branch, :assignee_id, :title, :state_event] | |
| 113 | - merge_request = user_project.merge_requests.find(params[:merge_request_id]) | |
| 113 | + set_current_user_for_thread do | |
| 114 | + attrs = attributes_for_keys [:source_branch, :target_branch, :assignee_id, :title, :state_event] | |
| 115 | + merge_request = user_project.merge_requests.find(params[:merge_request_id]) | |
| 114 | 116 | |
| 115 | - authorize! :modify_merge_request, merge_request | |
| 117 | + authorize! :modify_merge_request, merge_request | |
| 116 | 118 | |
| 117 | - if merge_request.update_attributes attrs | |
| 118 | - merge_request.reload_code | |
| 119 | - merge_request.mark_as_unchecked | |
| 120 | - present merge_request, with: Entities::MergeRequest | |
| 121 | - else | |
| 122 | - handle_merge_request_errors! merge_request.errors | |
| 119 | + if merge_request.update_attributes attrs | |
| 120 | + merge_request.reload_code | |
| 121 | + merge_request.mark_as_unchecked | |
| 122 | + present merge_request, with: Entities::MergeRequest | |
| 123 | + else | |
| 124 | + handle_merge_request_errors! merge_request.errors | |
| 125 | + end | |
| 123 | 126 | end |
| 124 | 127 | end |
| 125 | 128 | |
| ... | ... | @@ -133,16 +136,18 @@ module API |
| 133 | 136 | # POST /projects/:id/merge_request/:merge_request_id/comments |
| 134 | 137 | # |
| 135 | 138 | post ":id/merge_request/:merge_request_id/comments" do |
| 136 | - required_attributes! [:note] | |
| 139 | + set_current_user_for_thread do | |
| 140 | + required_attributes! [:note] | |
| 137 | 141 | |
| 138 | - merge_request = user_project.merge_requests.find(params[:merge_request_id]) | |
| 139 | - note = merge_request.notes.new(note: params[:note], project_id: user_project.id) | |
| 140 | - note.author = current_user | |
| 142 | + merge_request = user_project.merge_requests.find(params[:merge_request_id]) | |
| 143 | + note = merge_request.notes.new(note: params[:note], project_id: user_project.id) | |
| 144 | + note.author = current_user | |
| 141 | 145 | |
| 142 | - if note.save | |
| 143 | - present note, with: Entities::MRNote | |
| 144 | - else | |
| 145 | - not_found! | |
| 146 | + if note.save | |
| 147 | + present note, with: Entities::MRNote | |
| 148 | + else | |
| 149 | + not_found! | |
| 150 | + end | |
| 146 | 151 | end |
| 147 | 152 | end |
| 148 | 153 | ... | ... |
lib/api/milestones.rb
| ... | ... | @@ -40,15 +40,17 @@ module API |
| 40 | 40 | # Example Request: |
| 41 | 41 | # POST /projects/:id/milestones |
| 42 | 42 | post ":id/milestones" do |
| 43 | - authorize! :admin_milestone, user_project | |
| 44 | - required_attributes! [:title] | |
| 43 | + set_current_user_for_thread do | |
| 44 | + authorize! :admin_milestone, user_project | |
| 45 | + required_attributes! [:title] | |
| 45 | 46 | |
| 46 | - attrs = attributes_for_keys [:title, :description, :due_date] | |
| 47 | - @milestone = user_project.milestones.new attrs | |
| 48 | - if @milestone.save | |
| 49 | - present @milestone, with: Entities::Milestone | |
| 50 | - else | |
| 51 | - not_found! | |
| 47 | + attrs = attributes_for_keys [:title, :description, :due_date] | |
| 48 | + @milestone = user_project.milestones.new attrs | |
| 49 | + if @milestone.save | |
| 50 | + present @milestone, with: Entities::Milestone | |
| 51 | + else | |
| 52 | + not_found! | |
| 53 | + end | |
| 52 | 54 | end |
| 53 | 55 | end |
| 54 | 56 | |
| ... | ... | @@ -64,14 +66,16 @@ module API |
| 64 | 66 | # Example Request: |
| 65 | 67 | # PUT /projects/:id/milestones/:milestone_id |
| 66 | 68 | put ":id/milestones/:milestone_id" do |
| 67 | - authorize! :admin_milestone, user_project | |
| 69 | + set_current_user_for_thread do | |
| 70 | + authorize! :admin_milestone, user_project | |
| 68 | 71 | |
| 69 | - @milestone = user_project.milestones.find(params[:milestone_id]) | |
| 70 | - attrs = attributes_for_keys [:title, :description, :due_date, :state_event] | |
| 71 | - if @milestone.update_attributes attrs | |
| 72 | - present @milestone, with: Entities::Milestone | |
| 73 | - else | |
| 74 | - not_found! | |
| 72 | + @milestone = user_project.milestones.find(params[:milestone_id]) | |
| 73 | + attrs = attributes_for_keys [:title, :description, :due_date, :state_event] | |
| 74 | + if @milestone.update_attributes attrs | |
| 75 | + present @milestone, with: Entities::Milestone | |
| 76 | + else | |
| 77 | + not_found! | |
| 78 | + end | |
| 75 | 79 | end |
| 76 | 80 | end |
| 77 | 81 | end | ... | ... |
lib/api/notes.rb
| ... | ... | @@ -41,17 +41,19 @@ module API |
| 41 | 41 | # Example Request: |
| 42 | 42 | # POST /projects/:id/notes |
| 43 | 43 | post ":id/notes" do |
| 44 | - required_attributes! [:body] | |
| 44 | + set_current_user_for_thread do | |
| 45 | + required_attributes! [:body] | |
| 45 | 46 | |
| 46 | - @note = user_project.notes.new(note: params[:body]) | |
| 47 | - @note.author = current_user | |
| 47 | + @note = user_project.notes.new(note: params[:body]) | |
| 48 | + @note.author = current_user | |
| 48 | 49 | |
| 49 | - if @note.save | |
| 50 | - present @note, with: Entities::Note | |
| 51 | - else | |
| 52 | - # :note is exposed as :body, but :note is set on error | |
| 53 | - bad_request!(:note) if @note.errors[:note].any? | |
| 54 | - not_found! | |
| 50 | + if @note.save | |
| 51 | + present @note, with: Entities::Note | |
| 52 | + else | |
| 53 | + # :note is exposed as :body, but :note is set on error | |
| 54 | + bad_request!(:note) if @note.errors[:note].any? | |
| 55 | + not_found! | |
| 56 | + end | |
| 55 | 57 | end |
| 56 | 58 | end |
| 57 | 59 | |
| ... | ... | @@ -97,17 +99,19 @@ module API |
| 97 | 99 | # POST /projects/:id/issues/:noteable_id/notes |
| 98 | 100 | # POST /projects/:id/snippets/:noteable_id/notes |
| 99 | 101 | post ":id/#{noteables_str}/:#{noteable_id_str}/notes" do |
| 100 | - required_attributes! [:body] | |
| 102 | + set_current_user_for_thread do | |
| 103 | + required_attributes! [:body] | |
| 101 | 104 | |
| 102 | - @noteable = user_project.send(:"#{noteables_str}").find(params[:"#{noteable_id_str}"]) | |
| 103 | - @note = @noteable.notes.new(note: params[:body]) | |
| 104 | - @note.author = current_user | |
| 105 | - @note.project = user_project | |
| 105 | + @noteable = user_project.send(:"#{noteables_str}").find(params[:"#{noteable_id_str}"]) | |
| 106 | + @note = @noteable.notes.new(note: params[:body]) | |
| 107 | + @note.author = current_user | |
| 108 | + @note.project = user_project | |
| 106 | 109 | |
| 107 | - if @note.save | |
| 108 | - present @note, with: Entities::Note | |
| 109 | - else | |
| 110 | - not_found! | |
| 110 | + if @note.save | |
| 111 | + present @note, with: Entities::Note | |
| 112 | + else | |
| 113 | + not_found! | |
| 114 | + end | |
| 111 | 115 | end |
| 112 | 116 | end |
| 113 | 117 | end | ... | ... |
spec/models/project_spec.rb
| ... | ... | @@ -27,14 +27,8 @@ |
| 27 | 27 | require 'spec_helper' |
| 28 | 28 | |
| 29 | 29 | describe Project do |
| 30 | - let(:user) { create(:user) } | |
| 31 | - | |
| 32 | - before do | |
| 33 | - enable_observers | |
| 34 | - Thread.current[:current_user] = user | |
| 35 | - end | |
| 36 | - | |
| 37 | - after { disable_observers } | |
| 30 | + before { enable_observers } | |
| 31 | + after { disable_observers } | |
| 38 | 32 | |
| 39 | 33 | describe "Associations" do |
| 40 | 34 | it { should belong_to(:group) } | ... | ... |
spec/requests/api/issues_spec.rb
| ... | ... | @@ -100,4 +100,16 @@ describe API::API do |
| 100 | 100 | response.status.should == 405 |
| 101 | 101 | end |
| 102 | 102 | end |
| 103 | + | |
| 104 | + describe "PUT /projects/:id/issues/:issue_id to test observer on close" do | |
| 105 | + before { enable_observers } | |
| 106 | + after { disable_observers } | |
| 107 | + | |
| 108 | + it "should create an activity event when an issue is closed" do | |
| 109 | + Event.should_receive(:create) | |
| 110 | + | |
| 111 | + put api("/projects/#{project.id}/issues/#{issue.id}", user), | |
| 112 | + state_event: "close" | |
| 113 | + end | |
| 114 | + end | |
| 103 | 115 | end | ... | ... |
spec/requests/api/milestones_spec.rb
| ... | ... | @@ -90,4 +90,16 @@ describe API::API do |
| 90 | 90 | json_response['state'].should == 'closed' |
| 91 | 91 | end |
| 92 | 92 | end |
| 93 | + | |
| 94 | + describe "PUT /projects/:id/milestones/:milestone_id to test observer on close" do | |
| 95 | + before { enable_observers } | |
| 96 | + after { disable_observers } | |
| 97 | + | |
| 98 | + it "should create an activity event when an milestone is closed" do | |
| 99 | + Event.should_receive(:create) | |
| 100 | + | |
| 101 | + put api("/projects/#{project.id}/milestones/#{milestone.id}", user), | |
| 102 | + state_event: 'close' | |
| 103 | + end | |
| 104 | + end | |
| 93 | 105 | end | ... | ... |
spec/requests/api/notes_spec.rb
| ... | ... | @@ -176,4 +176,16 @@ describe API::API do |
| 176 | 176 | end |
| 177 | 177 | end |
| 178 | 178 | end |
| 179 | + | |
| 180 | + describe "POST /projects/:id/noteable/:noteable_id/notes to test observer on create" do | |
| 181 | + before { enable_observers } | |
| 182 | + after { disable_observers } | |
| 183 | + | |
| 184 | + it "should create an activity event when an issue note is created" do | |
| 185 | + Event.should_receive(:create) | |
| 186 | + | |
| 187 | + post api("/projects/#{project.id}/issues/#{issue.id}/notes", user), body: 'hi!' | |
| 188 | + end | |
| 189 | + end | |
| 190 | + | |
| 179 | 191 | end | ... | ... |
spec/support/test_env.rb