Merge branch 'open-redirect-host-fix' into 'security'

Fix for three open redirect vulns using redirect_to url_for(params.merge)))

See merge request !2082

7 files changed, 37 insertions, 3 deletions

diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb
index 5848ca62777..20336349c9a 100644
--- a/app/controllers/dashboard/todos_controller.rb
+++ b/app/controllers/dashboard/todos_controller.rb
@@ -7,7 +7,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController
     @sort = params[:sort]
     @todos = @todos.page(params[:page])
     if @todos.out_of_range? && @todos.total_pages != 0
-      redirect_to url_for(params.merge(page: @todos.total_pages))
+      redirect_to url_for(params.merge(page: @todos.total_pages, only_path: true))
     end
   end
 
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index 857d907c1a6..6bdf089d45c 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -29,7 +29,7 @@ class Projects::IssuesController < Projects::ApplicationController
     @issuable_meta_data = issuable_meta_data(@issues, @collection_type)
 
     if @issues.out_of_range? && @issues.total_pages != 0
-      return redirect_to url_for(params.merge(page: @issues.total_pages))
+      return redirect_to url_for(params.merge(page: @issues.total_pages, only_path: true))
     end
 
     if params[:label_name].present?
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 677a8a1a73a..943cc7a688f 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -42,7 +42,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     @issuable_meta_data = issuable_meta_data(@merge_requests, @collection_type)
 
     if @merge_requests.out_of_range? && @merge_requests.total_pages != 0
-      return redirect_to url_for(params.merge(page: @merge_requests.total_pages))
+      return redirect_to url_for(params.merge(page: @merge_requests.total_pages, only_path: true))
     end
 
     if params[:label_name].present?
diff --git a/changelogs/unreleased/open-redirect-host-field.yml b/changelogs/unreleased/open-redirect-host-field.yml
new file mode 100644
index 00000000000..bed4b47cf04
--- /dev/null
+++ b/changelogs/unreleased/open-redirect-host-field.yml
@@ -0,0 +1,4 @@
+---
+title: Fix for open redirect vulnerabilities in todos, issues, and MR controllers.
+merge_request: 
+author:
diff --git a/spec/controllers/dashboard/todos_controller_spec.rb b/spec/controllers/dashboard/todos_controller_spec.rb
index 7072bd5e87c..f0395fccfca 100644
--- a/spec/controllers/dashboard/todos_controller_spec.rb
+++ b/spec/controllers/dashboard/todos_controller_spec.rb
@@ -35,6 +35,13 @@ describe Dashboard::TodosController do
         expect(assigns(:todos).current_page).to eq(last_page)
         expect(response).to have_http_status(200)
       end
+
+      it 'does not redirect to external sites when provided a host field' do
+        external_host = "www.example.com"
+        get :index, page: (last_page + 1).to_param, host: external_host
+
+        expect(response).to redirect_to(dashboard_todos_path(page: last_page))
+      end
     end
   end
 
diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb
index 972005ede3a..b16a8db01c4 100644
--- a/spec/controllers/projects/issues_controller_spec.rb
+++ b/spec/controllers/projects/issues_controller_spec.rb
@@ -83,6 +83,17 @@ describe Projects::IssuesController do
         expect(assigns(:issues).current_page).to eq(last_page)
         expect(response).to have_http_status(200)
       end
+
+      it 'does not redirect to external sites when provided a host field' do
+        external_host = "www.example.com"
+        get :index,
+          namespace_id: project.namespace.to_param,
+          project_id: project,
+          page: (last_page + 1).to_param,
+          host: external_host
+
+        expect(response).to redirect_to(namespace_project_issues_path(page: last_page, state: controller.params[:state], scope: controller.params[:scope]))
+      end
     end
   end
 
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index c310d830e81..62f48f9d590 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -176,6 +176,18 @@ describe Projects::MergeRequestsController do
         expect(assigns(:merge_requests).current_page).to eq(last_page)
         expect(response).to have_http_status(200)
       end
+
+      it 'does not redirect to external sites when provided a host field' do
+        external_host = "www.example.com"
+        get :index,
+          namespace_id: project.namespace.to_param,
+          project_id: project,
+          state: 'opened',
+          page: (last_page + 1).to_param,
+          host: external_host
+
+        expect(response).to redirect_to(namespace_project_merge_requests_path(page: last_page, state: controller.params[:state], scope: controller.params[:scope]))
+      end
     end
 
     context 'when filtering by opened state' do