Merge branch 'security-fj-missing-csrf-system-hooks-ee' into 'master'

[master] Missing CSRF in System Hooks

See merge request gitlab/gitlab-ee!647

app/helpers/hooks_helper.rb

@@ -10,7 +10,7 @@ module HooksHelper
 
     trigger_human_name = trigger.to_s.tr('_', ' ').camelize
 
-    link_to path, rel: 'nofollow' do
+    link_to path, rel: 'nofollow', method: :post do
       content_tag(:span, trigger_human_name)
     end
   end

changelogs/unreleased/security-fj-missing-csrf-system-hooks.yml

+---
+title: Adding CSRF protection to Hooks test action
+merge_request:
+author:
+type: security

config/routes/admin.rb

@@ -60,7 +60,7 @@ namespace :admin do
 
   resources :hooks, only: [:index, :create, :edit, :update, :destroy] do
     member do
-      get :test
+      post :test
     end
 
     resources :hook_logs, only: [:show] do

config/routes/group.rb

@@ -83,7 +83,7 @@ constraints(::Constraints::GroupUrlConstrainer.new) do
 
     resources :hooks, only: [:index, :create, :destroy], constraints: { id: /\d+/ } do
       member do
-        get :test
+        post :test
       end
     end
 

config/routes/project.rb

@@ -337,7 +337,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
 
       resources :hooks, only: [:index, :create, :edit, :update, :destroy], constraints: { id: /\d+/ } do
         member do
-          get :test
+          post :test
         end
 
         resources :hook_logs, only: [:show] do

ee/app/views/groups/hooks/_project_hook.html.haml

@@ -9,7 +9,7 @@
     .col-md-4.col-lg-5.text-right-lg.prepend-top-5
       %span.append-right-10.inline
         SSL Verification: #{hook.enable_ssl_verification ? "enabled" : "disabled"}
-      = link_to "Test", test_group_hook_path(@group, hook), class: "btn btn-sm"
+      = link_to "Test", test_group_hook_path(@group, hook), class: "btn btn-sm", method: :post
       = link_to group_hook_path(@group, hook), data: { confirm: 'Are you sure?'}, method: :delete, class: "btn btn-transparent" do
         %span.sr-only Remove
         = icon('trash')

ee/spec/features/groups/hooks/user_tests_hooks_spec.rb

-require "spec_helper"
+require "rails_helper"
 
-describe "User tests hooks" do
-  set(:group) { create(:group) }
-  set(:hook) { create(:group_hook, group: group) }
-  set(:user) { create(:user) }
+describe "User tests hooks", :js do
+  let!(:group) { create(:group) }
+  let!(:hook) { create(:group_hook, group: group) }
+  let!(:user) { create(:user) }
 
   before do
     group.add_owner(user)

ee/spec/routing/webhook_routes_spec.rb

@@ -7,7 +7,7 @@ describe "routes to the proper webhooks controller", type: :routing do
 
     it "routes the test action" do
       expect(
-        get: polymorphic_path([project.namespace.becomes(Namespace), project, project_hook], action: :test)
+        post: polymorphic_path([project.namespace.becomes(Namespace), project, project_hook], action: :test)
       ).to route_to(controller: 'projects/hooks',
                     action: 'test',
                     namespace_id: project.namespace.name,
@@ -32,7 +32,7 @@ describe "routes to the proper webhooks controller", type: :routing do
 
     it "routes the test action" do
       expect(
-        get: polymorphic_path([group, group_hook], action: :test)
+        post: polymorphic_path([group, group_hook], action: :test)
       ).to route_to(controller: 'groups/hooks',
                     action: 'test',
                     group_id: group.name,

spec/routing/admin_routing_spec.rb

@@ -79,7 +79,7 @@ end
 # edit_admin_hook GET    /admin/hooks/:id(.:format)           admin/hooks#edit
 describe Admin::HooksController, "routing" do
   it "to #test" do
-    expect(get("/admin/hooks/1/test")).to route_to('admin/hooks#test', id: '1')
+    expect(post("/admin/hooks/1/test")).to route_to('admin/hooks#test', id: '1')
   end
 
   it "to #index" do

spec/routing/project_routing_spec.rb

@@ -389,7 +389,7 @@ describe 'project routing' do
   #                   DELETE /:project_id/hooks/:id(.:format)      hooks#destroy
   describe Projects::HooksController, 'routing' do
     it 'to #test' do
-      expect(get('/gitlab/gitlabhq/hooks/1/test')).to route_to('projects/hooks#test', namespace_id: 'gitlab', project_id: 'gitlabhq', id: '1')
+      expect(post('/gitlab/gitlabhq/hooks/1/test')).to route_to('projects/hooks#test', namespace_id: 'gitlab', project_id: 'gitlabhq', id: '1')
     end
 
     it_behaves_like 'RESTful project resources' do