Reuses InternalRedirect when possible

`InternalRedirect` prevents Open redirect issues by only allowing
redirection to paths on the same host.

It cleans up any unwanted strings from the path that could point to
another host (fe. //about.gitlab.com/hello). While preserving the
querystring and fragment of the uri.

It is already used by:

- `TermsController`
- `ContinueParams`
  - `ImportsController`
  - `ForksController`
- `SessionsController`: Only for verifying the host in CE. EE allows
   redirecting to a different instance using Geo.
This commit is contained in:
Bob Van Landuyt 2018-05-02 20:25:21 +02:00
parent 7684217d68
commit 39916fdfed
4 changed files with 50 additions and 10 deletions

View file

@ -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

View file

@ -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

View file

@ -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

View file

@ -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)