Add latest changes from gitlab-org/security/gitlab@14-6-stable-ee

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