Merge branch 'sh-slack-app-csrf-protection' into 'master'

Protect against CSRF attacks when adding Slack app

See merge request gitlab/gitlab-ee!666

ee/app/controllers/projects/settings/slacks_controller.rb

@@ -2,6 +2,7 @@ module Projects
   module Settings
     class SlacksController < Projects::ApplicationController
       before_action :handle_oauth_error, only: :slack_auth
+      before_action :check_oauth_state, only: :slack_auth
       before_action :authorize_admin_project!
       before_action :slack_integration, only: [:edit, :update]
       before_action :service, only: [:destroy, :edit, :update]
@@ -46,6 +47,12 @@ module Projects
         )
       end
 
+      def check_oauth_state
+        render_403 unless valid_authenticity_token?(session, params[:state])
+
+        true
+      end
+
       def handle_oauth_error
         if params[:error] == 'access_denied'
           flash[:alert] = 'Access denied'

ee/app/helpers/ee/services_helper.rb

 module EE
   module ServicesHelper
     def add_to_slack_link(project, slack_app_id)
-      "https://slack.com/oauth/authorize?scope=commands&client_id=#{slack_app_id}&redirect_uri=#{slack_auth_project_settings_slack_url(project)}"
+      "https://slack.com/oauth/authorize?scope=commands&client_id=#{slack_app_id}&redirect_uri=#{slack_auth_project_settings_slack_url(project)}&state=#{escaped_form_authenticity_token}"
     end
 
     def add_to_slack_data(projects)
@@ -16,5 +16,9 @@ module EE
         docs_path: help_page_path('user/project/integrations/gitlab_slack_application.md')
       }.to_json.html_safe
     end
+
+    def escaped_form_authenticity_token
+      CGI.escape(form_authenticity_token)
+    end
   end
 end

ee/changelogs/unreleased/sh-slack-app-csrf-protection.yml

+---
+title: Protect against CSRF attacks when adding Slack app
+merge_request:
+author:
+type: security

ee/spec/controllers/projects/settings/slacks_controller_spec.rb

@@ -24,24 +24,38 @@ describe Projects::Settings::SlacksController do
         .to receive(:new).with(project, user, anything).and_return(service)
     end
 
-    it 'calls service and redirects with no alerts if result is successful' do
-      stub_service(status: :success)
+    context 'when valid CSRF token is provided' do
+      before do
+        expect(controller).to receive(:check_oauth_state).and_return(true)
+      end
 
-      get :slack_auth, namespace_id: project.namespace, project_id: project
+      it 'calls service and redirects with no alerts if result is successful' do
+        stub_service(status: :success)
 
-      expect(response).to have_gitlab_http_status(302)
-      expect(response).to redirect_to(redirect_url(project))
-      expect(flash[:alert]).to be_nil
-    end
+        get :slack_auth, namespace_id: project.namespace, project_id: project
+
+        expect(response).to have_gitlab_http_status(302)
+        expect(response).to redirect_to(redirect_url(project))
+        expect(flash[:alert]).to be_nil
+      end
+
+      it 'calls service and redirects with the alert if there is error' do
+        stub_service(status: :error, message: 'error')
 
-    it 'calls service and redirects with the alert if there is error' do
-      stub_service(status: :error, message: 'error')
+        get :slack_auth, namespace_id: project.namespace, project_id: project
+
+        expect(response).to have_gitlab_http_status(302)
+        expect(response).to redirect_to(redirect_url(project))
+        expect(flash[:alert]).to eq('error')
+      end
+    end
 
-      get :slack_auth, namespace_id: project.namespace, project_id: project
+    context 'when no CSRF token is provided' do
+      it 'returns 403' do
+        get :slack_auth, namespace_id: project.namespace, project_id: project
 
-      expect(response).to have_gitlab_http_status(302)
-      expect(response).to redirect_to(redirect_url(project))
-      expect(flash[:alert]).to eq('error')
+        expect(response).to have_gitlab_http_status(403)
+      end
     end
   end
 end

ee/spec/helpers/ee/services_helper_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe EE::ServicesHelper do
+  let(:controller_class) do
+    Class.new(ActionController::Base) do
+      include EE::ServicesHelper
+
+      def slack_auth_project_settings_slack_url(project)
+        "http://some-path/project/1"
+      end
+    end
+  end
+
+  set(:project) { create(:project) }
+
+  subject { controller_class.new }
+
+  describe '#add_to_slack_link' do
+    it 'encodes a masked CSRF token' do
+      expect(subject).to receive(:form_authenticity_token).and_return('a token')
+      slack_link = subject.add_to_slack_link(project, '123456')
+
+      expect(slack_link).to start_with('https://slack.com/oauth/authorize')
+      expect(slack_link).to include('redirect_uri=http://some-path/project/1&state=a+token')
+    end
+  end
+end