diff --git a/CHANGELOG b/CHANGELOG index 9e003312a8..762ea42854 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -4,6 +4,7 @@ v 8.3.9 - Fix a window.opener bug that could lead to XSS and open redirects - Prevent XSS via custom issue tracker URL - Prevent privilege escalation via "impersonate" feature + - Prevent users from deleting Webhooks via API they do not own v 8.3.8 - Remove persistent XSS vulnerability in `commit_person_link` helper diff --git a/lib/api/project_hooks.rb b/lib/api/project_hooks.rb index cf9938d25a..ccca65cbe1 100644 --- a/lib/api/project_hooks.rb +++ b/lib/api/project_hooks.rb @@ -103,10 +103,10 @@ module API required_attributes! [:hook_id] begin - @hook = ProjectHook.find(params[:hook_id]) - @hook.destroy + @hook = user_project.hooks.destroy(params[:hook_id]) rescue # ProjectHook can raise Error if hook_id not found + not_found!("Error deleting hook #{params[:hook_id]}") end end end diff --git a/spec/requests/api/project_hooks_spec.rb b/spec/requests/api/project_hooks_spec.rb index 142b637d29..ffb93bbb12 100644 --- a/spec/requests/api/project_hooks_spec.rb +++ b/spec/requests/api/project_hooks_spec.rb @@ -148,14 +148,24 @@ describe API::API, 'ProjectHooks', api: true do expect(response.status).to eq(200) end - it "should return success when deleting non existent hook" do + it "should return a 404 error when deleting non existent hook" do delete api("/projects/#{project.id}/hooks/42", user) - expect(response.status).to eq(200) + expect(response.status).to eq(404) end it "should return a 405 error if hook id not given" do delete api("/projects/#{project.id}/hooks", user) expect(response.status).to eq(405) end + + it "shold return a 404 if a user attempts to delete project hooks he/she does not own" do + test_user = create(:user) + other_project = create(:project) + other_project.team << [test_user, :master] + + delete api("/projects/#{other_project.id}/hooks/#{hook.id}", test_user) + expect(response.status).to eq(404) + expect(WebHook.exists?(hook.id)).to be_truthy + end end end