diff options
-rw-r--r-- | app/controllers/concerns/continue_params.rb | 4 | ||||
-rw-r--r-- | app/controllers/sessions_controller.rb | 9 | ||||
-rw-r--r-- | spec/controllers/concerns/continue_params_spec.rb | 45 | ||||
-rw-r--r-- | spec/controllers/sessions_controller_spec.rb | 2 |
4 files changed, 50 insertions, 10 deletions
diff --git a/app/controllers/concerns/continue_params.rb b/app/controllers/concerns/continue_params.rb index eb3a623acdd..8b7355974df 100644 --- a/app/controllers/concerns/continue_params.rb +++ b/app/controllers/concerns/continue_params.rb @@ -1,4 +1,5 @@ module ContinueParams + include InternalRedirect extend ActiveSupport::Concern def continue_params @@ -6,8 +7,7 @@ module ContinueParams return nil unless continue_params continue_params = continue_params.permit(:to, :notice, :notice_now) - return unless continue_params[:to] && continue_params[:to].start_with?('/') - return if continue_params[:to].start_with?('//') + continue_params[:to] = safe_redirect_path(continue_params[:to]) continue_params end diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index f3a4aa849c7..1a339f76d26 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -1,4 +1,5 @@ class SessionsController < Devise::SessionsController + include InternalRedirect include AuthenticatesWithTwoFactor include Devise::Controllers::Rememberable include Recaptcha::ClientHelper @@ -102,18 +103,12 @@ class SessionsController < Devise::SessionsController # we should never redirect to '/users/sign_in' after signing in successfully. return true if redirect_uri.path == new_user_session_path - redirect_to = redirect_uri.to_s if redirect_allowed_to?(redirect_uri) + redirect_to = redirect_uri.to_s if host_allowed?(redirect_uri) @redirect_to = redirect_to store_location_for(:redirect, redirect_to) end - # Overridden in EE - def redirect_allowed_to?(uri) - uri.host == Gitlab.config.gitlab.host && - uri.port == Gitlab.config.gitlab.port - end - def two_factor_enabled? find_user&.two_factor_enabled? end diff --git a/spec/controllers/concerns/continue_params_spec.rb b/spec/controllers/concerns/continue_params_spec.rb new file mode 100644 index 00000000000..e2f683ae393 --- /dev/null +++ b/spec/controllers/concerns/continue_params_spec.rb @@ -0,0 +1,45 @@ +require 'spec_helper' + +describe ContinueParams do + let(:controller_class) do + Class.new(ActionController::Base) do + include ContinueParams + + def request + @request ||= Struct.new(:host, :port).new('test.host', 80) + end + end + end + subject(:controller) { controller_class.new } + + def strong_continue_params(params) + ActionController::Parameters.new(continue: params) + end + + it 'cleans up any params that are not allowed' do + allow(controller).to receive(:params) do + strong_continue_params(to: '/hello', + notice: 'world', + notice_now: '!', + something: 'else') + end + + expect(controller.continue_params.keys).to contain_exactly(*%w(to notice notice_now)) + end + + it 'does not allow cross host redirection' do + allow(controller).to receive(:params) do + strong_continue_params(to: '//example.com') + end + + expect(controller.continue_params[:to]).to be_nil + end + + it 'allows redirecting to a path with querystring' do + allow(controller).to receive(:params) do + strong_continue_params(to: '/hello/world?query=string') + end + + expect(controller.continue_params[:to]).to eq('/hello/world?query=string') + end +end diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index 55bd4352bd3..555b186fe31 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/spec/controllers/sessions_controller_spec.rb @@ -265,7 +265,7 @@ describe SessionsController do it 'redirects correctly for referer on same host with params' do search_path = '/search?search=seed_project' allow(controller.request).to receive(:referer) - .and_return('http://%{host}%{path}' % { host: Gitlab.config.gitlab.host, path: search_path }) + .and_return('http://%{host}%{path}' % { host: 'test.host', path: search_path }) get(:new, redirect_to_referer: :yes) |