Prevent open redirect attack with tampered headers

ee/app/controllers/ee/sessions_controller.rb

@@ -14,7 +14,7 @@ module EE
       return super if signed_in?
 
       if ::Gitlab::Geo.secondary_with_primary?
-        redirect_to oauth_geo_auth_url(state: geo_login_state.encode)
+        redirect_to oauth_geo_auth_url(host: ::Gitlab::Geo.current_node.url, state: geo_login_state.encode)
       else
         super
       end
@@ -41,7 +41,7 @@ module EE
     end
 
     def geo_return_to_after_login
-      stored_redirect_uri || ::Gitlab::Utils.append_path(root_url, session[:user_return_to].to_s)
+      ::Gitlab::Utils.append_path(root_url, sanitize_redirect(session[:user_return_to].to_s))
     end
 
     def geo_return_to_after_logout

ee/lib/gitlab/geo/oauth/login_state.rb

@@ -12,7 +12,7 @@ module Gitlab
         end
 
         def initialize(return_to:, salt: nil, hmac: nil)
-          @return_to = return_to
+          @return_to = Gitlab::ReturnToLocation.new(return_to).full_path
           @salt = salt
           @hmac = hmac
         end

ee/spec/controllers/ee/sessions_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe SessionsController do
+  include DeviseHelpers
+  include EE::GeoHelpers
+
+  describe '#new' do
+    before do
+      set_devise_mapping(context: @request)
+    end
+
+    context 'on a Geo secondary node' do
+      set(:primary_node) { create(:geo_node, :primary) }
+      set(:secondary_node) { create(:geo_node) }
+
+      let(:salt) { 'MTAwZDhjYmQxNzUw' }
+      let(:login_state) { Gitlab::Geo::Oauth::LoginState.new(salt: salt, return_to: '/') }
+
+      before do
+        stub_current_geo_node(secondary_node)
+        allow(SecureRandom).to receive(:hex).and_return(salt)
+      end
+
+      context 'with a tampered HOST header' do
+        it 'prevents open redirect attack' do
+          request.headers['HOST'] = 'http://this.is.not.my.host'
+
+          get(:new)
+
+          expect(response).to have_gitlab_http_status(302)
+          expect(response).to redirect_to(oauth_geo_auth_url(host: secondary_node.url, state: login_state.encode))
+        end
+      end
+
+      context 'with a tampered X-Forwarded-Host header' do
+        it 'prevents open redirect attack' do
+          request.headers['X-Forwarded-Host'] = 'http://this.is.not.my.host'
+
+          get(:new)
+
+          expect(response).to have_gitlab_http_status(302)
+          expect(response).to redirect_to(oauth_geo_auth_url(host: secondary_node.url, state: login_state.encode))
+        end
+      end
+
+      context 'without a tampered header' do
+        it 'redirects to oauth_geo_auth_url' do
+          get(:new)
+
+          expect(response).to have_gitlab_http_status(302)
+          expect(response).to redirect_to(oauth_geo_auth_url(host: secondary_node.url, state: login_state.encode))
+        end
+      end
+    end
+  end
+end

ee/spec/controllers/oauth/geo_auth_controller_spec.rb

@@ -58,7 +58,7 @@ describe Oauth::GeoAuthController do
       it 'redirects to redirect_url if state is valid' do
         get :callback, params: { state: login_state }
 
-        expect(response).to redirect_to(secondary_node.url)
+        expect(response).to redirect_to('/')
       end
 
       it 'does not display a flash message if state is valid' do

ee/spec/lib/gitlab/geo/oauth/login_state_spec.rb

@@ -4,8 +4,8 @@ require 'spec_helper'
 
 describe Gitlab::Geo::Oauth::LoginState do
   let(:salt) { 'b9653b6aa2ff6b54' }
-  let(:hmac) { '908844004aa6ba7237be5cd394499a79e64c054e9b8021bd9b43ff7dc508320b' }
-  let(:oauth_return_to) { 'http://fake-secondary.com:3000/project/test' }
+  let(:hmac) { 'd75afcc6faa0fd5133c4512080c42ae579e9d9691bd1731475c287f394a35208' }
+  let(:return_to) { 'http://fake-secondary.com:3000/project/test?foo=bar#zoo' }
 
   before do
     allow(Gitlab::Application.secrets).to receive(:secret_key_base)
@@ -22,7 +22,7 @@ describe Gitlab::Geo::Oauth::LoginState do
     end
 
     it 'returns a valid instance when state is valid' do
-      expect(described_class.from_state("#{salt}:#{hmac}:#{oauth_return_to}")).to be_valid
+      expect(described_class.from_state("#{salt}:#{hmac}:#{return_to}")).to be_valid
     end
   end
 
@@ -40,31 +40,31 @@ describe Gitlab::Geo::Oauth::LoginState do
     end
 
     it 'returns false when hmac is nil' do
-      subject = described_class.new(return_to: oauth_return_to, salt: salt, hmac: nil)
+      subject = described_class.new(return_to: return_to, salt: salt, hmac: nil)
 
       expect(subject.valid?).to eq false
     end
 
     it 'returns false when hmac is empty' do
-      subject = described_class.new(return_to: oauth_return_to, salt: salt, hmac: '')
+      subject = described_class.new(return_to: return_to, salt: salt, hmac: '')
 
       expect(subject.valid?).to eq false
     end
 
     it 'returns false when salt not match' do
-      subject = described_class.new(return_to: oauth_return_to, salt: 'salt', hmac: hmac)
+      subject = described_class.new(return_to: return_to, salt: 'salt', hmac: hmac)
 
       expect(subject.valid?).to eq(false)
     end
 
     it 'returns false when hmac does not match' do
-      subject = described_class.new(return_to: oauth_return_to, salt: salt, hmac: 'hmac')
+      subject = described_class.new(return_to: return_to, salt: salt, hmac: 'hmac')
 
       expect(subject.valid?).to eq(false)
     end
 
     it 'returns true when hmac matches' do
-      subject = described_class.new(return_to: oauth_return_to, salt: salt, hmac: hmac)
+      subject = described_class.new(return_to: return_to, salt: salt, hmac: hmac)
 
       expect(subject.valid?).to eq(true)
     end
@@ -78,13 +78,33 @@ describe Gitlab::Geo::Oauth::LoginState do
     end
 
     it 'returns a string with salt, hmac, and return_to colon separated' do
-      subject = described_class.new(return_to: oauth_return_to)
+      subject = described_class.new(return_to: return_to)
 
       salt, hmac, return_to = subject.encode.split(':', 3)
 
       expect(salt).not_to be_blank
       expect(hmac).not_to be_blank
-      expect(return_to).to eq oauth_return_to
+      expect(return_to).to eq return_to
+    end
+  end
+
+  describe '#return_to' do
+    it 'returns nil when return_to is nil' do
+      subject = described_class.new(return_to: nil)
+
+      expect(subject.return_to).to be_nil
+    end
+
+    it 'returns an empty string when return_to is empty' do
+      subject = described_class.new(return_to: '')
+
+      expect(subject.return_to).to eq('')
+    end
+
+    it 'returns the full path of the return_to URL' do
+      subject = described_class.new(return_to: return_to)
+
+      expect(subject.return_to).to eq('/project/test?foo=bar#zoo')
     end
   end
 end