Commit 73a97a67 authored by Bob Van Landuyt's avatar Bob Van Landuyt

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.
parent 5828d5c7
module ContinueParams module ContinueParams
include InternalRedirect
extend ActiveSupport::Concern extend ActiveSupport::Concern
def continue_params def continue_params
...@@ -6,8 +7,7 @@ module ContinueParams ...@@ -6,8 +7,7 @@ module ContinueParams
return nil unless continue_params return nil unless continue_params
continue_params = continue_params.permit(:to, :notice, :notice_now) continue_params = continue_params.permit(:to, :notice, :notice_now)
return unless continue_params[:to] && continue_params[:to].start_with?('/') continue_params[:to] = safe_redirect_path(continue_params[:to])
return if continue_params[:to].start_with?('//')
continue_params continue_params
end end
......
class SessionsController < Devise::SessionsController class SessionsController < Devise::SessionsController
include InternalRedirect
include AuthenticatesWithTwoFactor include AuthenticatesWithTwoFactor
include Devise::Controllers::Rememberable include Devise::Controllers::Rememberable
include Recaptcha::ClientHelper include Recaptcha::ClientHelper
...@@ -103,18 +104,12 @@ class SessionsController < Devise::SessionsController ...@@ -103,18 +104,12 @@ class SessionsController < Devise::SessionsController
# we should never redirect to '/users/sign_in' after signing in successfully. # we should never redirect to '/users/sign_in' after signing in successfully.
return true if redirect_uri.path == new_user_session_path 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 @redirect_to = redirect_to
store_location_for(:redirect, redirect_to) store_location_for(:redirect, redirect_to)
end 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? def two_factor_enabled?
find_user&.two_factor_enabled? find_user&.two_factor_enabled?
end end
......
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
...@@ -265,7 +265,7 @@ describe SessionsController do ...@@ -265,7 +265,7 @@ describe SessionsController do
it 'redirects correctly for referer on same host with params' do it 'redirects correctly for referer on same host with params' do
search_path = '/search?search=seed_project' search_path = '/search?search=seed_project'
allow(controller.request).to receive(:referer) 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) get(:new, redirect_to_referer: :yes)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment