summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorGitLab Bot <gitlab-bot@gitlab.com>2022-01-10 20:41:02 +0000
committerGitLab Bot <gitlab-bot@gitlab.com>2022-01-10 20:41:18 +0000
commit14d2af20ed388dc30da7cc103584b0229e0edb62 (patch)
treeb8eea54390428ecd2a2f9b1568d42bbf9516a47d
parentb69a74a63d5508767cd8b6ea5d1c966de0ee07fd (diff)
downloadgitlab-ce-14d2af20ed388dc30da7cc103584b0229e0edb62.tar.gz
Add latest changes from gitlab-org/security/gitlab@14-6-stable-ee
-rw-r--r--app/controllers/import/github_controller.rb21
-rw-r--r--lib/gitlab/legacy_github_import/client.rb5
-rw-r--r--lib/gitlab/url_blocker.rb4
-rw-r--r--spec/controllers/import/github_controller_spec.rb57
-rw-r--r--spec/lib/gitlab/url_blocker_spec.rb56
5 files changed, 110 insertions, 33 deletions
diff --git a/app/controllers/import/github_controller.rb b/app/controllers/import/github_controller.rb
index d7aebd25432..55f4563285d 100644
--- a/app/controllers/import/github_controller.rb
+++ b/app/controllers/import/github_controller.rb
@@ -28,8 +28,14 @@ class Import::GithubController < Import::BaseController
end
def callback
- session[access_token_key] = get_token(params[:code])
- redirect_to status_import_url
+ auth_state = session[auth_state_key]
+ session[auth_state_key] = nil
+ if auth_state.blank? || !ActiveSupport::SecurityUtils.secure_compare(auth_state, params[:state])
+ provider_unauthorized
+ else
+ session[access_token_key] = get_token(params[:code])
+ redirect_to status_import_url
+ end
end
def personal_access_token
@@ -154,13 +160,16 @@ class Import::GithubController < Import::BaseController
end
def authorize_url
+ state = SecureRandom.base64(64)
+ session[auth_state_key] = state
if Feature.enabled?(:remove_legacy_github_client)
oauth_client.auth_code.authorize_url(
redirect_uri: callback_import_url,
- scope: 'repo, user, user:email'
+ scope: 'repo, user, user:email',
+ state: state
)
else
- client.authorize_url(callback_import_url)
+ client.authorize_url(callback_import_url, state)
end
end
@@ -219,6 +228,10 @@ class Import::GithubController < Import::BaseController
alert: _('Missing OAuth configuration for GitHub.')
end
+ def auth_state_key
+ :"#{provider_name}_auth_state_key"
+ end
+
def access_token_key
:"#{provider_name}_access_token"
end
diff --git a/lib/gitlab/legacy_github_import/client.rb b/lib/gitlab/legacy_github_import/client.rb
index 48a8e0ce6d7..7a9dae3a3de 100644
--- a/lib/gitlab/legacy_github_import/client.rb
+++ b/lib/gitlab/legacy_github_import/client.rb
@@ -48,10 +48,11 @@ module Gitlab
)
end
- def authorize_url(redirect_uri)
+ def authorize_url(redirect_uri, state = nil)
client.auth_code.authorize_url({
redirect_uri: redirect_uri,
- scope: "repo, user, user:email"
+ scope: "repo, user, user:email",
+ state: state
})
end
diff --git a/lib/gitlab/url_blocker.rb b/lib/gitlab/url_blocker.rb
index 2c5d76ba41d..f092e03046a 100644
--- a/lib/gitlab/url_blocker.rb
+++ b/lib/gitlab/url_blocker.rb
@@ -252,13 +252,13 @@ module Gitlab
def internal_web?(uri)
uri.scheme == config.gitlab.protocol &&
uri.hostname == config.gitlab.host &&
- (uri.port.blank? || uri.port == config.gitlab.port)
+ get_port(uri) == config.gitlab.port
end
def internal_shell?(uri)
uri.scheme == 'ssh' &&
uri.hostname == config.gitlab_shell.ssh_host &&
- (uri.port.blank? || uri.port == config.gitlab_shell.ssh_port)
+ get_port(uri) == config.gitlab_shell.ssh_port
end
def domain_allowed?(uri)
diff --git a/spec/controllers/import/github_controller_spec.rb b/spec/controllers/import/github_controller_spec.rb
index d82fff1f7ae..fd380f9b763 100644
--- a/spec/controllers/import/github_controller_spec.rb
+++ b/spec/controllers/import/github_controller_spec.rb
@@ -6,6 +6,7 @@ RSpec.describe Import::GithubController do
include ImportSpecHelper
let(:provider) { :github }
+ let(:new_import_url) { public_send("new_import_#{provider}_url") }
include_context 'a GitHub-ish import controller'
@@ -50,13 +51,37 @@ RSpec.describe Import::GithubController do
stub_omniauth_provider('github')
end
- it "updates access token" do
- token = "asdasd12345"
+ context "when auth state param is missing from session" do
+ it "reports an error" do
+ get :callback
- get :callback
+ expect(controller).to redirect_to(new_import_url)
+ expect(flash[:alert]).to eq('Access denied to your GitHub account.')
+ end
+ end
+
+ context "when auth state param is present in session" do
+ let(:valid_auth_state) { "secret-state" }
+
+ before do
+ session[:github_auth_state_key] = valid_auth_state
+ end
- expect(session[:github_access_token]).to eq(token)
- expect(controller).to redirect_to(status_import_github_url)
+ it "updates access token if state param is valid" do
+ token = "asdasd12345"
+
+ get :callback, params: { state: valid_auth_state }
+
+ expect(session[:github_access_token]).to eq(token)
+ expect(controller).to redirect_to(status_import_github_url)
+ end
+
+ it "reports an error if state param is invalid" do
+ get :callback, params: { state: "different-state" }
+
+ expect(controller).to redirect_to(new_import_url)
+ expect(flash[:alert]).to eq('Access denied to your GitHub account.')
+ end
end
end
@@ -71,8 +96,6 @@ RSpec.describe Import::GithubController do
end
context 'when OAuth config is missing' do
- let(:new_import_url) { public_send("new_import_#{provider}_url") }
-
before do
allow(controller).to receive(:oauth_config).and_return(nil)
end
@@ -108,6 +131,16 @@ RSpec.describe Import::GithubController do
get :status
end
+
+ it 'gets authorization url using legacy client' do
+ allow(controller).to receive(:logged_in_with_provider?).and_return(true)
+ expect(controller).to receive(:go_to_provider_for_permissions).and_call_original
+ expect_next_instance_of(Gitlab::LegacyGithubImport::Client) do |client|
+ expect(client).to receive(:authorize_url).and_call_original
+ end
+
+ get :new
+ end
end
context 'when feature remove_legacy_github_client is enabled' do
@@ -130,6 +163,16 @@ RSpec.describe Import::GithubController do
get :status
end
+ it 'gets authorization url using oauth client' do
+ allow(controller).to receive(:logged_in_with_provider?).and_return(true)
+ expect(controller).to receive(:go_to_provider_for_permissions).and_call_original
+ expect_next_instance_of(OAuth2::Client) do |client|
+ expect(client.auth_code).to receive(:authorize_url).and_call_original
+ end
+
+ get :new
+ end
+
context 'pagination' do
context 'when no page is specified' do
it 'requests first page' do
diff --git a/spec/lib/gitlab/url_blocker_spec.rb b/spec/lib/gitlab/url_blocker_spec.rb
index e076815c4f6..0713475d59b 100644
--- a/spec/lib/gitlab/url_blocker_spec.rb
+++ b/spec/lib/gitlab/url_blocker_spec.rb
@@ -531,24 +531,6 @@ RSpec.describe Gitlab::UrlBlocker, :stub_invalid_dns_only do
end
end
end
-
- def stub_domain_resolv(domain, ip, port = 80, &block)
- address = instance_double(Addrinfo,
- ip_address: ip,
- ipv4_private?: true,
- ipv6_linklocal?: false,
- ipv4_loopback?: false,
- ipv6_loopback?: false,
- ipv4?: false,
- ip_port: port
- )
- allow(Addrinfo).to receive(:getaddrinfo).with(domain, port, any_args).and_return([address])
- allow(address).to receive(:ipv6_v4mapped?).and_return(false)
-
- yield
-
- allow(Addrinfo).to receive(:getaddrinfo).and_call_original
- end
end
context 'when enforce_user is' do
@@ -611,6 +593,44 @@ RSpec.describe Gitlab::UrlBlocker, :stub_invalid_dns_only do
expect(described_class).to be_blocked_url('http://foobar.x')
end
+
+ context 'when gitlab is running on a non-default port' do
+ let(:gitlab_port) { 3000 }
+
+ before do
+ stub_config(gitlab: { protocol: 'http', host: 'gitlab.local', port: gitlab_port })
+ end
+
+ it 'returns true for url targeting the wrong port' do
+ stub_domain_resolv('gitlab.local', '127.0.0.1') do
+ expect(described_class).to be_blocked_url("http://gitlab.local/foo")
+ end
+ end
+
+ it 'does not block url on gitlab port' do
+ stub_domain_resolv('gitlab.local', '127.0.0.1') do
+ expect(described_class).not_to be_blocked_url("http://gitlab.local:#{gitlab_port}/foo")
+ end
+ end
+ end
+
+ def stub_domain_resolv(domain, ip, port = 80, &block)
+ address = instance_double(Addrinfo,
+ ip_address: ip,
+ ipv4_private?: true,
+ ipv6_linklocal?: false,
+ ipv4_loopback?: false,
+ ipv6_loopback?: false,
+ ipv4?: false,
+ ip_port: port
+ )
+ allow(Addrinfo).to receive(:getaddrinfo).with(domain, port, any_args).and_return([address])
+ allow(address).to receive(:ipv6_v4mapped?).and_return(false)
+
+ yield
+
+ allow(Addrinfo).to receive(:getaddrinfo).and_call_original
+ end
end
describe '#validate_hostname' do