Merge branch '33601-add-csrf-token-verification-to-api' into 'master'

Resolve "Add CSRF token verification to API"

Closes #33601

See merge request !12154

8 files changed, 168 insertions, 14 deletions

diff --git a/changelogs/unreleased/33601-add-csrf-token-verification-to-api.yml b/changelogs/unreleased/33601-add-csrf-token-verification-to-api.yml
new file mode 100644
index 00000000000..88cfb99a73e
--- /dev/null
+++ b/changelogs/unreleased/33601-add-csrf-token-verification-to-api.yml
@@ -0,0 +1,4 @@
+---
+title: Add CSRF token verification to API
+merge_request: 12154
+author: Vitaliy @blackst0ne Klachkov
diff --git a/config/initializers/omniauth.rb b/config/initializers/omniauth.rb
index f7fa6d1c2de..a36e59c941a 100644
--- a/config/initializers/omniauth.rb
+++ b/config/initializers/omniauth.rb
@@ -16,7 +16,7 @@ OmniAuth.config.allowed_request_methods = [:post]
 # In case of auto sign-in, the GET method is used (users don't get to click on a button)
 OmniAuth.config.allowed_request_methods << :get if Gitlab.config.omniauth.auto_sign_in_with_provider.present?
 OmniAuth.config.before_request_phase do |env|
-  OmniAuth::RequestForgeryProtection.call(env)
+  Gitlab::RequestForgeryProtection.call(env)
 end
 
 if Gitlab.config.omniauth.enabled
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index 57e3e93500f..234825480f2 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -336,12 +336,14 @@ module API
       env['warden']
     end
 
+    # Check if the request is GET/HEAD, or if CSRF token is valid.
+    def verified_request?
+      Gitlab::RequestForgeryProtection.verified?(env)
+    end
+
     # Check the Rails session for valid authentication details
-    #
-    # Until CSRF protection is added to the API, disallow this method for
-    # state-changing endpoints
     def find_user_from_warden
-      warden.try(:authenticate) if %w[GET HEAD].include?(env['REQUEST_METHOD'])
+      warden.try(:authenticate) if verified_request?
     end
 
     def initial_current_user
diff --git a/lib/omni_auth/request_forgery_protection.rb b/lib/gitlab/request_forgery_protection.rb
index 69155131d8d..48dd0487790 100644
--- a/lib/omni_auth/request_forgery_protection.rb
+++ b/lib/gitlab/request_forgery_protection.rb
@@ -1,6 +1,8 @@
-# Protects OmniAuth request phase against CSRF.
+# A module to check CSRF tokens in requests.
+# It's used in API helpers and OmniAuth.
+# Usage: GitLab::RequestForgeryProtection.call(env)
 
-module OmniAuth
+module Gitlab
   module RequestForgeryProtection
     class Controller < ActionController::Base
       protect_from_forgery with: :exception
@@ -17,5 +19,13 @@ module OmniAuth
     def self.call(env)
       app.call(env)
     end
+
+    def self.verified?(env)
+      call(env)
+
+      true
+    rescue ActionController::InvalidAuthenticityToken
+      false
+    end
   end
 end
diff --git a/spec/features/oauth_login_spec.rb b/spec/features/oauth_login_spec.rb
index 0064c9ef25e..49d8e52f861 100644
--- a/spec/features/oauth_login_spec.rb
+++ b/spec/features/oauth_login_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-feature 'OAuth Login', js: true do
+feature 'OAuth Login', :js, :allow_forgery_protection do
   include DeviseHelpers
 
   def enter_code(code)
diff --git a/spec/lib/gitlab/request_forgery_protection_spec.rb b/spec/lib/gitlab/request_forgery_protection_spec.rb
new file mode 100644
index 00000000000..305de613866
--- /dev/null
+++ b/spec/lib/gitlab/request_forgery_protection_spec.rb
@@ -0,0 +1,89 @@
+require 'spec_helper'
+
+describe Gitlab::RequestForgeryProtection, :allow_forgery_protection do
+  let(:csrf_token) { SecureRandom.base64(ActionController::RequestForgeryProtection::AUTHENTICITY_TOKEN_LENGTH) }
+  let(:env) do
+    {
+      'rack.input' => '',
+      'rack.session' => {
+        _csrf_token: csrf_token
+      }
+    }
+  end
+
+  describe '.call' do
+    context 'when the request method is GET' do
+      before do
+        env['REQUEST_METHOD'] = 'GET'
+      end
+
+      it 'does not raise an exception' do
+        expect { described_class.call(env) }.not_to raise_exception
+      end
+    end
+
+    context 'when the request method is POST' do
+      before do
+        env['REQUEST_METHOD'] = 'POST'
+      end
+
+      context 'when the CSRF token is valid' do
+        before do
+          env['HTTP_X_CSRF_TOKEN'] = csrf_token
+        end
+
+        it 'does not raise an exception' do
+          expect { described_class.call(env) }.not_to raise_exception
+        end
+      end
+
+      context 'when the CSRF token is invalid' do
+        before do
+          env['HTTP_X_CSRF_TOKEN'] = 'foo'
+        end
+
+        it 'raises an ActionController::InvalidAuthenticityToken exception' do
+          expect { described_class.call(env) }.to raise_exception(ActionController::InvalidAuthenticityToken)
+        end
+      end
+    end
+  end
+
+  describe '.verified?' do
+    context 'when the request method is GET' do
+      before do
+        env['REQUEST_METHOD'] = 'GET'
+      end
+
+      it 'returns true' do
+        expect(described_class.verified?(env)).to be_truthy
+      end
+    end
+
+    context 'when the request method is POST' do
+      before do
+        env['REQUEST_METHOD'] = 'POST'
+      end
+
+      context 'when the CSRF token is valid' do
+        before do
+          env['HTTP_X_CSRF_TOKEN'] = csrf_token
+        end
+
+        it 'returns true' do
+          expect(described_class.verified?(env)).to be_truthy
+        end
+      end
+
+      context 'when the CSRF token is invalid' do
+        before do
+          env['HTTP_X_CSRF_TOKEN'] = 'foo'
+        end
+
+        it 'returns false' do
+          expect(described_class.verified?(env)).to be_falsey
+        end
+      end
+    end
+  end
+end
diff --git a/spec/requests/api/helpers_spec.rb b/spec/requests/api/helpers_spec.rb
index 25ec44fa036..7a1bd76af7a 100644
--- a/spec/requests/api/helpers_spec.rb
+++ b/spec/requests/api/helpers_spec.rb
@@ -10,8 +10,16 @@ describe API::Helpers do
   let(:key) { create(:key, user: user) }
 
   let(:params) { {} }
-  let(:env) { { 'REQUEST_METHOD' => 'GET' } }
-  let(:request) { Rack::Request.new(env) }
+  let(:csrf_token) { SecureRandom.base64(ActionController::RequestForgeryProtection::AUTHENTICITY_TOKEN_LENGTH) }
+  let(:env) do
+    {
+      'rack.input' => '',
+      'rack.session' => {
+        _csrf_token: csrf_token
+      },
+      'REQUEST_METHOD' => 'GET'
+    }
+  end
   let(:header) { }
 
   before do
@@ -58,7 +66,7 @@ describe API::Helpers do
   describe ".current_user" do
     subject { current_user }
 
-    describe "Warden authentication" do
+    describe "Warden authentication", :allow_forgery_protection do
       before do
         doorkeeper_guard_returns false
       end
@@ -99,7 +107,17 @@ describe API::Helpers do
             env['REQUEST_METHOD'] = 'PUT'
           end
 
-          it { is_expected.to be_nil }
+          context 'without CSRF token' do
+            it { is_expected.to be_nil }
+          end
+
+          context 'with CSRF token' do
+            before do
+              env['HTTP_X_CSRF_TOKEN'] = csrf_token
+            end
+
+            it { is_expected.to eq(user) }
+          end
         end
 
         context "POST request" do
@@ -107,7 +125,17 @@ describe API::Helpers do
             env['REQUEST_METHOD'] = 'POST'
           end
 
-          it { is_expected.to be_nil }
+          context 'without CSRF token' do
+            it { is_expected.to be_nil }
+          end
+
+          context 'with CSRF token' do
+            before do
+              env['HTTP_X_CSRF_TOKEN'] = csrf_token
+            end
+
+            it { is_expected.to eq(user) }
+          end
         end
 
         context "DELETE request" do
@@ -115,7 +143,17 @@ describe API::Helpers do
             env['REQUEST_METHOD'] = 'DELETE'
           end
 
-          it { is_expected.to be_nil }
+          context 'without CSRF token' do
+            it { is_expected.to be_nil }
+          end
+
+          context 'with CSRF token' do
+            before do
+              env['HTTP_X_CSRF_TOKEN'] = csrf_token
+            end
+
+            it { is_expected.to eq(user) }
+          end
         end
       end
     end
diff --git a/spec/support/forgery_protection.rb b/spec/support/forgery_protection.rb
new file mode 100644
index 00000000000..a5e7b761651
--- /dev/null
+++ b/spec/support/forgery_protection.rb
@@ -0,0 +1,11 @@
+RSpec.configure do |config|
+  config.around(:each, :allow_forgery_protection) do |example|
+    begin
+      ActionController::Base.allow_forgery_protection = true
+
+      example.call
+    ensure
+      ActionController::Base.allow_forgery_protection = false
+    end
+  end
+end