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 e3933e3d7b1..9d7e9656b57 100644
--- a/app/controllers/dashboard/todos_controller.rb
+++ b/app/controllers/dashboard/todos_controller.rb
@@ -5,7 +5,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 9f085e32776..b29dad8f63a 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -26,7 +26,7 @@ class Projects::IssuesController < Projects::ApplicationController
     @issues = issues_collection
     @issues = @issues.page(params[:page])
     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 5b1b4d19488..0af301277f7 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -39,7 +39,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     @merge_requests = merge_requests_collection
     @merge_requests = @merge_requests.page(params[:page])
     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 79ef3a1adad..faa9ea9e7d3 100644
--- a/spec/controllers/dashboard/todos_controller_spec.rb
+++ b/spec/controllers/dashboard/todos_controller_spec.rb
@@ -32,6 +32,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
 end
diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb
index 5a26c18fa3a..cdd93fc09ae 100644
--- a/spec/controllers/projects/issues_controller_spec.rb
+++ b/spec/controllers/projects/issues_controller_spec.rb
@@ -81,6 +81,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 cd6385fab61..a44899b92b9 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -170,6 +170,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