Commit ef50875d authored by Sean McGivern's avatar Sean McGivern

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

Resolve "Add CSRF token verification to API"

Closes #33601

See merge request !12154
parents 2850efcd bfe8b968
---
title: Add CSRF token verification to API
merge_request: 12154
author: Vitaliy @blackst0ne Klachkov
...@@ -16,7 +16,7 @@ OmniAuth.config.allowed_request_methods = [:post] ...@@ -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) # 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.allowed_request_methods << :get if Gitlab.config.omniauth.auto_sign_in_with_provider.present?
OmniAuth.config.before_request_phase do |env| OmniAuth.config.before_request_phase do |env|
OmniAuth::RequestForgeryProtection.call(env) Gitlab::RequestForgeryProtection.call(env)
end end
if Gitlab.config.omniauth.enabled if Gitlab.config.omniauth.enabled
......
...@@ -336,12 +336,14 @@ module API ...@@ -336,12 +336,14 @@ module API
env['warden'] env['warden']
end 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 # 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 def find_user_from_warden
warden.try(:authenticate) if %w[GET HEAD].include?(env['REQUEST_METHOD']) warden.try(:authenticate) if verified_request?
end end
def initial_current_user def initial_current_user
......
# 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 module RequestForgeryProtection
class Controller < ActionController::Base class Controller < ActionController::Base
protect_from_forgery with: :exception protect_from_forgery with: :exception
...@@ -17,5 +19,13 @@ module OmniAuth ...@@ -17,5 +19,13 @@ module OmniAuth
def self.call(env) def self.call(env)
app.call(env) app.call(env)
end end
def self.verified?(env)
call(env)
true
rescue ActionController::InvalidAuthenticityToken
false
end
end end
end end
require 'spec_helper' require 'spec_helper'
feature 'OAuth Login', js: true do feature 'OAuth Login', :js, :allow_forgery_protection do
include DeviseHelpers include DeviseHelpers
def enter_code(code) def enter_code(code)
......
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
...@@ -10,8 +10,16 @@ describe API::Helpers do ...@@ -10,8 +10,16 @@ describe API::Helpers do
let(:key) { create(:key, user: user) } let(:key) { create(:key, user: user) }
let(:params) { {} } let(:params) { {} }
let(:env) { { 'REQUEST_METHOD' => 'GET' } } let(:csrf_token) { SecureRandom.base64(ActionController::RequestForgeryProtection::AUTHENTICITY_TOKEN_LENGTH) }
let(:request) { Rack::Request.new(env) } let(:env) do
{
'rack.input' => '',
'rack.session' => {
_csrf_token: csrf_token
},
'REQUEST_METHOD' => 'GET'
}
end
let(:header) { } let(:header) { }
before do before do
...@@ -58,7 +66,7 @@ describe API::Helpers do ...@@ -58,7 +66,7 @@ describe API::Helpers do
describe ".current_user" do describe ".current_user" do
subject { current_user } subject { current_user }
describe "Warden authentication" do describe "Warden authentication", :allow_forgery_protection do
before do before do
doorkeeper_guard_returns false doorkeeper_guard_returns false
end end
...@@ -99,7 +107,17 @@ describe API::Helpers do ...@@ -99,7 +107,17 @@ describe API::Helpers do
env['REQUEST_METHOD'] = 'PUT' env['REQUEST_METHOD'] = 'PUT'
end 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
context "POST request" do context "POST request" do
...@@ -107,7 +125,17 @@ describe API::Helpers do ...@@ -107,7 +125,17 @@ describe API::Helpers do
env['REQUEST_METHOD'] = 'POST' env['REQUEST_METHOD'] = 'POST'
end 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
context "DELETE request" do context "DELETE request" do
...@@ -115,7 +143,17 @@ describe API::Helpers do ...@@ -115,7 +143,17 @@ describe API::Helpers do
env['REQUEST_METHOD'] = 'DELETE' env['REQUEST_METHOD'] = 'DELETE'
end 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 end
end end
......
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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment