diff options
author | Rémy Coutable <remy@gitlab.com> | 2016-04-25 09:26:58 +0000 |
---|---|---|
committer | Rémy Coutable <remy@rymai.me> | 2016-04-26 10:34:49 +0200 |
commit | 446f8866515014190a655647316efb1886b10d36 (patch) | |
tree | 3ef764ac8fdf174b2aee00bfa1f4395151af1411 | |
parent | 3b72afe0e1036b84ab690e965fac2295c0454de2 (diff) | |
download | gitlab-ce-446f8866515014190a655647316efb1886b10d36.tar.gz |
Merge branch 'fix-project-hook-delete-permissions' into 'master'
Prevent users from deleting Webhooks via API they do not own
Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/15576
See merge request !1959
Signed-off-by: Rémy Coutable <remy@rymai.me>
-rw-r--r-- | CHANGELOG | 1 | ||||
-rw-r--r-- | lib/api/project_hooks.rb | 4 | ||||
-rw-r--r-- | spec/requests/api/project_hooks_spec.rb | 14 |
3 files changed, 15 insertions, 4 deletions
diff --git a/CHANGELOG b/CHANGELOG index fe26e99b1bd..1b4cacdf92a 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -7,6 +7,7 @@ v 8.5.12 - Fix vulnerability that leaks private labels and milestones - Prevent XSS with in label dropdown - Prevent privilege escalation via "impersonate" feature + - Prevent users from deleting Webhooks via API they do not own - Prevent information disclosure via milestone API - Prevent information disclosure via snippet API - Prevent information disclosure via new merge request page diff --git a/lib/api/project_hooks.rb b/lib/api/project_hooks.rb index cf9938d25a7..ccca65cbe1c 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 142b637d291..ffb93bbb120 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 |