From 39916fdfeddfd75279d13fa976fdb07f3b9b0e26 Mon Sep 17 00:00:00 2001
From: Bob Van Landuyt <bob@vanlanduyt.co>
Date: Wed, 2 May 2018 20:25:21 +0200
Subject: 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.
---
 app/controllers/concerns/continue_params.rb       |  4 +-
 app/controllers/sessions_controller.rb            |  9 +----
 spec/controllers/concerns/continue_params_spec.rb | 45 +++++++++++++++++++++++
 spec/controllers/sessions_controller_spec.rb      |  2 +-
 4 files changed, 50 insertions(+), 10 deletions(-)
 create mode 100644 spec/controllers/concerns/continue_params_spec.rb

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)
 
-- 
cgit v1.2.3