Commit 7f5d12da authored by Illya Klymov's avatar Illya Klymov Committed by Mayra Cabrera

Add state param validation for Bitbucket OAuth flow

- pass state param and validate it via session

Changelog: security
parent 5b7647d9
...@@ -12,14 +12,21 @@ class Import::BitbucketController < Import::BaseController ...@@ -12,14 +12,21 @@ class Import::BitbucketController < Import::BaseController
rescue_from Bitbucket::Error::Unauthorized, with: :bitbucket_unauthorized rescue_from Bitbucket::Error::Unauthorized, with: :bitbucket_unauthorized
def callback def callback
response = oauth_client.auth_code.get_token(params[:code], redirect_uri: users_import_bitbucket_callback_url) auth_state = session[:bitbucket_auth_state]
session[:bitbucket_auth_state] = nil
session[:bitbucket_token] = response.token if auth_state.blank? || !ActiveSupport::SecurityUtils.secure_compare(auth_state, params[:state])
session[:bitbucket_expires_at] = response.expires_at go_to_bitbucket_for_permissions
session[:bitbucket_expires_in] = response.expires_in else
session[:bitbucket_refresh_token] = response.refresh_token response = oauth_client.auth_code.get_token(params[:code], redirect_uri: users_import_bitbucket_callback_url)
session[:bitbucket_token] = response.token
session[:bitbucket_expires_at] = response.expires_at
session[:bitbucket_expires_in] = response.expires_in
session[:bitbucket_refresh_token] = response.refresh_token
redirect_to status_import_bitbucket_url redirect_to status_import_bitbucket_url
end
end end
def status def status
...@@ -113,7 +120,9 @@ class Import::BitbucketController < Import::BaseController ...@@ -113,7 +120,9 @@ class Import::BitbucketController < Import::BaseController
end end
def go_to_bitbucket_for_permissions def go_to_bitbucket_for_permissions
redirect_to oauth_client.auth_code.authorize_url(redirect_uri: users_import_bitbucket_callback_url) state = SecureRandom.base64(64)
session[:bitbucket_auth_state] = state
redirect_to oauth_client.auth_code.authorize_url(redirect_uri: users_import_bitbucket_callback_url, state: state)
end end
def bitbucket_unauthorized(exception) def bitbucket_unauthorized(exception)
......
...@@ -26,31 +26,55 @@ RSpec.describe Import::BitbucketController do ...@@ -26,31 +26,55 @@ RSpec.describe Import::BitbucketController do
session[:oauth_request_token] = {} session[:oauth_request_token] = {}
end end
it "updates access token" do context "when auth state param is invalid" do
expires_at = Time.current + 1.day let(:random_key) { "pure_random" }
expires_in = 1.day let(:external_bitbucket_auth_url) { "http://fake.bitbucket.host/url" }
access_token = double(token: token,
secret: secret, it "redirects to external auth url" do
expires_at: expires_at, allow(SecureRandom).to receive(:base64).and_return(random_key)
expires_in: expires_in, allow_next_instance_of(OAuth2::Client) do |client|
refresh_token: refresh_token) allow(client).to receive_message_chain(:auth_code, :authorize_url)
allow_any_instance_of(OAuth2::Client) .with(redirect_uri: users_import_bitbucket_callback_url, state: random_key)
.to receive(:get_token) .and_return(external_bitbucket_auth_url)
.with(hash_including( end
'grant_type' => 'authorization_code',
'code' => code, get :callback, params: { code: code, state: "invalid-token" }
redirect_uri: users_import_bitbucket_callback_url),
{}) expect(controller).to redirect_to(external_bitbucket_auth_url)
.and_return(access_token) end
stub_omniauth_provider('bitbucket') end
get :callback, params: { code: code } context "when auth state param is valid" do
before do
expect(session[:bitbucket_token]).to eq(token) session[:bitbucket_auth_state] = 'state'
expect(session[:bitbucket_refresh_token]).to eq(refresh_token) end
expect(session[:bitbucket_expires_at]).to eq(expires_at)
expect(session[:bitbucket_expires_in]).to eq(expires_in) it "updates access token" do
expect(controller).to redirect_to(status_import_bitbucket_url) expires_at = Time.current + 1.day
expires_in = 1.day
access_token = double(token: token,
secret: secret,
expires_at: expires_at,
expires_in: expires_in,
refresh_token: refresh_token)
allow_any_instance_of(OAuth2::Client)
.to receive(:get_token)
.with(hash_including(
'grant_type' => 'authorization_code',
'code' => code,
redirect_uri: users_import_bitbucket_callback_url),
{})
.and_return(access_token)
stub_omniauth_provider('bitbucket')
get :callback, params: { code: code, state: 'state' }
expect(session[:bitbucket_token]).to eq(token)
expect(session[:bitbucket_refresh_token]).to eq(refresh_token)
expect(session[:bitbucket_expires_at]).to eq(expires_at)
expect(session[:bitbucket_expires_in]).to eq(expires_in)
expect(controller).to redirect_to(status_import_bitbucket_url)
end
end end
end end
...@@ -59,46 +83,68 @@ RSpec.describe Import::BitbucketController do ...@@ -59,46 +83,68 @@ RSpec.describe Import::BitbucketController do
@repo = double(name: 'vim', slug: 'vim', owner: 'asd', full_name: 'asd/vim', clone_url: 'http://test.host/demo/url.git', 'valid?' => true) @repo = double(name: 'vim', slug: 'vim', owner: 'asd', full_name: 'asd/vim', clone_url: 'http://test.host/demo/url.git', 'valid?' => true)
@invalid_repo = double(name: 'mercurialrepo', slug: 'mercurialrepo', owner: 'asd', full_name: 'asd/mercurialrepo', clone_url: 'http://test.host/demo/mercurialrepo.git', 'valid?' => false) @invalid_repo = double(name: 'mercurialrepo', slug: 'mercurialrepo', owner: 'asd', full_name: 'asd/mercurialrepo', clone_url: 'http://test.host/demo/mercurialrepo.git', 'valid?' => false)
allow(controller).to receive(:provider_url).and_return('http://demobitbucket.org') allow(controller).to receive(:provider_url).and_return('http://demobitbucket.org')
end
assign_session_tokens context "when token does not exists" do
let(:random_key) { "pure_random" }
let(:external_bitbucket_auth_url) { "http://fake.bitbucket.host/url" }
it 'redirects to authorize url with state included' do
allow(SecureRandom).to receive(:base64).and_return(random_key)
allow_next_instance_of(OAuth2::Client) do |client|
allow(client).to receive_message_chain(:auth_code, :authorize_url)
.with(redirect_uri: users_import_bitbucket_callback_url, state: random_key)
.and_return(external_bitbucket_auth_url)
end
get :status, format: :json
expect(controller).to redirect_to(external_bitbucket_auth_url)
end
end end
it_behaves_like 'import controller status' do context "when token is valid" do
before do before do
allow(controller).to receive(:provider_url).and_return('http://demobitbucket.org') assign_session_tokens
end end
let(:repo) { @repo } it_behaves_like 'import controller status' do
let(:repo_id) { @repo.full_name } before do
let(:import_source) { @repo.full_name } allow(controller).to receive(:provider_url).and_return('http://demobitbucket.org')
let(:provider_name) { 'bitbucket' } end
let(:client_repos_field) { :repos }
end
it 'returns invalid repos' do let(:repo) { @repo }
allow_any_instance_of(Bitbucket::Client).to receive(:repos).and_return([@repo, @invalid_repo]) let(:repo_id) { @repo.full_name }
let(:import_source) { @repo.full_name }
let(:provider_name) { 'bitbucket' }
let(:client_repos_field) { :repos }
end
get :status, format: :json it 'returns invalid repos' do
allow_any_instance_of(Bitbucket::Client).to receive(:repos).and_return([@repo, @invalid_repo])
expect(response).to have_gitlab_http_status(:ok) get :status, format: :json
expect(json_response['incompatible_repos'].length).to eq(1)
expect(json_response.dig("incompatible_repos", 0, "id")).to eq(@invalid_repo.full_name)
expect(json_response['provider_repos'].length).to eq(1)
expect(json_response.dig("provider_repos", 0, "id")).to eq(@repo.full_name)
end
context 'when filtering' do expect(response).to have_gitlab_http_status(:ok)
let(:filter) { '<html>test</html>' } expect(json_response['incompatible_repos'].length).to eq(1)
let(:expected_filter) { 'test' } expect(json_response.dig("incompatible_repos", 0, "id")).to eq(@invalid_repo.full_name)
expect(json_response['provider_repos'].length).to eq(1)
expect(json_response.dig("provider_repos", 0, "id")).to eq(@repo.full_name)
end
subject { get :status, params: { filter: filter }, as: :json } context 'when filtering' do
let(:filter) { '<html>test</html>' }
let(:expected_filter) { 'test' }
it 'passes sanitized filter param to bitbucket client' do subject { get :status, params: { filter: filter }, as: :json }
expect_next_instance_of(Bitbucket::Client) do |client|
expect(client).to receive(:repos).with(filter: expected_filter).and_return([@repo])
end
subject it 'passes sanitized filter param to bitbucket client' do
expect_next_instance_of(Bitbucket::Client) do |client|
expect(client).to receive(:repos).with(filter: expected_filter).and_return([@repo])
end
subject
end
end end
end 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